r/lb_target_group: fix compatibility with Network Load Balancers #2251

catsby · 2017-11-10T18:35:01Z

Supersedes #1884

adds a simple error check on setting health_check
add documentation about stickiness and Network LBs
Implement test

Fixes #1912

Test results

TF_ACC=1 go test ./aws -v -run=TestAccAWSLBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (26.49s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (26.17s)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (25.68s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (24.66s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (44.56s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (46.48s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (43.55s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (40.32s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (40.29s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (42.46s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (57.63s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       418.337s

catsby · 2017-11-10T18:37:11Z

Hey @DaveBlooman if you have a minute please take a look here! 5b1b3fc is my additions to your original PR #1884

dblooman · 2017-11-10T20:58:21Z

@catsby I squashed my commits, so your commit is based on my initial commit, not my latest 0d9e713

catsby · 2017-11-10T22:36:05Z

@DaveBlooman ah thanks for pointing that out, I rebased and should be fixed now

catsby · 2017-11-11T00:31:48Z

We had some failing tests due to a typo, I patched that in c93f98a

dblooman · 2017-11-12T20:51:44Z

@catsby LGTM

radeksimko

Just two, rather lower prio comments.
Otherwise LGTM - thanks @DaveBlooman for the original PR.

radeksimko · 2017-11-13T07:59:56Z

aws/resource_aws_lb_target_group.go

-		params.Matcher = &elbv2.Matcher{
-			HttpCode: aws.String(healthCheck["matcher"].(string)),
+
+		if *params.Protocol != "TCP" {


I think it would be a bit more future-proof and cleaner if we checked each argument separately regardless of the protocol. That said it would probably require removing all defaults from these fields and making them computed, which introduces some other potential troubles during updates.

The downside of the current solution is that we're silently ignoring certain arguments without ever telling the user which may backfire if AWS decides to support any of these arguments in the future.

Agreed this isn't ideal, but I'm going to go forward as is. Part of me wonders if the real solution would have been to keep ALB and LB as separate resources entirely 🤔

Agreed on that point, It is a shame to make a breaking change, but as more specific features are added to each type, they will diverge more. Let's see what aws do at re: invent

radeksimko · 2017-11-13T08:02:03Z

aws/resource_aws_lb_target_group.go

+	if *targetGroup.Protocol != "TCP" {
+		healthCheck["path"] = *targetGroup.HealthCheckPath
+		healthCheck["matcher"] = *targetGroup.Matcher.HttpCode
+	}


Nitpick, but we could do something like this

if targetGroup.HealthCheckPath != nil { healthCheck["path"] = *targetGroup.HealthCheckPath } if targetGroup.Matcher.HttpCode != nil { healthCheck["matcher"] = *targetGroup.Matcher.HttpCode }

I know that HTTP health check arguments will never be supported for TCP LB, but it feels a little bit cleaner 😉

👍 applied in 3310363

catsby · 2017-11-13T22:18:19Z

I applied feedback suggested and re-ran tests:

=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (23.06s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (21.93s)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (22.22s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (21.84s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (37.20s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (41.59s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (37.43s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (33.66s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (38.11s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (36.82s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (51.31s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       365.186s

Going to merge this now

dcloud9 · 2017-11-16T14:59:50Z

@catsby - thanks for merging to master, NLB target group TCP proto.
As 1.3.0 is yet to be released, we can make build master branch and move the bin/terraform-provider-aws to .terraform_plugins/ as interim, happy days.

catsby · 2017-11-16T16:11:30Z

@dcloud9 awesome! I believe v1.3.0 should come out soon

AnishAnil · 2017-11-17T09:42:16Z

This is a critical blocker for our atomation. I'm unable to create a Network LB.
What is the ETA to have this fixed

radeksimko · 2017-11-17T10:02:22Z

@AnishAnil this was shipped yesterday in 1.3.0

wjglerum · 2017-11-17T10:09:18Z

Today I got this error message on a deploy:
ValidationError: Health check matcher HTTP code cannot be empty

After some searching I found this in the changelog:
resource/aws_lb_target_group: We no longer provide defaults for health_check's path nor matcher in order to support network load balancers where these arguments aren't valid. Creating new ALB will therefore require you to specify these two arguments. Existing deployments are unaffected. (#2251)

However the documentation still says it is optional and provides a default:
matcher (Optional) The HTTP codes to use when checking for a successful response from a target. Defaults to 200. You can specify multiple values (for example, "200,202") or a range of values (for example, "200-299").

wjglerum · 2017-11-17T11:05:53Z

The same goes for the health_check path which is also not updated in the documentation

radeksimko · 2017-11-17T11:25:09Z

@wjglerum that's tracked under #2327 and #2343 - feel free to subscribe to that issue using the button in the right sidebar.

Sorry for the troubles.

wjglerum · 2017-11-17T12:10:52Z

Thanks, didn't see that issue.

catsby · 2017-11-20T21:05:40Z

#2380 is a follow-up PR to this

Followup to hashicorp#2251, 1.3.0 (November 16, 2017)

ghost · 2020-04-10T17:32:05Z

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!

load balancer fixes

0d9e713

catsby requested a review from radeksimko November 10, 2017 18:40

catsby mentioned this pull request Nov 10, 2017

Load balancer fixes #1884

Closed

r/lb_target_group: fix compatibility with Network Load Balancers

76a3ce9

catsby force-pushed the pr-1884 branch from 5b1b3fc to 76a3ce9 Compare November 10, 2017 22:34

fix typo

c93f98a

radeksimko approved these changes Nov 13, 2017

View reviewed changes

apply feedback

3310363

catsby merged commit 75f7a45 into master Nov 13, 2017

catsby deleted the pr-1884 branch November 13, 2017 22:18

This was referenced Nov 14, 2017

Impossible to create LBs of type network #1838

Closed

Crash during aws_lb_target_group import #2245

Closed

radeksimko mentioned this pull request Nov 16, 2017

t/lb_target_group: Add missing matcher in acc test #2315

Merged

radeksimko mentioned this pull request Nov 16, 2017

r/lb_target_group: Respect threshold & interval for TCP LBs #2323

Merged

wjglerum mentioned this pull request Nov 17, 2017

Incorrect documentation for lb_target_group #2343

Closed

joshuaspence mentioned this pull request Nov 19, 2017

Default timeout for aws_lb_target_group health check isn't valid #2366

Closed

catsby mentioned this pull request Nov 20, 2017

r/aws_lb_target_group: various fixes to behavior based on protocol type #2380

Merged

blaines added a commit to blaines/terraform-provider-aws that referenced this pull request Nov 22, 2017

Updated documentation to reflect ALB requires path and matcher

0be3f1b

Followup to hashicorp#2251, 1.3.0 (November 16, 2017)

blaines mentioned this pull request Nov 22, 2017

Updated documentation to reflect ALB requires path and matcher #2413

Merged

wvidana mentioned this pull request Dec 19, 2017

ALB target group defaults changed for healthy/unhealthy thresholds #2712

Closed

ghost locked and limited conversation to collaborators Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/lb_target_group: fix compatibility with Network Load Balancers #2251

r/lb_target_group: fix compatibility with Network Load Balancers #2251

catsby commented Nov 10, 2017 •

edited

Loading

catsby commented Nov 10, 2017 •

edited

Loading

dblooman commented Nov 10, 2017

catsby commented Nov 10, 2017

catsby commented Nov 11, 2017

dblooman commented Nov 12, 2017

radeksimko left a comment

radeksimko Nov 13, 2017

catsby Nov 13, 2017

dblooman Nov 13, 2017

radeksimko Nov 13, 2017

catsby Nov 13, 2017

catsby commented Nov 13, 2017

dcloud9 commented Nov 16, 2017

catsby commented Nov 16, 2017

AnishAnil commented Nov 17, 2017

radeksimko commented Nov 17, 2017

wjglerum commented Nov 17, 2017 •

edited

Loading

wjglerum commented Nov 17, 2017

radeksimko commented Nov 17, 2017 •

edited

Loading

wjglerum commented Nov 17, 2017

catsby commented Nov 20, 2017

ghost commented Apr 10, 2020

r/lb_target_group: fix compatibility with Network Load Balancers #2251

r/lb_target_group: fix compatibility with Network Load Balancers #2251

Conversation

catsby commented Nov 10, 2017 • edited Loading

catsby commented Nov 10, 2017 • edited Loading

dblooman commented Nov 10, 2017

catsby commented Nov 10, 2017

catsby commented Nov 11, 2017

dblooman commented Nov 12, 2017

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Nov 13, 2017

Choose a reason for hiding this comment

catsby Nov 13, 2017

Choose a reason for hiding this comment

dblooman Nov 13, 2017

Choose a reason for hiding this comment

radeksimko Nov 13, 2017

Choose a reason for hiding this comment

catsby Nov 13, 2017

Choose a reason for hiding this comment

catsby commented Nov 13, 2017

dcloud9 commented Nov 16, 2017

catsby commented Nov 16, 2017

AnishAnil commented Nov 17, 2017

radeksimko commented Nov 17, 2017

wjglerum commented Nov 17, 2017 • edited Loading

wjglerum commented Nov 17, 2017

radeksimko commented Nov 17, 2017 • edited Loading

wjglerum commented Nov 17, 2017

catsby commented Nov 20, 2017

ghost commented Apr 10, 2020

catsby commented Nov 10, 2017 •

edited

Loading

catsby commented Nov 10, 2017 •

edited

Loading

wjglerum commented Nov 17, 2017 •

edited

Loading

radeksimko commented Nov 17, 2017 •

edited

Loading