From 14f86c3d76196685ab2f44f569528759186b1a6c Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Sun, 21 Feb 2021 23:17:51 -0700 Subject: [PATCH 1/7] feat(ecs): allow user-defined cloudmap service association --- .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 86 ++++++++++- .../aws-ecs/test/base/test.base-service.ts | 141 ++++++++++++++++++ .../test/fargate/test.fargate-service.ts | 50 +++++++ 3 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index 8e2483f98838f..444699612a94b 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -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: UseCloudMapServiceOptions): 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, + }); + } + /** * 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 UseCloudMapServiceOptions { + /** + * 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 SelectContainerNamePortOptions { + 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: SelectContainerNamePortOptions) { + // 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 {}; +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts b/packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts new file mode 100644 index 0000000000000..f9a515a1c3e46 --- /dev/null +++ b/packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts @@ -0,0 +1,141 @@ +import * as cdk from '@aws-cdk/core'; +import * as cloudmap from '@aws-cdk/aws-servicediscovery'; +import { Test } from 'nodeunit'; +import * as ecs from '../../lib'; + +export = { + 'When associating with service registry': { + 'By default, container name and port are undefined'(test: Test) { + // 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 + test.strictEqual(containerName, undefined); + test.strictEqual(containerPort, undefined); + test.done(); + }, + + 'For SRV, by default, container name is default container and port is the default container port'(test: Test) { + // 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 + test.equal(containerName, 'main'); + test.equal(containerPort, 1234); + test.done(); + }, + + 'allows SRV service discovery to select the container and port'(test: Test) { + // 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 + test.equal(containerName, 'second'); + test.equal(containerPort, 4321); + test.done(); + }, + + 'throws if SRV and container is not part of task definition'(test: Test) { + 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, + }); + + test.throws(() => { + ecs.determineContainerNameAndPort({ + dnsRecordType: cloudmap.DnsRecordType.SRV, + taskDefinition, + container: wrongContainer, + containerPort: 4321, + }); + }, /another task definition/i); + test.done(); + }, + + 'throws if SRV and the container port is not mapped'(test: Test) { + 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 }); + + test.throws(() => { + ecs.determineContainerNameAndPort({ + dnsRecordType: cloudmap.DnsRecordType.SRV, + taskDefinition, + container: container, + containerPort: 4321, + }); + }, /container port.*not.*mapped/i); + test.done(); + }, + }, +}; diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts index ebb36580d19ba..585533ec6f815 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts @@ -262,6 +262,56 @@ export = { 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(); + }, + 'with all properties set'(test: Test) { // GIVEN const stack = new cdk.Stack(); From 3abbcfeed2d0891ba8cee4b655452681f67a8b93 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Sun, 21 Feb 2021 23:49:55 -0700 Subject: [PATCH 2/7] fix: poor options type name --- packages/@aws-cdk/aws-ecs/lib/base/base-service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index 444699612a94b..b0126e66c8b27 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -933,7 +933,7 @@ export enum PropagatedTagSource { * Options for `determineContainerNameAndPort` * @internal */ -export interface SelectContainerNamePortOptions { +export interface DetermineContainerNameAndPortOptions { dnsRecordType: cloudmap.DnsRecordType; taskDefinition: TaskDefinition; container?: ContainerDefinition; @@ -944,7 +944,7 @@ export interface SelectContainerNamePortOptions { * Determine the name of the container and port to target for the service registry. * @internal */ -export function determineContainerNameAndPort(options: SelectContainerNamePortOptions) { +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. From e8911be700194cb10e136810943e82b459f0aaa7 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Mon, 22 Feb 2021 00:17:27 -0700 Subject: [PATCH 3/7] docs: add readme section to satisfy pr linter --- packages/@aws-cdk/aws-ecs/README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/@aws-cdk/aws-ecs/README.md b/packages/@aws-cdk/aws-ecs/README.md index 438f63e14e009..c33580ba12d95 100644 --- a/packages/@aws-cdk/aws-ecs/README.md +++ b/packages/@aws-cdk/aws-ecs/README.md @@ -671,6 +671,20 @@ taskDefinition.addContainer('TheContainer', { }); ``` +## Associate With Pre-Existing CloudMap Services + +You may associate an ECS with a pre-existing 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. From 3b489f978b184816c5ff003db441de1a1ce03323 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Mon, 22 Feb 2021 00:19:26 -0700 Subject: [PATCH 4/7] fix: typo --- packages/@aws-cdk/aws-ecs/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/README.md b/packages/@aws-cdk/aws-ecs/README.md index c33580ba12d95..e6bd1a25e5bcb 100644 --- a/packages/@aws-cdk/aws-ecs/README.md +++ b/packages/@aws-cdk/aws-ecs/README.md @@ -673,8 +673,8 @@ taskDefinition.addContainer('TheContainer', { ## Associate With Pre-Existing CloudMap Services -You may associate an ECS with a pre-existing cloudmap service. To do this, use -the service's `associateCloudMapService` method: +You may associate an ECS service with a pre-existing CloudMap service. To do +this, use the service's `associateCloudMapService` method: ```ts const cloudMapService = new cloudmap.Service(...); From 34ec919efbec4d96da8831a41f76647ea7b03b77 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Mon, 22 Feb 2021 08:34:24 -0700 Subject: [PATCH 5/7] fix: rename other poorly named type name --- packages/@aws-cdk/aws-ecs/lib/base/base-service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index b0126e66c8b27..e8c3085d5ce47 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -602,7 +602,7 @@ export abstract class BaseService extends Resource /** * Associates this service with a CloudMap service */ - public associateCloudMapService(options: UseCloudMapServiceOptions): void { + public associateCloudMapService(options: AssociateCloudMapServiceOptions): void { const service = options.service; const { containerName, containerPort } = determineContainerNameAndPort({ @@ -826,7 +826,7 @@ export interface CloudMapOptions { /** * The options for using a cloudmap service. */ -export interface UseCloudMapServiceOptions { +export interface AssociateCloudMapServiceOptions { /** * The cloudmap service to register with. */ From 7b3c8f65247cf4038e4fd8adff2b4bda92e8e238 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Mon, 22 Feb 2021 09:12:22 -0700 Subject: [PATCH 6/7] chore: convert base-service test to jest --- .../aws-ecs/test/base/base-service.test.ts | 133 +++++++++++++++++ .../aws-ecs/test/base/test.base-service.ts | 141 ------------------ 2 files changed, 133 insertions(+), 141 deletions(-) create mode 100644 packages/@aws-cdk/aws-ecs/test/base/base-service.test.ts delete mode 100644 packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts diff --git a/packages/@aws-cdk/aws-ecs/test/base/base-service.test.ts b/packages/@aws-cdk/aws-ecs/test/base/base-service.test.ts new file mode 100644 index 0000000000000..1af26e3f75546 --- /dev/null +++ b/packages/@aws-cdk/aws-ecs/test/base/base-service.test.ts @@ -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); + }); +}); diff --git a/packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts b/packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts deleted file mode 100644 index f9a515a1c3e46..0000000000000 --- a/packages/@aws-cdk/aws-ecs/test/base/test.base-service.ts +++ /dev/null @@ -1,141 +0,0 @@ -import * as cdk from '@aws-cdk/core'; -import * as cloudmap from '@aws-cdk/aws-servicediscovery'; -import { Test } from 'nodeunit'; -import * as ecs from '../../lib'; - -export = { - 'When associating with service registry': { - 'By default, container name and port are undefined'(test: Test) { - // 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 - test.strictEqual(containerName, undefined); - test.strictEqual(containerPort, undefined); - test.done(); - }, - - 'For SRV, by default, container name is default container and port is the default container port'(test: Test) { - // 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 - test.equal(containerName, 'main'); - test.equal(containerPort, 1234); - test.done(); - }, - - 'allows SRV service discovery to select the container and port'(test: Test) { - // 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 - test.equal(containerName, 'second'); - test.equal(containerPort, 4321); - test.done(); - }, - - 'throws if SRV and container is not part of task definition'(test: Test) { - 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, - }); - - test.throws(() => { - ecs.determineContainerNameAndPort({ - dnsRecordType: cloudmap.DnsRecordType.SRV, - taskDefinition, - container: wrongContainer, - containerPort: 4321, - }); - }, /another task definition/i); - test.done(); - }, - - 'throws if SRV and the container port is not mapped'(test: Test) { - 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 }); - - test.throws(() => { - ecs.determineContainerNameAndPort({ - dnsRecordType: cloudmap.DnsRecordType.SRV, - taskDefinition, - container: container, - containerPort: 4321, - }); - }, /container port.*not.*mapped/i); - test.done(); - }, - }, -}; From 846c95f46a4dc0ba1f17fe2b3ab8455777b63493 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Mon, 8 Mar 2021 20:14:56 -0700 Subject: [PATCH 7/7] fix: addServiceRegistry now ensures at most 1 service registry --- .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 4 ++ .../test/fargate/fargate-service.test.ts | 45 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index a5090fa593e79..3ff3f20fd8acd 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -769,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); } diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts b/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts index 476e8066a2cb1..5531b53413f46 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts @@ -312,6 +312,51 @@ nodeunitShim({ 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();