Skip to content
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

aws-ecs: Race condition in Ec2Service & FargateService between updating the TaskRole default policy and the CfnService #24880

Closed
iancaffey opened this issue Mar 30, 2023 · 2 comments · Fixed by #26070
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@iancaffey
Copy link

Describe the bug

Within Ec2Service and FargateService, there is no dependency added between the TaskRole default policy and the CfnService.

If updates are made to the TaskRole default policy (e.g. adding new inline policies), CloudFormation will update both the CfnService and the TaskRole default policy at the same time, and this can result in ECS tasks spinning up without the proper permissions being updated in IAM for the task role yet.

Expected Behavior

The TaskRole and its children (e.g. the default policy) should always deploy first within a CloudFormation stack before the ECS service.

Current Behavior

The TaskRole default policy and the ECS service deploy at the same time.

Reproduction Steps

Every usage of Ec2Service and FargateService is vulnerable to this deployment race condition risk, if they ever make edits to the task role default policy.

Possible Solution

this.node.addDependency(this.taskDefinition.taskRole);

within the BaseService constructor.

Lambda L2 Function construct does this already (see here).

Additional Information/Context

No response

CDK CLI Version

2.72.0

Framework Version

No response

Node.js Version

v16.9.1

OS

Mac OSX

Language

Typescript

Language Version

TypeScript 4.9.5

Other information

No response

@iancaffey iancaffey added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 30, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Mar 30, 2023
@peterwoodworth
Copy link
Contributor

Thanks for reporting, this makes sense!

We could probably add this in the base service class to cover both of these constructs, just right after the CfnService is created

this.resource = new CfnService(this, 'Service', {

@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p1 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 30, 2023
lpizzinidev added a commit to lpizzinidev/aws-cdk that referenced this issue Apr 8, 2023
lpizzinidev added a commit to lpizzinidev/aws-cdk that referenced this issue Apr 27, 2023
lpizzinidev added a commit to lpizzinidev/aws-cdk that referenced this issue Jun 21, 2023
lpizzinidev added a commit to lpizzinidev/aws-cdk that referenced this issue Jun 22, 2023
mergify bot added a commit to lpizzinidev/aws-cdk that referenced this issue Jun 26, 2023
@mergify mergify bot closed this as completed in #26070 Jun 26, 2023
mergify bot pushed a commit that referenced this issue Jun 26, 2023
…with CfnService (#26070)

Prevents potential race conditions on TaskRole default policy update in EC2 and Fargate services by adding a dependency on the TaskRole.
This will update the TaskRole and its children first and the service after.

Closes #24880.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet
2 participants