resource/aws_lb: Add Cross Zone Load Balancing support for NLBs #3537

mpilar · 2018-02-26T19:59:50Z

Enable Cross Zone load balancing support for NLBs:
Just allows setting the load_balancing.cross_zone.enabled Attribute.
Docs:
https://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_LoadBalancerAttribute.html
https://docs.aws.amazon.com/sdk-for-go/api/service/elbv2/#LoadBalancerAttribute

TF_ACC=1 go test ./aws -v -run=TestAccAWSLB_networkLoadbalancer -timeout 120m 
=== RUN   TestAccAWSLB_networkLoadbalancer
--- PASS: TestAccAWSLB_networkLoadbalancer (245.89s)
=== RUN   TestAccAWSLB_networkLoadbalancerEIP
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (262.47s)
=== RUN   TestAccAWSLB_networkLoadbalancer_subnet_change
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (277.25s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	786.288s```

bflad

Thanks @mpilar, this is looking pretty good! Would you have time to also add a test that enables/disables this flag and ensure it gets set correctly? If not, no problem. 👍

bflad · 2018-03-01T02:56:01Z

aws/resource_aws_lb_test.go

@@ -157,6 +157,7 @@ func TestAccAWSLBBackwardsCompatibility(t *testing.T) {
 					resource.TestCheckResourceAttr("aws_alb.lb_test", "tags.%", "1"),
 					resource.TestCheckResourceAttr("aws_alb.lb_test", "tags.Name", "TestAccAWSALB_basic"),
 					resource.TestCheckResourceAttr("aws_alb.lb_test", "enable_deletion_protection", "false"),
+					resource.TestCheckResourceAttr("aws_alb.lb_test", "enable_cross_zone_load_balancing", "false"),


We should create a test that ensures enabling/disabling works as expected 😄

Any hints as to where I could look for an example? It's my first time digging into a largeish golang codebase.

Is it sufficient to add the directive to testAccAWSLBConfig_networkLoadBalancerEIP as true and assert on that test?

(See added commit)

I was unable to run the acceptance tests after the commit. As soon as I have a break I will.

My general workflow does usually involve adding a directive/parameter to the test configuration function for the new attribute. Generally speaking though, its usually easiest to just create a whole new acceptance test and configuration function so you don't wind up with needing to pass in way too many directives to a function. Copy paste of an existing is okay and generally encouraged for this situation as the testing is more important than any (potential) later refactoring. 👍

mpilar · 2018-03-01T05:29:51Z

@bflad there's a bug I ran into when running the new acceptance test. I will hunt it down.

mpilar · 2018-03-01T07:27:48Z

@bflad
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (305.89s)

I did run into an unrelated (random) issue where the EIP de-allocation failed because of the permission denied (since AWS controls the ENI).

--- FAIL: TestAccAWSLB_networkLoadbalancerEIP (325.03s)
	testing.go:573: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.

		Error: Error applying: 1 error(s) occurred:

		* aws_eip.lb[1] (destroy): 1 error(s) occurred:

		* aws_eip.lb.1: AuthFailure: You do not have permission to access the specified resource.
			status code: 400, request id: 18744029-9e3a-44ae-aca0-1edfaa9f45a3

		State: aws_eip.lb.1:
		  ID = eipalloc-4c905770
		  provider = provider.aws
		  association_id = eipassoc-28867ae5
		  domain = vpc
		  instance =
		  network_interface = eni-e0ef40d7
		  private_ip = 10.10.1.24
		  public_ip = 34.218.56.51
		  tags.% = 0
		  vpc = true

		  Dependencies:
		    data.aws_availability_zones.available
		data.aws_availability_zones.available:
		  ID = 2018-03-01 06:55:43.446792279 +0000 UTC
		  provider = provider.aws
		  names.# = 3
		  names.0 = us-west-2a
		  names.1 = us-west-2b
		  names.2 = us-west-2c

I have tried finding where best to fix this but haven't been able to replicate it. I did capture a debug log of it happening.

bflad · 2018-03-01T13:36:37Z

aws/resource_aws_lb_test.go

@@ -980,6 +984,7 @@ resource "aws_route_table_association" "a" {
 resource "aws_lb" "test" {
  name            = "%s"
  load_balancer_type = "network"
+  enable_cross_zone_load_balancing = true


See my note above -- we'll probably want to just copy paste the handling of the new attribute to a new test and config or at least pass the true/false value for this attributes is a directive into the above config function and use enable_cross_zone_load_balancing = %v 😄

mpilar · 2018-03-01T23:22:45Z

@bflad I made a new test and am running into some odd behavior, I thought I had a hang of the harness and how it works but this is baffling me.
https://gist.github.com/mpilar/dd0d9dcbf828e5b7fd3778fcce95ec5c

I captured a TRACE to see if I can figure out what's going on, but when the test runs it successfully creates the LB then destroys everything and doesn't create the second environment (or as I would prefer, update the env to the second configuration). This happens regardless of whether I make it cross-zone first or second.

I am fairly sure it's some mistake I am making with the test harness but can't figure it out.

e: I obfuscated the requests by removing my AWS key, also went ahead and rotated the key because I am paranoid.

E: I can't believe it took me so long to figure out I had typed "aws_lb.test" and not "aws_lb.lb_test" .

Caught my typo after an embarrasingly long time. I need more sleep in my life. TF_ACC=1 go test ./aws -v -run=TestAccAWSLB_networkLoadbalancer_updateCrossZone -timeout 15m === RUN TestAccAWSLB_networkLoadbalancer_updateCrossZone --- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (346.29s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 347.665s

mpilar · 2018-03-02T01:05:12Z

Flagged as WIP to add HTTP2 as well.

bflad · 2018-03-02T03:09:31Z

I would prefer to treat that functionality in a separate PR please (smaller is easier to review)

mpilar · 2018-03-02T03:20:50Z

@bflad then the PR should be good as is, or do you think it needs more?

The next commit has:

refactored to use a switch statement a la [WIP] Fix 3511 support all elbv2 attributes #3578
added enable_http2 as a flag
added tests for the above
made the boolean flag tests have 3 steps: on, off, on. to test creation, flag and unflag.
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)

It's ready just waiting for all Acceptance tests to finish running (10 down, 5 to go). I decided to run them all after changing the timeouts and making the test fixtures more consistent.

I will split the changes into separate PRs.

appilon · 2018-03-02T19:33:04Z

aws/resource_aws_lb.go

@@ -362,6 +368,14 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
 		})
 	}

+	// It's important to know that Idle timeout is only supported for Network Loadbalancers


Comment references Idle timeout (probably meant to be cross zone flag)

bflad

Thanks @mpilar for your work here! I'm going to pull this in now. You and @appilon can sort out the details of the next PR for HTTP/2 handling (please also fix that code comment). 😉

1 failure is unrelated

Tests failed: 1 (1 new), passed: 48
=== RUN   TestAccAWSLBListenerRule_multipleConditionThrowsError
--- PASS: TestAccAWSLBListenerRule_multipleConditionThrowsError (2.52s)
=== RUN   TestAccAWSLBCookieStickinessPolicy_basic
--- PASS: TestAccAWSLBCookieStickinessPolicy_basic (42.18s)
=== RUN   TestAccAWSLBCookieStickinessPolicy_missingLB
--- PASS: TestAccAWSLBCookieStickinessPolicy_missingLB (52.27s)
=== RUN   TestAccAWSLBCookieStickinessPolicy_drift
--- PASS: TestAccAWSLBCookieStickinessPolicy_drift (56.56s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (84.00s)
=== RUN   TestAccAWSLBSSLNegotiationPolicy_missingLB
--- PASS: TestAccAWSLBSSLNegotiationPolicy_missingLB (84.81s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (46.93s)
=== RUN   TestAccAWSLBSSLNegotiationPolicy_basic
--- PASS: TestAccAWSLBSSLNegotiationPolicy_basic (102.12s)
=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (137.18s)
=== RUN   TestAccAWSLBTargetGroupBackwardsCompatibility
--- PASS: TestAccAWSLBTargetGroupBackwardsCompatibility (138.11s)
=== RUN   TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility
--- PASS: TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility (158.83s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (148.50s)
=== RUN   TestAccAWSLBTargetGroup_defaults_application
--- PASS: TestAccAWSLBTargetGroup_defaults_application (65.30s)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (193.44s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (139.35s)
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError (26.22s)
=== RUN   TestAccAWSLBTargetGroupAttachment_withoutPort
--- PASS: TestAccAWSLBTargetGroupAttachment_withoutPort (290.82s)
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPDisabled
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPDisabled (72.61s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (202.81s)
=== RUN   TestAccAWSLBTargetGroup_defaults_network
--- PASS: TestAccAWSLBTargetGroup_defaults_network (101.25s)
=== RUN   TestAccAWSLBListener_https
--- PASS: TestAccAWSLBListener_https (350.29s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (298.25s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (282.21s)
=== RUN   TestAccAWSLBListener_basic
--- PASS: TestAccAWSLBListener_basic (455.36s)
=== RUN   TestAccAWSLBListenerRuleBackwardsCompatibility
--- PASS: TestAccAWSLBListenerRuleBackwardsCompatibility (476.60s)
=== RUN   TestAccAWSLBListenerBackwardsCompatibility
--- PASS: TestAccAWSLBListenerBackwardsCompatibility (486.55s)
=== RUN   TestAccAWSLB_basic
--- PASS: TestAccAWSLB_basic (249.97s)
=== RUN   TestAccAWSLBTargetGroupAttachment_basic
--- PASS: TestAccAWSLBTargetGroupAttachment_basic (494.86s)
=== RUN   TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew
--- PASS: TestAccAWSLBListenerRule_changeListenerRuleArnForcesNew (501.56s)
=== RUN   TestAccAWSLBListenerRule_basic
--- PASS: TestAccAWSLBListenerRule_basic (504.41s)
=== RUN   TestAccAWSLB_networkLoadbalancerBasic
--- PASS: TestAccAWSLB_networkLoadbalancerBasic (272.63s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (418.42s)
=== RUN   TestAccAWSLB_networkLoadbalancerEIP
--- PASS: TestAccAWSLB_networkLoadbalancerEIP (287.02s)
=== RUN   TestAccAWSLB_generatedName
--- PASS: TestAccAWSLB_generatedName (299.76s)
=== RUN   TestAccAWSLBBackwardsCompatibility
--- PASS: TestAccAWSLBBackwardsCompatibility (318.14s)
=== RUN   TestAccAWSLB_generatesNameForZeroValue
--- PASS: TestAccAWSLB_generatesNameForZeroValue (317.02s)
=== RUN   TestAccAWSLB_namePrefix
--- PASS: TestAccAWSLB_namePrefix (294.52s)
=== RUN   TestAccAWSLBListenerRule_updateRulePriority
--- PASS: TestAccAWSLBListenerRule_updateRulePriority (669.66s)
=== RUN   TestAccAWSLB_tags
--- PASS: TestAccAWSLB_tags (289.96s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (623.73s)
=== RUN   TestAccAWSLBListenerRule_priority
--- PASS: TestAccAWSLBListenerRule_priority (715.72s)
=== RUN   TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (746.18s)
=== RUN   TestAccAWSLB_updatedIpAddressType
--- FAIL: TestAccAWSLB_updatedIpAddressType (266.23s)
=== RUN   TestAccAWSLB_noSecurityGroup
--- PASS: TestAccAWSLB_noSecurityGroup (288.93s)
=== RUN   TestAccAWSLB_networkLoadbalancer_updateCrossZone
--- PASS: TestAccAWSLB_networkLoadbalancer_updateCrossZone (370.61s)
=== RUN   TestAccAWSLB_updatedSecurityGroups
--- PASS: TestAccAWSLB_updatedSecurityGroups (335.40s)
=== RUN   TestAccAWSLB_networkLoadbalancer_subnet_change
--- PASS: TestAccAWSLB_networkLoadbalancer_subnet_change (311.68s)
=== RUN   TestAccAWSLB_updatedSubnets
--- PASS: TestAccAWSLB_updatedSubnets (337.23s)
=== RUN   TestAccAWSLB_accesslogs
--- PASS: TestAccAWSLB_accesslogs (343.91s)

bflad · 2018-03-09T20:00:32Z

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.

* hashicorp/terraform-provider-aws#3537 * https://aws.amazon.com/about-aws/whats-new/2018/02/network-load-balancer-now-supports-cross-zone-load-balancing/

ghost · 2020-04-07T17:33:07Z

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/M Managed by automation to categorize the size of a PR. label Feb 26, 2018

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 Feb 27, 2018

bflad mentioned this pull request Feb 28, 2018

Feature request: cross_zone_load_balancing support for aws_lb #3574

Closed

bflad added this to the v1.11.0 milestone Feb 28, 2018

bflad reviewed Mar 1, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 1, 2018

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 1, 2018

Miguel Pilar added 3 commits March 1, 2018 01:10

resource/aws_lb: Add Cross Zone Load Balancing support

9b8a2e6

Add doc for cross-zone load balancing

e95bda4

Add a positive test

4275243

mpilar force-pushed the cross_zone_nlb branch from 96963f5 to 4275243 Compare March 1, 2018 05:10

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Mar 1, 2018

Fix dumb dumb errors

efe96e4

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

Fix typo

7d0b535

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

bflad mentioned this pull request Mar 1, 2018

[WIP] Fix 3511 support all elbv2 attributes #3578

Closed

2 tasks

bflad reviewed Mar 1, 2018

View reviewed changes

Miguel Pilar added 2 commits March 1, 2018 20:41

More consistent use of LB vs ALB.

7c89b7c

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 2, 2018

mpilar changed the title ~~resource/aws_lb: Add Cross Zone Load Balancing support for NLBs~~ WIP resource/aws_lb: Add Cross Zone Load Balancing support for NLBs Mar 2, 2018

mpilar changed the title ~~WIP resource/aws_lb: Add Cross Zone Load Balancing support for NLBs~~ resource/aws_lb: Add Cross Zone Load Balancing support for NLBs Mar 2, 2018

appilon reviewed Mar 2, 2018

View reviewed changes

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

bflad approved these changes Mar 2, 2018

View reviewed changes

bflad merged commit eae509a into hashicorp:master Mar 2, 2018

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

Update CHANGELOG for #3537

cc48583

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

Fix ordering for #3537

ee256c4

dghubble mentioned this pull request Mar 11, 2018

Enable AWS NLB cross-zone load balancing poseidon/typhoon#159

Merged

mpilar deleted the cross_zone_nlb branch March 12, 2018 21:34

ghost 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: Add Cross Zone Load Balancing support for NLBs #3537

resource/aws_lb: Add Cross Zone Load Balancing support for NLBs #3537

mpilar commented Feb 26, 2018 •

edited by bflad

Loading

bflad left a comment

bflad Mar 1, 2018

mpilar Mar 1, 2018 •

edited

Loading

mpilar Mar 1, 2018

bflad Mar 1, 2018

mpilar commented Mar 1, 2018 •

edited

Loading

mpilar commented Mar 1, 2018

bflad Mar 1, 2018

mpilar commented Mar 1, 2018 •

edited

Loading

mpilar commented Mar 2, 2018

bflad commented Mar 2, 2018

mpilar commented Mar 2, 2018 •

edited

Loading

appilon Mar 2, 2018

bflad left a comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

resource/aws_lb: Add Cross Zone Load Balancing support for NLBs #3537

resource/aws_lb: Add Cross Zone Load Balancing support for NLBs #3537

Conversation

mpilar commented Feb 26, 2018 • edited by bflad Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 1, 2018

Choose a reason for hiding this comment

mpilar Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

mpilar Mar 1, 2018

Choose a reason for hiding this comment

bflad Mar 1, 2018

Choose a reason for hiding this comment

mpilar commented Mar 1, 2018 • edited Loading

mpilar commented Mar 1, 2018

bflad Mar 1, 2018

Choose a reason for hiding this comment

mpilar commented Mar 1, 2018 • edited Loading

mpilar commented Mar 2, 2018

bflad commented Mar 2, 2018

mpilar commented Mar 2, 2018 • edited Loading

appilon Mar 2, 2018

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

mpilar commented Feb 26, 2018 •

edited by bflad

Loading

mpilar Mar 1, 2018 •

edited

Loading

mpilar commented Mar 1, 2018 •

edited

Loading

mpilar commented Mar 1, 2018 •

edited

Loading

mpilar commented Mar 2, 2018 •

edited

Loading