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

Add param "enabled" to lb health checks #7570

Closed
wants to merge 1 commit into from
Closed

Add param "enabled" to lb health checks #7570

wants to merge 1 commit into from

Conversation

felixb
Copy link
Contributor

@felixb felixb commented Feb 15, 2019

Fixes #6963

Changes proposed in this pull request:

  • Support enabled flag for lb health checks

Output from acceptance testing:

$ make testacc TESTARGS='-run=-run=TestAccAWSLBTargetGroup_basic'
=== RUN   TestAccAWSLBTargetGroup_basic
=== PAUSE TestAccAWSLBTargetGroup_basic
=== CONT  TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (17.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	17.608s
...

Notes:

The API/CLI defaults for health check enabled are true for ip or instance, false for lambdas.
Specifying the health check block indicates an interest in enabling them. So this code defaults to true in any case.

@ghost ghost added size/XS Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/elbv2 Issues and PRs that pertain to the elbv2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Feb 15, 2019
@felixb
Copy link
Contributor Author

felixb commented Feb 28, 2019

@bflad do you mind reviewing this PR?

@felixb
Copy link
Contributor Author

felixb commented Mar 13, 2019

@bflad @nywilken I just rebased my changes on current master.

It would be really nice to get this merged to finally fix #6963 and get rid of the ugly shell scripts in our pipeline enabling the health checks manually.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

@felixb thanks for the contribution. This is looking good. I left a few questions around the default value that I would like to get your thoughts on. There also appears to be an issue with the default interval health check value which we might need to address in this PR. Please let me know if you have any questions.

website/docs/r/lb_target_group.html.markdown Show resolved Hide resolved
aws/resource_aws_lb_target_group.go Show resolved Hide resolved
aws/resource_aws_lb_target_group_test.go Show resolved Hide resolved
@ghost 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 Mar 14, 2019
@felixb
Copy link
Contributor Author

felixb commented Mar 14, 2019

I mainly added tests and tweaked the docs a little bit.
I also fixed state management with lambda health checks. Previously the plan never got empty because protocol and port could not be set empty by terraform.

$ TF_ACC=1 go test -v -parallel 2 ./... -run TestAccAWSLBTargetGroup_.*
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSLBTargetGroup_basic
=== PAUSE TestAccAWSLBTargetGroup_basic
=== RUN   TestAccAWSLBTargetGroup_withoutHealthcheck
=== PAUSE TestAccAWSLBTargetGroup_withoutHealthcheck
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
=== PAUSE TestAccAWSLBTargetGroup_networkLB_TargetGroup
=== RUN   TestAccAWSLBTargetGroup_Protocol_Tls
=== PAUSE TestAccAWSLBTargetGroup_Protocol_Tls
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy
=== PAUSE TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy
=== RUN   TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck
=== PAUSE TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck
=== RUN   TestAccAWSLBTargetGroup_namePrefix
=== PAUSE TestAccAWSLBTargetGroup_namePrefix
=== RUN   TestAccAWSLBTargetGroup_generatedName
=== PAUSE TestAccAWSLBTargetGroup_generatedName
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
=== PAUSE TestAccAWSLBTargetGroup_changeNameForceNew
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
=== PAUSE TestAccAWSLBTargetGroup_changeProtocolForceNew
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
=== PAUSE TestAccAWSLBTargetGroup_changePortForceNew
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
=== PAUSE TestAccAWSLBTargetGroup_changeVpcForceNew
=== RUN   TestAccAWSLBTargetGroup_tags
=== PAUSE TestAccAWSLBTargetGroup_tags
=== RUN   TestAccAWSLBTargetGroup_enableHealthCheck
=== PAUSE TestAccAWSLBTargetGroup_enableHealthCheck
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
=== PAUSE TestAccAWSLBTargetGroup_updateHealthCheck
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
=== PAUSE TestAccAWSLBTargetGroup_updateSticknessEnabled
=== RUN   TestAccAWSLBTargetGroup_defaults_application
=== PAUSE TestAccAWSLBTargetGroup_defaults_application
=== RUN   TestAccAWSLBTargetGroup_defaults_network
=== PAUSE TestAccAWSLBTargetGroup_defaults_network
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPDisabled
=== PAUSE TestAccAWSLBTargetGroup_stickinessWithTCPDisabled
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError
=== PAUSE TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError
=== CONT  TestAccAWSLBTargetGroup_basic
=== CONT  TestAccAWSLBTargetGroup_defaults_network
--- PASS: TestAccAWSLBTargetGroup_basic (19.78s)
=== CONT  TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError (3.59s)
=== CONT  TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_defaults_network (25.59s)
=== CONT  TestAccAWSLBTargetGroup_defaults_application
--- PASS: TestAccAWSLBTargetGroup_defaults_application (18.91s)
=== CONT  TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (25.71s)
=== CONT  TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (33.72s)
=== CONT  TestAccAWSLBTargetGroup_enableHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (47.61s)
=== CONT  TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_enableHealthCheck (25.86s)
=== CONT  TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_tags (32.45s)
=== CONT  TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (32.78s)
=== CONT  TestAccAWSLBTargetGroup_stickinessWithTCPDisabled
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (33.75s)
=== CONT  TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPDisabled (19.73s)
=== CONT  TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (34.43s)
=== CONT  TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (33.27s)
=== CONT  TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_generatedName (19.25s)
=== CONT  TestAccAWSLBTargetGroup_Protocol_Tls
--- PASS: TestAccAWSLBTargetGroup_namePrefix (18.83s)
=== CONT  TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy
--- PASS: TestAccAWSLBTargetGroup_Protocol_Tls (19.37s)
=== CONT  TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy (32.00s)
=== CONT  TestAccAWSLBTargetGroup_withoutHealthcheck
--- PASS: TestAccAWSLBTargetGroup_withoutHealthcheck (14.98s)
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (44.60s)
PASS
PASS	github.com/terraform-providers/terraform-provider-aws/aws	275.995s

@felixb
Copy link
Contributor Author

felixb commented Mar 20, 2019

@nywilken @bflad Any thoughts on the current changes?

@felixb
Copy link
Contributor Author

felixb commented Mar 28, 2019

@nywilken @bflad could you please review these changes. I'd really like to fix that bug.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Hi @felixb sorry for the slow feedback here. I took at look and the changes and provided a few suggestions to get this merged.

The test look good so thank for adding those in. While testing I found an issue with the import functionality, and the use of the Update function in the Create function, but that is out of scope for this PR. I will open a new issue for making those fixes.

aws/resource_aws_lb_target_group.go Outdated Show resolved Hide resolved
aws/resource_aws_lb_target_group.go Outdated Show resolved Hide resolved
aws/resource_aws_lb_target_group.go Outdated Show resolved Hide resolved
aws/resource_aws_lb_target_group_test.go Outdated Show resolved Hide resolved
@nywilken nywilken added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 4, 2019
@felixb
Copy link
Contributor Author

felixb commented Apr 4, 2019

@nywilken I just checked in the changes you asked for. Tests are still passing.

Do you want me to squash and rebase for more compact history?

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 4, 2019
@nywilken
Copy link
Contributor

nywilken commented Apr 4, 2019

Do you want me to squash and rebase for more compact history?

That would be great! Thanks for making the changes.

@nywilken nywilken added this to the v2.5.0 milestone Apr 4, 2019
@felixb
Copy link
Contributor Author

felixb commented Apr 4, 2019

Do you want me to squash and rebase for more compact history?

That would be great! Thanks for making the changes.

Done. Thanks for merging.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Hi @felixb apologies again for the slow feedback, but thank you for making the requested updates. I'm going to make one change before merging (remove the DiffSuppressFunc form the top protocol attribute as it is not needed). This will be released in version 2.5 slated for later today, or tomorrow.

aws/resource_aws_lb_target_group.go Outdated Show resolved Hide resolved
nywilken added a commit that referenced this pull request Apr 4, 2019
Add param "enabled" to lb health checks
@nywilken
Copy link
Contributor

nywilken commented Apr 4, 2019

This has been merged to master via a manual merge 7f90859. I was hoping GitHub would've picked it up.

@nywilken nywilken closed this Apr 4, 2019
nywilken added a commit that referenced this pull request Apr 4, 2019
@bflad
Copy link
Contributor

bflad commented Apr 5, 2019

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

@ghost
Copy link

ghost commented Mar 30, 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 Mar 30, 2020
@felixb felixb deleted the lb-healthcheck-enabled branch April 3, 2024 13:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/elbv2 Issues and PRs that pertain to the elbv2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to enable health check for lambda target group
3 participants