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();
     },