fix(ecs): potential race condition on TaskRole default policy update with CfnService

[lpizzinidev]

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

[corymhall]

From the linked issue it looks like we should be adding a dependency on the Policy not the role. There should already be an implicit dependency on the role and in cases where the policy is updated the role itself will probably not be updated so will not be in the dependency chain.

[lpizzinidev]

Thanks for the feedback.

The default policy is a private property in the role.
Should I create a public getter to retrieve it and add a dependency with:

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

?

[iancaffey]

FWIW, the L2 Lambda Function construct does a similar thing (adding the dependency on the Role).

Seems pretty harmless to add the dependency on the task role to catch all its children too (the Role will get an implicit dependency for itself anyways from being in the task definition passed to the service).

[corymhall]

I don't think dependencies work that way. If I declare an explicit dependency on resource A, CloudFormation does not view that as adding an explicit dependency on every resource that resource A depends on. It only depends on resource A, so if resource A is not updated then it doesn't have to wait.

But we can always run tests to confirm :)

[corymhall]

We might be able to accomplish this globally though if we can assume that anytime someone adds a dependency on a role that they also want to add a dependency on the default policy (I think that is a safe assumption)

I think by adding something like this to Role

Dependable.implement(this, {
  dependencyRoots: [this, this.defaultPolicy],
});

Whenever you do addDependency(role) it will also add a dependsOn for the policy.

[iancaffey]

I don't think dependencies work that way. If I declare an explicit dependency on resource A, CloudFormation does not view that as adding an explicit dependency on every resource that resource A depends on. It only depends on resource A, so if resource A is not updated then it doesn't have to wait.

For sure, but I'm only talking about child constructs of the Role. The default policy is scoped to the role, so addDependency(role) takes care of it.

FWIW, this fix worked for us across all our services. :)

[corymhall]

ah you are right. @lpizzinidev can you add a test that asserts the dependency is added on the policy as well?

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[aws-cdk-automation]

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

[gitpod-io]

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 6a8e4c7
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[corymhall]

The DependsOn should also contain the policy resource.

[aws-cdk-automation]

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[iancaffey]

@lpizzinidev @corymhall any chance we can get this PR across the finish line? :)

[lpizzinidev]

@iancaffey @corymhall
I was looking into this, the problem is that for FargateService the default policy is not added to the dependencies array.
Could it be that the construct is not creating a default policy?
I see that defaultPolicy is initialized in the addToPrincipalPolicy but I can't find a call to the method in FargateService.
I tried adding the dependency manually in the Role as suggested here without luck (also, this step should be unnecessary as reported here).

[iancaffey]

You'll see a lot of taskDefinition.addToTaskRolePolicy(...). Or callers might directly reference taskDefinition.taskRole when adding grants.

But yeah, the default props don't end up triggering adding any permissions that would end up initializing the policy.

Could you manually add task role permissions in the unit test to trigger initializing the policy?

[lpizzinidev]

@iancaffey
Thanks for clarifying.
I've opened another PR with the changes.