From 30a0d3314dfd0ad40a56cc7c69a1ccbefe1e1871 Mon Sep 17 00:00:00 2001 From: juinqqn <54617982+juinquok@users.noreply.github.com> Date: Thu, 18 Jan 2024 09:44:35 +0800 Subject: [PATCH] fix(ecs): EC2 metadata access is blocked when using EC2 capacity provider for autoscaling (#28437) ### Why is this needed? When adding a auto scaling group as a capacity provider using `Cluster.addAsgCapacityProvider` and when the task definition being run uses the AWS_VPC network mode, it results in the metadata service at `169.254.169.254` being blocked . This is a security best practice as detailed [here](https://docs.aws.amazon.com/AmazonECS/latest/bestpracticesguide/security-iam-roles.html). This practice is implemented [here](https://github.com/aws/aws-cdk/blame/2d9de189e583186f2b77386ae4fcfff42c864568/packages/aws-cdk-lib/aws-ecs/lib/cluster.ts#L502-L504). However by doing this, some applications such as those raised in #28270 as well as the aws-otel package will not be able to source for the AWS region and thus, cause the application to crash and exit. ### What does it implement? This PR add an override to the addContainer method when using the Ec2TaskDefinition to add in the AWS_REGION environment variable to the container if the network mode is set as AWS_VPC. The region is sourced by referencing to the stack which includes this construct at synth time.This environment variable is only required in the EC2 Capacity Provider mode and not in Fargate as this issue of not being able to source for the region on startup is only present when using the EC2 Capacity Provider with the AWS_VPC networking mode. The initial issue addresses this during the `addAsgCapacityProvider` action which targets the cluster. However, we cannot mutate the task definition at that point in time thus, this change addresses it when the task definition is actually added to a service that meets all the requirements whereby the failure to source for region will occur. Updated the relevant integration tests to reflect the new environment variable being created alongside user-defined environment variables. Closes #28270 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../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 | 23 +++- .../test/ec2/ec2-task-definition.test.ts | 120 ++++++++++++++++++ 8 files changed, 196 insertions(+), 2 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 ae33b67f5c003..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 @@ -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. */ @@ -146,4 +147,22 @@ 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 super.addContainer(id, { + ...props, + environment: { + ...props.environment, + AWS_REGION: Stack.of(this).region, + }, + }); + } + // If network mode is not AWSVPC, then just add the container as normal + 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 0f6b316dd8f26..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 @@ -1110,6 +1110,126 @@ describe('ec2 task definition', () => { }], }); }); + + 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', { + 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', + }, + }], + }], + }); + }); + + test('correctly sets env variables when using EC2 capacity provider with HOST mode', () => { + // GIVEN HOST network mode + const stack = new cdk.Stack(); + const taskDefiniton = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef', { + networkMode: ecs.NetworkMode.HOST, + }); + 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.HOST, + ContainerDefinitions: [{ + Name: 'some-container', + Image: 'amazon/amazon-ecs-sample', + Memory: 512, + Environment: [{ + Name: 'SOME_VARIABLE', + Value: 'some-value', + }], + }], + }); + }); + + 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', () => {