-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from 8 commits
14f86c3
3abbcfe
e8911be
3b489f9
34ec919
e9540a8
7b3c8f6
d5ae522
821b8d9
83319ee
846c95f
eb19953
b9324b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import { Annotations, Duration, IResolvable, IResource, Lazy, Resource, Stack } | |
import { Construct } from 'constructs'; | ||
import { LoadBalancerTargetOptions, NetworkMode, TaskDefinition } from '../base/task-definition'; | ||
import { ICluster, CapacityProviderStrategy } from '../cluster'; | ||
import { Protocol } from '../container-definition'; | ||
import { ContainerDefinition, Protocol } from '../container-definition'; | ||
import { CfnService } from '../ecs.generated'; | ||
import { ScalableTaskCount } from './scalable-task-count'; | ||
|
||
|
@@ -599,6 +599,27 @@ export abstract class BaseService extends Resource | |
return cloudmapService; | ||
} | ||
|
||
/** | ||
* Associates this service with a CloudMap service | ||
*/ | ||
public associateCloudMapService(options: AssociateCloudMapServiceOptions): void { | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be helpful to see if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
*/ | ||
|
@@ -802,6 +823,28 @@ export interface CloudMapOptions { | |
readonly failureThreshold?: 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 | ||
*/ | ||
|
@@ -885,3 +928,44 @@ export enum PropagatedTagSource { | |
*/ | ||
NONE = 'NONE' | ||
} | ||
|
||
/** | ||
* Options for `determineContainerNameAndPort` | ||
* @internal | ||
*/ | ||
export interface DetermineContainerNameAndPortOptions { | ||
dnsRecordType: cloudmap.DnsRecordType; | ||
taskDefinition: TaskDefinition; | ||
container?: ContainerDefinition; | ||
containerPort?: number; | ||
} | ||
|
||
/** | ||
* Determine the name of the container and port to target for the service registry. | ||
* @internal | ||
*/ | ||
export function determineContainerNameAndPort(options: DetermineContainerNameAndPortOptions) { | ||
// If the record type is SRV, then provide the containerName and containerPort to target. | ||
// We use the name of the default container and the default port of the default container | ||
// unless the user specifies otherwise. | ||
if (options.dnsRecordType === cloudmap.DnsRecordType.SRV) { | ||
// Ensure the user-provided container is from the right task definition. | ||
if (options.container && options.container.taskDefinition != options.taskDefinition) { | ||
throw new Error('Cannot add discovery for a container from another task definition'); | ||
} | ||
|
||
const container = options.container ?? options.taskDefinition.defaultContainer!; | ||
|
||
// Ensure that any port given by the user is mapped. | ||
if (options.containerPort && !container.portMappings.some(mapping => mapping.containerPort === options.containerPort)) { | ||
throw new Error('Cannot add discovery for a container port that has not been mapped'); | ||
} | ||
|
||
return { | ||
containerName: container.containerName, | ||
containerPort: options.containerPort ?? options.taskDefinition.defaultContainer!.containerPort, | ||
}; | ||
} | ||
|
||
return {}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
import * as cdk from '@aws-cdk/core'; | ||
import * as cloudmap from '@aws-cdk/aws-servicediscovery'; | ||
import * as ecs from '../../lib'; | ||
|
||
describe('When associating with service registry', () => { | ||
test('By default, container name and port are undefined', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
const taskDefinition = new ecs.TaskDefinition(stack, 'Task', { | ||
compatibility: ecs.Compatibility.EC2, | ||
}); | ||
|
||
taskDefinition.addContainer('main', { | ||
image: ecs.ContainerImage.fromRegistry('some'), | ||
}).addPortMappings({ containerPort: 1234 }); | ||
|
||
taskDefinition.addContainer('second', { | ||
image: ecs.ContainerImage.fromRegistry('some'), | ||
}).addPortMappings({ containerPort: 4321 }); | ||
|
||
// WHEN | ||
const { containerName, containerPort } = ecs.determineContainerNameAndPort({ | ||
dnsRecordType: cloudmap.DnsRecordType.A, | ||
taskDefinition, | ||
}); | ||
|
||
// THEN | ||
expect(containerName).toBeUndefined(); | ||
expect(containerPort).toBeUndefined(); | ||
}); | ||
|
||
test('For SRV, by default, container name is default container and port is the default container port', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
const taskDefinition = new ecs.TaskDefinition(stack, 'Task', { | ||
compatibility: ecs.Compatibility.EC2, | ||
}); | ||
|
||
taskDefinition.addContainer('main', { | ||
image: ecs.ContainerImage.fromRegistry('some'), | ||
}).addPortMappings({ containerPort: 1234 }); | ||
|
||
taskDefinition.addContainer('second', { | ||
image: ecs.ContainerImage.fromRegistry('some'), | ||
}).addPortMappings({ containerPort: 4321 }); | ||
|
||
// WHEN | ||
const { containerName, containerPort } = ecs.determineContainerNameAndPort({ | ||
dnsRecordType: cloudmap.DnsRecordType.SRV, | ||
taskDefinition, | ||
}); | ||
|
||
// THEN | ||
expect(containerName).toEqual('main'); | ||
expect(containerPort).toEqual(1234); | ||
}); | ||
|
||
test('allows SRV service discovery to select the container and port', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
const taskDefinition = new ecs.TaskDefinition(stack, 'Task', { | ||
compatibility: ecs.Compatibility.EC2, | ||
}); | ||
|
||
taskDefinition.addContainer('main', { | ||
image: ecs.ContainerImage.fromRegistry('some'), | ||
}).addPortMappings({ containerPort: 1234 }); | ||
|
||
const secondContainer = taskDefinition.addContainer('second', { | ||
image: ecs.ContainerImage.fromRegistry('some'), | ||
}); | ||
secondContainer.addPortMappings({ containerPort: 4321 }); | ||
|
||
// WHEN | ||
const { containerName, containerPort } = ecs.determineContainerNameAndPort({ | ||
dnsRecordType: cloudmap.DnsRecordType.SRV, | ||
taskDefinition, | ||
container: secondContainer, | ||
containerPort: 4321, | ||
}); | ||
|
||
// THEN | ||
expect(containerName).toEqual('second'); | ||
expect(containerPort).toEqual(4321); | ||
}); | ||
|
||
test('throws if SRV and container is not part of task definition', () => { | ||
const stack = new cdk.Stack(); | ||
const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef'); | ||
// The right container | ||
taskDefinition.addContainer('MainContainer', { | ||
image: ecs.ContainerImage.fromRegistry('hello'), | ||
memoryLimitMiB: 512, | ||
}); | ||
|
||
const wrongTaskDefinition = new ecs.FargateTaskDefinition(stack, 'WrongFargateTaskDef'); | ||
// The wrong container | ||
const wrongContainer = wrongTaskDefinition.addContainer('MainContainer', { | ||
image: ecs.ContainerImage.fromRegistry('hello'), | ||
memoryLimitMiB: 512, | ||
}); | ||
|
||
expect(() => { | ||
ecs.determineContainerNameAndPort({ | ||
dnsRecordType: cloudmap.DnsRecordType.SRV, | ||
taskDefinition, | ||
container: wrongContainer, | ||
containerPort: 4321, | ||
}); | ||
}).toThrow(/another task definition/i); | ||
}); | ||
|
||
test('throws if SRV and the container port is not mapped', () => { | ||
const stack = new cdk.Stack(); | ||
const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef'); | ||
|
||
const container = taskDefinition.addContainer('MainContainer', { | ||
image: ecs.ContainerImage.fromRegistry('hello'), | ||
memoryLimitMiB: 512, | ||
}); | ||
|
||
container.addPortMappings({ containerPort: 8000 }); | ||
|
||
expect(() => { | ||
ecs.determineContainerNameAndPort({ | ||
dnsRecordType: cloudmap.DnsRecordType.SRV, | ||
taskDefinition, | ||
container: container, | ||
containerPort: 4321, | ||
}); | ||
}).toThrow(/container port.*not.*mapped/i); | ||
}); | ||
}); |
There was a problem hiding this comment.
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 makeenableCloudMap
accept acloudmap.IService
viaCloudMapOptions
. To make a non-breaking change, I think thatenableCloudMap
must continue to return the concretecloudmap.Service
type. But, I felt it would be useful to allow the customer to provide an importedcloudmap.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 returnscloudmap.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?
There was a problem hiding this comment.
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 anenableCloudMapV2
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 forenableCloudMap
.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SoManyHs Sounds good! Looking forward to v2. :)
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. :)