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

Add an imgpkg delete command. #239

Open
GrahamDumpleton opened this issue Sep 8, 2021 · 4 comments
Open

Add an imgpkg delete command. #239

GrahamDumpleton opened this issue Sep 8, 2021 · 4 comments
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request Hacktoberfest Issues that are ready for Hacktoberfest participants. priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.

Comments

@GrahamDumpleton
Copy link

Describe the problem/challenge you have

I want to be able to delete OCI artifacts in an image registry created by imgpkg push or imgpkg copy using imgpkg, rather than having to login to the web interface for the image registry, use a separate tools such as skopeo or by using a tool like curl against the image registry REST API directly to delete it.

Describe the solution you'd like

I want to see an imgpkg delete command. So if I originally did:

imgpkg push -i your-user/app1-config:0.1.1 -f config/

I want to then be able to do:

imgpkg delete your-user/app1-config:0.1.1

The imgpkg delete command should by default work like kapp and provide a metadata summary and prompt you whether you do want to delete the image or not. If you what to delete it without being prompted, then you would be able to supply a -y option.

Note that the expectation is that imgpkg delete would verify that the image you are asking to delete was in fact created by imgpkg and it would not by default delete arbitrary images. Attempting to delete an OCI artifact not created by imgpkg would be an error, unless you supplied a --force option.

Note that am not distinguishing between an image (-i) and a bundle (-b) when doing impkg delete as don't see the need to. The fact that it verifies that it at least was something created by imgpkg is enough.

In addition to deleting a single OCI artifact for an image or bundle, consideration also needs to be given to being able to delete a set of OCI artifacts created as a result of imgpkg copy. The imgpkg delete command in this case should be able to identify somehow that when deleting a bundle, that it was created using imgpkg copy and had associated OCI artifacts that go with it. In this case when prompted, the metadata should show a list of all the associated image artifacts and confirming deletion should delete all of them. As it stands this may not be possible since assume that the copied bundle doesn't contain any metadata or audit trail information to record the fact that there were associated images that go with the bundle which would also need to be deleted. That audit trail information should exist for debugging and other tracking reasons anyway, so if not present should be added for that reason anyway, as well as being useful in this case of imgpkg delete for deleting all associated images.

Note that for this latter use case of deleting everything created when doing a imgpkg copy, then perhaps by default it could warn there are associated images and list them, but warn they will not be deleted, and require an --all-images option to say that all associated images should also be deleted in addition to the primary bundle. If a bundle results in other bundles being copied, then maybe an --all-bundles option should be supplied to effectively do the delete recursively.

Anything else you would like to add:

An imgpkg delete command would be a useful complement to imgpkg push and imgpkg copy and from a user perspective would make the imgpkg tool look more complete as it wouldn't be necessary to go seek out a completely different way of deleting the images from the registry created by imgpkg.

cc @jorgemoralespou


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@GrahamDumpleton GrahamDumpleton added carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request labels Sep 8, 2021
@GrahamDumpleton GrahamDumpleton changed the title Add a imgpkg delete command. Add an imgpkg delete command. Sep 8, 2021
@cppforlife
Copy link
Contributor

quick thoughts so far:

  • we should still use -i/-b flags for delete to stay consistent with other commands (all carvel tools use flags for args)
  • when -b is provided we can use --recursive (default=false) to indicate whether we want to delete referenced assets
    • what happens when referenced assets live in different registries? do we only want to delete from the registry where bundle is located? still unsolved problem of multiple bundles use the same assets...
  • -i should be able to delete any image (including bundles)

@GrahamDumpleton
Copy link
Author

It doesn't make sense to me that if going to require -i and -b that -i would still delete any image type, including bundles.

Why would stuff in other registries ever matter? You would only delete assets in the same registry as you are targeting, and only if they were put their due to an imgpkg copy.

Dealing with other registries would likely fail anyway due to current issues with only being able to deal with the auth credentials of one registry.

@cppforlife
Copy link
Contributor

It doesn't make sense to me that if going to require -i and -b that -i would still delete any image type, including bundles.

bundles are images too. -i actually allows to pull bundles as plain images via --image-is-bundle-check=false flag.

Why would stuff in other registries ever matter? You would only delete assets in the same registry as you are targeting, and only if they were put their due to an imgpkg copy.

copy is only one case. if user did imgpkg push then they would want to be able to delete it as well. if that pushed bundle points to multiple registries (or multiple repositories within the same registry), there is an argument to be made for being able to delete them.

Dealing with other registries would likely fail anyway due to current issues with only being able to deal with the auth credentials of one registry.

you can auth creds to multiple registries based on their hostname. see auth docs.

@DennisDenuto
Copy link
Contributor

DennisDenuto commented Sep 14, 2021

Thanks for creating this issue!

From a UX perspective:

  • I like prompting the user about all the different artifacts that will be deleted. If a user wants to delete a bundle, then imgpkg should output: bundle, nested bundles, dependent images, tags and metadata images (such as signatures and image-locations) should all be listed. (FWIW This has some overlap with the describe command (this describe command 'code' can be re-used here, or vice-versa whichever issue gets done first). Create a describe command for a bundle #124)

  • @cppforlife --recursive (default=false) I'm assuming this should also recursively delete nested-bundles.

  • @GrahamDumpleton I think that by requiring -i for 'any' image and -b for bundles is not only consistent, but has the benefit of symmetry with the copy command. Meaning, copy -b copies the bundle and copies the dependent images, delete -b will delete the bundle and delete the dependent images. That symmetry I hope will make this command more intuitive for our users.

As @cppforlife points out though, "deleting the dependent images belonging to a bundle" needs further clarification. I think as a starting point we can try and be flexible and delete images referenced in a bundle even if they are referenced in multiple registries. Authentication would be possible via env variables. I don't think flexibility will cost much additional engineering time, and should help cover more use-cases. Lastly, I think an additional flag to toggle deleting images that are co-located with the bundle should be introduced to change that 'default' delete behavior wrt bundles.

  • --lock should also be supported if this command is to be used / compatible with the copy command.

Feedback:

That audit trail information should exist for debugging and other tracking reasons anyway

I think that in order to delete a bundle and any referenced images we have enough information present, without requiring an audit trail. We can inspect the bundle's images.yml file or inspect the images-location OCI image that is created during a copy.

Also, in terms of what is responsible for auditing (write/get/delete operations) on the registry. I think the registry should assume that responsibility (some already do). Let me know if you had some other idea of what imgpkg auditing should be.

I'm going to carvel accept this issue, meaning we plan on working on this in the future. We can continue to flesh out the details in this thread.

Note:

  • what order should we delete resources in? consider what should happen if a delete fails in the 'middle'. should we try and delete from the bottom of the image graph -> root bundle?
  • deleting a bundle should also delete metadata such as image-locations, signatures and tags

@DennisDenuto DennisDenuto added carvel accepted This issue should be considered for future work and that the triage process has been completed priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. and removed carvel triage This issue has not yet been reviewed for validity labels Sep 14, 2021
@DennisDenuto DennisDenuto added the Hacktoberfest Issues that are ready for Hacktoberfest participants. label Oct 6, 2021
@aaronshurley aaronshurley moved this to To Triage in Carvel Jul 25, 2022
@github-project-automation github-project-automation bot moved this to To Triage in Carvel Feb 14, 2023
@joaopapereira joaopapereira moved this from To Triage to Unprioritized in Carvel Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request Hacktoberfest Issues that are ready for Hacktoberfest participants. priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.
Projects
Status: Unprioritized
Development

No branches or pull requests

3 participants