fix(ecs): Removed explicit addition of ECS DeploymentType when circui…

…tBreaker is enabled.
aws · Oct 4, 2022 · 4a1eece · 4a1eece
1 parent 0358d51
commit 4a1eece
Show file tree

Hide file tree

Showing 9 changed files with 92 additions and 28 deletions.
diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/ec2/l3s.test.ts
@@ -1573,9 +1573,6 @@ test('ALB with circuit breaker', () => {
         Rollback: true,
       },
     },
-    DeploymentController: {
-      Type: 'ECS',
-    },
   });
 });
 
@@ -1610,9 +1607,6 @@ test('NLB with circuit breaker', () => {
         Rollback: true,
       },
     },
-    DeploymentController: {
-      Type: 'ECS',
-    },
   });
 });
 

diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/ec2/queue-processing-ecs-service.test.ts
@@ -301,9 +301,6 @@ testDeprecated('test ECS queue worker service construct - with optional props',
     },
     LaunchType: 'EC2',
     ServiceName: 'ecs-test-service',
-    DeploymentController: {
-      Type: 'ECS',
-    },
     PlacementConstraints: [{ Type: 'memberOf', Expression: 'attribute:ecs.instance-type =~ m5a.*' }],
     PlacementStrategies: [{ Field: 'instanceId', Type: 'spread' }, { Field: 'CPU', Type: 'binpack' }, { Type: 'random' }],
   });

diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.alb-fargate-service-https.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.alb-fargate-service-https.ts
@@ -1,5 +1,5 @@
 import { Vpc } from '@aws-cdk/aws-ec2';
-import { Cluster, ContainerImage } from '@aws-cdk/aws-ecs';
+import {Cluster, ContainerImage, DeploymentCircuitBreaker} from '@aws-cdk/aws-ecs';
 import { ApplicationProtocol } from '@aws-cdk/aws-elasticloadbalancingv2';
 import * as route53 from '@aws-cdk/aws-route53';
 import { App, Stack } from '@aws-cdk/core';
@@ -27,6 +27,7 @@ new ApplicationLoadBalancedFargateService(stack, 'myService', {
     zoneName: 'example.com.',
   }),
   redirectHTTP: true,
+  circuitBreaker: { rollback: true }
 });
 
 new integ.IntegTest(app, 'albFargateServiceTest', {

diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/load-balanced-fargate-service.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/load-balanced-fargate-service.test.ts
@@ -451,9 +451,6 @@ test('setting ALB circuitBreaker works', () => {
         Rollback: true,
       },
     },
-    DeploymentController: {
-      Type: 'ECS',
-    },
   });
 });
 
@@ -477,9 +474,6 @@ test('setting NLB circuitBreaker works', () => {
         Rollback: true,
       },
     },
-    DeploymentController: {
-      Type: 'ECS',
-    },
   });
 });
 
@@ -1165,4 +1159,4 @@ test('NetworkLoadBalancedFargateService multiple capacity provider strategies ar
       },
     ]),
   });
-});
+});
diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts
@@ -446,9 +446,6 @@ testDeprecated('test Fargate queue worker service construct - with optional prop
     LaunchType: 'FARGATE',
     ServiceName: 'fargate-test-service',
     PlatformVersion: ecs.FargatePlatformVersion.VERSION1_4,
-    DeploymentController: {
-      Type: 'ECS',
-    },
   });
 
   Template.fromStack(stack).hasResourceProperties('AWS::SQS::Queue', { QueueName: 'fargate-test-sqs-queue' });

diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
@@ -449,9 +449,7 @@ export abstract class BaseService extends Resource
       },
       propagateTags: propagateTagsFromSource === PropagatedTagSource.NONE ? undefined : props.propagateTags,
       enableEcsManagedTags: props.enableECSManagedTags ?? false,
-      deploymentController: props.circuitBreaker ? {
-        type: DeploymentControllerType.ECS,
-      } : props.deploymentController,
+      deploymentController: props.circuitBreaker,
       launchType: launchType,
       enableExecuteCommand: props.enableExecuteCommand,
       capacityProviderStrategy: props.capacityProviderStrategies,
@@ -465,7 +463,9 @@ export abstract class BaseService extends Resource
     if (props.deploymentController?.type === DeploymentControllerType.EXTERNAL) {
       Annotations.of(this).addWarning('taskDefinition and launchType are blanked out when using external deployment controller.');
     }
-
+    if (props.circuitBreaker && props.deploymentController?.type !== DeploymentControllerType.ECS) {
+      Annotations.of(this).addWarning('Deployment circuit breaker requires the ECS deployment controller.');
+    }
     this.serviceArn = this.getResourceArnAttribute(this.resource.ref, {
       service: 'ecs',
       resource: 'service',

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
@@ -1201,6 +1201,36 @@ describe('ec2 service', () => {
 
     });
 
+
+    test('add warning to annotations if circuitBreaker is specified with a non-ECS DeploymentControllerType', () => {
+      // 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');
+
+      taskDefinition.addContainer('web', {
+        image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
+        memoryLimitMiB: 512,
+      });
+
+      const service = new ecs.Ec2Service(stack, 'Ec2Service', {
+        cluster,
+        taskDefinition,
+        deploymentController: {
+          type: DeploymentControllerType.EXTERNAL,
+        },
+        circuitBreaker: { rollback: true },
+      });
+
+      // THEN
+      expect(service.node.metadata[0].data).toEqual('taskDefinition and launchType are blanked out when using external deployment controller.');
+      expect(service.node.metadata[1].data).toEqual('Deployment circuit breaker requires the ECS deployment controller.');
+
+    });
+
+
     test('errors if daemon and desiredCount both specified', () => {
       // GIVEN
       const stack = new cdk.Stack();

diff --git a/packages/@aws-cdk/aws-ecs/test/external/external-service.test.ts b/packages/@aws-cdk/aws-ecs/test/external/external-service.test.ts
@@ -5,7 +5,7 @@ import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
 import * as cloudmap from '@aws-cdk/aws-servicediscovery';
 import * as cdk from '@aws-cdk/core';
 import * as ecs from '../../lib';
-import { LaunchType } from '../../lib/base/base-service';
+import { DeploymentControllerType, LaunchType } from '../../lib/base/base-service';
 import { addDefaultCapacityProvider } from '../util';
 
 describe('external service', () => {
@@ -520,7 +520,34 @@ describe('external service', () => {
       containerPort: 8000,
     })).toThrow('Cloud map service association is not supported for an external service');
 
+
+  });
+
+  test('add warning to annotations if circuitBreaker is specified with a non-ECS DeploymentControllerType', () => {
+    // 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.ExternalTaskDefinition(stack, 'TaskDef');
+
+    taskDefinition.addContainer('web', {
+      image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
+      memoryLimitMiB: 512,
+    });
+
+    const service = new ecs.ExternalService(stack, 'ExternalService', {
+      cluster,
+      taskDefinition,
+      deploymentController: {
+        type: DeploymentControllerType.EXTERNAL,
+      },
+      circuitBreaker: { rollback: true },
+    });
+
     // THEN
+    expect(service.node.metadata[0].data).toEqual('taskDefinition and launchType are blanked out when using external deployment controller.');
+    expect(service.node.metadata[1].data).toEqual('Deployment circuit breaker requires the ECS deployment controller.');
 
   });
 });
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
@@ -657,6 +657,33 @@ describe('fargate service', () => {
       });
     });
 
+    test('add warning to annotations if circuitBreaker is specified with a non-ECS DeploymentControllerType', () => {
+      // 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');
+
+      taskDefinition.addContainer('web', {
+        image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
+      });
+
+      const service = new ecs.FargateService(stack, 'FargateService', {
+        cluster,
+        taskDefinition,
+        deploymentController: {
+          type: DeploymentControllerType.EXTERNAL,
+        },
+        circuitBreaker: { rollback: true },
+      });
+
+      // THEN
+      expect(service.node.metadata[0].data).toEqual('taskDefinition and launchType are blanked out when using external deployment controller.');
+      expect(service.node.metadata[1].data).toEqual('Deployment circuit breaker requires the ECS deployment controller.');
+
+    });
+
+
     test('errors when no container specified on task definition', () => {
       // GIVEN
       const stack = new cdk.Stack();
@@ -2416,9 +2443,6 @@ describe('fargate service', () => {
             Rollback: true,
           },
         },
-        DeploymentController: {
-          Type: ecs.DeploymentControllerType.ECS,
-        },
       });
     });