Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(aws-ecs): handle long ARN formats for services #2176

Merged
merged 5 commits into from
Apr 5, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
SoManyHs marked this conversation as resolved.
Show resolved Hide resolved
*
* @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;
SoManyHs marked this conversation as resolved.
Show resolved Hide resolved

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
SoManyHs marked this conversation as resolved.
Show resolved Hide resolved
});

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