diff --git a/libimage/filters.go b/libimage/filters.go index ff50321b7..995f89c78 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -394,10 +394,12 @@ func filterID(value string) filterFunc { } } -// filterDigest creates an digest filter for matching the specified value. +// filterDigest creates a digest filter for matching the specified value. func filterDigest(value string) filterFunc { + // TODO: return an error if value is not a digest + // if _, err := digest.Parse(value); err != nil {...} return func(img *Image) (bool, error) { - return string(img.Digest()) == value, nil + return img.hasDigest(value), nil } } diff --git a/libimage/filters_test.go b/libimage/filters_test.go index 12091b130..6315c5a8b 100644 --- a/libimage/filters_test.go +++ b/libimage/filters_test.go @@ -63,6 +63,19 @@ func TestFilterReference(t *testing.T) { {"busybox", 1}, {"alpine", 1}, {"alpine@" + alpine.Digest().String(), 1}, + {"alpine:latest@" + alpine.Digest().String(), 1}, + {"quay.io/libpod/alpine@" + alpine.Digest().String(), 1}, + {"quay.io/libpod/alpine:latest@" + alpine.Digest().String(), 1}, + // Make sure that tags are ignored + {"alpine:ignoreme@" + alpine.Digest().String(), 1}, + {"alpine:123@" + alpine.Digest().String(), 1}, + {"quay.io/libpod/alpine:hurz@" + alpine.Digest().String(), 1}, + {"quay.io/libpod/alpine:456@" + alpine.Digest().String(), 1}, + // Make sure that repo and digest must match + {"alpine:busyboxdigest@" + busybox.Digest().String(), 0}, + {"alpine:busyboxdigest@" + busybox.Digest().String(), 0}, + {"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String(), 0}, + {"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String(), 0}, } { listOptions := &ListImagesOptions{ Filters: []string{"reference=" + test.filter}, diff --git a/libimage/image.go b/libimage/image.go index da4ff8b7a..86c1c08d7 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -144,6 +144,9 @@ func (i *Image) ID() string { // possibly many digests that we have stored for the image, so many // applications are better off using the entire list returned by Digests(). func (i *Image) Digest() digest.Digest { + // TODO: we return the image digest or the one of the manifest list + // which can lead to issues depending on the callers' assumptions. + // Hence, deprecate in favor of Digest_s_. return i.storageImage.Digest } @@ -154,6 +157,18 @@ func (i *Image) Digests() []digest.Digest { return i.storageImage.Digests } +// hasDigest returns whether the specified value matches any digest of the +// image. +func (i *Image) hasDigest(value string) bool { + // TODO: change the argument to a typed digest.Digest + for _, d := range i.Digests() { + if string(d) == value { + return true + } + } + return false +} + // IsReadOnly returns whether the image is set read only. func (i *Image) IsReadOnly() bool { return i.storageImage.ReadOnly @@ -656,6 +671,8 @@ func (i *Image) NamedTaggedRepoTags() ([]reference.NamedTagged, error) { // NamedRepoTags returns the repotags associated with the image as a // slice of reference.Named. func (i *Image) NamedRepoTags() ([]reference.Named, error) { + // FIXME: the NamedRepoTags name is a bit misleading as it can return + // repo@digest values if that’s how an image was pulled. var repoTags []reference.Named for _, name := range i.Names() { parsed, err := reference.Parse(name) @@ -669,32 +686,37 @@ func (i *Image) NamedRepoTags() ([]reference.Named, error) { return repoTags, nil } -// inRepoTags looks for the specified name/tag pair in the image's repo tags. -func (i *Image) inRepoTags(namedTagged reference.NamedTagged) (reference.Named, error) { +// inRepoTags looks for the specified name/tag in the image's repo tags. If +// `ignoreTag` is set, only the repo must match and the tag is ignored. +func (i *Image) inRepoTags(namedTagged reference.NamedTagged, ignoreTag bool) (reference.Named, error) { repoTags, err := i.NamedRepoTags() if err != nil { return nil, err } - pairs, err := ToNameTagPairs(repoTags) - if err != nil { - return nil, err - } - name := namedTagged.Name() tag := namedTagged.Tag() - for _, pair := range pairs { - if tag != pair.Tag { - continue + for _, r := range repoTags { + if !ignoreTag { + var repoTag string + tagged, isTagged := r.(reference.NamedTagged) + if isTagged { + repoTag = tagged.Tag() + } + if !isTagged || tag != repoTag { + continue + } } - if !strings.HasSuffix(pair.Name, name) { + + repoName := r.Name() + if !strings.HasSuffix(repoName, name) { continue } - if len(pair.Name) == len(name) { // full match - return pair.named, nil + if len(repoName) == len(name) { // full match + return r, nil } - if pair.Name[len(pair.Name)-len(name)-1] == '/' { // matches at repo - return pair.named, nil + if repoName[len(repoName)-len(name)-1] == '/' { // matches at repo + return r, nil } } diff --git a/libimage/image_test.go b/libimage/image_test.go index c22cec6bc..bc671684e 100644 --- a/libimage/image_test.go +++ b/libimage/image_test.go @@ -182,6 +182,64 @@ func TestImageFunctions(t *testing.T) { require.Equal(t, image.NamesHistory(), imageData.NamesHistory, "inspect data should match") } +func TestLookupImage(t *testing.T) { + alpineNoTag := "quay.io/libpod/alpine" + alpineLatest := alpineNoTag + ":latest" + + runtime, cleanup := testNewRuntime(t) + defer cleanup() + ctx := context.Background() + + pullOptions := &PullOptions{} + pullOptions.Writer = os.Stdout + + pulledImages, err := runtime.Pull(ctx, alpineLatest, config.PullPolicyMissing, pullOptions) + require.NoError(t, err) + require.Len(t, pulledImages, 1) + alpine := pulledImages[0] + + digestStr := alpine.Digest().String() + alpineDigest := alpineNoTag + "@" + digestStr + + for _, test := range []struct { + input string + expectedName string + mustFail bool + }{ + // Name only + {"alpine", alpineLatest, false}, + {"alpine:latest", alpineLatest, false}, + {"alpine:wrongtag", "", true}, + {"alpine@" + digestStr, alpineDigest, false}, + {"alpine:latest@" + digestStr, alpineDigest, false}, // Tag will be trimmed + {"alpine:wrongtag@" + digestStr, alpineDigest, false}, // Tag will be ignored and trimmed + // Repo + name + {"libpod/alpine", alpineLatest, false}, + {"libpod/alpine:latest", alpineLatest, false}, + {"libpod/alpine:wrongtag", "", true}, + {"libpod/alpine@" + digestStr, alpineDigest, false}, + {"libpod/alpine:latest@" + digestStr, alpineDigest, false}, // Tag will be trimmed + {"libpod/alpine:wrongtag@" + digestStr, alpineDigest, false}, // Tag will be ignored and trimmed + // Domain + repo + name + {alpineNoTag, alpineLatest, false}, + {alpineLatest, alpineLatest, false}, + {alpineNoTag + ":wrongtag", "", true}, + {alpineDigest, alpineDigest, false}, + {alpineNoTag + ":latest@" + digestStr, alpineDigest, false}, // Tag will be trimmed + {alpineNoTag + ":wrongtag@" + digestStr, alpineDigest, false}, // Tag will be ignored and trimmed + } { + resolvedImage, resolvedName, err := runtime.LookupImage(test.input, nil) + if test.mustFail { + require.Error(t, err) + continue + } + require.NoError(t, err) + require.NotNil(t, resolvedImage) + require.Equal(t, alpine.ID(), resolvedImage.ID()) + require.Equal(t, test.expectedName, resolvedName, "input resolved to the expected name") + } +} + func TestInspectHealthcheck(t *testing.T) { runtime, cleanup := testNewRuntime(t) defer cleanup() diff --git a/libimage/manifest_list.go b/libimage/manifest_list.go index 3a75709e0..0223fb355 100644 --- a/libimage/manifest_list.go +++ b/libimage/manifest_list.go @@ -217,6 +217,11 @@ func (i *Image) getManifestList() (manifests.List, error) { // image index (OCI). This information may be critical to make certain // execution paths more robust (e.g., suppress certain errors). func (i *Image) IsManifestList(ctx context.Context) (bool, error) { + // FIXME: due to `ImageDigestBigDataKey` we'll always check the + // _last-written_ manifest which is causing issues for multi-arch image + // pulls. + // + // See https://github.com/containers/common/pull/1505#discussion_r1242677279. ref, err := i.StorageReference() if err != nil { return false, err diff --git a/libimage/normalize.go b/libimage/normalize.go index bb3cdbc7c..9619b1a0d 100644 --- a/libimage/normalize.go +++ b/libimage/normalize.go @@ -100,22 +100,22 @@ func ToNameTagPairs(repoTags []reference.Named) ([]NameTagPair, error) { // normalizeTaggedDigestedString strips the tag off the specified string iff it // is tagged and digested. Note that the tag is entirely ignored to match // Docker behavior. -func normalizeTaggedDigestedString(s string) (string, error) { +func normalizeTaggedDigestedString(s string) (string, reference.Named, error) { // Note that the input string is not expected to be parseable, so we // return it verbatim in error cases. ref, err := reference.Parse(s) if err != nil { - return "", err + return "", nil, err } named, ok := ref.(reference.Named) if !ok { - return s, nil + return s, nil, nil } named, err = normalizeTaggedDigestedNamed(named) if err != nil { - return "", err + return "", nil, err } - return named.String(), nil + return named.String(), named, nil } // normalizeTaggedDigestedNamed strips the tag off the specified named diff --git a/libimage/normalize_test.go b/libimage/normalize_test.go index 206c0eb35..9b9dfb464 100644 --- a/libimage/normalize_test.go +++ b/libimage/normalize_test.go @@ -132,12 +132,14 @@ func TestNormalizeTaggedDigestedString(t *testing.T) { {"localhost/fedora:anothertag" + digestSuffix, "localhost/fedora" + digestSuffix}, {"localhost:5000/fedora:v1.2.3.4.5" + digestSuffix, "localhost:5000/fedora" + digestSuffix}, } { - res, err := normalizeTaggedDigestedString(test.input) + res, named, err := normalizeTaggedDigestedString(test.input) if test.expected == "" { assert.Error(t, err, "%v", test) } else { assert.NoError(t, err, "%v", test) assert.Equal(t, test.expected, res, "%v", test) + assert.NotNil(t, named, "%v", test) + assert.Equal(t, res, named.String(), "%v", test) } } } diff --git a/libimage/pull.go b/libimage/pull.go index 188ecb5ef..296d003a8 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -86,7 +86,7 @@ func (r *Runtime) Pull(ctx context.Context, name string, pullPolicy config.PullP // Docker compat: strip off the tag iff name is tagged and digested // (e.g., fedora:latest@sha256...). In that case, the tag is stripped // off and entirely ignored. The digest is the sole source of truth. - normalizedName, normalizeError := normalizeTaggedDigestedString(name) + normalizedName, _, normalizeError := normalizeTaggedDigestedString(name) if normalizeError != nil { return nil, normalizeError } diff --git a/libimage/runtime.go b/libimage/runtime.go index 95da83bb9..7707d2e3b 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -16,6 +16,7 @@ import ( "github.com/containers/storage" deepcopy "github.com/jinzhu/copier" jsoniter "github.com/json-iterator/go" + "github.com/opencontainers/go-digest" "github.com/sirupsen/logrus" ) @@ -239,7 +240,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, // Docker compat: strip off the tag iff name is tagged and digested // (e.g., fedora:latest@sha256...). In that case, the tag is stripped // off and entirely ignored. The digest is the sole source of truth. - normalizedName, err := normalizeTaggedDigestedString(name) + normalizedName, possiblyUnqualifiedNamedReference, err := normalizeTaggedDigestedString(name) if err != nil { return nil, "", err } @@ -259,7 +260,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, // If the name clearly refers to a local image, try to look it up. if byFullID || byDigest { - img, err := r.lookupImageInLocalStorage(originalName, name, options) + img, err := r.lookupImageInLocalStorage(originalName, name, nil, options) if err != nil { return nil, "", err } @@ -297,7 +298,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, } for _, candidate := range candidates { - img, err := r.lookupImageInLocalStorage(name, candidate.String(), options) + img, err := r.lookupImageInLocalStorage(name, candidate.String(), candidate, options) if err != nil { return nil, "", err } @@ -308,7 +309,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, // The specified name may refer to a short ID. Note that this *must* // happen after the short-name expansion as done above. - img, err := r.lookupImageInLocalStorage(name, name, options) + img, err := r.lookupImageInLocalStorage(name, name, nil, options) if err != nil { return nil, "", err } @@ -316,21 +317,51 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, return img, name, err } - return r.lookupImageInDigestsAndRepoTags(name, options) + return r.lookupImageInDigestsAndRepoTags(name, possiblyUnqualifiedNamedReference, options) } // lookupImageInLocalStorage looks up the specified candidate for name in the // storage and checks whether it's matching the system context. -func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *LookupImageOptions) (*Image, error) { +func (r *Runtime) lookupImageInLocalStorage(name, candidate string, namedCandidate reference.Named, options *LookupImageOptions) (*Image, error) { logrus.Debugf("Trying %q ...", candidate) - img, err := r.store.Image(candidate) - if err != nil && !errors.Is(err, storage.ErrImageUnknown) { - return nil, err - } - if img == nil { - return nil, nil + + var err error + var img *storage.Image + var ref types.ImageReference + + // FIXME: the lookup logic for manifest lists needs improvement. + // See https://github.com/containers/common/pull/1505#discussion_r1242677279 + // for details. + + // For images pulled by tag, Image.Names does not currently contain a + // repo@digest value, so such an input would not match directly in + // c/storage. + if namedCandidate != nil { + namedCandidate = reference.TagNameOnly(namedCandidate) + ref, err = storageTransport.Transport.NewStoreReference(r.store, namedCandidate, "") + if err != nil { + return nil, err + } + img, err = storageTransport.Transport.GetStoreImage(r.store, ref) + if err != nil { + if errors.Is(err, storage.ErrImageUnknown) { + return nil, nil + } + return nil, err + } + // NOTE: we must reparse the reference another time below since + // an ordinary image may have resolved into a per-platform image + // without any regard to options.{Architecture,OS,Variant}. + } else { + img, err = r.store.Image(candidate) + if err != nil { + if errors.Is(err, storage.ErrImageUnknown) { + return nil, nil + } + return nil, err + } } - ref, err := storageTransport.Transport.ParseStoreReference(r.store, img.ID) + ref, err = storageTransport.Transport.ParseStoreReference(r.store, img.ID) if err != nil { return nil, err } @@ -417,76 +448,71 @@ func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *Loo // lookupImageInDigestsAndRepoTags attempts to match name against any image in // the local containers storage. If name is digested, it will be compared // against image digests. Otherwise, it will be looked up in the repo tags. -func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, options *LookupImageOptions) (*Image, string, error) { - // Until now, we've tried very hard to find an image but now it is time - // for limbo. If the image includes a digest that we couldn't detect - // verbatim in the storage, we must have a look at all digests of all - // images. Those may change over time (e.g., via manifest lists). - // Both Podman and Buildah want us to do that dance. - allImages, err := r.ListImages(context.Background(), nil, nil) - if err != nil { - return nil, "", err - } +func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifiedNamedReference reference.Named, options *LookupImageOptions) (*Image, string, error) { + originalName := name // we may change name below - ref, err := reference.Parse(name) // Warning! This is not ParseNormalizedNamed - if err != nil { - return nil, "", err - } - named, isNamed := ref.(reference.Named) - if !isNamed { - return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown) + if possiblyUnqualifiedNamedReference == nil { + return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) } - digested, isDigested := named.(reference.Digested) + // In case of a digested reference, we strip off the digest and require + // any image matching the repo/tag to also match the specified digest. + var requiredDigest digest.Digest + digested, isDigested := possiblyUnqualifiedNamedReference.(reference.Digested) if isDigested { - logrus.Debug("Looking for image with matching recorded digests") - digest := digested.Digest() - for _, image := range allImages { - for _, d := range image.Digests() { - if d != digest { - continue - } - // Also make sure that the matching image fits all criteria (e.g., manifest list). - if _, err := r.lookupImageInLocalStorage(name, image.ID(), options); err != nil { - return nil, "", err - } - return image, name, nil - - } - } - return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown) + requiredDigest = digested.Digest() + possiblyUnqualifiedNamedReference = reference.TrimNamed(possiblyUnqualifiedNamedReference) + name = possiblyUnqualifiedNamedReference.String() } if !shortnames.IsShortName(name) { - return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown) + return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) } - named = reference.TagNameOnly(named) // Make sure to add ":latest" if needed - namedTagged, isNammedTagged := named.(reference.NamedTagged) - if !isNammedTagged { - // NOTE: this should never happen since we already know it's - // not a digested reference. - return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", name, storage.ErrImageUnknown) + // Docker compat: make sure to add the "latest" tag if needed. The tag + // will be ignored if we're looking for a digest match. + possiblyUnqualifiedNamedReference = reference.TagNameOnly(possiblyUnqualifiedNamedReference) + namedTagged, isNamedTagged := possiblyUnqualifiedNamedReference.(reference.NamedTagged) + if !isNamedTagged { + // NOTE: this should never happen since we already stripped off + // the digest. + return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", originalName, storage.ErrImageUnknown) + } + + allImages, err := r.ListImages(context.Background(), nil, nil) + if err != nil { + return nil, "", err } for _, image := range allImages { - named, err := image.inRepoTags(namedTagged) + named, err := image.inRepoTags(namedTagged, isDigested) if err != nil { return nil, "", err } if named == nil { continue } - img, err := r.lookupImageInLocalStorage(name, named.String(), options) + img, err := r.lookupImageInLocalStorage(name, named.String(), named, options) if err != nil { return nil, "", err } if img != nil { - return img, named.String(), err + if isDigested { + if !img.hasDigest(requiredDigest.String()) { + continue + } + named = reference.TrimNamed(named) + canonical, err := reference.WithDigest(named, requiredDigest) + if err != nil { + return nil, "", fmt.Errorf("building canonical reference with digest %q and matched %q: %w", requiredDigest.String(), named.String(), err) + } + return img, canonical.String(), nil + } + return img, named.String(), nil } } - return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown) + return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) } // ResolveName resolves the specified name. If the name resolves to a local