Skip to content

Commit

Permalink
fix(aws-ecs): handle long ARN formats for services
Browse files Browse the repository at this point in the history
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 aws#1849.
  • Loading branch information
SoManyHs committed Apr 4, 2019
1 parent 732aa5b commit bcdda38
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 8 deletions.
33 changes: 30 additions & 3 deletions packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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<CfnService.LoadBalancerProperty>();
Expand Down Expand Up @@ -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);
}

}

/**
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -522,9 +522,16 @@
},
"/",
{
"Fn::GetAtt": [
"ServiceD69D759B",
"Name"
"Fn::Select": [
2,
{
"Fn::Split": [
"/",
{
"Ref": "ServiceD69D759B"
}
]
}
]
}
]
Expand Down Expand Up @@ -682,4 +689,4 @@
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ container.addPortMappings({
const service = new ecs.FargateService(stack, "Service", {
cluster,
taskDefinition,
longArnEnabled: true
});

const scaling = service.autoScaleTaskCount({ maxCapacity: 10 });
Expand Down
85 changes: 84 additions & 1 deletion packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand All @@ -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();
}
},
Expand Down

0 comments on commit bcdda38

Please sign in to comment.