feat(elasticloadbalancingv2): health check interval greater than timeout by msambol · Pull Request #29075 · aws/aws-cdk

msambol · 2024-02-12T00:04:19Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

msambol · 2024-02-12T00:05:04Z

Exemption request: I'm adding a validation for parameters. Shouldn't need an integration test here.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

msambol · 2024-02-12T00:27:51Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/nlb/network-target-group.ts

      }
-      if (healthCheck.interval && healthCheck.interval.toSeconds() < timeoutSeconds) {
-        ret.push(`Health check timeout '${timeoutSeconds}' must not be greater than the interval '${healthCheck.interval.toSeconds()}'`);
-      }


moving this validation to shared

tmokmss

LGTM overall, with a few minor comment.

tmokmss · 2024-02-25T11:49:30Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/shared/base-target-group.ts

+
+    if (intervalSeconds && timeoutSeconds) {
+      if (intervalSeconds < timeoutSeconds) {
+        ret.push('Health check interval must be greater than the timeout; received interval ' +


must be greater than

shouldn't the validation be intervalSeconds <= timeoutSeconds then? A link to the doc (in PR comment) stating this constraint will help.

added a link to the doc. should be <.

Thanks for the link.

Timeout
This value must be less than the Interval value.

So we shouldn't allow the case when intervalSeconds == timeoutSeconds. Then the condition should be intervalSeconds <= timeoutSeconds

@tmokmss I removed the doc link. That link is actually for Classic Load Balancers. I think you're correct in that it's intervalSeconds <= timeoutSeconds but the old code for NLBs (I moved it to shared above) has <. Thoughts? It doesn't say this anywhere in the docs but if you look at the issue the errors says: Health check interval must be greater than the timeout.

The PR introduced the validation #26031 also described the requirement as below:

Ensure that HealthCheckTimeoutSeconds is not greater than HealthCheckIntervalSeconds.

~~So I guess it was just a mistake?~~ edit) No, the original implementation matches the comment.

Ideally we should try the configuration (interval==timeout) both for ALB and NLB and it emits the same error.

Take a look here: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/nlb/target-group.test.ts#L386-L409. We have a specific test to not throw an error when they're the same. Confusing!

@pahud Is this something you could ask the AWS team about? It would be nice to have this added to the AWS documentation, and then we can decide which way the code needs to be.

There was actually a similar discussion in the PR: #26031 (comment)

So I guess we should keep the validation intervalSeconds < timeoutSeconds, and update the comment to e.g. must be greater than or equal to.

I changed back to < and added a note. Thanks, @tmokmss 👍🏼

tmokmss · 2024-02-25T11:50:52Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/shared/base-target-group.ts

+    const intervalSeconds = this.healthCheck.interval?.toSeconds();
+    const timeoutSeconds = this.healthCheck.timeout?.toSeconds();
+
+    if (intervalSeconds && timeoutSeconds) {


Will this work as expected if either intervalSeconds or timeoutSeconds equals to zero?

HealthCheckTimeoutSeconds has a range of 2-120 and HealthCheckIntervalSeconds is 5-300. We have separate checks on the ranges, so they'll never be 0.

tmokmss

Thanks, LGTM

msambol · 2024-03-06T21:31:53Z

@kaizencc could I have a review here please ?

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

paulhcsun

Code changes look good, just left one comment about adding an additional test for the edge case.

paulhcsun · 2024-03-08T00:48:45Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/nlb/target-group.test.ts

    }).toThrow(/Health check interval '3' not supported. Must be between 5 and 300./);
  });

+  test('Throws error for health check interval less than timeout', () => {


Could you add an edge case test for interval == timeout?

I added a test for this. See discussion above on why we chose to allow them to be equal.

paulhcsun

Nice work @msambol! Thanks for the contribution. And thank you @tmokmss for the review!

mergify · 2024-03-08T17:16:57Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-03-08T17:44:53Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 7e84d68
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-03-08T17:45:17Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions bot added bug This issue is a bug. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Feb 12, 2024

aws-cdk-automation requested a review from a team February 12, 2024 00:04

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 12, 2024

aws-cdk-automation previously requested changes Feb 12, 2024

View reviewed changes

msambol force-pushed the 29062 branch from c9ab299 to 1a08f6b Compare February 12, 2024 00:27

msambol commented Feb 12, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 12, 2024

pahud mentioned this pull request Feb 13, 2024

aws-ecs-patterns: Health Check Interval > Timeout Fails On Deploy #29062

Closed

tmokmss reviewed Feb 25, 2024

View reviewed changes

msambol force-pushed the 29062 branch 2 times, most recently from dffea61 to af8d3e6 Compare February 27, 2024 13:44

tmokmss approved these changes Feb 28, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Feb 28, 2024

github-actions bot mentioned this pull request Mar 1, 2024

Monthly PR metrics report - Feb 2023 #29328

Closed

paulhcsun added pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes labels Mar 8, 2024

paulhcsun reviewed Mar 8, 2024

View reviewed changes

msambol added 7 commits March 8, 2024 09:49

feat(elasticloadbalancingv2): health check interval greater than timeout

604a8ef

cleanup

f4693ff

add doc link

94f9c2f

changer to less than equal to, remove doc link

26276b8

set back to <

bfa2b3d

fix

649dae2

add edge case test

6375b3e

msambol force-pushed the 29062 branch from 3634a38 to 6375b3e Compare March 8, 2024 15:56

Merge branch 'main' into 29062

69b92d4

paulhcsun approved these changes Mar 8, 2024

View reviewed changes

Merge branch 'main' into 29062

7e84d68

mergify bot merged commit 576d034 into aws:main Mar 8, 2024

msambol deleted the 29062 branch March 8, 2024 21:46

Conversation

msambol commented Feb 12, 2024

Uh oh!

msambol commented Feb 12, 2024

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmokmss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msambol Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmokmss Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmokmss left a comment

Choose a reason for hiding this comment

Uh oh!

msambol commented Mar 6, 2024

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 8, 2024

Uh oh!

aws-cdk-automation commented Mar 8, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

msambol Feb 26, 2024 •

edited

Loading

tmokmss Feb 26, 2024 •

edited

Loading