fix(ecr): autoDeleteImages fails when repository is renamed

[go-to-k]

This PR fixes the bug that ECRAutoDeleteImages fails on repo rename.

The customResource depends on the role, and when the repository name changes, the role is updated to match the new repository instead of the old one, before customResource runs and the old repository is deleted.

It was difficult to delete the old repo before the role update ran, so I changed the resource of the role to a wildcard.

Closes #26711.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

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.

[go-to-k]

Exemption Request because the unit tests covered changing role.

[rix0rrr]

Thanks for this!

I think a better solution for this would be add an AWS::IAM::Policy resource for every repository, and make sure the logical identifier of those resources is generated based off the repository name.

If we do that, standard CloudFormation sequencing rules will make everything work out correctly.

[go-to-k]

@rix0rrr

Thanks for comments!

I think a better solution for this would be add an AWS::IAM::Policy resource for every repository, and make sure the logical identifier of those resources is generated based off the repository name.

Does this mean using iam.Policy instead of addToRolePolicy?

new iam.Policy(this, `${this.repositoryName}Policy`, { // There may be differences in the rules between repositoryName and logical identifier
    roles: ... // specifying the new role instance generated from provider.roleArn
    statements: ... // for the repository
});

I still don't understand why this is good.

When the repository name is changed, the old repository is deleted, a new repository is created, and the images in the old repository are deleted by the custom resource.

At that time, wouldn’t the IAM policy then also be updated to match the new repository name, and eventually deletion of the old repository would no longer be allowed?

[rix0rrr]

Does this mean using iam.Policy instead of addToRolePolicy?

Yes.

Doing this, this.repositoryName may not be a token though so that may be hard to pull off.

I still don't understand why this is good.

Because of the order of CloudFormation resource operations during a stack update. There are two phases during a deployment:

1. CREATE/UPDATE
2. CLEANUP (in fact, DELETE)

So the policy that grants permissions to the old repository name will be deleted during the CLEANUP phase, when the actual ECR repository itself is also cleaned up.

---

I'm really not comfortable with the *.

If, because of the token-ness and unpredictability of this.repositoryName, this cannot work, a good compromise may be to put a tag on the ECR repository (cdk:allow-deleting-images=true; in fact there should already be a tag on there) and use tag-based access control with a Condition.

[go-to-k]

@rix0rrr

The repositoryName was Token. So following codes didn't work.

    const role = iam.Role.fromRoleArn(this, `${this.repositoryName}Role`, provider.roleArn);

    // Use a iam policy to allow the custom resource to list & delete
    // images in the repository and the ability to get all repositories to find the arn needed on delete.
    new iam.Policy(this, `${this.repositoryName}Policy`, {
      roles: [role],
      statements: [
        new iam.PolicyStatement({
          effect: iam.Effect.ALLOW,
          actions: [
            'ecr:BatchDeleteImage',
            'ecr:DescribeRepositories',
            'ecr:ListImages',
            'ecr:ListTagsForResource',
          ],
          resources: [this._resource.attrArn],
        }),
      ],
    });

If, because of the token-ness and unpredictability of this.repositoryName, this cannot work, a good compromise may be to put a tag on the ECR repository (cdk:allow-deleting-images=true; in fact there should already be a tag on there) and use tag-based access control with a Condition.

In that case, is this the way to go?

[go-to-k]

@rix0rrr

I used physicalName instead of repositoryName and it worked. Because this.repositoryName is a Token (lazy value), but physicalName is props.repositoryName.

However, props.repositoryName may be unspecified, in which case the props.repositoryName will be undefined and the IAM policy's logical IDs will duplicate. So this is not a good practice.

[rix0rrr]

I agree, you can't use physicalName in this case. We should probably go the tag route then, a naked * is too dangerous for my tastes.

[go-to-k]

@rix0rrr

I implemented tag-based access control with a Condition for now.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[mergify]

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).

Pull request has been modified.

[mergify]

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-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

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).