Add stickiness support for NLB target_groups #15295

madpipeline · 2020-09-23T06:03:31Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

Allow `stickiness` to be set for NLB `aws_lb_target_group` resources. 
Allow `source_ip` as only valid `stickiness` `type` for `aws_lb_target_group` resource.

This PR is meant to update the #13762, that has a minor syntax issue and no response from the author.

References

YakDriver

@madpipeline This looks great! Thank you for your work on this. Acceptance tests are passing. There are only two things we need before we can merge: a documentation update (website/docs/r/lb_target_group.html.markdown) and a test for the new value (aws/resource_aws_lb_target_group_test.go). Would you be willing to add these?

--- PASS: TestLBTargetGroupCloudwatchSuffixFromARN (0.54s)
--- PASS: TestAccAWSLBTargetGroup_withoutHealthcheck (49.26s)
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tls (55.92s)
--- PASS: TestAccAWSLBTargetGroup_BackwardsCompatibility (64.22s)
--- PASS: TestAccAWSLBTargetGroup_basicUdp (64.93s)
--- PASS: TestAccAWSLBTargetGroup_namePrefix (64.95s)
--- PASS: TestAccAWSLBTargetGroup_generatedName (67.61s)
--- PASS: TestAccAWSLBTargetGroup_basic (81.96s)
--- PASS: TestAccAWSLBTargetGroupAttachment_lambda (83.06s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy (113.30s)
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (120.44s)
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (123.12s)
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError (11.54s)
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (126.08s)
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (126.42s)
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tcp_HealthCheck_Protocol (127.86s)
--- PASS: TestAccAWSLBTargetGroupAttachment_BackwardsCompatibility (129.86s)
--- PASS: TestAccAWSLBTargetGroup_defaults_application (63.61s)
--- PASS: TestAccAWSLBTargetGroupAttachment_basic (133.31s)
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPDisabled (52.81s)
--- PASS: TestAccAWSLBTargetGroupAttachment_Port (140.71s)
--- PASS: TestAccAWSLBTargetGroupAttachment_disappears (143.58s)
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (97.31s)
--- PASS: TestAccAWSLBTargetGroup_defaults_network (65.93s)
--- PASS: TestAccAWSLBTargetGroup_enableHealthCheck (85.06s)
--- PASS: TestAccAWSLBTargetGroupAttachment_ipAddress (150.13s)
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (87.33s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (152.79s)
--- PASS: TestAccAWSLBTargetGroup_tags (105.88s)
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (99.57s)

madpipeline · 2020-10-01T18:51:56Z

Hey @YakDriver. Sure. I can handle the docs, but I'm not sure how to add the test, since I see right now there are no UDP tests, and I'd like to keep your tests pattern. Any advice on how to add that test? I could use a hand with it.

YakDriver · 2020-10-01T23:21:52Z

No problem. The 10,000 ft overview is that there's a basic test that should test everything possible. Each additional test should only test specifically what it's about since the basic test has the bases covered. Here's a short overview of acceptance testing.

For context, the basic test for aws_lb_target_group is here: https://github.com/terraform-providers/terraform-provider-aws/blob/baf451417714e7e3498b5468a7bd39727c647ca5/aws/resource_aws_lb_target_group_test.go#L94

You want to create a test and the config/HCL that the test will need. This will involve adding probably two functions: the test and a function that returns a big string with the config. You can use this test as a template: https://github.com/terraform-providers/terraform-provider-aws/blob/baf451417714e7e3498b5468a7bd39727c647ca5/aws/resource_aws_lb_target_group_test.go#L967-L989. The config for that test is here: https://github.com/terraform-providers/terraform-provider-aws/blob/baf451417714e7e3498b5468a7bd39727c647ca5/aws/resource_aws_lb_target_group_test.go#L1926-L1948

Basically, copy/paste those functions, rename them, change the config, make sure the test is testing for what you've added, and Bob's your uncle.

Here are more in-depth docs and we're always here to help!

madpipeline · 2020-10-05T22:15:48Z

It seems that the changes are not so easy. Stickiness is now allowed with NLBs acording to the docs. So I'll make some more changes to alter the TCP behaviour regarding stickiness.

Not sure yet if embarking on this change wagon is a good idea or not.

madpipeline · 2020-10-06T12:31:37Z

I have a WIP for the tests, but it's not clean yet, so I'm not ready to commit them.

YakDriver · 2020-10-07T14:48:23Z

@madpipeline Excellent! Thanks for continuing on with this. I recognize this is probably not the only thing you've got going on so I appreciate your willingness to continue and keeping us informed about your status. If you get stuck or want to bounce anything off me, please feel free. (I add the "waiting response" label to help organize things on my end and not to put any pressure on you!)

madpipeline · 2020-10-07T19:20:28Z

Tests seem ok. However, I can't find in the CI logs the per test output you've sent in a previous message. I wanted to see the new tests I've added in that list.

Please review and let me know if I need to change anything, or you disagree with my approach.

YakDriver · 2020-10-07T20:26:02Z

@madpipeline My first impression is that this is looking great! Thanks for your work on it. I have some quick questions and will take a closer look shortly.

I notice that you've added a stickiness block to many of the tests that didn't have them before. Is this necessary? If they didn't need one before, it seems like they should not need one now. If you're adding them simply to test the functionality, adding the block to one existing test, in addition to your new tests, should be sufficient.
~~Does the API have enums for the lb_cookie and source_ip literals? If so, we should use those.~~ No

madpipeline · 2020-10-08T05:46:27Z

@YakDriver I've added those before I've figured out how to set the default type for NLB TG. They are not needed. I've removed them.

I wanted to use API enums for the type literals, but didn't find any. I've considered creating my own consts but they are not used that many times in each file, so I gave up on that idea.

YakDriver

@madpipeline Thank you for all your help with this! I made a few changes in order to get it in. I share with you a couple of lessons from this:

The creation process usually involves 3 parts: Create, Update, Read. In order to reduce duplication in the code, generally Create does a very basic create of a skeleton resource. The Create function only needs to check for things that would prevent the creation of the "skeleton". Then, the creation process goes to Update where everything is modified to look like the config. Most of the work is done in Update. Finally, the last part of the creation process is Read. This basically checks to make sure the config, state, and infrastructure are the same. If not, you get a non-empty plan.
Generally you want to avoid breaking changes unless there's a very good reason. For example, there is a default cookie_duration. This could cause problems because the AWS API will give an error if you send a cookie_duration for the source_ip type. Then there's another problem because if the config says 86400, which is what happens when there is a default and no cookie_duration, but AWS says there is no cookie_duration, the two don't match and you end up with non-empty plans. But, getting rid of the default could break people's configs if they rely on it. Thus, I kept the default but added a DiffSuppressFunc to avoid the default causing problems.
If possible, let the AWS API do the error checking. It does take longer to wait for errors to come back but it is more sustainable. The API changes a lot and if we duplicate the error checking, we'll have to change a lot too. Here, instead of checking for problems with stickiness types and protocols, I let the documentation and the AWS API take care of it. I was able to remove all the error checking for that.

Let me know if you have any questions! Thank you for all your work on this. Great job! 💯

YakDriver · 2020-10-09T01:36:30Z

The output from acceptance tests (Commercial):

--- PASS: TestAccAWSLBTargetGroup_withoutHealthcheck (19.98s)
--- PASS: TestAccAWSLBTargetGroup_stickinessDefaultALB (35.46s)
--- PASS: TestAccAWSLBTargetGroup_namePrefix (35.54s)
--- PASS: TestAccAWSLBTargetGroup_defaults_network (43.91s)
--- PASS: TestAccAWSLBTargetGroup_BackwardsCompatibility (45.35s)
--- PASS: TestAccAWSLBTargetGroup_defaults_application (47.32s)
--- PASS: TestAccAWSLBTargetGroup_generatedName (48.84s)
--- PASS: TestAccAWSLBTargetGroup_stickinessInvalidALB (54.16s)
--- PASS: TestAccAWSLBTargetGroup_stickinessInvalidNLB (57.75s)
--- PASS: TestAccAWSLBTargetGroup_basic (59.56s)
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tls (26.88s)
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (65.38s)
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (66.73s)
--- PASS: TestAccAWSLBTargetGroup_stickinessValidALB (66.98s)
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (69.98s)
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (70.97s)
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (74.88s)
--- PASS: TestAccAWSLBTargetGroup_basicUdp (30.61s)
--- PASS: TestAccAWSLBTargetGroup_enableHealthCheck (32.60s)
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tcp_HealthCheck_Protocol (66.01s)
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (91.06s)
--- PASS: TestAccAWSLBTargetGroup_stickinessDefaultNLB (92.93s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy (58.09s)
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (64.01s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (112.27s)
--- PASS: TestAccAWSLBTargetGroup_stickinessValidNLB (114.47s)
--- PASS: TestAccAWSLBTargetGroup_tags (81.13s)

The output from acceptance tests (GovCloud):

--- PASS: TestAccAWSLBTargetGroup_stickinessDefaultALB (39.73s)
--- PASS: TestAccAWSLBTargetGroup_generatedName (39.78s)
--- PASS: TestAccAWSLBTargetGroup_BackwardsCompatibility (45.48s)
--- PASS: TestAccAWSLBTargetGroup_enableHealthCheck (45.79s)
--- PASS: TestAccAWSLBTargetGroup_defaults_network (48.42s)
--- PASS: TestAccAWSLBTargetGroup_basic (53.77s)
--- PASS: TestAccAWSLBTargetGroup_defaults_application (56.01s)
--- PASS: TestAccAWSLBTargetGroup_stickinessInvalidALB (62.96s)
--- PASS: TestAccAWSLBTargetGroup_stickinessInvalidNLB (63.08s)
--- PASS: TestAccAWSLBTargetGroup_withoutHealthcheck (22.01s)
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tls (29.95s)
--- PASS: TestAccAWSLBTargetGroup_stickinessValidALB (72.45s)
--- PASS: TestAccAWSLBTargetGroup_namePrefix (35.46s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy (77.26s)
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (79.60s)
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (80.24s)
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (82.10s)
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tcp_HealthCheck_Protocol (83.67s)
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (84.85s)
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (88.29s)
--- PASS: TestAccAWSLBTargetGroup_basicUdp (35.55s)
--- PASS: TestAccAWSLBTargetGroup_stickinessDefaultNLB (103.41s)
--- PASS: TestAccAWSLBTargetGroup_tags (116.82s)
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (70.59s)
--- PASS: TestAccAWSLBTargetGroup_stickinessValidNLB (133.45s)
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (94.76s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (114.37s)

aws/resource_aws_lb_target_group.go

YakDriver · 2020-10-09T13:57:34Z

I'm guessing that as people use this, other problems will surface. But, that's the nature of small incremental changes made often! Thanks again for your contribution. We look forward to your help again!

ghost · 2020-10-09T16:17:50Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

hashicorp/terraform-provider-aws#15295

YakDriver · 2020-10-09T22:43:07Z

Potentially helpful notes for this resource that I'll leave here:

ALB = HTTP, HTTPS
deregistration_delay.timeout_seconds
stickiness.enabled
stickiness.type = lb_cookie
load_balancing.algorithm.type
slow_start.duration_seconds
stickiness.lb_cookie.duration_seconds

NLB = TCP, UDP, TCP_UDP
deregistration_delay.timeout_seconds
stickiness.enabled
stickiness.type = source_ip
proxy_protocol_v2.enabled

NLB = TLS
deregistration_delay.timeout_seconds
stickiness.enabled = false
stickiness.type = source_ip
proxy_protocol_v2.enabled

YakDriver · 2020-10-13T16:23:33Z

Closes #10494, #12726

madpipeline · 2020-10-13T19:58:05Z

Potentially helpful notes for this resource that I'll leave here:

ALB = HTTP, HTTPS
deregistration_delay.timeout_seconds
stickiness.enabled
stickiness.type = lb_cookie
load_balancing.algorithm.type
slow_start.duration_seconds
stickiness.lb_cookie.duration_seconds

NLB = TCP, UDP, TCP_UDP
deregistration_delay.timeout_seconds
stickiness.enabled
stickiness.type = source_ip
proxy_protocol_v2.enabled

NLB = TLS
deregistration_delay.timeout_seconds
stickiness.enabled = false
stickiness.type = source_ip
proxy_protocol_v2.enabled

These should go in a new issue. It will get buried here.

ghost · 2020-11-08T17:10:01Z

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!

Add source_ip as option for target_group stickiness type

1981bfa

madpipeline requested a review from a team September 23, 2020 06:03

ghost added size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Sep 23, 2020

madpipeline mentioned this pull request Sep 23, 2020

Error: Network Load Balancers do not support Stickiness #9093

Closed

YakDriver self-assigned this Sep 28, 2020

YakDriver added partition/aws-us-gov Pertains to the aws-us-gov partition. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 28, 2020

YakDriver requested changes Sep 29, 2020

View reviewed changes

YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 29, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 1, 2020

YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 1, 2020

Update documentation

55d25b0

ghost added the documentation Introduces or discusses updates to documentation. label Oct 5, 2020

Ovidiu-Florin BOGDAN added 2 commits October 6, 2020 01:11

Clarify stickiness usage in documentation

25276fa

Check proper stickiness usage in create LB TG

cdb3433

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 5, 2020

Add support for stickiness for NLBs

9614849

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Oct 6, 2020

madpipeline changed the title ~~Add source_ip as option for target_group stickiness type~~ Add stickiness support for NLB target_groups Oct 6, 2020

madpipeline marked this pull request as draft October 6, 2020 12:32

dannyleesmith mentioned this pull request Oct 6, 2020

Add toggle to stickiness due to TF/AWS API issues rackspace-infrastructure-automation/aws-terraform-nlb#22

Merged

YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020

Remove unecessary stickiness block from TCP tests

944e420

Ovidiu-Florin BOGDAN and others added 2 commits October 8, 2020 08:51

Restore invalidThreshold test config

70fef2d

Add stickiness fixes

6e6c684

YakDriver approved these changes Oct 9, 2020

View reviewed changes

YakDriver added this to the v3.10.0 milestone Oct 9, 2020

YakDriver merged commit 66d28e8 into hashicorp:master Oct 9, 2020

madpipeline commented Oct 9, 2020

View reviewed changes

aws/resource_aws_lb_target_group.go Outdated Show resolved Hide resolved

aws/resource_aws_lb_target_group.go Show resolved Hide resolved

trobinsonpp added a commit to PatientPing/terraform-aws-bastion that referenced this pull request Oct 9, 2020

NLB target groups only support stickiness setting source_ip

5afebd7

hashicorp/terraform-provider-aws#15295

This was referenced Oct 9, 2020

NLB target groups only support stickiness setting source_ip PatientPing/terraform-aws-bastion#8

Closed

NLB target groups only support stickiness setting source_ip PatientPing/terraform-aws-bastion#9

Merged

trobinsonpp added a commit to PatientPing/terraform-aws-bastion that referenced this pull request Oct 9, 2020

NLB target groups only support stickiness setting source_ip (#9)

6b3cc0c

hashicorp/terraform-provider-aws#15295

madpipeline deleted the patch-1 branch October 9, 2020 22:06

alex1x mentioned this pull request Oct 12, 2020

Stickiness support for source_ip broke valid configurations for NLB #15603

Closed

svanharmelen mentioned this pull request Oct 12, 2020

Update the code to work with the latest AWS provider schubergphilis/terraform-aws-mcaf-fargate#19

Merged

YakDriver mentioned this pull request Oct 12, 2020

r/lb_target_group: Don't allow invalid configurations #15612

Closed

ghost locked as resolved and limited conversation to collaborators Nov 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stickiness support for NLB target_groups #15295

Add stickiness support for NLB target_groups #15295

madpipeline commented Sep 23, 2020 •

edited by YakDriver

Loading

YakDriver left a comment

madpipeline commented Oct 1, 2020

YakDriver commented Oct 1, 2020 •

edited

Loading

madpipeline commented Oct 5, 2020 •

edited

Loading

madpipeline commented Oct 6, 2020

YakDriver commented Oct 7, 2020

madpipeline commented Oct 7, 2020 •

edited

Loading

YakDriver commented Oct 7, 2020 •

edited

Loading

madpipeline commented Oct 8, 2020 •

edited

Loading

YakDriver left a comment •

edited

Loading

YakDriver commented Oct 9, 2020

YakDriver commented Oct 9, 2020

ghost commented Oct 9, 2020

YakDriver commented Oct 9, 2020 •

edited

Loading

YakDriver commented Oct 13, 2020 •

edited

Loading

madpipeline commented Oct 13, 2020

ghost commented Nov 8, 2020

Add stickiness support for NLB target_groups #15295

Add stickiness support for NLB target_groups #15295

Conversation

madpipeline commented Sep 23, 2020 • edited by YakDriver Loading

Community Note

References

YakDriver left a comment

Choose a reason for hiding this comment

madpipeline commented Oct 1, 2020

YakDriver commented Oct 1, 2020 • edited Loading

madpipeline commented Oct 5, 2020 • edited Loading

madpipeline commented Oct 6, 2020

YakDriver commented Oct 7, 2020

madpipeline commented Oct 7, 2020 • edited Loading

YakDriver commented Oct 7, 2020 • edited Loading

madpipeline commented Oct 8, 2020 • edited Loading

YakDriver left a comment • edited Loading

Choose a reason for hiding this comment

YakDriver commented Oct 9, 2020

YakDriver commented Oct 9, 2020

ghost commented Oct 9, 2020

YakDriver commented Oct 9, 2020 • edited Loading

YakDriver commented Oct 13, 2020 • edited Loading

madpipeline commented Oct 13, 2020

ghost commented Nov 8, 2020

madpipeline commented Sep 23, 2020 •

edited by YakDriver

Loading

YakDriver commented Oct 1, 2020 •

edited

Loading

madpipeline commented Oct 5, 2020 •

edited

Loading

madpipeline commented Oct 7, 2020 •

edited

Loading

YakDriver commented Oct 7, 2020 •

edited

Loading

madpipeline commented Oct 8, 2020 •

edited

Loading

YakDriver left a comment •

edited

Loading

YakDriver commented Oct 9, 2020 •

edited

Loading

YakDriver commented Oct 13, 2020 •

edited

Loading