Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Dec 14, 2017

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Nov 20, 2017

This PR includes a number of fixes and adjustments for working with Application and Network Load Balancers. Specifically, some items in #2251 had their defaults removed, others changed.

Here we guard against setting certain health check attributes for network load balancers, et. al, and mark other fields as computed.

The rules are unfortunately complicated:

Fixes / related:


Test results:

TF_ACC=1 go test ./aws -v -run=TestAccAWSLBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (23.48s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (43.68s)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (22.23s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (22.28s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (38.19s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (44.55s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (37.90s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (38.93s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (37.78s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (37.99s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (55.36s)
=== RUN   TestAccAWSLBTargetGroup_defaults_application
--- PASS: TestAccAWSLBTargetGroup_defaults_application (21.98s)
=== RUN   TestAccAWSLBTargetGroup_defaults_network
--- PASS: TestAccAWSLBTargetGroup_defaults_network (26.25s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       450.653s

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLBTargetGroup_'  
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (27.70s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (84.74s)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (19.04s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (22.38s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (34.49s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (38.82s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (34.36s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (33.45s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (33.30s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (33.94s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (47.48s)
=== RUN   TestAccAWSLBTargetGroup_defaults_application
--- PASS: TestAccAWSLBTargetGroup_defaults_application (20.78s)
=== RUN   TestAccAWSLBTargetGroup_defaults_network
--- PASS: TestAccAWSLBTargetGroup_defaults_network (24.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	455.442s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSELB_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSELB_ -timeout 120m
=== RUN   TestAccAWSELB_importBasic
--- PASS: TestAccAWSELB_importBasic (42.61s)
=== RUN   TestAccAWSELB_basic
--- PASS: TestAccAWSELB_basic (37.88s)
=== RUN   TestAccAWSELB_fullCharacterRange
--- PASS: TestAccAWSELB_fullCharacterRange (34.60s)
=== RUN   TestAccAWSELB_AccessLogs_enabled
--- PASS: TestAccAWSELB_AccessLogs_enabled (151.74s)
=== RUN   TestAccAWSELB_AccessLogs_disabled
--- PASS: TestAccAWSELB_AccessLogs_disabled (161.91s)
=== RUN   TestAccAWSELB_namePrefix
--- PASS: TestAccAWSELB_namePrefix (34.22s)
=== RUN   TestAccAWSELB_generatedName
--- PASS: TestAccAWSELB_generatedName (35.90s)
=== RUN   TestAccAWSELB_generatesNameForZeroValue
--- PASS: TestAccAWSELB_generatesNameForZeroValue (34.40s)
=== RUN   TestAccAWSELB_availabilityZones
--- PASS: TestAccAWSELB_availabilityZones (60.47s)
=== RUN   TestAccAWSELB_tags
--- PASS: TestAccAWSELB_tags (61.60s)
=== RUN   TestAccAWSELB_iam_server_cert
--- PASS: TestAccAWSELB_iam_server_cert (49.01s)
=== RUN   TestAccAWSELB_swap_subnets
--- PASS: TestAccAWSELB_swap_subnets (130.52s)
=== RUN   TestAccAWSELB_InstanceAttaching
--- PASS: TestAccAWSELB_InstanceAttaching (186.00s)
=== RUN   TestAccAWSELB_HealthCheck
--- PASS: TestAccAWSELB_HealthCheck (36.92s)
=== RUN   TestAccAWSELB_Timeout
--- PASS: TestAccAWSELB_Timeout (36.34s)
=== RUN   TestAccAWSELB_ConnectionDraining
--- PASS: TestAccAWSELB_ConnectionDraining (32.51s)
=== RUN   TestAccAWSELB_SecurityGroups
--- PASS: TestAccAWSELB_SecurityGroups (68.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1195.144s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSALBTargetGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSALBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (56.86s)
=== RUN   TestAccAWSALBTargetGroup_namePrefix
--- PASS: TestAccAWSALBTargetGroup_namePrefix (52.85s)
=== RUN   TestAccAWSALBTargetGroup_generatedName
--- PASS: TestAccAWSALBTargetGroup_generatedName (63.26s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (92.05s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- FAIL: TestAccAWSALBTargetGroup_changeProtocolForceNew (63.23s)
	testing.go:503: Step 1 error: Error applying: 1 error(s) occurred:
		
		* aws_vpc.test2: 1 error(s) occurred:
		
		* aws_vpc.test2: Error creating VPC: VpcLimitExceeded: The maximum number of VPCs has been reached.
			status code: 400, request id: 23df6f53-e792-41f9-9835-bcc0837a0c2d
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (91.91s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (83.08s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (89.42s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (88.38s)
=== RUN   TestAccAWSALBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSALBTargetGroup_updateSticknessEnabled (124.21s)
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	805.318s
make: *** [testacc] Error 1

(errors are not about those tests)

* `healthy_threshold` - (Optional) The number of consecutive health checks successes required before considering an unhealthy target healthy. Defaults to 5.
* `unhealthy_threshold` - (Optional) The number of consecutive health check failures required before considering the target unhealthy. Defaults to 2.
* `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").
* `matcher` (Optiona, only supported on Application Load Balancers): The HTTP codes to use when checking for a successful response from a target. You can specify multiple values (for example, "200,202") or a range of values (for example, "200-299").
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Optiona/Optional/

@catsby catsby changed the title r/aws_lb_target_group r/aws_lb_target_group: various fixes to behavior based on protocol type Nov 21, 2017
@radeksimko radeksimko added the bug Addresses a defect in current functionality. label Nov 30, 2017
@danstepanov
Copy link

Anything I can do to help resolve the merge conflicts here to get this PR merged?

@igordavid
Copy link

Hello,

Are there any updates for this one?

$ terraform_0.11.1 --version
Terraform v0.11.1

  • provider.aws v1.5.0
  • provider.template v1.0.0

There are still issues when trying to use aws_lb_target_group:

Error:

  • aws_lb_target_group.NLB-TARGET-GROUP-HTTP: Error creating LB Target Group: ValidationError: Health check matcher HTTP code cannot be empty
  • module.mesg_nlb2.aws_lb_target_group.NLB-TARGET-GROUP-HTTP: 1 error(s) occurred:

  • aws_lb_target_group.NLB-TARGET-GROUP-HTTP: Error creating LB Target Group: ValidationError: Health check matcher HTTP code cannot be empty

This is how I configured aws_lb_target_group:

resource "aws_lb_target_group" "NLB-TARGET-GROUP-HTTP" {
  name = "NLB-${var.environment}-TG-MESG-HTTP"
  port     = 80
  protocol = "HTTP"
  vpc_id   = "${var.vpc_id}"
  health_check {
  interval = 10
  path = "/"
  timeout = 5
  healthy_threshold = 3
}
}

@catsby
Copy link
Contributor Author

catsby commented Dec 14, 2017

I've rebased to fix the merge conflict. I'm re-running tests and will update the original post with the results. The conflict was in Docs so if the tests pass again I'll go ahead and merge

@catsby
Copy link
Contributor Author

catsby commented Dec 14, 2017

screen shot 2017-12-14 at 4 20 02 pm

Tests pass, so I'm going to merge this!

@ghost
Copy link

ghost commented Apr 10, 2020

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 ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants