resource/aws_lb_target_group: Attempt to resolve catch-22 around NLB's stickiness #2954
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The catch-22 is that stickiness is not allowed for NLBs, but stickiness is enabled by default and the check to see if an NLB has enabled stickiness only looks for the existence of the stickiness block. This change, instead, evaluates whether or not that block disables stickiness (which is a reasonable thing to do when told that your NLB doesn’t support stickiness). There may be more principled approaches that make use of breaking changes.
radeksimko
added
the
service/elbv2
radeksimko
changed the title
|
Any idea when this will be merged? |
This is a continuation of 08d7b8c, which modifies the diff validation check to allow for stickiness to be defined on TCP target groups, but requires that this setting be false. While this allows the diff to be applied, this still fails on the AWS end because it still sets the stickiness attributes on target group types that don't support it. This update skips sending the stickiness attribute data to AWS when using a TCP load balancer, in addition to skipping reading it back in when the real-world protocol is TCP. This ensures both that the apply will succeed in AWS, and that spurious diffs aren't caused by ensuring that the state coming from config is not overwritten. Also, tests have been added for both ensuring that enabling stickiness is still blocked, and ensuring that a disabled stickiness block succeeds with an empty post-apply plan. Finally, documentation has been added denoting that you can still declare a stickiness block when TCP is used, but it must be disabled.
|
Hey all, I've updated this PR to ensure it works - before it actually was still failing on the API end because even though the diff was passing validation, the attributes were still being set, which can't happen for TCP target groups period. I will work on getting this reviewed and merged next week. Thanks! |
|
@vancluever I'm seeing spurious diffs on the |
|
Argh! I should have run the rest. Will jump on this this morning @bflad! |
This fixes spurious diffs when someone actually doesn't specify the stickiness sub-resource, which was the only way to not do false before.
|
I'm trying to get a clean test run, it's been tough because there are so many LB tests when running off a more general regexp (ie: |
|
Tests largely passed, save for one timeout and also a bunch of certificate-related errors because the key I was using did not have IAM permissions. The target group tests all pass now. The spurious diff error does not seem to have the |
bflad
approved these changes
Feb 24, 2018
There was a problem hiding this comment.
Thanks so much for this @mattgiles and @vancluever! Let's get this in 👍
bflad
added a commit
that referenced
this pull request
Feb 24, 2018
|
This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #2746
The catch-22 is that stickiness is not allowed for NLBs, stickiness is enabled by default for all LBs, and the check to make sure an NLB has not enabled stickiness only looks for the existence of a stickiness block -- leaving you damned if you define it damned if you don't.
There might be more principled approaches which break current defaults, but it seems natural enough to go to define the stickiness block (to disable stickiness) after being told your LB doesn't support it.