resource/aws_lb: enable_http2 flag for LBs and small refactor #3609

mpilar · 2018-03-02T20:58:17Z

Closes: #3511

refactored to use a switch statement a la [WIP] Fix 3511 support all elbv2 attributes #3578
added enable_http2 as a flag with tests and docs
made the boolean flag tests have 3 steps: on, off, on. to test creation, removal and enabling of a running LB
increased wait time to wait for network interfaces to be destroyed after deleting an NLB (this caused the EIP hanging resource I saw yesterday)
made all test fixtures use lb_test as the name for the aws_lb (to prevent the typo issue I had)

Note about the last two: The fail in acceptance tests seemed to happen more with cross-zone enabled, it's not an issue of the acceptance test but the speed at which AWS can successfully destroy the resources related to the NLB, seems that cross-zone makes that destruction slower somewhat.

Test results:

$ TF_ACC=1 go test ./aws -v -run=TestAccAWSLB_ -timeout 120m
=== RUN   TestAccAWSLB_basic
--- PASS: TestAccAWSLB_basic (232.18s)
=== RUN   TestAccAWSLB_networkLoadbalancerBasic
--- PASS: TestAccAWSLB_networkLoadbalancerBasic (264.53s)
=== RUN   TestAccAWSLB_networkLoadbalancerEIP
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (270.10s)
=== RUN   TestAccAWSLB_generatedName
--- PASS: TestAccAWSLB_generatedName (252.17s)
=== RUN   TestAccAWSLB_generatesNameForZeroValue
--- PASS: TestAccAWSLB_generatesNameForZeroValue (232.66s)
=== RUN   TestAccAWSLB_namePrefix
--- PASS: TestAccAWSLB_namePrefix (260.69s)
=== RUN   TestAccAWSLB_tags
--- PASS: TestAccAWSLB_tags (297.39s)
=== RUN   TestAccAWSLB_networkLoadbalancer_updateCrossZone
--- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (370.76s)
=== RUN   TestAccAWSLB_networkLoadbalancer_updateHttp2
--- PASS: TestAccAWSLB_networkLoadbalancer_updateHttp2 (368.44s)
=== RUN   TestAccAWSLB_updatedSecurityGroups
--- PASS: TestAccAWSLB_updatedSecurityGroups (318.00s)
=== RUN   TestAccAWSLB_updatedSubnets
--- PASS: TestAccAWSLB_updatedSubnets (318.37s)
=== RUN   TestAccAWSLB_updatedIpAddressType
--- PASS: TestAccAWSLB_updatedIpAddressType (318.56s)
=== RUN   TestAccAWSLB_noSecurityGroup
--- PASS: TestAccAWSLB_noSecurityGroup (227.11s)
=== RUN   TestAccAWSLB_accesslogs
--- PASS: TestAccAWSLB_accesslogs (441.91s)
=== RUN   TestAccAWSLB_networkLoadbalancer_subnet_change
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (500.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	4673.668s

- refactored to use a switch statement a la hashicorp#3578 - added enable_http2 as a flag with tests - made the boolean flag tests have 3 steps: on, off, on. to test creation, removal and enabling of a running LB - increased timeout and delay for aws_internet_gateway detachment refresh - increased wait time to wait for network interfaces to be destroyed after deleting an NLB (this, combined with the above, caused the EIP hanging resource I saw yesterday) - made all test fixtures use lb_test as the name for the aws_lb (to prevent the typo issue I had)

bflad

Thanks again @mpilar! I left some initial review comments. Can you please take a look and let me know if you have any questions? 👍

bflad · 2018-03-02T21:17:40Z

aws/resource_aws_lb_test.go

@@ -1026,7 +1139,7 @@ resource "aws_lb" "test" {
 }

 resource "aws_eip" "lb" {
-  count = "${length(data.aws_availability_zones.available.names)}"
+  count = "${length(data.aws_availability_zones.available.names)>2?2:length(data.aws_availability_zones.available.names)}"


Are there any regions with only 1 AZ? Seems like this should just be count = 2

bflad · 2018-03-02T21:19:28Z

aws/resource_aws_lb_test.go

@@ -874,6 +919,75 @@ resource "aws_security_group" "alb_test" {
 }`, lbName)
 }

+func testAccAWSLBConfig_basicWithHttp2Toggle(lbName string, http2 bool) string {


Minor nitpick: generally we would just name this testAccAWSLBConfig_enableHttp2

bflad · 2018-03-02T21:24:29Z

aws/resource_aws_lb.go

+				Value: aws.String(fmt.Sprintf("%t", d.Get("enable_cross_zone_load_balancing").(bool))),
+			})
+		}
+	default:


When would we hit this scenario? This currently will never hit. Also, I'm not sure the benefit of postulating if/when there may be new load balancer types and having any sort of implicit support of them. We should force a provider update with acceptance testing.

This was a mistake in my PR (which was coded offline on plane), I forgot Go's switch doesn't fallthrough by default

bflad · 2018-03-02T21:28:54Z

aws/resource_aws_lb.go

+			})
+		}
+	default:
+		if d.HasChange("enable_deletion_protection") {


Given the note above about default:, shouldn't this be a part of both application and network load balancers? It can be moved outside the switch statement. Also, this likely would've been caught in an acceptance test, which looks like it should be added to enable and then disable this (so it can actually be destroyed).

I will add an acceptance test. (and fix the switch statement)

bflad · 2018-03-02T21:30:22Z

aws/resource_aws_internet_gateway.go

+		Timeout:                   15 * time.Minute,
+		Delay:                     20 * time.Second,
+		NotFoundChecks:            30,
+		ContinuousTargetOccurence: 2,


We should probably PR and acceptance test this separately 😄

I will remove it for now.

=== RUN TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection --- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (373.19s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 373.854s

mpilar · 2018-03-02T22:05:26Z

@bflad

=== RUN   TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (373.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	373.854s

After last commit.

mpilar · 2018-03-02T23:45:38Z

@appilon I would love a review from you as well.

appilon · 2018-03-05T20:10:11Z

Looks good to me! (I'm still new to the project and can only really comment on general code/go things)

bflad

LGTM, excellent job! 🚀

16 tests passed (all tests)
=== RUN   TestAccAWSLB_networkLoadbalancerBasic
--- PASS: TestAccAWSLB_networkLoadbalancerBasic (218.94s)
=== RUN   TestAccAWSLB_noSecurityGroup
--- PASS: TestAccAWSLB_noSecurityGroup (230.41s)
=== RUN   TestAccAWSLB_namePrefix
--- PASS: TestAccAWSLB_namePrefix (253.01s)
=== RUN   TestAccAWSLB_networkLoadbalancer_subnet_change
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (332.01s)
=== RUN   TestAccAWSLB_networkLoadbalancerEIP
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (345.82s)
=== RUN   TestAccAWSLB_basic
--- PASS: TestAccAWSLB_basic (349.05s)
=== RUN   TestAccAWSLB_updatedIpAddressType
--- PASS: TestAccAWSLB_updatedIpAddressType (379.98s)
=== RUN   TestAccAWSLB_accesslogs
--- PASS: TestAccAWSLB_accesslogs (381.61s)
=== RUN   TestAccAWSLB_generatesNameForZeroValue
--- PASS: TestAccAWSLB_generatesNameForZeroValue (404.03s)
=== RUN   TestAccAWSLB_generatedName
--- PASS: TestAccAWSLB_generatedName (404.05s)
=== RUN   TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (412.53s)
=== RUN   TestAccAWSLB_updatedSubnets
--- PASS: TestAccAWSLB_updatedSubnets (464.73s)
=== RUN   TestAccAWSLB_applicationLoadBalancer_updateHttp2
--- PASS: TestAccAWSLB_applicationLoadBalancer_updateHttp2 (469.82s)
=== RUN   TestAccAWSLB_tags
--- PASS: TestAccAWSLB_tags (471.29s)
=== RUN   TestAccAWSLB_networkLoadbalancer_updateCrossZone
--- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (493.27s)
=== RUN   TestAccAWSLB_updatedSecurityGroups
--- PASS: TestAccAWSLB_updatedSecurityGroups (581.41s)

bflad · 2018-03-06T02:29:29Z

aws/resource_aws_lb_test.go

+	})
+}
+
+func TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection(t *testing.T) {


Thank you!!!

bflad · 2018-03-09T19:57:25Z

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

ghost · 2020-04-07T17:39:31Z

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!

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

bflad reviewed Mar 2, 2018

View reviewed changes

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Mar 2, 2018

Correct lb type switch statement and add tests

4c4f988

=== RUN TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection --- PASS: TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection (373.19s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 373.854s

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

bflad self-requested a review March 6, 2018 02:35

bflad added this to the v1.11.0 milestone Mar 6, 2018

bflad approved these changes Mar 6, 2018

View reviewed changes

aws/resource_aws_lb_test.go

})

}

func TestAccAWSLB_applicationLoadBalancer_updateDeletionProtection(t *testing.T) {

Copy link

Member

bflad Mar 6, 2018

Choose a reason for hiding this comment

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

Thank you!!!

bflad merged commit ce7e8bc into hashicorp:master Mar 6, 2018

bflad added a commit that referenced this pull request Mar 6, 2018

Update CHANGELOG for #3609

856dde5

mpilar deleted the add_http2 branch March 6, 2018 17:50

hashicorp locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_lb: enable_http2 flag for LBs and small refactor #3609

resource/aws_lb: enable_http2 flag for LBs and small refactor #3609

mpilar commented Mar 2, 2018 •

edited

bflad left a comment

bflad Mar 2, 2018

bflad Mar 2, 2018

bflad Mar 2, 2018

appilon Mar 2, 2018

bflad Mar 2, 2018

mpilar Mar 2, 2018 •

edited

bflad Mar 2, 2018

mpilar Mar 2, 2018

mpilar commented Mar 2, 2018 •

edited

mpilar commented Mar 2, 2018

appilon commented Mar 5, 2018

bflad left a comment

bflad Mar 6, 2018

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

resource/aws_lb: enable_http2 flag for LBs and small refactor #3609

resource/aws_lb: enable_http2 flag for LBs and small refactor #3609

Conversation

mpilar commented Mar 2, 2018 • edited

bflad 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

mpilar Mar 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpilar commented Mar 2, 2018 • edited

mpilar commented Mar 2, 2018

appilon commented Mar 5, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

mpilar commented Mar 2, 2018 •

edited

mpilar Mar 2, 2018 •

edited

mpilar commented Mar 2, 2018 •

edited