fix: ECS service replacement regression (#22978)

The behavior introduced in #22467 was too eager, changing the no-feature flag behavior for all services, not just services with a circuit breaker. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Nov 18, 2022 · 0292d3f · 0292d3f
1 parent ba4896a
commit 0292d3f
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 8 deletions.
diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
@@ -532,9 +532,16 @@ export abstract class BaseService extends Resource
     const disableCircuitBreakerEcsDeploymentControllerFeatureFlag =
         FeatureFlags.of(this).isEnabled(cxapi.ECS_DISABLE_EXPLICIT_DEPLOYMENT_CONTROLLER_FOR_CIRCUIT_BREAKER);
 
-    return disableCircuitBreakerEcsDeploymentControllerFeatureFlag ? undefined : {
-      type: DeploymentControllerType.ECS,
-    };
+    if (!disableCircuitBreakerEcsDeploymentControllerFeatureFlag && props.circuitBreaker) {
+      // This is undesirable behavior (the controller is implicitly ECS anyway when left
+      // undefined, so specifying it is not necessary but DOES trigger a CFN replacement)
+      // but we leave it in for backwards compat.
+      return {
+        type: DeploymentControllerType.ECS,
+      };
+    }
+
+    return undefined;
   }
 
   private executeCommandLogConfiguration() {

diff --git a/packages/@aws-cdk/aws-ecs/test/base-service.test.ts b/packages/@aws-cdk/aws-ecs/test/base-service.test.ts
@@ -1,13 +1,16 @@
+import { Template, Match } from '@aws-cdk/assertions';
+import * as ec2 from '@aws-cdk/aws-ec2';
 import * as cdk from '@aws-cdk/core';
+import { App, Stack } from '@aws-cdk/core';
 import * as ecs from '../lib';
 
-let stack: cdk.Stack;
+describe('When import an ECS Service', () => {
+  let stack: cdk.Stack;
 
-beforeEach(() => {
-  stack = new cdk.Stack();
-});
+  beforeEach(() => {
+    stack = new cdk.Stack();
+  });
 
-describe('When import an ECS Service', () => {
   test('with serviceArnWithCluster', () => {
     // GIVEN
     const clusterName = 'cluster-name';
@@ -42,3 +45,40 @@ describe('When import an ECS Service', () => {
     }).toThrowError(/is not using the ARN cluster format/);
   });
 });
+
+test.each([
+  /* breaker, flag => controller in template */
+  /* Flag off => value present if circuitbreaker */
+  [false, false, false],
+  [true, false, true],
+  /* Flag on => value never present */
+  [false, true, false],
+  [true, true, false],
+])('circuitbreaker is %p /\\ flag is %p => DeploymentController in output: %p', (circuitBreaker, flagValue, controllerInTemplate) => {
+  // GIVEN
+  const app = new App({
+    context: {
+      '@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker': flagValue,
+    },
+  });
+  const stack = new Stack(app, 'Stack');
+  const vpc = new ec2.Vpc(stack, 'Vpc');
+  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'),
+  });
+
+  // WHEN
+  new ecs.FargateService(stack, 'FargateService', {
+    cluster,
+    taskDefinition,
+    circuitBreaker: circuitBreaker ? { } : undefined,
+  });
+
+  // THEN
+  const template = Template.fromStack(stack);
+  template.hasResourceProperties('AWS::ECS::Service', {
+    DeploymentController: controllerInTemplate ? { Type: 'ECS' } : Match.absent(),
+  });
+});