From 89114fe7d6937752d8f7cd9ddaf8797a259a36b6 Mon Sep 17 00:00:00 2001 From: corymhall <43035978+corymhall@users.noreply.github.com> Date: Fri, 7 Oct 2022 16:00:04 +0000 Subject: [PATCH] Revert "fix(ecs): removed explicit addition of ecs deployment type when circuit breaker is enabled (#22328)" This reverts commit 635129ca95313afef7b3d8fc62d077afbfd0c088. --- .../aws-ecs-patterns/test/ec2/l3s.test.ts | 6 ++++ .../ec2/queue-processing-ecs-service.test.ts | 3 ++ .../aws-ecs-integ-alb-fg-https.template.json | 6 +--- ...ws-ecs-integ-circuit-breaker.template.json | 3 ++ .../tree.json | 3 ++ .../aws-ecs-patterns-queue.template.json | 3 ++ .../tree.json | 3 ++ .../integ.alb-fargate-service-https.ts | 1 - .../load-balanced-fargate-service.test.ts | 8 ++++- .../queue-processing-fargate-service.test.ts | 3 ++ .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 8 ++--- .../aws-ecs/test/ec2/ec2-service.test.ts | 30 ------------------- .../test/external/external-service.test.ts | 29 +----------------- .../test/fargate/fargate-service.test.ts | 30 ++----------------- 14 files changed, 40 insertions(+), 96 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 index 0aefb7078272d..6dbdcf417509d 100644 --- 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,6 +1573,9 @@ test('ALB with circuit breaker', () => { Rollback: true, }, }, + DeploymentController: { + Type: 'ECS', + }, }); }); @@ -1607,6 +1610,9 @@ 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 index d13d4a19517c2..b666e4a277957 100644 --- 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,6 +301,9 @@ 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/alb-fargate-service-https.integ.snapshot/aws-ecs-integ-alb-fg-https.template.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/alb-fargate-service-https.integ.snapshot/aws-ecs-integ-alb-fg-https.template.json index d09617d4adbae..48d21c5db4a7d 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/alb-fargate-service-https.integ.snapshot/aws-ecs-integ-alb-fg-https.template.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/alb-fargate-service-https.integ.snapshot/aws-ecs-integ-alb-fg-https.template.json @@ -741,11 +741,7 @@ }, "DeploymentConfiguration": { "MaximumPercent": 200, - "MinimumHealthyPercent": 50, - "DeploymentCircuitBreaker": { - "Enable": true, - "Rollback": true - } + "MinimumHealthyPercent": 50 }, "EnableECSManagedTags": true, "EnableExecuteCommand": true, diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/aws-ecs-integ-circuit-breaker.template.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/aws-ecs-integ-circuit-breaker.template.json index f073ac4a7fd48..a79289992941d 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/aws-ecs-integ-circuit-breaker.template.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/aws-ecs-integ-circuit-breaker.template.json @@ -634,6 +634,9 @@ "MaximumPercent": 200, "MinimumHealthyPercent": 50 }, + "DeploymentController": { + "Type": "ECS" + }, "EnableECSManagedTags": false, "HealthCheckGracePeriodSeconds": 60, "LaunchType": "FARGATE", diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/tree.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/tree.json index 2a495b91ea9b2..1f0c3ca8e98da 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/tree.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-load-balanced-fargate-service.integ.snapshot/tree.json @@ -1121,6 +1121,9 @@ "rollback": true } }, + "deploymentController": { + "type": "ECS" + }, "enableEcsManagedTags": false, "healthCheckGracePeriodSeconds": 60, "launchType": "FARGATE", diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/aws-ecs-patterns-queue.template.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/aws-ecs-patterns-queue.template.json index 957cbeabede24..5042c4c50d813 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/aws-ecs-patterns-queue.template.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/aws-ecs-patterns-queue.template.json @@ -621,6 +621,9 @@ "MaximumPercent": 200, "MinimumHealthyPercent": 50 }, + "DeploymentController": { + "Type": "ECS" + }, "EnableECSManagedTags": false, "LaunchType": "FARGATE", "NetworkConfiguration": { diff --git a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/tree.json b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/tree.json index ccb01dc24a48b..fe6ace0b02e7c 100644 --- a/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/tree.json +++ b/packages/@aws-cdk/aws-ecs-patterns/test/fargate/circuit-breaker-queue-processing-fargate-service.integ.snapshot/tree.json @@ -1107,6 +1107,9 @@ "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 index c72b687673879..97c12fd9aa13d 100644 --- 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,7 +27,6 @@ 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 index 78cfbbf728270..9eb367248779a 100644 --- 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,6 +451,9 @@ test('setting ALB circuitBreaker works', () => { Rollback: true, }, }, + DeploymentController: { + Type: 'ECS', + }, }); }); @@ -474,6 +477,9 @@ test('setting NLB circuitBreaker works', () => { Rollback: true, }, }, + DeploymentController: { + Type: 'ECS', + }, }); }); @@ -1159,4 +1165,4 @@ test('NetworkLoadBalancedFargateService multiple capacity provider strategies ar }, ]), }); -}); +}); \ No newline at end of file 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 index 020ca2ea25075..8cbb1d579d5b0 100644 --- 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,6 +446,9 @@ 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 index e158f9a0b36f5..df4a9fd4d372e 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -449,7 +449,9 @@ export abstract class BaseService extends Resource }, propagateTags: propagateTagsFromSource === PropagatedTagSource.NONE ? undefined : props.propagateTags, enableEcsManagedTags: props.enableECSManagedTags ?? false, - deploymentController: props.deploymentController, + deploymentController: props.circuitBreaker ? { + type: DeploymentControllerType.ECS, + } : props.deploymentController, launchType: launchType, enableExecuteCommand: props.enableExecuteCommand, capacityProviderStrategy: props.capacityProviderStrategies, @@ -463,9 +465,7 @@ 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 index 4ba4aa9b0a908..7523fc936227e 100644 --- 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,36 +1201,6 @@ 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 index 5ec5def7a7621..69626f7473fb0 100644 --- 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 { DeploymentControllerType, LaunchType } from '../../lib/base/base-service'; +import { LaunchType } from '../../lib/base/base-service'; import { addDefaultCapacityProvider } from '../util'; describe('external service', () => { @@ -520,34 +520,7 @@ 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 index a5cae44042dbc..1bc1bea59ef92 100644 --- 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,33 +657,6 @@ 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(); @@ -2443,6 +2416,9 @@ describe('fargate service', () => { Rollback: true, }, }, + DeploymentController: { + Type: ecs.DeploymentControllerType.ECS, + }, }); });