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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

kirintwn
Copy link
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

gitpod-io bot commented Aug 7, 2021

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

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

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

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

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

@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

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

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
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
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
Contributor

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

@rittneje rittneje rittneje left review comments

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

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