Skip to content

Add warnings about direct use of ImageSource #2742

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

Merged
merged 2 commits into from
Apr 4, 2025

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Feb 28, 2025

Cc: @Luap99 , I’ve noticed some problematic users recently. I’ll also audit our codebases.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mtrmac
Copy link
Collaborator Author

mtrmac commented Mar 3, 2025

Switching to draft, per containers/buildah#6014 (review) :

The godoc for the ImageSource type mentions that validation of things is left up to the caller, but it's not clear to me that switching to using the Unparsed... family of types and functions was how we were intended to do it. Is it that Image.Manifest() verifies the manifest's digest while caching it, but ImageSource.GetManifest() doesn't?

  • Update/review the documentation at the top level ImageSource … I must have been completely blind to that already existing
  • Document what UnparsedInstance guarantees.

@mtrmac mtrmac marked this pull request as draft March 3, 2025 22:23
mtrmac added 2 commits April 2, 2025 21:44
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 2, 2025

Updated, PTAL.

@mtrmac mtrmac marked this pull request as ready for review April 2, 2025 19:45
@@ -15,6 +15,9 @@ type UnparsedImage = image.UnparsedImage
// UnparsedInstance returns a types.UnparsedImage implementation for (source, instanceDigest).
// If instanceDigest is not nil, it contains a digest of the specific manifest instance to retrieve (when the primary manifest is a manifest list).
//
// This implementation of [types.UnparsedImage] ensures that [types.UnparsedImage.Manifest] validates the image
// against instanceDigest if set, or, if not, a digest implied by src.Reference, if any.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The “implied by” is intentionally a bit vague, there is #1049 and I’m not yet sure how to handle that. For now, we must provide this feature for at least registry references, and in that case the meaning is clear enough.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mtrmac mtrmac merged commit f1abed7 into containers:main Apr 4, 2025
10 checks passed
@mtrmac mtrmac deleted the digest-warning branch April 4, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants