Allows users to provide their own S3 bucket for use in holding LB Access Logs

[tvaughan77]

Our organization has a single bucket designated for LB access logs in each environment, and we'd like to avoid having a 1:1 ratio between LBs and log buckets.

This commit enables the module user to provide the ID of a pre-existing S3 bucket that the LB is configured to use. This becomes mutually exclusive with the existing "enable_s3_logs" boolean.

One weird thing about this pull request is that because the current default is "true" for enable_s3_logs, to use this new bring-your-own-bucket functionality, the module definition looks like this:

module "load_balancer_bring_your_own_bucket" {
  ...
  enable_s3_logs  = false                  # This is weird.  I obviously want logs because I'm providing my own bucket
  log_bucket_id   = aws_s3_bucket.bucket.id
}

However, because this module has defaulted that value to true and this is a really popular module, I think it'd be a really bad idea to suddenly switch the default from true to false because that might end up deleting a bunch of people's logging buckets who aren't paying 100% attention to their terraform plan outputs.

What do you think? Is there a more elegant solution for this?

[jnonino]

Hi @tvaughan77, thank you for contributing. I think that you can keep this enabled = var.enable_s3_logs as a method to enable or disable sending logs regardless of how the target bucket was created.

Then, this module module "lb_logs_s3" can be created based on a condition like if var.enable_s3_logs and var.log_bucket_id == ""

I'll place comments in the PR too.

[jnonino]

The module lb_logs_s3 should be created with a conditional like this.

module "lb_logs_s3" {
  count = var.enable_s3_logs and log_bucket_id == "" ? 1 : 0

Maybe it's worth checking if its better to have log_bucket_id = null by default instead of ""

[jnonino]

Leave this as a variable to enable or disable the logs block.

[jnonino]

The dynamic block should be something like this:

dynamic "access_logs" {
    for_each = var.enable_s3_logs ? [1] : []
    content {
      bucket  = if var.log_bucket_id ? var.log_bucket_id : module.lb_logs_s3[0].s3_bucket_id
      enabled = var.enable_s3_logs
      prefix  = var.access_logs_prefix
    }
  }

[tvaughan77]

I like this a lot; I'll update my local branch and test this out and re-submit. Thanks for the idea!

[jnonino]

Hi @tvaughan77, the code looks good to me, but the plan job for test example is failing.

│ Error: Invalid count argument
│ 
│   on ../../main.tf line 8, in module "lb_logs_s3":
│    8:   count = var.enable_s3_logs && var.log_bucket_id == null ? 1 : 0
│ 
│ The "count" value depends on resource attributes that cannot be determined
│ until apply, so Terraform cannot predict how many instances will be
│ created. To work around this, use the -target argument to first apply only
│ the resources that the count depends on.

[tvaughan77]

Hi @jnonino - I think I've got the build working, except it's missing some API Key for a cost based service I don't have?

Error: No INFRACOST_API_KEY environment variable is set.

[jnonino]

Hi @tvaughan77, don't worry about it. It is now released in the new version.