feat(ecs): set default health check grace period to 60s (#2942)

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.
aws · Jun 21, 2019 · 0535d36 · 0535d36
1 parent c42523e
commit 0535d36
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 7 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
@@ -779,6 +779,7 @@
  }
  }
  ],
+ "HealthCheckGracePeriodSeconds": 60,
  "NetworkConfiguration": {
  "AwsvpcConfiguration": {
  "AssignPublicIp": "DISABLED",
@@ -1038,4 +1039,4 @@
  "Description": "Artifact hash for asset \"aws-ecs-integ/AdoptEcrRepositorydbc60defc59544bcaa5c28c95d68f62c/Code\""
  }
  }
-}
+}
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
@@ -615,6 +615,7 @@
  },
  "DesiredCount": 1,
  "LaunchType": "FARGATE",
+ "HealthCheckGracePeriodSeconds": 60,
  "LoadBalancers": [
  {
  "ContainerName": "web",
@@ -701,4 +702,4 @@
  }
  }
  }
-}
+}
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
@@ -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
@@ -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, Stack } from '@aws-cdk/cdk';
+import { Construct, Duration, Fn, IResolvable, IResource, Lazy, PhysicalName, Resource, 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 ??? FIXME
+ * @default - defaults to 60 seconds if at least one load balancer is in-use and it is not already set
  */
  readonly healthCheckGracePeriod?: Duration;
 
@@ -150,7 +150,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 }),
@@ -389,6 +389,18 @@ 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?: Duration): IResolvable {
+ return Lazy.anyValue({
+ produce: () => providedHealthCheckGracePeriod !== undefined ? providedHealthCheckGracePeriod.toSeconds() :
+ this.loadBalancers.length > 0 ? 60 :
+ undefined
+ });
+ }
 }
 
 /**

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
@@ -852,6 +852,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
@@ -874,6 +874,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
@@ -598,11 +598,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
@@ -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
@@ -251,6 +251,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();
  },