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 image.UnparsedInstanceWithReference and storage.ResolveReference #2056

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 24, 2023

We need to implement this locally, instead of having external callers do it, because external wrapping would make the sigstore-relevant UntrustedSignatures unavailable.

See conversation in cri-o/cri-o#7046 .

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Aug 4, 2023
@mtrmac mtrmac force-pushed the unparsed-with-ref branch 2 times, most recently from fdae822 to 31f3c28 Compare September 11, 2023 16:27
@mtrmac mtrmac force-pushed the unparsed-with-ref branch 3 times, most recently from 7dae432 to ca4e107 Compare October 19, 2023 12:57
@mtrmac mtrmac changed the title WIP: Add UnparsedInstanceWithReference WIP: Add image.UnparsedInstanceWithReference and storage.ResolveReference Oct 19, 2023
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 19, 2023

Also adding storage.ResolveReference, so that something like this is possible:

ref := ParseReference(userInput)
ref2, img := ResolveReference(ref)
src := ref2.NewImageSource

while ensuring that img and src are guaranteed to refer to the same image.

@vrothberg PTAL; ResolveReference might be relevant to how c/common/libimage.Runtime.storageToImage maintains both *storage.Image and types.ImageReference for “the same” image. lookupImageInLocalStorage sort of does that manually, but that seems to lose the original name — i.e. it might then not use the manifest with the requested manifest digest.


Temporarily includes #2147 , so that ResolveReference can be tested. That will either go away, or be incorporated properly, depending on the outcome of that other PR.

This is useful for combining image data with other reference values,
e.g. to check signatures on a locally-pulled image based on
a remote-registry policy.

This was historically possible to do by simply providing an external
UnparsedInstance implementation; now that we have sigstore signatures
which can't be represent that way, and a private.UnparsedImage,
external users are not allowed to directly access/implement
private.UnparsedImage, so this needs to be an explicitly-provided operation.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
See the added comments for details.

This allows things like
> ref := ParseReference(userInput)
> ref2, img := ResolveReference(ref)
> src := ref2.NewImageSource

while ensuring that img and src are _guaranteed_ to refer
to the same image.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thank you!

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 25, 2023

This is now ready for review/merging.

cri-o/cri-o#7435 is a proof-of-concept of the ResolveReference use (in internal/storage.imageService.imageStatusByName)

@mtrmac mtrmac marked this pull request as ready for review October 25, 2023 16:43
@mtrmac mtrmac changed the title WIP: Add image.UnparsedInstanceWithReference and storage.ResolveReference Add image.UnparsedInstanceWithReference and storage.ResolveReference Oct 25, 2023
@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2023

LGTM

@rhatdan rhatdan merged commit 2ac1b9a into containers:main Oct 25, 2023
9 checks passed
@mtrmac mtrmac deleted the unparsed-with-ref branch October 30, 2023 17:37
mtrmac added a commit to mtrmac/cri-o that referenced this pull request Oct 31, 2023
... and add some temporary //nolint comments for newly-deprecated functions.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/cri-o that referenced this pull request Nov 1, 2023
... and add some temporary //nolint comments for newly-deprecated functions.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
flouthoc added a commit to flouthoc/podman that referenced this pull request Nov 2, 2023
GetStoreImage is deprecated after containers/image#2056

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc
Copy link
Contributor

flouthoc commented Nov 2, 2023

With deprecation of these API CI is forcing podman and buildah to use ResolveReference which is nice !. But it seems that error behavior of ResolvedReference is not same as GetStoreImage. Where GetStoreImage returned storage.ErrImageUnknown but ResolveReference return a formatted error for the same use-case https://github.com/containers/image/blob/main/storage/storage_reference.go#L303 which is not same as previous behavior.

For buildah side (containers/buildah#5129) I have edited changed the code but I wonder if this is a breaking change ?

@flouthoc
Copy link
Contributor

flouthoc commented Nov 2, 2023

@mtrmac ^ Is this something which is breaking change ? or am I just overthinking here.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 2, 2023

Oh, yes, I forgot to document that here.

It’s not a breaking change because ResolveReference is a new API, and the behavior of the old one didn’t change (AFAIK), but it is very surprising and the deprecation notice should document that.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 2, 2023

Alternatively, we could change the error type back… but on balance, I think returning a c/image/storage-defined error type is a bit cleaner than returning a c/storage-defined one (although the c/image one is actually an alias).

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 2, 2023

#2170 should help with the documentation, and/or change the returned error types.

mtrmac added a commit to mtrmac/cri-o that referenced this pull request Nov 29, 2023
... to include containers/image#2056 .

Also add some temporary //nolint comments for newly-deprecated functions.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants