Access Logs should be optional

[lvampa]

Current Module version - 3.1.0

I apologize ahead of time for dragging this issue back up.

Issue

By including the access_log block like this -

access_logs {
    enabled = true
    bucket  = "${var.log_bucket_name}"
    prefix  = "${var.log_location_prefix}"
}

You are prescribing a certain resource structure instead of letting the module user decide what they want.

Background

Previous related issue - #31
Summarized outcome from issue - make access logs required

Based upon the AWS Documentation -
"Access logging is an optional feature of Elastic Load Balancing that is disabled by default."

Proposal

resource "aws_lb" "lb_without_logs" {
  count = {create if use_log var is not set}
  ...
}

resource "aws_lb" "lb_with_logs" {
  count = {create if use_log var is set}
  ...
  access_logs {
    ...
  }
}

# to dry things out a little
locals {
  lb_arn = {conditional arn based on use_log var}
}

[brandonjbjelland]

@lvampa -

Typically this would be simple to do but Terraform is not cooperating with how I had this originally in mind (failing branch is here) :

Error: Error running plan: 2 error(s) occurred:

       * module.alb.aws_lb.application_no_logs: aws_lb.application_no_logs: value of 'count' cannot be computed
       * module.alb.aws_lb.application: aws_lb.application: value of 'count' cannot be computed

I'm able to work around the problem by providing a dedicated variable for access_logs_enabled as a boolean but the typical string check conditionals combined to create a count don't work here for some reason I can't seem to figure out. Feel free to submit a PR if you've got it working without an extra variable.

I'll try to release this functionality this week but honestly shipping the ability for someone to not log requests is a somewhat low priority if you catch my drift 😄 .

[mohsen0]

Hi,
Can you review my changes: #60
Thanks

[etherops]

Bump, just checking on this. Would love to switch over to using community modules from our own custom cruft, but we have high volume internal ALBs that we do not require logging for. I see some work on this by @mohsen0 but it doesn't look like that work was merged to master or released. Thanks!

[brandonjbjelland]

This is an incredibly messy changeset (see the diff) as I expected it would be but this feature request is now fulfilled by #69 and released as 3.4.0.

[etherops]

Wow thanks for the speedy work @brandoconnor! kudos. Admittedly I don't understand now how the main.tf is all comment block and it seems like has been forked into two different tf files... but presumably that is part of the messyness you are referring to. anyways, thanks!

[brandonjbjelland]

Thanks on the kudos. I'd been avoiding this work for a while knowing it'd be kind of gross but it's not so bad I suppose.

I broke out the resources into the alb_w_logs.tf and alb_no_logs.tf since the dependency chain required conditionally created resources of both flavors. I hoped to not have to do this for the entire dependency chain but it was required in the end. Even still, it works and I arrived here only after trying to the simpler and more straightforward ways you'd hope would work. I think this actually highlights one of the best reasons to use modules: hide the ugly underbelly in favor of exposing a simple interface.

main.tf now holds the documentation generated by the terraform-doc tool since antonbabenko added that a day earlier but hadn't shuffled docs there just yet. This is how I'm familiar with using terraform-doc anyway.

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.