-
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 12 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 |
---|---|---|
|
@@ -601,6 +601,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. | ||
*/ | ||
|
@@ -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); | ||
} | ||
|
@@ -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 | ||
*/ | ||
|
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. :)