feat(ecs-patterns) add idle timeout to alb fargate and ec2 services by michaeldrey · Pull Request #21261 · aws/aws-cdk

michaeldrey · 2022-07-21T04:00:52Z

This PR exposes the idleTime property for both EC2 and Fargate ALB services. Per the CFN specs, I have set the idleTimeout to default to 60 seconds, and it cannot exceed 4000 seconds.

If no value is provided we set the value to undefined. We do this because:

The default value is set for the user via the CFN. The behavior for the property is to either add a value or leave it undefined and let CFN set the default. See here
Setting a default causes snapshot tests to fail for dependent services as we're adding a new attribute to the cloudformation.

A README entry has been created for this property.

Relates to
#21221
#21266

closes #12913

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-07-21T04:00:58Z

…o alb_idle_timeout

TheRealAmazonKendra · 2022-07-21T22:32:58Z

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

+  /**
+   * The load balancer idle timeout, in seconds
+   *
+   * @default 60


We're not defaulting it to 60 seconds so I don't think this should say 60 here. I'd remove the @default altogether.

Only issue with removing @default is that the linter and spec requires that optional properties must have @default documentation

error: [awslint:props-default-doc:@aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedServiceBaseProps.idleTimeout] Optional property must have @default documentation

What are your thoughts on explaining the behavior if nothing is provided
@default - idle timeout is set to 60 seconds

Ah, forgot about that. Just put that it's undefined. I think it's fine to note that cloudformation will set it to 60 when undefined, but we're not setting anything on their behalf so let's make sure it doesn't look like the default is coming from our code.

TheRealAmazonKendra · 2022-07-21T22:35:44Z

packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s.test.ts

  }).toThrow();
 });

+test('errors when idleTimeout is over 4000 seconds', () => {


Let's add three more test cases here: one that uses a value below 1 and the timeout, one with a valid timeout, and one that doesn't add a timeout.

Working through this, I am able to add the first two tests without issue, but for the third, it appears that idle_timeout.timeout_seconds does not show up as an attribute in the CFN output. Still trying to figure out the best way to test this given that the CFN spec says it's 60 (i've also checked loadbalancers i've created and in the console it says 60)

Take a look at the test below the one you've written. That's how we want to test it against the template instead of using .toBeTruthy(); We know that if it's not showing up in the template, we're not actually passing that value to CloudFormation so it will default back to 60 seconds.

It's fine for it to be undefined in the 4th case, that's what we're looking for. CloudFormation has their own internal logic that takes an undefined value and translates it to 60 seconds. From the integ test, you can see that the 120 seconds you set are showing up in the snapshots, so it looks like it is being added correctly.

AH! Okay I was under the impression that if we didn't add idle_timeout it would still show up in the CFN.

I'll add the other test checking for idle_time to be undefined when it's not added in a little bit and get that pushed up for ya. Really appreciate you taking the time to run through this with me!

TheRealAmazonKendra · 2022-07-21T22:37:42Z

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts


+    if (props.idleTimeout) {
+      if (props.idleTimeout > Duration.seconds(4000)) {
+        throw new Error( 'IdleTime cannot exceed 4000 seconds');


Looks like the minimum is also 1 so we should check against that as well. We don't want to allow for a number below 1 either.

Ah yeah totally agree. Added that guardrail in

…o alb_idle_timeout

Pull request has been modified.

TheRealAmazonKendra · 2022-07-22T16:01:18Z

...ate-service-https-idle-timeout.integ.snapshot/aws-ecs-integ-alb-fg-idletimeout.template.json

+      "Value": "false"
+     },
+     {
+      "Key": "idle_timeout.timeout_seconds",


Shows up here in this test.

TheRealAmazonKendra · 2022-07-22T16:01:27Z

...ws-ecs-patterns/test/fargate/alb-fargate-service-https-idle-timeout.integ.snapshot/tree.json

+                            "value": "false"
+                          },
+                          {
+                            "key": "idle_timeout.timeout_seconds",


And here in this test

TheRealAmazonKendra · 2022-07-22T16:02:28Z

packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.alb-fargate-service-https-idle-timeout.ts

+    zoneName: 'example.com.',
+  }),
+  redirectHTTP: true,
+  idleTimeout: Duration.seconds(120),


Matches the 120 you added here.

TheRealAmazonKendra · 2022-07-22T16:05:59Z

packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s.test.ts

  }).toThrow();
 });

+test('errors when idleTimeout is over 4000 seconds', () => {


Take a look at the test below the one you've written. That's how we want to test it against the template instead of using .toBeTruthy(); We know that if it's not showing up in the template, we're not actually passing that value to CloudFormation so it will default back to 60 seconds.

It's fine for it to be undefined in the 4th case, that's what we're looking for. CloudFormation has their own internal logic that takes an undefined value and translates it to 60 seconds. From the integ test, you can see that the 120 seconds you set are showing up in the snapshots, so it looks like it is being added correctly.

TheRealAmazonKendra

This is VERY close to being ready to go. See my notes about the tests. I also pointed out where you are seeing the output you expect, so you can see where it shows up in the template. Please also take the feedback on this one and apply it to the other PR you have open and then that review will go super quick. We appreciate all your work on this!

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

…alanced-service-base.ts Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Pull request has been modified.

TheRealAmazonKendra

Putting this back in request changes for those test cases. I use that status to know when to come back and re-review.

Pull request has been modified.

TheRealAmazonKendra

@michaeldrey Just a reminder to please add one more test case where we leave idleTimeout as undefined.

It also looks like you'll need to run the integ tests locally with yarn integ --update-on-failed integ.alb-fargate-service-https-idle-timeout.js. It's now failing due to us merging an unrelated change that added the Memory field in your template.

TheRealAmazonKendra · 2022-07-29T17:49:37Z

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

      vpc: this.cluster.vpc,
      loadBalancerName: props.loadBalancerName,
      internetFacing,
+      idleTimeout: props.idleTimeout ?? undefined,


This is redundant. You only need idleTimeout: props.idleTimeout; here.

Pull request has been modified.

michaeldrey · 2022-07-29T19:10:45Z

Hmm i added the extra test and did a rebase on main and now there are extra files in this PR 🤦

michaeldrey · 2022-07-29T19:50:51Z

I'll try to see if i can undo the rebase to clean up the pr

TheRealAmazonKendra · 2022-07-29T23:17:40Z

@Mergifyio update

mergify · 2022-07-29T23:18:22Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2022-07-29T23:19:30Z

I'll try to see if i can undo the rebase to clean up the pr

@michaeldrey looks like mergify has your back here.

TheRealAmazonKendra

Just one last thing.

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

…alanced-service-base.ts Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

michaeldrey · 2022-07-29T23:43:48Z

Ah i missed that. Committed now. I'll begin to make the same updates to the multi ALB PR this weekend

Pull request has been modified.

TheRealAmazonKendra

Thank you for all your work on this!

mergify · 2022-07-30T00:20:10Z

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 · 2022-07-30T01:36:18Z

AWS CodeBuild CI Report

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

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

mergify · 2022-07-30T01:36:40Z

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).

…ices (#21266) This PR exposes the idleTime property for both EC2 and Fargate **multi** ALB services. [Per the CFN specs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-loadbalancer-loadbalancerattributes.html), I have set the idleTimeout to default to 60 seconds, and it cannot exceed 4000 seconds. If no value is provided we set the value to undefined. We do this because: 1. The default value is set for the user via the CFN. The behavior for the property is to either add a value or leave it undefined and let CFN set the default.[ See here](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts#L102) 2. Setting a default causes snapshot tests to fail for dependent services as we're adding a new attribute to the cloudformation. A README entry has been created for this property. Relates to #21221 #21261 closes #12913 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Mike Dreyfus added 2 commits July 20, 2022 20:52

adds idle timeout to alb fargate services

4200acd

added example to readme

b5a54ba

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 21, 2022

aws-cdk-automation requested a review from a team July 21, 2022 04:01

Mike Dreyfus and others added 5 commits July 21, 2022 09:29

adds idle timeout to alb fargate services

89b0101

added example to readme

15f0128

rebased

5665b0d

Merge branch 'alb_idle_timeout' of github.com:michaeldrey/aws-cdk int…

de66f80

…o alb_idle_timeout

Merge branch 'main' into alb_idle_timeout

98b8284

michaeldrey mentioned this pull request Jul 21, 2022

feat(ecs-patterns) add idle timeout to multi alb fargate and ec2 services #21266

Merged

4 tasks

TheRealAmazonKendra previously requested changes Jul 21, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~feat(ecs-patterns) adds idle timeout to alb fargate and ec2 services~~ feat(ecs-patterns) add idle timeout to alb fargate and ec2 services Jul 22, 2022

Mike Dreyfus added 2 commits July 21, 2022 23:05

updated tests and better input checking

d249871

Merge branch 'alb_idle_timeout' of github.com:michaeldrey/aws-cdk int…

924bae2

…o alb_idle_timeout

Mike Dreyfus and others added 4 commits July 21, 2022 23:15

removing unnecessary test

d741c9a

updated property comment

efd6764

Merge branch 'main' into alb_idle_timeout

b479448

error message cleanup

a627199

TheRealAmazonKendra reviewed Jul 22, 2022

View reviewed changes

TheRealAmazonKendra previously requested changes Jul 22, 2022

View reviewed changes

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-b…

38eb72f

…alanced-service-base.ts Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

TheRealAmazonKendra previously requested changes Jul 22, 2022

View reviewed changes

Merge branch 'main' into alb_idle_timeout

c37df6f

TheRealAmazonKendra previously requested changes Jul 29, 2022

View reviewed changes

Mike Dreyfus added 11 commits July 29, 2022 11:35

adds idle timeout to alb fargate services

546581a

added example to readme

f4f3c6a

rebased

f110836

adds idle timeout to alb fargate services

ba522bd

updated tests and better input checking

3294c5c

removing unnecessary test

697a8ef

updated property comment

ab8101e

error message cleanup

fa3725d

added test for undefined

a17dcde

updated tests;added undefined test

4bae13e

resolve merge conflicts

00009b4

Merge branch 'main' into alb_idle_timeout

45d7dc6

TheRealAmazonKendra previously requested changes Jul 29, 2022

View reviewed changes

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-b…

0209b1a

…alanced-service-base.ts Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

TheRealAmazonKendra approved these changes Jul 29, 2022

View reviewed changes

Merge branch 'main' into alb_idle_timeout

6b163ec

mergify bot merged commit 00a3388 into aws:main Jul 30, 2022

emossman mentioned this pull request Aug 11, 2022

aws-cdk-lib.aws_ecs_patterns: unable to set value of idle_timeout to value greater than Duration.seconds(400) #21560

Closed

Conversation

michaeldrey commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

closes #12913

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jul 21, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeldrey commented Jul 29, 2022

Uh oh!

michaeldrey commented Jul 29, 2022

Uh oh!

TheRealAmazonKendra commented Jul 29, 2022

Uh oh!

mergify bot commented Jul 29, 2022

✅ Branch has been successfully updated

Uh oh!

TheRealAmazonKendra commented Jul 29, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaeldrey commented Jul 29, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 30, 2022

Uh oh!

aws-cdk-automation commented Jul 30, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 30, 2022

Uh oh!

Reviewers

Assignees

Labels

michaeldrey commented Jul 21, 2022 •

edited

Loading