Always set elb access log bucket prefix.

[jmcarp]

If a user sets the elb access log bucket prefix to a non-empty value,
then sets it to empty, we have to set the bucket prefix in the
ModifyLoadBalancerAttributes request so that AWS sets an empty bucket
prefix rather than ignoring the value.

[Fixes #4361]

cc @Zordrak

[jmcarp]

I think there were two bugs responsible for #4361. First, the provider logic only set the bucket prefix if its value isn't the empty string. Second, terraform core doesn't recognize a diff if a value is the zero value for its type. So to fix this behavior, I had to both modify the provider logic to always set the bucket prefix, as well as creating a null value for the bucket prefix that isn't the empty string. I picked "/", since I think that's a reasonable null value for a path, and stripped leading and trailing slashes from the prefix in the provider so that we don't try to create invalid bucket paths in the load balancer attributes. Does that make sense @bflad ?

[bflad]

as well as creating a null value for the bucket prefix that isn't the empty string

Can you please explain why this was necessary?

[jmcarp]

Looks like I misunderstood something about zero values in terraform. I noticed that setting the access logs bucket prefix to the empty string, or deleting the value entirely, was ignored and didn't lead to a diff in the plan. I thought that that was caused by hashicorp/terraform#5471, but now that I'm looking at this again, I think the issue is that the prefix field is marked as Computed. If I drop the Computed flag on the prefix property, then terraform recognizes a diff when I set the prefix to the empty string. Would it make sense to drop Computed and add a default of empty string?

[bflad]

This needs to be rebased now that the other testing PR got merged 👍

[bflad]

Would it make sense to drop Computed and add a default of empty string?

If I'm understanding everything here, yes please 😄 Generally speaking we should try to avoid Computed where possible. It might have been coded like that in error. You won't need to set Default: "" as that is the TypeString zero value. 👍

[jmcarp]

Cool, I dropped Computed and reverted the unnecessary change to the path zero value. Ready for review!

[bflad]

LGTM -- thanks @jmcarp! 🚀

23 tests passed (all tests)
=== RUN   TestAccAWSELB_HealthCheck
--- PASS: TestAccAWSELB_HealthCheck (10.93s)
=== RUN   TestAccAWSELB_basic
--- PASS: TestAccAWSELB_basic (64.97s)
=== RUN   TestAccAWSELBUpdate_ConnectionDraining
--- PASS: TestAccAWSELBUpdate_ConnectionDraining (22.44s)
=== RUN   TestAccAWSELB_tags
--- PASS: TestAccAWSELB_tags (97.45s)
=== RUN   TestAccAWSELB_generatedName
--- PASS: TestAccAWSELB_generatedName (99.51s)
=== RUN   TestAccAWSELB_SecurityGroups
--- PASS: TestAccAWSELB_SecurityGroups (16.22s)
=== RUN   TestAccAWSELB_AccessLogs_disabled
--- PASS: TestAccAWSELB_AccessLogs_disabled (118.86s)
=== RUN   TestAccAWSELB_namePrefix
--- PASS: TestAccAWSELB_namePrefix (131.67s)
=== RUN   TestAccAWSELBAttachment_drift
--- PASS: TestAccAWSELBAttachment_drift (145.79s)
=== RUN   TestAccAWSELB_fullCharacterRange
--- PASS: TestAccAWSELB_fullCharacterRange (177.99s)
=== RUN   TestAccAWSELB_generatesNameForZeroValue
--- PASS: TestAccAWSELB_generatesNameForZeroValue (190.54s)
=== RUN   TestAccAWSELB_ConnectionDraining
--- PASS: TestAccAWSELB_ConnectionDraining (187.21s)
=== RUN   TestAccAWSELBUpdate_Timeout
--- PASS: TestAccAWSELBUpdate_Timeout (204.40s)
=== RUN   TestAccAWSELB_listener
--- PASS: TestAccAWSELB_listener (209.01s)
=== RUN   TestAccAWSELB_Timeout
--- PASS: TestAccAWSELB_Timeout (213.91s)
=== RUN   TestAccAWSELBAttachment_basic
--- PASS: TestAccAWSELBAttachment_basic (216.98s)
=== RUN   TestAccAWSELBUpdate_HealthCheck
--- PASS: TestAccAWSELBUpdate_HealthCheck (217.34s)
=== RUN   TestAccAWSELB_importBasic
--- PASS: TestAccAWSELB_importBasic (218.65s)
=== RUN   TestAccAWSELB_Listener_SSLCertificateID_IAMServerCertificate
--- PASS: TestAccAWSELB_Listener_SSLCertificateID_IAMServerCertificate (230.60s)
=== RUN   TestAccAWSELB_AccessLogs_enabled
--- PASS: TestAccAWSELB_AccessLogs_enabled (235.74s)
=== RUN   TestAccAWSELB_swap_subnets
--- PASS: TestAccAWSELB_swap_subnets (256.74s)
=== RUN   TestAccAWSELB_InstanceAttaching
--- PASS: TestAccAWSELB_InstanceAttaching (283.55s)
=== RUN   TestAccAWSELB_availabilityZones
--- PASS: TestAccAWSELB_availabilityZones (475.83s)

Test failures unrelated

Tests failed: 3 (3 new), passed: 49
=== RUN   TestAccAWSLBListenerRule_multipleConditionThrowsError
--- PASS: TestAccAWSLBListenerRule_multipleConditionThrowsError (7.97s)
=== RUN   TestAccAWSLBCookieStickinessPolicy_missingLB
--- PASS: TestAccAWSLBCookieStickinessPolicy_missingLB (42.61s)
=== RUN   TestAccAWSLBCookieStickinessPolicy_basic
--- PASS: TestAccAWSLBCookieStickinessPolicy_basic (58.28s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy (129.01s)
=== RUN   TestAccAWSLBSSLNegotiationPolicy_basic
--- PASS: TestAccAWSLBSSLNegotiationPolicy_basic (137.84s)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (85.92s)
=== RUN   TestAccAWSLBTargetGroupBackwardsCompatibility
--- PASS: TestAccAWSLBTargetGroupBackwardsCompatibility (110.51s)
=== RUN   TestAccAWSLBCookieStickinessPolicy_drift
--- PASS: TestAccAWSLBCookieStickinessPolicy_drift (168.74s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (182.09s)
=== RUN   TestAccAWSLBSSLNegotiationPolicy_missingLB
--- PASS: TestAccAWSLBSSLNegotiationPolicy_missingLB (184.32s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (79.04s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (107.72s)
=== RUN   TestAccAWSLBTargetGroupAttachment_withoutPort
--- PASS: TestAccAWSLBTargetGroupAttachment_withoutPort (274.39s)
=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (303.14s)
=== RUN   TestAccAWSLBTargetGroupAttachment_basic
--- PASS: TestAccAWSLBTargetGroupAttachment_basic (336.78s)
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError (1.84s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (162.46s)
=== RUN   TestAccAWSLBListener_https
--- PASS: TestAccAWSLBListener_https (352.60s)
=== RUN   TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (353.44s)
=== RUN   TestAccAWSLBListenerRule_updateRulePriority
--- PASS: TestAccAWSLBListenerRule_updateRulePriority (383.87s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (209.32s)
=== RUN   TestAccAWSLBListener_basic
--- PASS: TestAccAWSLBListener_basic (398.00s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (195.61s)
=== RUN   TestAccAWSLBTargetGroup_defaults_application
--- PASS: TestAccAWSLBTargetGroup_defaults_application (170.02s)
=== RUN   TestAccAWSLBListenerBackwardsCompatibility
--- PASS: TestAccAWSLBListenerBackwardsCompatibility (434.93s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (290.92s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (330.96s)
=== RUN   TestAccAWSLBTargetGroup_defaults_network
--- PASS: TestAccAWSLBTargetGroup_defaults_network (208.41s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (392.27s)
=== RUN   TestAccAWSLBBackwardsCompatibility
--- PASS: TestAccAWSLBBackwardsCompatibility (238.37s)
=== RUN   TestAccAWSLB_networkLoadbalancerBasic
--- FAIL: TestAccAWSLB_networkLoadbalancerBasic (258.94s)
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPDisabled
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPDisabled (310.28s)
=== RUN   TestAccAWSLB_generatedName
--- PASS: TestAccAWSLB_generatedName (277.93s)
=== RUN   TestAccAWSLB_namePrefix
--- PASS: TestAccAWSLB_namePrefix (269.91s)
=== RUN   TestAccAWSLB_updatedIpAddressType
--- FAIL: TestAccAWSLB_updatedIpAddressType (107.58s)
=== RUN   TestAccAWSLBListenerRule_basic
--- PASS: TestAccAWSLBListenerRule_basic (720.24s)
=== RUN   TestAccAWSLB_networkLoadbalancerEIP
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (402.91s)
=== RUN   TestAccAWSLBListenerRule_priority
--- PASS: TestAccAWSLBListenerRule_priority (757.11s)
=== RUN   TestAccAWSLB_tags
--- PASS: TestAccAWSLB_tags (375.35s)
=== RUN   TestAccAWSLBListenerRuleBackwardsCompatibility
--- PASS: TestAccAWSLBListenerRuleBackwardsCompatibility (827.54s)
=== RUN   TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (385.88s)
=== RUN   TestAccAWSLB_noSecurityGroup
--- PASS: TestAccAWSLB_noSecurityGroup (233.59s)
=== RUN   TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew
--- PASS: TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew (834.36s)
=== RUN   TestAccAWSLB_networkLoadbalancer_updateCrossZone
--- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (468.68s)
=== RUN   TestAccAWSLB_updatedSubnets
--- PASS: TestAccAWSLB_updatedSubnets (472.45s)
=== RUN   TestAccAWSLB_networkLoadbalancer_subnet_change
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (348.08s)
=== RUN   TestAccAWSLB_applicationLoadBalancer_updateHttp2
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateHttp2 (535.35s)
=== RUN   TestAccAWSLB_generatesNameForZeroValue
--- FAIL: TestAccAWSLB_generatesNameForZeroValue (578.68s)
=== RUN   TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility
--- PASS: TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility (1010.83s)
=== RUN   TestAccAWSLB_accesslogs
--- PASS: TestAccAWSLB_accesslogs (425.43s)
=== RUN   TestAccAWSLB_basic
--- PASS: TestAccAWSLB_basic (700.09s)
=== RUN   TestAccAWSLB_updatedSecurityGroups
--- PASS: TestAccAWSLB_updatedSecurityGroups (791.45s)

[bflad]

This has been released in version 1.19.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!