From bcdda38071490c1a8bebbb53fd0021bb8b8df402 Mon Sep 17 00:00:00 2001 From: Hsing-Hui Hsu Date: Wed, 27 Mar 2019 10:31:12 -0700 Subject: [PATCH 1/3] fix(aws-ecs): handle long ARN formats for services Due to a bug in Cloudformation, Fn::GetAtt for service name on an ECS service was returning the cluster name instead of the service name. This was causing the ResourceId on AWS::ApplicationAutoScaling::ScalableTarget to be set incorrectly. This change forces the customer to specify whether they have opted into the long ARN format for services (which can be achieved through the following command: `aws ecs put-account-setting-default --name serviceLongArnFormat --value enabled`). The CDK cannot currently detect if the customer has the these settings enabled so there could be a potential conflict on deployment. Fixes #1849. --- .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 33 ++++++- .../fargate/integ.lb-awsvpc-nw.expected.json | 15 +++- .../test/fargate/integ.lb-awsvpc-nw.ts | 1 + .../test/fargate/test.fargate-service.ts | 85 ++++++++++++++++++- 4 files changed, 126 insertions(+), 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 index b8bb3a6bf3aa2..8639be1cfe2b3 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -62,6 +62,21 @@ export interface BaseServiceProps { * Options for enabling AWS Cloud Map service discovery for the service */ readonly serviceDiscoveryOptions?: ServiceDiscoveryOptions; + + /** + * Whether the new long ARN format has been enabled on ECS services. + * NOTE: This assumes customer has opted into the new format for the IAM role used for the service, and is a + * workaround for a current bug in Cloudformation in which the service name is not correctly returned when long ARN is + * enabled. + * + * Old ARN format: arn:aws:ecs:region:aws_account_id:service/service-name + * New ARN format: arn:aws:ecs:region:aws_account_id:service/cluster-name/service-name + * + * See: https://docs.aws.amazon.com/AmazonECS/latest/userguide/ecs-resource-ids.html + * + * @default false + */ + readonly longArnEnabled?: boolean; } /** @@ -95,6 +110,11 @@ export abstract class BaseService extends cdk.Construct */ public readonly taskDefinition: TaskDefinition; + /** + * Whether the new long ARN format has been enabled on ECS services. + */ + public readonly longArnEnabled: boolean; + protected cloudmapService?: cloudmap.Service; protected cluster: ICluster; protected loadBalancers = new Array(); @@ -128,15 +148,22 @@ export abstract class BaseService extends cdk.Construct serviceRegistries: new cdk.Token(() => this.serviceRegistries), ...additionalProps }); + + this.longArnEnabled = props.longArnEnabled !== undefined ? props.longArnEnabled : false; this.serviceArn = this.resource.serviceArn; - this.serviceName = this.resource.serviceName; + + // This is a workaround for CFN bug that returns the cluster name instead of the service name when long ARN formats + // are enabled for the principal in a given region. + this.serviceName = this.longArnEnabled + ? cdk.Fn.select(2, cdk.Fn.split('/', this.serviceArn)) + : this.resource.serviceName; + this.clusterName = clusterName; this.cluster = props.cluster; if (props.serviceDiscoveryOptions) { this.enableServiceDiscovery(props.serviceDiscoveryOptions); } - } /** @@ -177,7 +204,7 @@ export abstract class BaseService extends cdk.Construct return this.scalableTaskCount = new ScalableTaskCount(this, 'TaskCount', { serviceNamespace: appscaling.ServiceNamespace.Ecs, - resourceId: `service/${this.clusterName}/${this.resource.serviceName}`, + resourceId: `service/${this.clusterName}/${this.serviceName}`, dimension: 'ecs:service:DesiredCount', role: this.makeAutoScalingRole(), ...props diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json index 21beb1d2c0d63..5c8fdaaa77931 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json @@ -522,9 +522,16 @@ }, "/", { - "Fn::GetAtt": [ - "ServiceD69D759B", - "Name" + "Fn::Select": [ + 2, + { + "Fn::Split": [ + "/", + { + "Ref": "ServiceD69D759B" + } + ] + } ] } ] @@ -682,4 +689,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts index 769caf9e97f78..4d3ede97c7fbe 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts @@ -27,6 +27,7 @@ container.addPortMappings({ const service = new ecs.FargateService(stack, "Service", { cluster, taskDefinition, + longArnEnabled: true }); const scaling = service.autoScaleTaskCount({ maxCapacity: 10 }); diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts index ec4bd3d47bb6d..5011a937736e2 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts @@ -216,7 +216,25 @@ export = { // THEN expect(stack).to(haveResource('AWS::ApplicationAutoScaling::ScalableTarget', { MaxCapacity: 10, - MinCapacity: 1 + MinCapacity: 1, + ResourceId: { + "Fn::Join": [ + "", + [ + "service/", + { + Ref: "EcsCluster97242B84" + }, + "/", + { + "Fn::GetAtt": [ + "ServiceD69D759B", + "Name" + ] + } + ] + ] + }, })); expect(stack).to(haveResource('AWS::ApplicationAutoScaling::ScalingPolicy', { @@ -236,6 +254,71 @@ export = { } })); + test.done(); + }, + + 'allows auto scaling by ALB with new service arn format'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const vpc = new ec2.VpcNetwork(stack, 'MyVpc', {}); + const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc }); + const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef'); + const container = taskDefinition.addContainer('MainContainer', { + image: ContainerImage.fromRegistry('hello'), + }); + container.addPortMappings({ containerPort: 8000 }); + + const service = new ecs.FargateService(stack, 'Service', { + cluster, + taskDefinition, + longArnEnabled: true + }); + + const lb = new elbv2.ApplicationLoadBalancer(stack, "lb", { vpc }); + const listener = lb.addListener("listener", { port: 80 }); + const targetGroup = listener.addTargets("target", { + port: 80, + targets: [service] + }); + + // WHEN + const capacity = service.autoScaleTaskCount({ maxCapacity: 10, minCapacity: 1 }); + capacity.scaleOnRequestCount("ScaleOnRequests", { + requestsPerTarget: 1000, + targetGroup + }); + + // THEN + expect(stack).to(haveResource('AWS::ApplicationAutoScaling::ScalableTarget', { + MaxCapacity: 10, + MinCapacity: 1, + ResourceId: { + "Fn::Join": [ + "", + [ + "service/", + { + Ref: "EcsCluster97242B84" + }, + "/", + { + "Fn::Select": [ + 2, + { + "Fn::Split": [ + "/", + { + Ref: "ServiceD69D759B" + } + ] + } + ] + } + ] + ] + }, + })); + test.done(); } }, From 4c04a30ccb7175b1957ae5140ef3ae5349420a9d Mon Sep 17 00:00:00 2001 From: Hsing-Hui Hsu Date: Thu, 4 Apr 2019 07:53:26 -0700 Subject: [PATCH 2/3] Revert integ test change Test account won't be able to verify long ARN settings --- .../test/fargate/integ.lb-awsvpc-nw.expected.json | 15 ++++----------- .../aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts | 1 - 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json index 5c8fdaaa77931..21beb1d2c0d63 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json @@ -522,16 +522,9 @@ }, "/", { - "Fn::Select": [ - 2, - { - "Fn::Split": [ - "/", - { - "Ref": "ServiceD69D759B" - } - ] - } + "Fn::GetAtt": [ + "ServiceD69D759B", + "Name" ] } ] @@ -689,4 +682,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts index 4d3ede97c7fbe..769caf9e97f78 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.ts @@ -27,7 +27,6 @@ container.addPortMappings({ const service = new ecs.FargateService(stack, "Service", { cluster, taskDefinition, - longArnEnabled: true }); const scaling = service.autoScaleTaskCount({ maxCapacity: 10 }); From f9b434840476b16c84fb1731fe7b97c5da3ffbff Mon Sep 17 00:00:00 2001 From: Hsing-Hui Hsu Date: Thu, 4 Apr 2019 07:57:42 -0700 Subject: [PATCH 3/3] Remove longArnEnabled as public property on service --- packages/@aws-cdk/aws-ecs/lib/base/base-service.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 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 index 8639be1cfe2b3..653676700d77b 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -110,11 +110,6 @@ export abstract class BaseService extends cdk.Construct */ public readonly taskDefinition: TaskDefinition; - /** - * Whether the new long ARN format has been enabled on ECS services. - */ - public readonly longArnEnabled: boolean; - protected cloudmapService?: cloudmap.Service; protected cluster: ICluster; protected loadBalancers = new Array(); @@ -149,12 +144,12 @@ export abstract class BaseService extends cdk.Construct ...additionalProps }); - this.longArnEnabled = props.longArnEnabled !== undefined ? props.longArnEnabled : false; this.serviceArn = this.resource.serviceArn; // This is a workaround for CFN bug that returns the cluster name instead of the service name when long ARN formats // are enabled for the principal in a given region. - this.serviceName = this.longArnEnabled + const longArnEnabled = props.longArnEnabled !== undefined ? props.longArnEnabled : false; + this.serviceName = longArnEnabled ? cdk.Fn.select(2, cdk.Fn.split('/', this.serviceArn)) : this.resource.serviceName;