-
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
fix(ecs): EC2 metadata access is blocked when using EC2 capacity provider for autoscaling #28437
Conversation
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.
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case should be separated into another test
.
@@ -1110,6 +1110,67 @@ describe('ec2 task definition', () => { | |||
}], | |||
}); | |||
}); | |||
|
|||
test('correctly sets env variables when using EC2 capacity provider with AWSVPC mode', () => { |
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.
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! :) |
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.
Thanks 👍
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.
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.addAsgCapacityProvider
and when the task definition being run uses the AWS_VPC network mode, it results in the metadata service at169.254.169.254
being 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
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