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.

Fixes aws#1849.
  • Loading branch information
SoManyHs committed Apr 4, 2019
1 parent 732aa5b commit e4b0b2f
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 e4b0b2f

Please sign in to comment.