Skip to content

feat(ecr): add option to auto delete images upon ECR repo removal#15932

Closed
kirintwn wants to merge 1 commit intoaws:masterfrom
kirintwn:ecr-auto-delete-images
Closed

feat(ecr): add option to auto delete images upon ECR repo removal#15932
kirintwn wants to merge 1 commit intoaws:masterfrom
kirintwn:ecr-auto-delete-images

Conversation

@kirintwn
Copy link
Copy Markdown
Contributor

@kirintwn kirintwn commented Aug 7, 2021

Closes #2765 #12618.

Use Case

ECR repositories currently do not get delete if they contain images even the removalPolicy is set to DESTROY.

It was reported in #2765, and was thought to be a responsibility of CloudFormation team aws-cloudformation/cloudformation-coverage-roadmap#515.

Proposed Solution

I do think we should add a option to force delete the images using custom resource provider, which is just like #12090 that uses a custom resource provider to delete objects in s3.

The code might looks like this:

const bucket = new ecr.Repository(this, 'Repo', {
  repositoryName: 'delete-even-if-contains-images',
  removalPolicy: cdk.RemovalPolicy.DESTROY,
  autoDeleteImages: true,
});

The props autoDeleteImages can only to be true if removalPolicy is set to DESTROY.

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

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Aug 7, 2021

@kirintwn kirintwn mentioned this pull request Aug 7, 2021
Closed
2 tasks
@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Aug 7, 2021

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 4b7ff58
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@peterwoodworth peterwoodworth added the @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry label Aug 11, 2021
@peterwoodworth peterwoodworth added the effort/small Small work item – less than a day of effort label Aug 11, 2021
@heiba
Copy link
Copy Markdown

heiba commented Aug 26, 2021 •
edited
Loading

Hello @madeline-k, hope you're well. Any chance we can have this PR reviewed please ? We are in dire need for this feature (ability to delete non-empty ECR). @kirintwn has thankfully developed it but it is waiting to be merged.

@heiba
Copy link
Copy Markdown

heiba commented Sep 3, 2021

Hello @peterwoodworth, hope you're well. Any chance we can have this PR reviewed please ? We are in dire need for this feature (ability to delete non-empty ECR). @kirintwn has thankfully developed it but it is waiting to be merged.

@heiba
Copy link
Copy Markdown

heiba commented Sep 17, 2021

Hello @madeline-k would greatly appreciate if we can have this PR reviewed and possibly merged please ?

@rittneje
Copy link
Copy Markdown

rittneje commented Sep 21, 2021

@kirintwn I have a question about your implementation. Suppose someone deploys their stack with an ECR repo with autoDeleteImages: true. That will in turn create this custom resource. But then they change their mind and deploy again with autoDeleteImages: false. Would that not cause the custom resource to be deleted, and consequently all the images in the repo would be destroyed at that time?

@rittneje
Copy link
Copy Markdown

rittneje commented Sep 22, 2021 •
edited
Loading

@kirintwn We have confirmed that the bug I described exists, so unfortunately your current implementation is not safe. The same bug affects the S3 auto_delete_objects feature, for which I will be filing a bug. #16603

rittneje
rittneje reviewed Sep 23, 2021
View reviewed changes
packages/@aws-cdk/aws-ecr/lib/auto-delete-images-handler/index.ts
* @param ECR.ListImagesRequest the repositoryName & nextToken if presented
*/
async function emptyRepository(params: ECR.ListImagesRequest) {
const listedImages = await ecr.listImages(params).promise();
Copy link
Copy Markdown

@rittneje rittneje Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kirintwn If the ECR repo was already deleted out-of-band, this will cause the custom resource deletion to fail with an error.

@ryparker
Copy link
Copy Markdown
Contributor

ryparker commented Oct 1, 2021 •
edited
Loading

@kirintwn We have confirmed that the bug I described exists, so unfortunately your current implementation is not safe. The same bug affects the S3 auto_delete_objects feature, for which I will be filing a bug. #16603

Great find! This is how we're going to patch that in the S3 package: #16756

madeline-k
madeline-k suggested changes Feb 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing this @kirintwn!

We need to include the same approach in this PR as the fix for the emptying s3 buckets issue that @rittneje and @ryparker have called out.

@madeline-k madeline-k removed their assignment Feb 9, 2022
@rix0rrr rix0rrr added feature-request A feature should be added or improved. p2 and removed effort/small Small work item – less than a day of effort @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry labels Mar 4, 2022
@TheRealAmazonKendra
Copy link
Copy Markdown
Contributor

TheRealAmazonKendra commented Mar 30, 2022

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

@randyridgley randyridgley mentioned this pull request Mar 10, 2023
Merged
mergify bot pushed a commit that referenced this pull request Mar 21, 2023
@randyridgley
feat(ecr): add option to auto delete images upon ECR repository remov…
7de5b00 
…al (#24572)

This request fixes the ECR Repository resource to allow setting a flag on the resource to auto delete the images in the repository. This is similar to the way S3 handles the autoDeleteObjects attribute. This code base starts from a stalled PR [#15932](#15932). This also takes into account the functionality added into S3 to create tag to not delete images if the flag is flipped from true to false. 

Closes [#12618](#12618)
References closed and not merged PR  [#15932](#15932)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@missourian55 missourian55 mentioned this pull request Mar 28, 2023
Closed
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
@randyridgley @homakk
feat(ecr): add option to auto delete images upon ECR repository remov…
e466e6e 
…al (aws#24572)

This request fixes the ECR Repository resource to allow setting a flag on the resource to auto delete the images in the repository. This is similar to the way S3 handles the autoDeleteObjects attribute. This code base starts from a stalled PR [aws#15932](aws#15932). This also takes into account the functionality added into S3 to create tag to not delete images if the flag is flipped from true to false. 

Closes [aws#12618](aws#12618)
References closed and not merged PR  [aws#15932](aws#15932)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@rittneje rittneje rittneje left review comments

@madeline-k madeline-k madeline-k requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature-request A feature should be added or improved. p2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ECR: support force upon deletion

9 participants

@kirintwn @aws-cdk-automation @heiba @rittneje @ryparker @TheRealAmazonKendra @madeline-k @rix0rrr @peterwoodworth