fix(ecs): removed explicit addition of ecs deployment type when circu…

…it breaker is enabled (#22328) Fixes #16126 Copied from #16919 with review comments as it went stale and closed. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Oct 7, 2022 · 635129c · 635129c
1 parent 0d44db2
commit 635129c
Show file tree

Hide file tree

Showing 14 changed files with 96 additions and 40 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/...fargate/alb-fargate-service-https.integ.snapshot/aws-ecs-integ-alb-fg-https.template.json b/...fargate/alb-fargate-service-https.integ.snapshot/aws-ecs-integ-alb-fg-https.template.json
@@ -735,7 +735,11 @@
     },
     "DeploymentConfiguration": {
      "MaximumPercent": 200,
-     "MinimumHealthyPercent": 50
+     "MinimumHealthyPercent": 50,
+     "DeploymentCircuitBreaker": {
+      "Enable": true,
+      "Rollback": true
+     }
     },
     "EnableECSManagedTags": true,
     "EnableExecuteCommand": true,

diff --git a/...-load-balanced-fargate-service.integ.snapshot/aws-ecs-integ-circuit-breaker.template.json b/...-load-balanced-fargate-service.integ.snapshot/aws-ecs-integ-circuit-breaker.template.json
@@ -634,9 +634,6 @@
      "MaximumPercent": 200,
      "MinimumHealthyPercent": 50
     },
-    "DeploymentController": {
-     "Type": "ECS"
-    },
     "EnableECSManagedTags": false,
     "HealthCheckGracePeriodSeconds": 60,
     "LaunchType": "FARGATE",

diff --git a/...terns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/tree.json b/...terns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/tree.json
@@ -1121,9 +1121,6 @@
                             "rollback": true
                           }
                         },
-                        "deploymentController": {
-                          "type": "ECS"
-                        },
                         "enableEcsManagedTags": false,
                         "healthCheckGracePeriodSeconds": 60,
                         "launchType": "FARGATE",

diff --git a/...aker-queue-processing-fargate-service.integ.snapshot/aws-ecs-patterns-queue.template.json b/...aker-queue-processing-fargate-service.integ.snapshot/aws-ecs-patterns-queue.template.json
@@ -621,9 +621,6 @@
      "MaximumPercent": 200,
      "MinimumHealthyPercent": 50
     },
-    "DeploymentController": {
-     "Type": "ECS"
-    },
     "EnableECSManagedTags": false,
     "LaunchType": "FARGATE",
     "NetworkConfiguration": {

diff --git a/...ns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/tree.json b/...ns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/tree.json
@@ -1107,9 +1107,6 @@
                             "rollback": true
                           }
                         },
-                        "deploymentController": {
-                          "type": "ECS"
-                        },
                         "enableEcsManagedTags": false,
                         "launchType": "FARGATE",
                         "networkConfiguration": {

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
@@ -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.deploymentController,
       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).addError('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 });
+      addDefaultCapacityProvider(cluster, stack, vpc);
+      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 });
+    addDefaultCapacityProvider(cluster, stack, vpc);
+    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,
-        },
       });
     });