Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support optional access logs #85

Closed
wants to merge 4 commits into from
Closed

Support optional access logs #85

wants to merge 4 commits into from

Conversation

brainsik
Copy link
Contributor

This came up while I was drafting what a Hello, World project might look like. I don't want the student to have to create an S3 bucket + policy just to bring up an ALB. This felt like a simple change that doesn't impact the default of logging.

Provides a way to disable logs. Logging is enabled by default.
Copy link
Contributor

@eeeady eeeady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized ... do you really want to disable logs??? I think that's a weird pattern to put in our module.

Copy link
Contributor

@eeeady eeeady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked... I agree this is relatively harmless since our default will still be to have logs. This will give folks that consume this more options.

prefix = "alb/${var.name}-${var.environment}"
dynamic "access_logs" {
# Skips creating the block if logs_s3_bucket is empty string
for_each = var.logs_s3_bucket == "" ? [] : ["create block"]

This comment was marked as resolved.

This comment was marked as resolved.

@brainsik
Copy link
Contributor Author

@LinuxBozo @dynamike @eeeady I believe this is ready for a real review now.

Copy link
Contributor

@dynamike dynamike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing around with this locally as @LinuxBozo has so amazingly commented on in the test code, I'm able to reproduce this same bug locally. It only happens when you spin up the logs module and this module from scratch. A second apply passes. Running this problem without logging, like @brainsik, doesn't trigger the bug.

The bit that I don't understand is it doesn't appear to be a problem with the https://github.com/trussworks/terraform-aws-s3-private-bucket module, which has he same dynamic logic being used here.

My only request is can we add a Note to the README highlighting this bug, it's a pretty painful one for those using the module with logging for the first time.


This is a bug in the provider, which should be reported in the provider's own
issue tracker.
https://github.com/terraform-providers/terraform-provider-aws/issues/10297
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another bug report specific to AWS access logs and ALBs
hashicorp/terraform-provider-aws#16674

@dynamike
Copy link
Contributor

@brainsik how we hanging on this?

@dynamike dynamike closed this Apr 21, 2021
@dynamike dynamike deleted the optional-logs branch June 15, 2021 23:03
@brainsik brainsik restored the optional-logs branch June 23, 2021 17:28
@brainsik
Copy link
Contributor Author

Woot! The dynamic block planning problem got fixed in Terraform v0.15.2:
hashicorp/terraform#28424

I'm going to open a new PR with my changes. Apparently the change from mastermain means I can't reopen this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants