From 3ca772032653abf884709fb3b025d6686f5de61c Mon Sep 17 00:00:00 2001 From: Yenlin Chen <3822365+hencrice@users.noreply.github.com> Date: Wed, 19 Jun 2019 14:12:52 -0700 Subject: [PATCH 1/4] feat(ecs): set the default `HealthCheckGracePeriodSeconds` in base-service to 60 seconds if it's not already set and at least one load balancer is configured This was a FIXME in the ECS CDK codebase. Closes #2936. --- packages/@aws-cdk/aws-ecs/lib/base/base-service.ts | 9 +++++++-- .../aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json | 1 + .../aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json | 1 + .../@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts | 11 +++++++++-- .../test/fargate/integ.lb-awsvpc-nw.expected.json | 1 + .../aws-ecs/test/fargate/test.fargate-service.ts | 6 ++++++ 6 files changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index a7f22e24836f5..76cbe532f3345 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -64,7 +64,7 @@ export interface BaseServiceProps { /** * Time after startup to ignore unhealthy load balancer checks. * - * @default ??? FIXME + * @default - defaults to 60 seconds if not set and at least one load balancer is in-use */ readonly healthCheckGracePeriodSeconds?: number; @@ -151,7 +151,12 @@ export abstract class BaseService extends Resource maximumPercent: props.maximumPercent || 200, minimumHealthyPercent: props.minimumHealthyPercent === undefined ? 50 : props.minimumHealthyPercent }, - healthCheckGracePeriodSeconds: props.healthCheckGracePeriodSeconds, + // only set the grace period when load balancers are configured and + // healthCheckGracePeriodSeconds is not already set + healthCheckGracePeriodSeconds: Lazy.anyValue({ produce: () => + this.loadBalancers + && this.loadBalancers.length + && props.healthCheckGracePeriodSeconds === undefined ? 60 : props.healthCheckGracePeriodSeconds}), /* role: never specified, supplanted by Service Linked Role */ networkConfiguration: Lazy.anyValue({ produce: () => this.networkConfiguration }), serviceRegistries: Lazy.anyValue({ produce: () => this.serviceRegistries }), diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json index 45625b2d4a15e..e9e28754c21ef 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json @@ -844,6 +844,7 @@ "MinimumHealthyPercent": 50 }, "DesiredCount": 1, + "HealthCheckGracePeriodSeconds": 60, "LaunchType": "EC2", "LoadBalancers": [ { diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json index 4316141bd1952..25d53440deb31 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json @@ -866,6 +866,7 @@ "MinimumHealthyPercent": 50 }, "DesiredCount": 1, + "HealthCheckGracePeriodSeconds": 60, "LaunchType": "EC2", "LoadBalancers": [ { diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts b/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts index 891d3592325bc..c229ee0b2a148 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts @@ -48,6 +48,7 @@ export = { test.done(); }, + "errors if daemon and desiredCount both specified"(test: Test) { // GIVEN const stack = new cdk.Stack(); @@ -600,11 +601,17 @@ export = { ContainerPort: 808, LoadBalancerName: { Ref: "LB8A12904C" } } - ], + ] + })); + + expect(stack).to(haveResource('AWS::ECS::Service', { + // if any load balancer is configured and healthCheckGracePeriodSeconds is not + // set, then it should default to 60 seconds. + HealthCheckGracePeriodSeconds: 60 })); test.done(); - }, + } }, 'When enabling service discovery': { diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json index 307aabd37bcd9..4cc54ebaaac04 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json @@ -423,6 +423,7 @@ "MinimumHealthyPercent": 50 }, "DesiredCount": 1, + "HealthCheckGracePeriodSeconds": 60, "LaunchType": "FARGATE", "LoadBalancers": [ { diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts index 16950b85d80ef..98619c1382b5e 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts @@ -254,6 +254,12 @@ export = { } })); + expect(stack).to(haveResource('AWS::ECS::Service', { + // if any load balancer is configured and healthCheckGracePeriodSeconds is not + // set, then it should default to 60 seconds. + HealthCheckGracePeriodSeconds: 60 + })); + test.done(); }, From 633b52744d3d42b532afda208177fc6d6bb07ec8 Mon Sep 17 00:00:00 2001 From: Yenlin Chen <3822365+hencrice@users.noreply.github.com> Date: Thu, 20 Jun 2019 15:04:29 -0700 Subject: [PATCH 2/4] Fixed formatting issue. --- packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts b/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts index c229ee0b2a148..7048cd1c5150b 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts @@ -48,7 +48,6 @@ export = { test.done(); }, - "errors if daemon and desiredCount both specified"(test: Test) { // GIVEN const stack = new cdk.Stack(); From 90723630985b07c166ad4111a84e8fa7d3ab3f2a Mon Sep 17 00:00:00 2001 From: Yenlin Chen <3822365+hencrice@users.noreply.github.com> Date: Thu, 20 Jun 2019 16:05:52 -0700 Subject: [PATCH 3/4] Moved the logic that determines the value of `healthCheckGracePeriodSeconds` into a private helper method as requested --- .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index 7cef4c4ce18a9..55e46e2f86e4c 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -4,7 +4,7 @@ import ec2 = require('@aws-cdk/aws-ec2'); import elbv2 = require('@aws-cdk/aws-elasticloadbalancingv2'); import iam = require('@aws-cdk/aws-iam'); import cloudmap = require('@aws-cdk/aws-servicediscovery'); -import { Construct, Duration, Fn, IResource, Lazy, PhysicalName, Resource, ResourceIdentifiers, Stack } from '@aws-cdk/cdk'; +import { Construct, Duration, Fn, IResolvable, IResource, Lazy, PhysicalName, Resource, ResourceIdentifiers, Stack } from '@aws-cdk/cdk'; import { NetworkMode, TaskDefinition } from '../base/task-definition'; import { ICluster } from '../cluster'; import { CfnService } from '../ecs.generated'; @@ -63,7 +63,7 @@ export interface BaseServiceProps { /** * Time after startup to ignore unhealthy load balancer checks. * - * @default - defaults to 60 seconds if not set and at least one load balancer is in-use + * @default - defaults to 60 seconds if at least one load balancer is in-use and it is not already set */ readonly healthCheckGracePeriod?: Duration; @@ -152,7 +152,7 @@ export abstract class BaseService extends Resource maximumPercent: props.maximumPercent || 200, minimumHealthyPercent: props.minimumHealthyPercent === undefined ? 50 : props.minimumHealthyPercent }, - healthCheckGracePeriodSeconds: props.healthCheckGracePeriod && props.healthCheckGracePeriod.toSeconds(), + healthCheckGracePeriodSeconds: this.evaluateHealthGracePeriod(props.healthCheckGracePeriod), /* role: never specified, supplanted by Service Linked Role */ networkConfiguration: Lazy.anyValue({ produce: () => this.networkConfiguration }), serviceRegistries: Lazy.anyValue({ produce: () => this.serviceRegistries }), @@ -392,6 +392,21 @@ export abstract class BaseService extends Resource return cloudmapService; } + + /** + * Return the default grace period when load balancers are configured and + * healthCheckGracePeriod is not already set + */ + private evaluateHealthGracePeriod(providedHealthCheckGracePeriod: any): IResolvable { + return Lazy.anyValue({ + produce: () => + this.loadBalancers + && this.loadBalancers.length + && providedHealthCheckGracePeriod === undefined ? + 60 : providedHealthCheckGracePeriod + && providedHealthCheckGracePeriod.toSeconds() + }); + } } /** From d0d1ec09fe6685ddf05552c0a5ba0a0239429531 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 21 Jun 2019 12:12:49 +0200 Subject: [PATCH 4/4] Simplify evaluator, update expectations --- .../test/fargate/integ.asset-image.expected.json | 3 ++- .../test/fargate/integ.executionrole.expected.json | 3 ++- .../test/fargate/integ.l3.expected.json | 1 + packages/@aws-cdk/aws-ecs/lib/base/base-service.ts | 11 ++++------- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.asset-image.expected.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.asset-image.expected.json index 509076c8949a5..7a24b26882683 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.asset-image.expected.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.asset-image.expected.json @@ -779,6 +779,7 @@ } } ], + "HealthCheckGracePeriodSeconds": 60, "NetworkConfiguration": { "AwsvpcConfiguration": { "AssignPublicIp": "DISABLED", @@ -1038,4 +1039,4 @@ "Description": "Artifact hash for asset \"aws-ecs-integ/AdoptEcrRepositorydbc60defc59544bcaa5c28c95d68f62c/Code\"" } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.executionrole.expected.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.executionrole.expected.json index 3169c50366974..0fde02d4dfda5 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.executionrole.expected.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.executionrole.expected.json @@ -615,6 +615,7 @@ }, "DesiredCount": 1, "LaunchType": "FARGATE", + "HealthCheckGracePeriodSeconds": 60, "LoadBalancers": [ { "ContainerName": "web", @@ -701,4 +702,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.l3.expected.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.l3.expected.json index eb3d65d3c3cde..1124545dc7763 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.l3.expected.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.l3.expected.json @@ -602,6 +602,7 @@ }, "DesiredCount": 1, "LaunchType": "FARGATE", + "HealthCheckGracePeriodSeconds": 60, "LoadBalancers": [ { "ContainerName": "web", diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index 1614ae6ceb44e..987f8522b3be6 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -397,14 +397,11 @@ export abstract class BaseService extends Resource * Return the default grace period when load balancers are configured and * healthCheckGracePeriod is not already set */ - private evaluateHealthGracePeriod(providedHealthCheckGracePeriod: any): IResolvable { + private evaluateHealthGracePeriod(providedHealthCheckGracePeriod?: Duration): IResolvable { return Lazy.anyValue({ - produce: () => - this.loadBalancers - && this.loadBalancers.length - && providedHealthCheckGracePeriod === undefined ? - 60 : providedHealthCheckGracePeriod - && providedHealthCheckGracePeriod.toSeconds() + produce: () => providedHealthCheckGracePeriod !== undefined ? providedHealthCheckGracePeriod.toSeconds() : + this.loadBalancers.length > 0 ? 60 : + undefined }); } }