fix(ecs): services essential container exceptions thrown too soon (#1…

…3240) This PR defers the `A TaskDefinition must have at least one essential container` error to the validate phase. Closes #13239 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Feb 26, 2021 · c174f6c · c174f6c
1 parent d331afe
commit c174f6c
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 15 deletions.
diff --git a/packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts b/packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts
@@ -229,9 +229,9 @@ export class Ec2Service extends BaseService implements IEc2Service {
     this.addPlacementConstraints(...props.placementConstraints || []);
     this.addPlacementStrategies(...props.placementStrategies || []);
 
-    if (!this.taskDefinition.defaultContainer) {
-      throw new Error('A TaskDefinition must have at least one essential container');
-    }
+    this.node.addValidation({
+      validate: () => !this.taskDefinition.defaultContainer ? ['A TaskDefinition must have at least one essential container'] : [],
+    });
   }
 
   /**

diff --git a/packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts b/packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts
@@ -172,9 +172,9 @@ export class FargateService extends BaseService implements IFargateService {
 
     this.configureAwsVpcNetworkingWithSecurityGroups(props.cluster.vpc, props.assignPublicIp, props.vpcSubnets, securityGroups);
 
-    if (!props.taskDefinition.defaultContainer) {
-      throw new Error('A TaskDefinition must have at least one essential container');
-    }
+    this.node.addValidation({
+      validate: () => !this.taskDefinition.defaultContainer ? ['A TaskDefinition must have at least one essential container'] : [],
+    });
   }
 }
 

diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts b/packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts
@@ -1,4 +1,4 @@
-import { expect, haveResource } from '@aws-cdk/assert';
+import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as elb from '@aws-cdk/aws-elasticloadbalancing';
 import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
@@ -541,14 +541,48 @@ nodeunitShim({
       const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
       const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');
 
+      // Errors on validation, not on construction.
+      new ecs.Ec2Service(stack, 'Ec2Service', {
+        cluster,
+        taskDefinition,
+      });
+
       // THEN
       test.throws(() => {
-        new ecs.Ec2Service(stack, 'Ec2Service', {
-          cluster,
-          taskDefinition,
-        });
+        expect(stack);
+      }, /one essential container/);
+
+      test.done();
+    },
+
+    'allows adding the default container after creating the service'(test: Test) {
+      // GIVEN
+      const stack = new cdk.Stack();
+      const vpc = new ec2.Vpc(stack, 'MyVpc', {});
+      const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
+      cluster.addCapacity('DefaultAutoScalingGroup', { instanceType: new ec2.InstanceType('t2.micro') });
+      const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');
+
+      new ecs.Ec2Service(stack, 'FargateService', {
+        cluster,
+        taskDefinition,
       });
 
+      // Add the container *after* creating the service
+      taskDefinition.addContainer('main', {
+        image: ecs.ContainerImage.fromRegistry('somecontainer'),
+        memoryReservationMiB: 10,
+      });
+
+      // THEN
+      expect(stack).to(haveResourceLike('AWS::ECS::TaskDefinition', {
+        ContainerDefinitions: [
+          {
+            Name: 'main',
+          },
+        ],
+      }));
+
       test.done();
     },
 

diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts b/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts
@@ -482,14 +482,46 @@ nodeunitShim({
       const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
       const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef');
 
+      // Errors on validation, not on construction.
+      new ecs.FargateService(stack, 'FargateService', {
+        cluster,
+        taskDefinition,
+      });
+
       // THEN
       test.throws(() => {
-        new ecs.FargateService(stack, 'FargateService', {
-          cluster,
-          taskDefinition,
-        });
+        expect(stack);
+      }, /one essential container/);
+
+      test.done();
+    },
+
+    'allows adding the default container after creating the service'(test: Test) {
+      // GIVEN
+      const stack = new cdk.Stack();
+      const vpc = new ec2.Vpc(stack, 'MyVpc', {});
+      const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
+      const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef');
+
+      new ecs.FargateService(stack, 'FargateService', {
+        cluster,
+        taskDefinition,
       });
 
+      // Add the container *after* creating the service
+      taskDefinition.addContainer('main', {
+        image: ecs.ContainerImage.fromRegistry('somecontainer'),
+      });
+
+      // THEN
+      expect(stack).to(haveResourceLike('AWS::ECS::TaskDefinition', {
+        ContainerDefinitions: [
+          {
+            Name: 'main',
+          },
+        ],
+      }));
+
       test.done();
     },