fix(ecs): EC2 metadata access is blocked when using EC2 capacity provider for autoscaling#28437
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
50a115e to
be56ba6
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
be56ba6 to
fbd90d7
Compare
lpizzinidev
left a comment
There was a problem hiding this comment.
Thanks 👍
I left some notes for improvements.
Also, the title should be changed to describe the bug (not the solution).
Something like fix(ecs): EC2 metadata access is blocked on auto scaling.
| 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 }); |
There was a problem hiding this comment.
| 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 }); | |
| 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); |
Let's reuse the parent's class method
| }); | ||
|
|
||
| // GIVEN HOST network mode | ||
| const anotherStack = new cdk.Stack(); |
There was a problem hiding this comment.
This case should be separated into another test.
| }); | ||
| }); | ||
|
|
||
| test('correctly sets env variables when using EC2 capacity provider with AWSVPC mode', () => { |
There was a problem hiding this comment.
Can you also add another test with awsvpc network mode and no added environment variables? (will set only AWS_REGION)
Thank You! I have updated the method and also the tests! :) |
paulhcsun
left a comment
There was a problem hiding this comment.
Great work @juinquok!
And thanks for the review @lpizzinidev!
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Why is this needed?
When adding a auto scaling group as a capacity provider using
Cluster.addAsgCapacityProviderand when the task definition being run uses the AWS_VPC network mode, it results in the metadata service at169.254.169.254being blocked . This is a security best practice as detailed here. This practice is implemented here. 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
addAsgCapacityProvideraction 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