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

feat(ecs): allow users to provide a CloudMap service to associate with an ECS service #13192

Merged
merged 13 commits into from Mar 9, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/@aws-cdk/aws-ecs/README.md
Expand Up @@ -728,6 +728,20 @@ new ecs.Ec2Service(stack, 'Service', {
});
```

### Associate With a Specific CloudMap Service

You may associate an ECS service with a specific CloudMap service. To do
this, use the service's `associateCloudMapService` method:

```ts
const cloudMapService = new cloudmap.Service(...);
const ecsService = new ecs.FargateService(...);

ecsService.associateCloudMapService({
service: cloudMapService,
});
```

## Capacity Providers

Currently, only `FARGATE` and `FARGATE_SPOT` capacity providers are supported.
Expand Down
47 changes: 47 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
Expand Up @@ -601,6 +601,27 @@ export abstract class BaseService extends Resource
return cloudmapService;
}

/**
* Associates this service with a CloudMap service
*/
public associateCloudMapService(options: AssociateCloudMapServiceOptions): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SoManyHs Hey. I saw you self-assigned this PR. I thought I might leave a note on my thinking here:

I created associateCloudMapService only because I wasn't sure how I could make enableCloudMap accept a cloudmap.IService via CloudMapOptions. To make a non-breaking change, I think that enableCloudMap must continue to return the concrete cloudmap.Service type. But, I felt it would be useful to allow the customer to provide an imported cloudmap.IService - in this case, there would be no concrete type to return. So, I added another member function to work around this issue.

Another option might be to create an enableCloudMapV2 that does the same as the original or accepts the customer's service, but returns cloudmap.IService instead of the concrete type. Then we'd deprecate the old function. In this case, the customer could provide the service by construct prop, which would be pretty nice.

What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @misterjoshua,

Oooh, yeah not having the cloudmapService field have an interface type was definitely a miss in our initial implementation. I like the approach you took here, since having an enableCloudMapV2 method is a little clunky for sure. The good news is that we are in the process of working towards a v2 of the ECS modules, so we can incorporate breaking changes like changing the method signature for enableCloudMap.

Tangentially, I don't know if you've checked out ECS Service Extensions, but the philosophy there is to treat components such as cloudmap (here, wrapped up in Appmesh) more atomically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @misterjoshua,

Oooh, yeah not having the cloudmapService field have an interface type was definitely a miss in our initial implementation. I like the approach you took here, since having an enableCloudMapV2 method is a little clunky for sure. The good news is that we are in the process of working towards a v2 of the ECS modules, so we can incorporate breaking changes like changing the method signature for enableCloudMap.

@SoManyHs Sounds good! Looking forward to v2. :)

Tangentially, I don't know if you've checked out ECS Service Extensions, but the philosophy there is to treat components such as cloudmap (here, wrapped up in Appmesh) more atomically.

Ah yes, that's an excellent module. I had originally wanted this PR's capability to work around difficulties in adding the service name as an environment variable in the first container on a task definition. It was to get JGroups clustering working on ECS for an embedded distributed cache & KV store. Thankfully, after #13240 hit it isn't as tricky. And, with this PR, there will be a couple of good ways to go about this. :)

const service = options.service;

const { containerName, containerPort } = determineContainerNameAndPort({
taskDefinition: this.taskDefinition,
dnsRecordType: service.dnsRecordType,
container: options.container,
containerPort: options.containerPort,
});

// add Cloudmap service to the ECS Service's serviceRegistry
this.addServiceRegistry({
arn: service.serviceArn,
containerName,
containerPort,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be helpful to see if this.serviceRegistries is empty, or add a validation within addServiceRegistry -- otherwise the ECS service will fail its validations, since it can have at most 1 service registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SoManyHs Sounds good to me. This got me a while back and I had already forgotten. Haha.

});
}

/**
* This method returns the specified CloudWatch metric name for this service.
*/
Expand Down Expand Up @@ -748,6 +769,10 @@ export abstract class BaseService extends Resource
* Associate Service Discovery (Cloud Map) service
*/
private addServiceRegistry(registry: ServiceRegistry) {
if (this.serviceRegistries.length >= 1) {
throw new Error('Cannot associate with the given service discovery registry. ECS supports at most one service registry per service.');
}

const sr = this.renderServiceRegistry(registry);
this.serviceRegistries.push(sr);
}
Expand Down Expand Up @@ -816,6 +841,28 @@ export interface CloudMapOptions {
readonly containerPort?: number;
}

/**
* The options for using a cloudmap service.
*/
export interface AssociateCloudMapServiceOptions {
/**
* The cloudmap service to register with.
*/
readonly service: cloudmap.IService;

/**
* The container to point to for a SRV record.
* @default - the task definition's default container
*/
readonly container?: ContainerDefinition;

/**
* The port to point to for a SRV record.
* @default - the default port of the task definition's default container
*/
readonly containerPort?: number;
}

/**
* Service Registry for ECS service
*/
Expand Down
95 changes: 95 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts
Expand Up @@ -262,6 +262,101 @@ nodeunitShim({
test.done();
},

'with user-provided cloudmap service'(test: Test) {
// 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');

const container = taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
memoryLimitMiB: 512,
});
container.addPortMappings({ containerPort: 8000 });

const cloudMapNamespace = new cloudmap.PrivateDnsNamespace(stack, 'TestCloudMapNamespace', {
name: 'scorekeep.com',
vpc,
});

const cloudMapService = new cloudmap.Service(stack, 'Service', {
name: 'service-name',
namespace: cloudMapNamespace,
dnsRecordType: cloudmap.DnsRecordType.SRV,
});

const ecsService = new ecs.FargateService(stack, 'FargateService', {
cluster,
taskDefinition,
});

// WHEN
ecsService.associateCloudMapService({
service: cloudMapService,
container: container,
containerPort: 8000,
});

// THEN
expect(stack).to(haveResource('AWS::ECS::Service', {
ServiceRegistries: [
{
ContainerName: 'web',
ContainerPort: 8000,
RegistryArn: { 'Fn::GetAtt': ['ServiceDBC79909', 'Arn'] },
},
],
}));

test.done();
},

'errors when more than one service registry used'(test: Test) {
// 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');

const container = taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
memoryLimitMiB: 512,
});
container.addPortMappings({ containerPort: 8000 });

const cloudMapNamespace = new cloudmap.PrivateDnsNamespace(stack, 'TestCloudMapNamespace', {
name: 'scorekeep.com',
vpc,
});

const ecsService = new ecs.FargateService(stack, 'FargateService', {
cluster,
taskDefinition,
});

ecsService.enableCloudMap({
cloudMapNamespace,
});

const cloudMapService = new cloudmap.Service(stack, 'Service', {
name: 'service-name',
namespace: cloudMapNamespace,
dnsRecordType: cloudmap.DnsRecordType.SRV,
});

// WHEN / THEN
test.throws(() => {
ecsService.associateCloudMapService({
service: cloudMapService,
container: container,
containerPort: 8000,
});
}, /at most one service registry/i);

test.done();
},

'with all properties set'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down