From df24694f79ed82074a5133bb8e3a802722b1a10f Mon Sep 17 00:00:00 2001 From: juinqqn <54617982+juinquok@users.noreply.github.com> Date: Wed, 20 Dec 2023 21:42:06 +0800 Subject: [PATCH 1/3] add in AWS_REGION env var for AWS_VPC network mode --- .../aws-ecs/lib/ec2/ec2-task-definition.ts | 23 ++++++- .../test/ec2/ec2-task-definition.test.ts | 61 +++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts b/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts index ae33b67f5c003..d202c0f832c51 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts @@ -1,16 +1,18 @@ import { Construct } from 'constructs'; +import { Stack } from '../../../core'; import { ImportedTaskDefinition } from '../base/_imported-task-definition'; import { CommonTaskDefinitionAttributes, CommonTaskDefinitionProps, Compatibility, + InferenceAccelerator, IpcMode, ITaskDefinition, NetworkMode, PidMode, TaskDefinition, - InferenceAccelerator, } from '../base/task-definition'; +import { ContainerDefinition, ContainerDefinitionOptions } from '../container-definition' import { PlacementConstraint } from '../placement'; /** @@ -83,7 +85,6 @@ export interface Ec2TaskDefinitionAttributes extends CommonTaskDefinitionAttribu * @resource AWS::ECS::TaskDefinition */ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinition { - /** * Imports a task definition from the specified task definition ARN. */ @@ -130,6 +131,24 @@ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinit } } + /** + * Tasks running in AWSVPC networking mode requires an additional environment variable for the region to be sourced. + * This override adds in the additional environment variable as required + */ + override addContainer(id: string, props: ContainerDefinitionOptions): ContainerDefinition { + if (this.networkMode === NetworkMode.AWS_VPC) { + return new ContainerDefinition(this, id, { + taskDefinition: this, ...props, + environment: { + ...props.environment, + AWS_REGION: Stack.of(this).region + } + }); + } + // If network mode is not AWSVPC, then just add the container as normal + return new ContainerDefinition(this, id, {taskDefinition: this, ...props}); + } + /** * Constructs a new instance of the Ec2TaskDefinition class. */ diff --git a/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts b/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts index 0f6b316dd8f26..5628ca6f074ba 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts @@ -1110,6 +1110,67 @@ describe('ec2 task definition', () => { }], }); }); + + test('correctly sets env variables when using EC2 capacity provider with AWSVPC mode', () => { + // GIVEN AWS-VPC network mode + const stack = new cdk.Stack(); + const taskDefiniton = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { + networkMode: ecs.NetworkMode.AWS_VPC, + }); + taskDefiniton.addContainer('some-container', { + image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + memoryLimitMiB: 512, + environment: { + SOME_VARIABLE: 'some-value' + } + }) + + // THEN it should include the AWS_REGION env variable + Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { + NetworkMode: ecs.NetworkMode.AWS_VPC, + ContainerDefinitions: [{ + Name: 'some-container', + Image: 'amazon/amazon-ecs-sample', + Memory: 512, + Environment: [{ + Name: 'SOME_VARIABLE', + Value: 'some-value' + }, { + Name: 'AWS_REGION', + Value: { + Ref: 'AWS::Region' + } + }] + }] + }); + + // GIVEN HOST network mode + const anotherStack = new cdk.Stack(); + const anotherTaskDefiniton = new ecs.Ec2TaskDefinition(anotherStack, 'Ec2TaskDef', { + networkMode: ecs.NetworkMode.HOST, + }); + anotherTaskDefiniton.addContainer('some-container', { + image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + memoryLimitMiB: 512, + environment: { + SOME_VARIABLE: 'some-value' + } + }) + + // THEN it should add in any env variables + Template.fromStack(anotherStack).hasResourceProperties('AWS::ECS::TaskDefinition', { + NetworkMode: ecs.NetworkMode.HOST, + ContainerDefinitions: [{ + Name: 'some-container', + Image: 'amazon/amazon-ecs-sample', + Memory: 512, + Environment: [{ + Name: 'SOME_VARIABLE', + Value: 'some-value' + }] + }] + }); + }) }); describe('setting inferenceAccelerators', () => { From fbd90d71cb2ba9e3dd968245a27254c5184c8437 Mon Sep 17 00:00:00 2001 From: juinqqn <54617982+juinquok@users.noreply.github.com> Date: Wed, 20 Dec 2023 23:06:16 +0800 Subject: [PATCH 2/3] update integrations tests and fix linting as required --- .../aws-ecs-integ-appmesh-proxy.template.json | 16 ++++++++ .../aws-ecs-integ.template.json | 8 ++++ .../aws-ecs-integ.template.json | 12 ++++++ .../aws-ecs/test/ec2/integ.lb-awsvpc-nw.ts | 3 ++ ...ws-ecs-integ-pseudo-terminal.template.json | 8 ++++ .../aws-ecs-integ-ecs.template.json | 8 ++++ .../aws-ecs/lib/ec2/ec2-task-definition.ts | 39 ++++++++++--------- .../test/ec2/ec2-task-definition.test.ts | 30 +++++++------- 8 files changed, 90 insertions(+), 34 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.app-mesh-proxy-config.js.snapshot/aws-ecs-integ-appmesh-proxy.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.app-mesh-proxy-config.js.snapshot/aws-ecs-integ-appmesh-proxy.template.json index 4407c5e315caf..4abdf05274bdf 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.app-mesh-proxy-config.js.snapshot/aws-ecs-integ-appmesh-proxy.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.app-mesh-proxy-config.js.snapshot/aws-ecs-integ-appmesh-proxy.template.json @@ -919,12 +919,28 @@ "Properties": { "ContainerDefinitions": [ { + "Environment": [ + { + "Name": "AWS_REGION", + "Value": { + "Ref": "AWS::Region" + } + } + ], "Essential": true, "Image": "amazon/amazon-ecs-sample", "Memory": 256, "Name": "web" }, { + "Environment": [ + { + "Name": "AWS_REGION", + "Value": { + "Ref": "AWS::Region" + } + } + ], "Essential": true, "Image": "envoyproxy/envoy:v1.16.2", "Memory": 256, diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.firelens-s3-config.js.snapshot/aws-ecs-integ.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.firelens-s3-config.js.snapshot/aws-ecs-integ.template.json index 802fbf44d2afd..9ec17a6f8aaaa 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.firelens-s3-config.js.snapshot/aws-ecs-integ.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.firelens-s3-config.js.snapshot/aws-ecs-integ.template.json @@ -964,6 +964,14 @@ "Name": "log_router" }, { + "Environment": [ + { + "Name": "AWS_REGION", + "Value": { + "Ref": "AWS::Region" + } + } + ], "Essential": true, "Image": "nginx", "LogConfiguration": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.js.snapshot/aws-ecs-integ.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.js.snapshot/aws-ecs-integ.template.json index 466bd90542523..a59fc8fb4881b 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.js.snapshot/aws-ecs-integ.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.js.snapshot/aws-ecs-integ.template.json @@ -919,6 +919,18 @@ "Properties": { "ContainerDefinitions": [ { + "Environment": [ + { + "Name": "SOME_VARIABLE", + "Value": "value" + }, + { + "Name": "AWS_REGION", + "Value": { + "Ref": "AWS::Region" + } + } + ], "Essential": true, "Image": "amazon/amazon-ecs-sample", "Memory": 256, diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.ts index 36b43c5680ce5..8667d63c7c143 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.lb-awsvpc-nw.ts @@ -20,6 +20,9 @@ const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef', { const container = taskDefinition.addContainer('web', { image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), memoryLimitMiB: 256, + environment: { + SOME_VARIABLE: 'value', + }, }); container.addPortMappings({ diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.pseudo-terminal.js.snapshot/aws-ecs-integ-pseudo-terminal.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.pseudo-terminal.js.snapshot/aws-ecs-integ-pseudo-terminal.template.json index 665bcf835bda3..2716e688d9d92 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.pseudo-terminal.js.snapshot/aws-ecs-integ-pseudo-terminal.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.pseudo-terminal.js.snapshot/aws-ecs-integ-pseudo-terminal.template.json @@ -919,6 +919,14 @@ "Properties": { "ContainerDefinitions": [ { + "Environment": [ + { + "Name": "AWS_REGION", + "Value": { + "Ref": "AWS::Region" + } + } + ], "Essential": true, "Image": "amazon/amazon-ecs-sample", "Memory": 256, diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.sd-awsvpc-nw.js.snapshot/aws-ecs-integ-ecs.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.sd-awsvpc-nw.js.snapshot/aws-ecs-integ-ecs.template.json index 055b31a478be9..f6431ca60bcb3 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.sd-awsvpc-nw.js.snapshot/aws-ecs-integ-ecs.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test/ec2/integ.sd-awsvpc-nw.js.snapshot/aws-ecs-integ-ecs.template.json @@ -928,6 +928,14 @@ "Properties": { "ContainerDefinitions": [ { + "Environment": [ + { + "Name": "AWS_REGION", + "Value": { + "Ref": "AWS::Region" + } + } + ], "Essential": true, "Image": "amazon/amazon-ecs-sample", "Memory": 256, diff --git a/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts b/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts index d202c0f832c51..f831775b7e3d7 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts @@ -12,7 +12,7 @@ import { PidMode, TaskDefinition, } from '../base/task-definition'; -import { ContainerDefinition, ContainerDefinitionOptions } from '../container-definition' +import { ContainerDefinition, ContainerDefinitionOptions } from '../container-definition'; import { PlacementConstraint } from '../placement'; /** @@ -131,24 +131,6 @@ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinit } } - /** - * Tasks running in AWSVPC networking mode requires an additional environment variable for the region to be sourced. - * This override adds in the additional environment variable as required - */ - override addContainer(id: string, props: ContainerDefinitionOptions): ContainerDefinition { - if (this.networkMode === NetworkMode.AWS_VPC) { - return new ContainerDefinition(this, id, { - taskDefinition: this, ...props, - environment: { - ...props.environment, - AWS_REGION: Stack.of(this).region - } - }); - } - // If network mode is not AWSVPC, then just add the container as normal - return new ContainerDefinition(this, id, {taskDefinition: this, ...props}); - } - /** * Constructs a new instance of the Ec2TaskDefinition class. */ @@ -165,4 +147,23 @@ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinit // Validate the placement constraints Ec2TaskDefinition.validatePlacementConstraints(props.placementConstraints ?? []); } + + /** + * Tasks running in AWSVPC networking mode requires an additional environment variable for the region to be sourced. + * This override adds in the additional environment variable as required + */ + override addContainer(id: string, props: ContainerDefinitionOptions): ContainerDefinition { + if (this.networkMode === NetworkMode.AWS_VPC) { + return new ContainerDefinition(this, id, { + taskDefinition: this, + ...props, + environment: { + ...props.environment, + AWS_REGION: Stack.of(this).region, + }, + }); + } + // If network mode is not AWSVPC, then just add the container as normal + return new ContainerDefinition(this, id, { taskDefinition: this, ...props }); + } } diff --git a/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts b/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts index 5628ca6f074ba..ede45583332dc 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts @@ -1121,9 +1121,9 @@ describe('ec2 task definition', () => { image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), memoryLimitMiB: 512, environment: { - SOME_VARIABLE: 'some-value' - } - }) + SOME_VARIABLE: 'some-value', + }, + }); // THEN it should include the AWS_REGION env variable Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { @@ -1134,14 +1134,14 @@ describe('ec2 task definition', () => { Memory: 512, Environment: [{ Name: 'SOME_VARIABLE', - Value: 'some-value' + Value: 'some-value', }, { Name: 'AWS_REGION', Value: { - Ref: 'AWS::Region' - } - }] - }] + Ref: 'AWS::Region', + }, + }], + }], }); // GIVEN HOST network mode @@ -1153,9 +1153,9 @@ describe('ec2 task definition', () => { image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), memoryLimitMiB: 512, environment: { - SOME_VARIABLE: 'some-value' - } - }) + SOME_VARIABLE: 'some-value', + }, + }); // THEN it should add in any env variables Template.fromStack(anotherStack).hasResourceProperties('AWS::ECS::TaskDefinition', { @@ -1166,11 +1166,11 @@ describe('ec2 task definition', () => { Memory: 512, Environment: [{ Name: 'SOME_VARIABLE', - Value: 'some-value' - }] - }] + Value: 'some-value', + }], + }], }); - }) + }); }); describe('setting inferenceAccelerators', () => { From 4d434b5e74630333916ff263e2fdbfb2eb2aba07 Mon Sep 17 00:00:00 2001 From: juinqqn <54617982+juinquok@users.noreply.github.com> Date: Tue, 26 Dec 2023 11:44:56 +0800 Subject: [PATCH 3/3] use addContainer method from parent class and update tests --- .../aws-ecs/lib/ec2/ec2-task-definition.ts | 5 +- .../test/ec2/ec2-task-definition.test.ts | 71 +++++++++++++++++-- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts b/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts index f831775b7e3d7..e4092f76d9cf5 100644 --- a/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts +++ b/packages/aws-cdk-lib/aws-ecs/lib/ec2/ec2-task-definition.ts @@ -154,8 +154,7 @@ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinit */ override addContainer(id: string, props: ContainerDefinitionOptions): ContainerDefinition { if (this.networkMode === NetworkMode.AWS_VPC) { - return new ContainerDefinition(this, id, { - taskDefinition: this, + return super.addContainer(id, { ...props, environment: { ...props.environment, @@ -164,6 +163,6 @@ export class Ec2TaskDefinition extends TaskDefinition implements IEc2TaskDefinit }); } // If network mode is not AWSVPC, then just add the container as normal - return new ContainerDefinition(this, id, { taskDefinition: this, ...props }); + return super.addContainer(id, props); } } diff --git a/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts b/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts index ede45583332dc..a7109bfc5c7d8 100644 --- a/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts +++ b/packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-task-definition.test.ts @@ -1111,7 +1111,35 @@ describe('ec2 task definition', () => { }); }); - test('correctly sets env variables when using EC2 capacity provider with AWSVPC mode', () => { + test('correctly sets env variables when using EC2 capacity provider with AWSVPC mode - with no other user-defined env variables', () => { + // GIVEN AWS-VPC network mode + const stack = new cdk.Stack(); + const taskDefiniton = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { + networkMode: ecs.NetworkMode.AWS_VPC, + }); + taskDefiniton.addContainer('some-container', { + image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + memoryLimitMiB: 512, + }); + + // THEN it should include the AWS_REGION env variable - when no user defined env variables are provided + Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { + NetworkMode: ecs.NetworkMode.AWS_VPC, + ContainerDefinitions: [{ + Name: 'some-container', + Image: 'amazon/amazon-ecs-sample', + Memory: 512, + Environment: [{ + Name: 'AWS_REGION', + Value: { + Ref: 'AWS::Region', + }, + }], + }], + }); + }); + + test('correctly sets env variables when using EC2 capacity provider with AWSVPC mode - with other user-defined env variables', () => { // GIVEN AWS-VPC network mode const stack = new cdk.Stack(); const taskDefiniton = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { @@ -1143,13 +1171,15 @@ describe('ec2 task definition', () => { }], }], }); + }); + test('correctly sets env variables when using EC2 capacity provider with HOST mode', () => { // GIVEN HOST network mode - const anotherStack = new cdk.Stack(); - const anotherTaskDefiniton = new ecs.Ec2TaskDefinition(anotherStack, 'Ec2TaskDef', { + const stack = new cdk.Stack(); + const taskDefiniton = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { networkMode: ecs.NetworkMode.HOST, }); - anotherTaskDefiniton.addContainer('some-container', { + taskDefiniton.addContainer('some-container', { image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), memoryLimitMiB: 512, environment: { @@ -1157,8 +1187,8 @@ describe('ec2 task definition', () => { }, }); - // THEN it should add in any env variables - Template.fromStack(anotherStack).hasResourceProperties('AWS::ECS::TaskDefinition', { + // THEN it should not include the AWS_REGION env variable + Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { NetworkMode: ecs.NetworkMode.HOST, ContainerDefinitions: [{ Name: 'some-container', @@ -1171,6 +1201,35 @@ describe('ec2 task definition', () => { }], }); }); + + test('correctly sets env variables when using EC2 capacity provider with BRIDGE mode', () => { + // GIVEN HOST network mode + const stack = new cdk.Stack(); + const taskDefiniton = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { + networkMode: ecs.NetworkMode.BRIDGE, + }); + taskDefiniton.addContainer('some-container', { + image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + memoryLimitMiB: 512, + environment: { + SOME_VARIABLE: 'some-value', + }, + }); + + // THEN it should not include the AWS_REGION env variable + Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { + NetworkMode: ecs.NetworkMode.BRIDGE, + ContainerDefinitions: [{ + Name: 'some-container', + Image: 'amazon/amazon-ecs-sample', + Memory: 512, + Environment: [{ + Name: 'SOME_VARIABLE', + Value: 'some-value', + }], + }], + }); + }); }); describe('setting inferenceAccelerators', () => {