Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Untag() to oci.Store #648

Closed
eiffel-fl opened this issue Nov 23, 2023 · 5 comments
Closed

Add Untag() to oci.Store #648

eiffel-fl opened this issue Nov 23, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@eiffel-fl
Copy link
Contributor

Hi.

There is currently a way to tag OCI descriptors, but not to untag them.
This can cause problems when you want to remove container image.
Indeed, in the following scenario, we have two tags pointing to the same underlying descriptor, as shown by the digest:

$ ig image list
REPOSITORY            TAG    DIGEST
docker.io/library/bar latest f959f580ba01
docker.io/library/foo latest f959f580ba01

In this case, we should not try to remove the image, as removing the descriptor will remove it for both image, but rather to untag one image:

$ ig remove docker.io/library/foo:latest
$ ig image list
REPOSITORY            TAG    DIGEST
docker.io/library/bar latest f959f580ba01

To do so, I opened #647 but we should first discuss this here before going further with the PR.
If you want more details on my use case, I invite you to check inspektor-gadget/inspektor-gadget#2162.

Best regards.

@wangxiaoxuan273
Copy link
Contributor

I think of a design question regarding this feature: If the given reference string is a digest, what should the behavior of Untag() be? What behavior satisfies your scenario? @eiffel-fl

@eiffel-fl
Copy link
Contributor Author

eiffel-fl commented Dec 4, 2023

Hi!

I think of a design question regarding this feature: If the given reference string is a digest, what should the behavior of Untag() be? What behavior satisfies your scenario? @eiffel-fl

Maybe it could be better for implementation to have a digest as Untag() argument?
To delete a tag, caller would have to call the following:

	descriptor, err := ociStore.Resolve(ctx, tag)
	if err != nil {
		return nil, fmt.Errorf("resolving src: %w", err)
	}
	ociStore.Untag(ctx, tag)

Also, it would avoid repeating the code as pointed in your comment.

Best regards.

@yizha1 yizha1 added the enhancement New feature or request label Dec 5, 2023
@wangxiaoxuan273
Copy link
Contributor

Hi @eiffel-fl,

We have discussed internally in our team, in general we like the idea of Untag and we think you may continue the work in PR #647. You can add unit tests now, we will review it and get it merged.

Here are a few things we would like your Untag implementation to have:

  1. Your implementation is only for oci.Store for now. Probably we need to have Untag for other types later. So how about you add an Untagger interface in content.resolver.go ? It may just look like the Tagger interface. Its signature would be Untag(ctx context.Context, reference string) error.
  2. For the behavior of the Store.Untag() function, if the input is a valid reference, it should perform untag operations. But if the reference is a valid digest of an existing content, it should do nothing. I think your last implementation is good, you just need to add an extra case of handling digest input. You can use s.tagResolver.Resolve(ctx, reference) and check if reference is a valid digest.

Let us know if there are further concerns. If you want, our team can co-author on PR #647 to speed up progress.

@eiffel-fl
Copy link
Contributor Author

Hi @eiffel-fl,

Hi!

We have discussed internally in our team, in general we like the idea of Untag and we think you may continue the work in PR #647. You can add unit tests now, we will review it and get it merged.
Here are a few things we would like your Untag implementation to have:

1. Your implementation is only for `oci.Store` for now. Probably we need to have `Untag` for other types later. So how about you add an `Untagger` interface in `content.resolver.go` ? It may just look like the `Tagger` interface. Its signature would be `Untag(ctx context.Context, reference string) error`.

2. For the behavior of the `Store.Untag()` function, if the input is a valid reference, it should perform untag operations. But if the reference is a valid digest of an existing content, it should do nothing. I think your last implementation is good, you just need to add an extra case of handling digest input. You can use `s.tagResolver.Resolve(ctx, reference)` and check if `reference` is a valid digest.

Let us know if there are further concerns. If you want, our team can co-author on PR #647 to speed up progress.

Sure! I will polish everything, as I first wanted to have your opinion before writing "fully polished code"!
I should be able to handle it, but I do not have a broad knowledge about OCI, so I may ask for advice on the PR itself!

Best regards.

@eiffel-fl
Copy link
Contributor Author

Closing as addressed in #647.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants