From 9ea963d59d248dd7db0c6551212792c02ba26536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 21:52:15 +0200 Subject: [PATCH 1/9] Parse a digest in filterDigest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This causes an immediate failure on invalid values, instead of silently not matching anything. Signed-off-by: Miloslav Trmač --- libimage/filters.go | 19 +++++++++++++------ libimage/filters_test.go | 5 +++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index 995f89c78..6b6605a1a 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -11,6 +11,7 @@ import ( filtersPkg "github.com/containers/common/pkg/filters" "github.com/containers/common/pkg/timetype" "github.com/containers/image/v5/docker/reference" + "github.com/opencontainers/go-digest" "github.com/sirupsen/logrus" ) @@ -147,7 +148,11 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp filter = filterID(value) case "digest": - filter = filterDigest(value) + f, err := filterDigest(value) + if err != nil { + return nil, err + } + filter = f case "intermediate": intermediate, err := r.bool(duplicate, key, value) @@ -395,12 +400,14 @@ func filterID(value string) filterFunc { } // 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 img.hasDigest(value), nil +func filterDigest(value string) (filterFunc, error) { + d, err := digest.Parse(value) + if err != nil { + return nil, fmt.Errorf("invalid value %q for digest filter: %w", value, err) } + return func(img *Image) (bool, error) { + return img.hasDigest(d.String()), nil + }, nil } // filterIntermediate creates an intermediate filter for images. An image is diff --git a/libimage/filters_test.go b/libimage/filters_test.go index 6315c5a8b..6ecb374a6 100644 --- a/libimage/filters_test.go +++ b/libimage/filters_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/containers/common/pkg/config" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -123,6 +124,10 @@ func TestFilterDigest(t *testing.T) { require.Len(t, listedImages, test.matches, "%s -> %v", test.filter, listedImages) require.Equal(t, listedImages[0].ID(), test.id) } + _, err = runtime.ListImages(ctx, nil, &ListImagesOptions{ + Filters: []string{"digest=this-is-not-a-digest"}, + }) + assert.Error(t, err) } func TestFilterManifest(t *testing.T) { From 995965bac0f0e3d6bec6e94ba0a03b06bed4467e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 21:54:19 +0200 Subject: [PATCH 2/9] Use a digest.Digest type for the hasDigest argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior, both callers now have a value of that type. Signed-off-by: Miloslav Trmač --- libimage/filters.go | 2 +- libimage/image.go | 5 ++--- libimage/runtime.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index 6b6605a1a..0c97fe5b7 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -406,7 +406,7 @@ func filterDigest(value string) (filterFunc, error) { return nil, fmt.Errorf("invalid value %q for digest filter: %w", value, err) } return func(img *Image) (bool, error) { - return img.hasDigest(d.String()), nil + return img.hasDigest(d), nil }, nil } diff --git a/libimage/image.go b/libimage/image.go index dc47030c5..717f50f30 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -159,10 +159,9 @@ func (i *Image) Digests() []digest.Digest { // 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 +func (i *Image) hasDigest(wantedDigest digest.Digest) bool { for _, d := range i.Digests() { - if string(d) == value { + if d == wantedDigest { return true } } diff --git a/libimage/runtime.go b/libimage/runtime.go index 7707d2e3b..0d71a1e57 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -498,7 +498,7 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi } if img != nil { if isDigested { - if !img.hasDigest(requiredDigest.String()) { + if !img.hasDigest(requiredDigest) { continue } named = reference.TrimNamed(named) From 68e4499faa2467c9f083b8dc229a703992d9808b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:01:29 +0200 Subject: [PATCH 3/9] Rename inRepoTags to referenceFuzzilyMatchingRepoAndTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scary features should have scary names. Also add a comment to make it less likely that this semantics will spread. Should not change behavior. Signed-off-by: Miloslav Trmač --- libimage/image.go | 10 +++++++--- libimage/runtime.go | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/libimage/image.go b/libimage/image.go index 717f50f30..70b9d90b9 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -685,9 +685,13 @@ func (i *Image) NamedRepoTags() ([]reference.Named, error) { return repoTags, nil } -// 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) { +// referenceFuzzilyMatchingRepoAndTag checks if the image’s repo (and optionally tag) matches a fuzzy short input, +// and if so, returns the matching reference. +// If `ignoreTag` is set, only the repo must match and the tag is ignored. +// +// DO NOT ADD ANY NEW USERS OF THIS SEMANTICS. Rely on existing libimage calls like LookupImage instead, +// and handle unqualified the way it does (c/image/pkg/shortnames). +func (i *Image) referenceFuzzilyMatchingRepoAndTag(namedTagged reference.NamedTagged, ignoreTag bool) (reference.Named, error) { repoTags, err := i.NamedRepoTags() if err != nil { return nil, err diff --git a/libimage/runtime.go b/libimage/runtime.go index 0d71a1e57..5200a0308 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -485,7 +485,7 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi } for _, image := range allImages { - named, err := image.inRepoTags(namedTagged, isDigested) + named, err := image.referenceFuzzilyMatchingRepoAndTag(namedTagged, isDigested) if err != nil { return nil, "", err } From bf8905ef7ccf75b8cc99443a29cf51d9351f13bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:02:37 +0200 Subject: [PATCH 4/9] Simplify referenceFuzzilyMatchingRepoAndTag a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- libimage/image.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/libimage/image.go b/libimage/image.go index 70b9d90b9..47191b3d9 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -701,12 +701,8 @@ func (i *Image) referenceFuzzilyMatchingRepoAndTag(namedTagged reference.NamedTa tag := namedTagged.Tag() for _, r := range repoTags { if !ignoreTag { - var repoTag string tagged, isTagged := r.(reference.NamedTagged) - if isTagged { - repoTag = tagged.Tag() - } - if !isTagged || tag != repoTag { + if !isTagged || tag != tagged.Tag() { continue } } From 3c7492b39a6a47b8be3a2f4ad57907790a01f53f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:04:09 +0200 Subject: [PATCH 5/9] Move the !IsShortName early exit a bit forward MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that early exits are colocated. Should not change behavior, reference.TrimNamed() updating "name" should not change the IsShortName value. Signed-off-by: Miloslav Trmač --- libimage/runtime.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libimage/runtime.go b/libimage/runtime.go index 5200a0308..d318862d4 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -454,6 +454,9 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi if possiblyUnqualifiedNamedReference == nil { return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) } + if !shortnames.IsShortName(name) { + return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) + } // 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. @@ -465,10 +468,6 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi name = possiblyUnqualifiedNamedReference.String() } - if !shortnames.IsShortName(name) { - return nil, "", fmt.Errorf("%s: %w", originalName, 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) From 0d6cec8c5c7272f98b00c5b11f266c8501c76d77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:07:49 +0200 Subject: [PATCH 6/9] Pass the required tag, not just a bool, to referenceFuzzilyMatchingRepoAndTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right now that's not simpler, but it will enable simplification of the caller. Should not change behavior. Signed-off-by: Miloslav Trmač --- libimage/image.go | 10 ++++------ libimage/runtime.go | 8 ++++++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/libimage/image.go b/libimage/image.go index 47191b3d9..d30a20712 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -685,24 +685,22 @@ func (i *Image) NamedRepoTags() ([]reference.Named, error) { return repoTags, nil } -// referenceFuzzilyMatchingRepoAndTag checks if the image’s repo (and optionally tag) matches a fuzzy short input, +// referenceFuzzilyMatchingRepoAndTag checks if the image’s repo (and tag if requiredTag != "") matches a fuzzy short input, // and if so, returns the matching reference. -// If `ignoreTag` is set, only the repo must match and the tag is ignored. // // DO NOT ADD ANY NEW USERS OF THIS SEMANTICS. Rely on existing libimage calls like LookupImage instead, // and handle unqualified the way it does (c/image/pkg/shortnames). -func (i *Image) referenceFuzzilyMatchingRepoAndTag(namedTagged reference.NamedTagged, ignoreTag bool) (reference.Named, error) { +func (i *Image) referenceFuzzilyMatchingRepoAndTag(namedTagged reference.NamedTagged, requiredTag string) (reference.Named, error) { repoTags, err := i.NamedRepoTags() if err != nil { return nil, err } name := namedTagged.Name() - tag := namedTagged.Tag() for _, r := range repoTags { - if !ignoreTag { + if requiredTag != "" { tagged, isTagged := r.(reference.NamedTagged) - if !isTagged || tag != tagged.Tag() { + if !isTagged || tagged.Tag() != requiredTag { continue } } diff --git a/libimage/runtime.go b/libimage/runtime.go index d318862d4..60b8e8961 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -458,9 +458,10 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) } + var requiredDigest digest.Digest + var requiredTag string // or "" // 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 { requiredDigest = digested.Digest() @@ -477,6 +478,9 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi // the digest. return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", originalName, storage.ErrImageUnknown) } + if !isDigested { + requiredTag = namedTagged.Tag() + } allImages, err := r.ListImages(context.Background(), nil, nil) if err != nil { @@ -484,7 +488,7 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi } for _, image := range allImages { - named, err := image.referenceFuzzilyMatchingRepoAndTag(namedTagged, isDigested) + named, err := image.referenceFuzzilyMatchingRepoAndTag(namedTagged, requiredTag) if err != nil { return nil, "", err } From 9c9627067df15494e47ed5d0378c2d05e5ef84dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:09:42 +0200 Subject: [PATCH 7/9] Eliminate the isDigested variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- libimage/runtime.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/libimage/runtime.go b/libimage/runtime.go index 60b8e8961..1d76ab657 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -458,12 +458,11 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown) } - var requiredDigest digest.Digest - var requiredTag string // or "" + var requiredDigest digest.Digest // or "" + var requiredTag string // or "" // 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. - digested, isDigested := possiblyUnqualifiedNamedReference.(reference.Digested) - if isDigested { + if digested, ok := possiblyUnqualifiedNamedReference.(reference.Digested); ok { requiredDigest = digested.Digest() possiblyUnqualifiedNamedReference = reference.TrimNamed(possiblyUnqualifiedNamedReference) name = possiblyUnqualifiedNamedReference.String() @@ -478,7 +477,7 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi // the digest. return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", originalName, storage.ErrImageUnknown) } - if !isDigested { + if requiredDigest == "" { requiredTag = namedTagged.Tag() } @@ -500,7 +499,7 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi return nil, "", err } if img != nil { - if isDigested { + if requiredDigest != "" { if !img.hasDigest(requiredDigest) { continue } From bda8c0e79ff4c5867a278346b738458723e0d369 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:11:47 +0200 Subject: [PATCH 8/9] Accept a reference.Named in referenceFuzzilyMatchingRepoAndTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't need a reference.NamedTagged now. That also makes the namedTagged variable in the caller more local. Should not change behavior. Signed-off-by: Miloslav Trmač --- libimage/image.go | 4 ++-- libimage/runtime.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libimage/image.go b/libimage/image.go index d30a20712..5311a2377 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -690,13 +690,13 @@ func (i *Image) NamedRepoTags() ([]reference.Named, error) { // // DO NOT ADD ANY NEW USERS OF THIS SEMANTICS. Rely on existing libimage calls like LookupImage instead, // and handle unqualified the way it does (c/image/pkg/shortnames). -func (i *Image) referenceFuzzilyMatchingRepoAndTag(namedTagged reference.NamedTagged, requiredTag string) (reference.Named, error) { +func (i *Image) referenceFuzzilyMatchingRepoAndTag(requiredRepo reference.Named, requiredTag string) (reference.Named, error) { repoTags, err := i.NamedRepoTags() if err != nil { return nil, err } - name := namedTagged.Name() + name := requiredRepo.Name() for _, r := range repoTags { if requiredTag != "" { tagged, isTagged := r.(reference.NamedTagged) diff --git a/libimage/runtime.go b/libimage/runtime.go index 1d76ab657..330ec7dad 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -487,7 +487,7 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi } for _, image := range allImages { - named, err := image.referenceFuzzilyMatchingRepoAndTag(namedTagged, requiredTag) + named, err := image.referenceFuzzilyMatchingRepoAndTag(possiblyUnqualifiedNamedReference, requiredTag) if err != nil { return nil, "", err } From a4c1168b19a27303eaca541a38d504997fda5e0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Jul 2023 22:16:40 +0200 Subject: [PATCH 9/9] Reorganize how requiredDigest/requiredTag is determined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- libimage/runtime.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/libimage/runtime.go b/libimage/runtime.go index 330ec7dad..6d90272c3 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -460,25 +460,15 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, possiblyUnqualifi var requiredDigest digest.Digest // or "" var requiredTag string // or "" - // 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. + + possiblyUnqualifiedNamedReference = reference.TagNameOnly(possiblyUnqualifiedNamedReference) // Docker compat: make sure to add the "latest" tag if needed. if digested, ok := possiblyUnqualifiedNamedReference.(reference.Digested); ok { requiredDigest = digested.Digest() - possiblyUnqualifiedNamedReference = reference.TrimNamed(possiblyUnqualifiedNamedReference) - name = possiblyUnqualifiedNamedReference.String() - } - - // 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) - } - if requiredDigest == "" { + name = reference.TrimNamed(possiblyUnqualifiedNamedReference).String() + } else if namedTagged, ok := possiblyUnqualifiedNamedReference.(reference.NamedTagged); ok { requiredTag = namedTagged.Tag() + } else { // This should never happen after the reference.TagNameOnly above. + return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", originalName, storage.ErrImageUnknown) } allImages, err := r.ListImages(context.Background(), nil, nil)