resource/aws_lb: Disable access_logs for network load balancers #2256

gmccue · 2017-11-11T12:33:47Z

Ninir

This seems rather good!
Just left a question to discuss before I test & merge.

Thanks for the work! 👍

Ninir · 2017-11-15T11:07:19Z

CHANGELOG.md

@@ -35,6 +35,7 @@ BUG FIXES:
 * resource/aws_cloudwatch_log_group: Use ID as name [GH-2190]
 * resource/aws_elasticsearch_domain: Added ForceNew to vpc_options [GH-2157]
 * resource/aws_redshift_cluster: Make snapshot identifiers `ForceNew` [GH-2212]
+* resource/aws_lb: Disable access logs for network load balancers [GH-2145]


This should be removed as it is made post-merge :)

Ninir · 2017-11-15T13:22:17Z

aws/resource_aws_lb.go

@@ -316,7 +316,8 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {

 	attributes := make([]*elbv2.LoadBalancerAttribute, 0)

-	if d.HasChange("access_logs") {
+	// Access Logs are not supported for Network Loadbalancers
+	if d.Get("load_balancer_type").(string) != "network" && d.HasChange("access_logs") {


I'm not feeling comfortable in allowing a terraform apply when the code will just ignore it. I do think we should add a guard when planning/applying, saying that this is not allowed rather than ignoring it, which may people think that logs are configured whereas it is not.

Thoughts? 😄

I do agree, but preventing apply entirely might cause some issues as well. For my use case, see: https://github.com/terraform-aws-modules/terraform-aws-alb/blob/master/main.tf#L15. In certain modules it wouldn't be possible to create network load balancers this way.
But I'm a bit of a TF noob. Is there a way to do a check within the module?

Otherwise we could check against the access_logs.enabled parameter.

What do you think?

We could also add a log.Printf("[WARN]"), but I'm not sure what the preferred method is for handling these cases.

@Ninir Any thoughts?

Let's go for an INFO notice here :)

@Ninir Done!

Ninir · 2017-11-21T15:53:12Z

aws/resource_aws_lb_test.go

+  enable_deletion_protection = false
+
+  access_logs {
+    bucket = "${aws_s3_bucket.logs.bucket}"


This line and the one after will trigger issues:

no such var.bucket_prefix variable

no such logs bucket

Could you have a look? otherwise looks good to me!

gmccue · 2017-12-02T13:48:18Z

@Ninir I'm a bit stuck here. The problem is that the new acceptance test to check that access logs are disabled fails:

=== RUN   TestAccAWSLB_networkLoadbalancerAccessLogsDisabled
--- FAIL: TestAccAWSLB_networkLoadbalancerAccessLogsDisabled (233.30s)
	testing.go:503: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: aws_lb.lb_test
		  access_logs.0.enabled: "" => "true"
		  access_logs.0.prefix:  "" => "test_bucket"

As far as I can tell, this is because the test setup does a diff against two different states. Because we are passing an access_log attribute, one of the states attempts to create the access logs, while the other does not.
I guess the simple fix for this would just be not to create a new acceptance test for this case, like is currently done for the idle_timeout parameter (doesn't have a unique acceptance test, but does skip creation if the load balancer type is network). Any thoughts?

gmccue · 2017-12-06T18:51:10Z

@Ninir any thoughts?

gmccue · 2018-01-03T17:50:59Z

@Ninir @radeksimko Sorry to bother again with this, but do you have any more guidance or input for this PR? Or should I just close it?

gmccue · 2018-02-09T15:37:27Z

@bflad @radeksimko is this still relevant, or should I go ahead and close this PR?

bflad · 2018-02-28T17:34:00Z

Hi @gmccue! 👋 Sorry that we've been slow to re-review this PR. I can't speak for the other maintainers (and would prefer they continued their review unless they don't have time) but it might be beneficial to migrate some of the logic into the CustomizeDiff function that was recently updated to handle plan-time validation of stickiness for ALB vs NLB in #2746. Without diving too much into your actual code changes in this PR, I would imagine we'll need similar handling to allow the access_logs configuration for NLBs as long as its disabled to handle Terraform modules easier. If you're already doing that then you're probably already on the right track. 👍

Admittedly, the handling of ALB vs NLB in these resources has proven to be quite onerous and generated a ton of issues. To better remedy the situation, we have talked internally about potentially splitting the resources into aws_alb*/aws_nlb*, but there are currently no official plans or timelines for doing that.

* upstream/master: (1043 commits) Fix ordering for #3537 Update CHANGELOG for #3537 duplicate preferred_backup_window attribute Update CHANGELOG for #3597 Create validateRFC3339TimeString ValidateFunc instead of inline resource/aws_ssm_activation: Prevent crash with expiration_date Update directory_service_directory.html.markdown More consistent use of LB vs ALB. Add test with progressive update of NLB Update CHANGELOG.md New Resource: aws_iot_thing deps: Bump [email protected] Update CHANGELOG for #3513 docs/resource/directory_service_directory: Additional examples Adds env check so that acceptance tests pass Fix typo Fix dumb dumb errors Add a positive test Add doc for cross-zone load balancing resource/aws_lb: Add Cross Zone Load Balancing support ...

ghost · 2020-03-30T17:55:54Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

gmccue mentioned this pull request Nov 11, 2017

Support new aws_lb resource? terraform-aws-modules/terraform-aws-alb#12

Closed

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

Ninir reviewed Nov 15, 2017

View reviewed changes

radeksimko added bug Addresses a defect in current functionality. waiting-response Maintainers are waiting on response from community or contributor. labels Nov 15, 2017

Ninir removed the waiting-response Maintainers are waiting on response from community or contributor. label Nov 20, 2017

Disable access logs for network load balancers.

f7caf39

Ninir reviewed Nov 21, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Nov 26, 2017

radeksimko added the service/elbv2 Issues and PRs that pertain to the elbv2 service. label Jan 16, 2018

radeksimko changed the title ~~Disable access_logs for network load balancers.~~ resource/aws_lb: Disable access_logs for network load balancers Jan 16, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 23, 2018

bflad requested a review from Ninir February 28, 2018 17:35

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 3, 2018

brandonjbjelland mentioned this pull request Jun 4, 2018

Extend this module to support NLB terraform-aws-modules/terraform-aws-alb#70

Closed

gmccue closed this Apr 2, 2019

gmccue deleted the nlb-logfiles branch April 2, 2019 09:23

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_lb: Disable access_logs for network load balancers #2256

resource/aws_lb: Disable access_logs for network load balancers #2256

gmccue commented Nov 11, 2017

Ninir left a comment

Ninir Nov 15, 2017

Ninir Nov 15, 2017

gmccue Nov 15, 2017

gmccue Nov 15, 2017

gmccue Nov 17, 2017

Ninir Nov 20, 2017

gmccue Nov 21, 2017

Ninir Nov 21, 2017

gmccue commented Dec 2, 2017

gmccue commented Dec 6, 2017

gmccue commented Jan 3, 2018

gmccue commented Feb 9, 2018

bflad commented Feb 28, 2018

ghost commented Mar 30, 2020

resource/aws_lb: Disable access_logs for network load balancers #2256

resource/aws_lb: Disable access_logs for network load balancers #2256

Conversation

gmccue commented Nov 11, 2017

Ninir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmccue commented Dec 2, 2017

gmccue commented Dec 6, 2017

gmccue commented Jan 3, 2018

gmccue commented Feb 9, 2018

bflad commented Feb 28, 2018

ghost commented Mar 30, 2020