From a4fcb38f93b8962852704a78d28d626b56b71361 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 19 Oct 2021 14:45:35 +0200 Subject: [PATCH 1/8] libimage: speed up image filters With commit 86afb94e0054 the dangling checks have been changed to be compatible with Docker. Since then, the dangling also need to compute children. Speed up the dangling and intermediate checks by computing the layer tree *once* instead of for each filter invocation. **Before:** real 0m10.837s user 0m11.308s sys 0m4.231s **After:** real 0m0.476s user 0m0.478s sys 0m0.151s Context: github.com/containers/podman/issues/11997 Signed-off-by: Valentin Rothberg --- libimage/filters.go | 32 ++++++++++++++++++++++++++------ libimage/image.go | 36 ++++++++++++++++++++++++++---------- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index 0cc5cc311..447ea5701 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -50,6 +50,18 @@ func filterImages(images []*Image, filters []filterFunc) ([]*Image, error) { func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([]filterFunc, error) { logrus.Tracef("Parsing image filters %s", filters) + var tree *layerTree + getTree := func() (*layerTree, error) { + if tree == nil { + t, err := r.layerTree() + if err != nil { + return nil, err + } + tree = t + } + return tree, nil + } + filterFuncs := []filterFunc{} for _, filter := range filters { var key, value string @@ -88,7 +100,11 @@ func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([] if err != nil { return nil, errors.Wrapf(err, "non-boolean value %q for dangling filter", value) } - filterFuncs = append(filterFuncs, filterDangling(ctx, dangling)) + t, err := getTree() + if err != nil { + return nil, err + } + filterFuncs = append(filterFuncs, filterDangling(ctx, dangling, t)) case "id": filterFuncs = append(filterFuncs, filterID(value)) @@ -98,7 +114,11 @@ func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([] if err != nil { return nil, errors.Wrapf(err, "non-boolean value %q for intermediate filter", value) } - filterFuncs = append(filterFuncs, filterIntermediate(ctx, intermediate)) + t, err := getTree() + if err != nil { + return nil, err + } + filterFuncs = append(filterFuncs, filterIntermediate(ctx, intermediate, t)) case "label": filterFuncs = append(filterFuncs, filterLabel(ctx, value)) @@ -201,9 +221,9 @@ func filterContainers(value bool) filterFunc { } // filterDangling creates a dangling filter for matching the specified value. -func filterDangling(ctx context.Context, value bool) filterFunc { +func filterDangling(ctx context.Context, value bool, tree *layerTree) filterFunc { return func(img *Image) (bool, error) { - isDangling, err := img.IsDangling(ctx) + isDangling, err := img.isDangling(ctx, tree) if err != nil { return false, err } @@ -221,9 +241,9 @@ func filterID(value string) filterFunc { // filterIntermediate creates an intermediate filter for images. An image is // considered to be an intermediate image if it is dangling (i.e., no tags) and // has no children (i.e., no other image depends on it). -func filterIntermediate(ctx context.Context, value bool) filterFunc { +func filterIntermediate(ctx context.Context, value bool, tree *layerTree) filterFunc { return func(img *Image) (bool, error) { - isIntermediate, err := img.IsIntermediate(ctx) + isIntermediate, err := img.isIntermediate(ctx, tree) if err != nil { return false, err } diff --git a/libimage/image.go b/libimage/image.go index 8456d5280..d592b9661 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -127,10 +127,16 @@ func (i *Image) IsReadOnly() bool { // IsDangling returns true if the image is dangling, that is an untagged image // without children. func (i *Image) IsDangling(ctx context.Context) (bool, error) { + return i.isDangling(ctx, nil) +} + +// isDangling returns true if the image is dangling, that is an untagged image +// without children. If tree is nil, it will created for this invocation only. +func (i *Image) isDangling(ctx context.Context, tree *layerTree) (bool, error) { if len(i.Names()) > 0 { return false, nil } - children, err := i.getChildren(ctx, false) + children, err := i.getChildren(ctx, false, tree) if err != nil { return false, err } @@ -140,10 +146,17 @@ func (i *Image) IsDangling(ctx context.Context) (bool, error) { // IsIntermediate returns true if the image is an intermediate image, that is // an untagged image with children. func (i *Image) IsIntermediate(ctx context.Context) (bool, error) { + return i.isIntermediate(ctx, nil) +} + +// isIntermediate returns true if the image is an intermediate image, that is +// an untagged image with children. If tree is nil, it will created for this +// invocation only. +func (i *Image) isIntermediate(ctx context.Context, tree *layerTree) (bool, error) { if len(i.Names()) > 0 { return false, nil } - children, err := i.getChildren(ctx, false) + children, err := i.getChildren(ctx, false, tree) if err != nil { return false, err } @@ -188,7 +201,7 @@ func (i *Image) Parent(ctx context.Context) (*Image, error) { // HasChildren returns indicates if the image has children. func (i *Image) HasChildren(ctx context.Context) (bool, error) { - children, err := i.getChildren(ctx, false) + children, err := i.getChildren(ctx, false, nil) if err != nil { return false, err } @@ -197,7 +210,7 @@ func (i *Image) HasChildren(ctx context.Context) (bool, error) { // Children returns the image's children. func (i *Image) Children(ctx context.Context) ([]*Image, error) { - children, err := i.getChildren(ctx, true) + children, err := i.getChildren(ctx, true, nil) if err != nil { return nil, err } @@ -205,13 +218,16 @@ func (i *Image) Children(ctx context.Context) ([]*Image, error) { } // getChildren returns a list of imageIDs that depend on the image. If all is -// false, only the first child image is returned. -func (i *Image) getChildren(ctx context.Context, all bool) ([]*Image, error) { - tree, err := i.runtime.layerTree() - if err != nil { - return nil, err +// false, only the first child image is returned. If tree is nil, it will be +// created for this invocation only. +func (i *Image) getChildren(ctx context.Context, all bool, tree *layerTree) ([]*Image, error) { + if tree == nil { + t, err := i.runtime.layerTree() + if err != nil { + return nil, err + } + tree = t } - return tree.children(ctx, i, all) } From 769b05f64f07f65956105c249472ea718f5a5937 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 20 Dec 2021 14:06:49 +0100 Subject: [PATCH 2/8] pull: fix pulling from dir transport Path-based transports may contain characters that are invalid for a reference. In such cases, we should pessimistically generate an ID and not attempt to look at the (possibly path-based) string within the transport. This fixes an error when running `podman run dir:/tmp/CapitalChar` and will prevent the same issue for the upcoming SIF transport. Extend the tests to make sure we're not going to regress in the future. Signed-off-by: Valentin Rothberg --- libimage/pull.go | 10 ++++++++-- libimage/pull_test.go | 3 +++ ...948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef | Bin 0 -> 1024 bytes ...3c67dcf4cdbd69f9224c74e961c53b589b70499eac443 | 1 + .../testdata/scratch-dir-5pec!@L/manifest.json | 1 + libimage/testdata/scratch-dir-5pec!@L/version | 1 + 6 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 libimage/testdata/scratch-dir-5pec!@L/5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef create mode 100644 libimage/testdata/scratch-dir-5pec!@L/61e17f84d763cc086d43c67dcf4cdbd69f9224c74e961c53b589b70499eac443 create mode 100644 libimage/testdata/scratch-dir-5pec!@L/manifest.json create mode 100644 libimage/testdata/scratch-dir-5pec!@L/version diff --git a/libimage/pull.go b/libimage/pull.go index 1c322c37e..484b6e3fa 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -227,8 +227,14 @@ func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference, imageName = named.String() default: - storageName = toLocalImageName(ref.StringWithinTransport()) - imageName = storageName + // Path-based transports (e.g., dir) may include invalid + // characters, so we should pessimistically generate an ID + // instead of looking at the StringWithinTransport(). + storageName, err = getImageID(ctx, ref, nil) + if err != nil { + return nil, err + } + imageName = "sha256:" + storageName[1:] } // Create a storage reference. diff --git a/libimage/pull_test.go b/libimage/pull_test.go index eebda644d..a79a64c6e 100644 --- a/libimage/pull_test.go +++ b/libimage/pull_test.go @@ -51,6 +51,9 @@ func TestPull(t *testing.T) { {"docker://docker.io/library/alpine", false, 1, []string{"docker.io/library/alpine:latest"}}, {"quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00", false, 1, []string{"quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00"}}, {"quay.io/libpod/alpine:pleaseignorethistag@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00", false, 1, []string{"quay.io/libpod/alpine@sha256:634a8f35b5f16dcf4aaa0822adc0b1964bb786fca12f6831de8ddc45e5986a00"}}, + + // DIR + {"dir:testdata/scratch-dir-5pec!@L", false, 1, []string{"61e17f84d763cc086d43c67dcf4cdbd69f9224c74e961c53b589b70499eac443"}}, } { pulledImages, err := runtime.Pull(ctx, test.input, config.PullPolicyAlways, pullOptions) if test.expectError { diff --git a/libimage/testdata/scratch-dir-5pec!@L/5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef b/libimage/testdata/scratch-dir-5pec!@L/5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef new file mode 100644 index 0000000000000000000000000000000000000000..06d7405020018ddf3cacee90fd4af10487da3d20 GIT binary patch literal 1024 ScmZQz7zLvtFd70QH3R?z00031 literal 0 HcmV?d00001 diff --git a/libimage/testdata/scratch-dir-5pec!@L/61e17f84d763cc086d43c67dcf4cdbd69f9224c74e961c53b589b70499eac443 b/libimage/testdata/scratch-dir-5pec!@L/61e17f84d763cc086d43c67dcf4cdbd69f9224c74e961c53b589b70499eac443 new file mode 100644 index 000000000..0f29a3146 --- /dev/null +++ b/libimage/testdata/scratch-dir-5pec!@L/61e17f84d763cc086d43c67dcf4cdbd69f9224c74e961c53b589b70499eac443 @@ -0,0 +1 @@ +{"created":"2021-12-20T13:03:01.601633431Z","architecture":"amd64","os":"linux","config":{"Env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"],"Labels":{"io.buildah.version":"1.23.1"}},"rootfs":{"type":"layers","diff_ids":["sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef"]},"history":[{"created":"2021-12-20T13:03:01.602339865Z","created_by":"/bin/sh"}]} \ No newline at end of file diff --git a/libimage/testdata/scratch-dir-5pec!@L/manifest.json b/libimage/testdata/scratch-dir-5pec!@L/manifest.json new file mode 100644 index 000000000..00e9e0d37 --- /dev/null +++ b/libimage/testdata/scratch-dir-5pec!@L/manifest.json @@ -0,0 +1 @@ +{"schemaVersion":2,"config":{"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:61e17f84d763cc086d43c67dcf4cdbd69f9224c74e961c53b589b70499eac443","size":402},"layers":[{"mediaType":"application/vnd.oci.image.layer.v1.tar","digest":"sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef","size":1024}],"annotations":{"org.opencontainers.image.base.digest":"","org.opencontainers.image.base.name":""}} \ No newline at end of file diff --git a/libimage/testdata/scratch-dir-5pec!@L/version b/libimage/testdata/scratch-dir-5pec!@L/version new file mode 100644 index 000000000..75a4f5701 --- /dev/null +++ b/libimage/testdata/scratch-dir-5pec!@L/version @@ -0,0 +1 @@ +Directory Transport Version: 1.1 From 8888c13f58610badc5770c9d10ccbd52000c5c30 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 3 Dec 2021 10:46:13 +0100 Subject: [PATCH 3/8] reference filter: match exact behavior of Docker The previously inherited behavior from Podman was matching too aggressively. Now, the filter matches the exact behavior of Docker, simplifies the code and is tested directly in libimage. Context: containers/podman#11905 Signed-off-by: Valentin Rothberg --- libimage/filters.go | 22 ++++++-------- libimage/filters_test.go | 62 ++++++++++++++++++++++++++++++++++++++++ libimage/image.go | 20 +++++++++++++ 3 files changed, 91 insertions(+), 13 deletions(-) create mode 100644 libimage/filters_test.go diff --git a/libimage/filters.go b/libimage/filters.go index 447ea5701..38c1a33c3 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -3,13 +3,13 @@ package libimage import ( "context" "fmt" - "path/filepath" "strconv" "strings" "time" filtersPkg "github.com/containers/common/pkg/filters" "github.com/containers/common/pkg/timetype" + "github.com/containers/image/v5/docker/reference" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -155,20 +155,16 @@ func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([] // filterReference creates a reference filter for matching the specified value. func filterReference(value string) filterFunc { - // Replacing all '/' with '|' so that filepath.Match() can work '|' - // character is not valid in image name, so this is safe. - // - // TODO: this has been copied from Podman and requires some more review - // and especially tests. - filter := fmt.Sprintf("*%s*", value) - filter = strings.ReplaceAll(filter, "/", "|") return func(img *Image) (bool, error) { - if len(value) < 1 { - return true, nil + refs, err := img.NamesReferences() + if err != nil { + return false, err } - for _, name := range img.Names() { - newName := strings.ReplaceAll(name, "/", "|") - match, _ := filepath.Match(filter, newName) + for _, ref := range refs { + match, err := reference.FamiliarMatch(value, ref) + if err != nil { + return false, err + } if match { return true, nil } diff --git a/libimage/filters_test.go b/libimage/filters_test.go new file mode 100644 index 000000000..632bd8871 --- /dev/null +++ b/libimage/filters_test.go @@ -0,0 +1,62 @@ +package libimage + +import ( + "context" + "os" + "testing" + + "github.com/containers/common/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestFilterReference(t *testing.T) { + busyboxLatest := "quay.io/libpod/busybox:latest" + alpineLatest := "quay.io/libpod/alpine:latest" + + runtime, cleanup := testNewRuntime(t) + defer cleanup() + ctx := context.Background() + + pullOptions := &PullOptions{} + pullOptions.Writer = os.Stdout + + pulledImages, err := runtime.Pull(ctx, busyboxLatest, config.PullPolicyMissing, pullOptions) + require.NoError(t, err) + require.Len(t, pulledImages, 1) + busybox := pulledImages[0] + + pulledImages, err = runtime.Pull(ctx, alpineLatest, config.PullPolicyMissing, pullOptions) + require.NoError(t, err) + require.Len(t, pulledImages, 1) + alpine := pulledImages[0] + + err = busybox.Tag("localhost/image:tag") + require.NoError(t, err) + err = alpine.Tag("localhost/image:another-tag") + require.NoError(t, err) + + for _, test := range []struct { + filter string + matches int + }{ + {"image", 0}, + {"localhost/image", 2}, + {"localhost/image:tag", 1}, + {"localhost/image:another-tag", 1}, + {"localhost/*", 2}, + {"localhost/image:*tag", 2}, + {"busybox", 0}, + {"alpine", 0}, + {"quay.io/libpod/busybox", 1}, + {"quay.io/libpod/alpine", 1}, + {"quay.io/libpod", 0}, + {"quay.io/libpod/*", 2}, + } { + listOptions := &ListImagesOptions{ + Filters: []string{"reference=" + test.filter}, + } + listedImages, err := runtime.ListImages(ctx, nil, listOptions) + require.NoError(t, err, "%v", test) + require.Len(t, listedImages, test.matches, "%s -> %v", test.filter, listedImages) + } +} diff --git a/libimage/image.go b/libimage/image.go index d592b9661..b79d48806 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -43,6 +43,8 @@ type Image struct { completeInspectData *ImageData // Corresponding OCI image. ociv1Image *ociv1.Image + // Names() parsed into references. + namesReferences []reference.Reference } } @@ -58,6 +60,7 @@ func (i *Image) reload() error { i.cached.partialInspectData = nil i.cached.completeInspectData = nil i.cached.ociv1Image = nil + i.cached.namesReferences = nil return nil } @@ -88,6 +91,23 @@ func (i *Image) Names() []string { return i.storageImage.Names } +// NamesReferences returns Names() as references. +func (i *Image) NamesReferences() ([]reference.Reference, error) { + if i.cached.namesReferences != nil { + return i.cached.namesReferences, nil + } + refs := make([]reference.Reference, 0, len(i.Names())) + for _, name := range i.Names() { + ref, err := reference.Parse(name) + if err != nil { + return nil, err + } + refs = append(refs, ref) + } + i.cached.namesReferences = refs + return refs, nil +} + // StorageImage returns the underlying storage.Image. func (i *Image) StorageImage() *storage.Image { return i.storageImage From 8a1c696e53fb2452c7e6dc68041a9d67e38bb0ad Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Sat, 4 Dec 2021 19:22:11 +0100 Subject: [PATCH 4/8] libimage: fix reference filters It turns out that FamiliarMatch is only useful for matching Docker Hub but we should not limit it to that and match values against registry. For instance, FamiliarMatch is *not* able to match a FQN reference against a Docker Hub image. I am convinced that we should *not* behave as Docker does in this case. This brings us back to the behavior prior to commit 721661d9c60a but with a fixed matching algorithm. The specified value will now be matched against 1) the FQN 2) without domain 3) without domain and path. If specified also a second time without digest/tag. Signed-off-by: Valentin Rothberg --- libimage/filters.go | 40 ++++++++++++++++++++++++++++++++-------- libimage/filters_test.go | 24 +++++++++++++++++------- 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index 38c1a33c3..4aa94b561 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -3,6 +3,7 @@ package libimage import ( "context" "fmt" + "path" "strconv" "strings" "time" @@ -131,7 +132,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([] filterFuncs = append(filterFuncs, filterReadOnly(readOnly)) case "reference": - filterFuncs = append(filterFuncs, filterReference(value)) + filterFuncs = append(filterFuncs, filterReferences(value)) case "until": ts, err := timetype.GetTimestamp(value, time.Now()) @@ -153,20 +154,43 @@ func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([] return filterFuncs, nil } -// filterReference creates a reference filter for matching the specified value. -func filterReference(value string) filterFunc { +// filterReferences creates a reference filter for matching the specified value. +func filterReferences(value string) filterFunc { return func(img *Image) (bool, error) { refs, err := img.NamesReferences() if err != nil { return false, err } + for _, ref := range refs { - match, err := reference.FamiliarMatch(value, ref) - if err != nil { - return false, err + refString := ref.String() // FQN with tag/digest + candidates := []string{refString} + + // Split the reference into 3 components (twice if diggested/tagged): + // 1) Fully-qualified reference + // 2) Without domain + // 3) Without domain and path + if named, isNamed := ref.(reference.Named); isNamed { + candidates = append(candidates, + reference.Path(named), // path/name without tag/digest (Path() removes it) + refString[strings.LastIndex(refString, "/")+1:]) // name with tag/digest + + trimmedString := reference.TrimNamed(named).String() + if refString != trimmedString { + tagOrDigest := refString[len(trimmedString):] + candidates = append(candidates, + trimmedString, // FQN without tag/digest + reference.Path(named)+tagOrDigest, // path/name with tag/digest + trimmedString[strings.LastIndex(trimmedString, "/")+1:]) // name without tag/digest + } } - if match { - return true, nil + + for _, candidate := range candidates { + // path.Match() is also used by Docker's reference.FamiliarMatch(). + matched, _ := path.Match(value, candidate) + if matched { + return true, nil + } } } return false, nil diff --git a/libimage/filters_test.go b/libimage/filters_test.go index 632bd8871..6d0440e4b 100644 --- a/libimage/filters_test.go +++ b/libimage/filters_test.go @@ -32,25 +32,35 @@ func TestFilterReference(t *testing.T) { err = busybox.Tag("localhost/image:tag") require.NoError(t, err) - err = alpine.Tag("localhost/image:another-tag") + err = alpine.Tag("localhost/another-image:tag") + require.NoError(t, err) + err = alpine.Tag("docker.io/library/image:another-tag") require.NoError(t, err) for _, test := range []struct { filter string matches int }{ - {"image", 0}, - {"localhost/image", 2}, + {"image", 2}, + {"*mage*", 2}, + {"image:*", 2}, + {"image:tag", 1}, + {"image:another-tag", 1}, + {"localhost/image", 1}, {"localhost/image:tag", 1}, - {"localhost/image:another-tag", 1}, + {"library/image", 1}, + {"docker.io/library/image*", 1}, + {"docker.io/library/image:*", 1}, + {"docker.io/library/image:another-tag", 1}, {"localhost/*", 2}, - {"localhost/image:*tag", 2}, - {"busybox", 0}, - {"alpine", 0}, + {"localhost/image:*tag", 1}, + {"localhost/*mage:*ag", 2}, {"quay.io/libpod/busybox", 1}, {"quay.io/libpod/alpine", 1}, {"quay.io/libpod", 0}, {"quay.io/libpod/*", 2}, + {"busybox", 1}, + {"alpine", 1}, } { listOptions := &ListImagesOptions{ Filters: []string{"reference=" + test.filter}, From 1f44de5f71449d96ff725e3a74016607ec530aee Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 24 Sep 2021 09:51:46 +0200 Subject: [PATCH 5/8] libimage: load: improve error messages When loading a path we have to "guess" the underlying format and hence attempt loading supported formats in a specific order. When all attempts have failed make sure that all loading errors are reported up, in addition to debug logs, such that users can parse them for useful information. Fixes: github.com/containers/podman/issues/11730 Signed-off-by: Valentin Rothberg --- libimage/load.go | 59 +++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/libimage/load.go b/libimage/load.go index 33dc1a22f..f2b57c43a 100644 --- a/libimage/load.go +++ b/libimage/load.go @@ -2,7 +2,7 @@ package libimage import ( "context" - "errors" + "fmt" "os" "time" @@ -28,66 +28,69 @@ func (r *Runtime) Load(ctx context.Context, path string, options *LoadOptions) ( defer r.writeEvent(&Event{ID: "", Name: path, Time: time.Now(), Type: EventTypeImageLoad}) } - var ( - loadedImages []string - loadError error - ) - if options == nil { options = &LoadOptions{} } - for _, f := range []func() ([]string, error){ + var loadErrors []error + + for _, f := range []func() ([]string, string, error){ // OCI - func() ([]string, error) { + func() ([]string, string, error) { logrus.Debugf("-> Attempting to load %q as an OCI directory", path) ref, err := ociTransport.NewReference(path, "") if err != nil { - return nil, err + return nil, ociTransport.Transport.Name(), err } - return r.copyFromDefault(ctx, ref, &options.CopyOptions) + images, err := r.copyFromDefault(ctx, ref, &options.CopyOptions) + return images, ociTransport.Transport.Name(), err }, // OCI-ARCHIVE - func() ([]string, error) { + func() ([]string, string, error) { logrus.Debugf("-> Attempting to load %q as an OCI archive", path) ref, err := ociArchiveTransport.NewReference(path, "") if err != nil { - return nil, err + return nil, ociArchiveTransport.Transport.Name(), err } - return r.copyFromDefault(ctx, ref, &options.CopyOptions) + images, err := r.copyFromDefault(ctx, ref, &options.CopyOptions) + return images, ociArchiveTransport.Transport.Name(), err }, // DIR - func() ([]string, error) { + func() ([]string, string, error) { logrus.Debugf("-> Attempting to load %q as a Docker dir", path) ref, err := dirTransport.NewReference(path) if err != nil { - return nil, err + return nil, dirTransport.Transport.Name(), err } - return r.copyFromDefault(ctx, ref, &options.CopyOptions) + images, err := r.copyFromDefault(ctx, ref, &options.CopyOptions) + return images, dirTransport.Transport.Name(), err }, // DOCKER-ARCHIVE - func() ([]string, error) { + func() ([]string, string, error) { logrus.Debugf("-> Attempting to load %q as a Docker archive", path) ref, err := dockerArchiveTransport.ParseReference(path) if err != nil { - return nil, err + return nil, dockerArchiveTransport.Transport.Name(), err } - return r.loadMultiImageDockerArchive(ctx, ref, &options.CopyOptions) - }, - - // Give a decent error message if nothing above worked. - func() ([]string, error) { - return nil, errors.New("payload does not match any of the supported image formats (oci, oci-archive, dir, docker-archive)") + images, err := r.loadMultiImageDockerArchive(ctx, ref, &options.CopyOptions) + return images, dockerArchiveTransport.Transport.Name(), err }, } { - loadedImages, loadError = f() - if loadError == nil { - return loadedImages, loadError + loadedImages, transportName, err := f() + if err == nil { + return loadedImages, nil } - logrus.Debugf("Error loading %s: %v", path, loadError) + logrus.Debugf("Error loading %s (%s): %v", path, transportName, err) + loadErrors = append(loadErrors, fmt.Errorf("%s: %v", transportName, err)) + } + + // Give a decent error message if nothing above worked. + loadError := fmt.Errorf("payload does not match any of the supported image formats:") + for _, err := range loadErrors { + loadError = fmt.Errorf("%v\n * %v", loadError, err) } return nil, loadError From 9e37da5b9635ee88b84950827a02332300d4c9d6 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 10 Dec 2021 13:18:01 +0100 Subject: [PATCH 6/8] load: support buildkit archives Archives generated with buildkit have some kind of "hybrid" layout which is the same for OCI and Docker archives. OCI ones ship with a manifest.json but set the image's reference in the index.json but in a custom annotation and not the one the OCI image spec wants. Archives in the Docker format set the reference in `RepoTags` of the manifest.json. To support these archives, simply look for the custom containerd annotation *and* change the order back to give OCI archives precedence. Fixes: containers/podman/issues/12560 Signed-off-by: Valentin Rothberg --- libimage/load.go | 11 +++++++++++ libimage/load_test.go | 2 ++ libimage/pull.go | 24 ++++++++++++++++++++---- libimage/testdata/buildkit-docker.tar | Bin 0 -> 7168 bytes libimage/testdata/buildkit-oci.tar | Bin 0 -> 7168 bytes 5 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 libimage/testdata/buildkit-docker.tar create mode 100644 libimage/testdata/buildkit-oci.tar diff --git a/libimage/load.go b/libimage/load.go index f2b57c43a..e6f5690f6 100644 --- a/libimage/load.go +++ b/libimage/load.go @@ -57,6 +57,17 @@ func (r *Runtime) Load(ctx context.Context, path string, options *LoadOptions) ( return images, ociArchiveTransport.Transport.Name(), err }, + // DOCKER-ARCHIVE + func() ([]string, string, error) { + logrus.Debugf("-> Attempting to load %q as a Docker archive", path) + ref, err := dockerArchiveTransport.ParseReference(path) + if err != nil { + return nil, dockerArchiveTransport.Transport.Name(), err + } + images, err := r.loadMultiImageDockerArchive(ctx, ref, &options.CopyOptions) + return images, dockerArchiveTransport.Transport.Name(), err + }, + // DIR func() ([]string, string, error) { logrus.Debugf("-> Attempting to load %q as a Docker dir", path) diff --git a/libimage/load_test.go b/libimage/load_test.go index 8464a3655..92842824a 100644 --- a/libimage/load_test.go +++ b/libimage/load_test.go @@ -27,12 +27,14 @@ func TestLoad(t *testing.T) { {"testdata/docker-two-names.tar.xz", false, 1, []string{"localhost/pretty-empty:latest", "example.com/empty:latest"}}, {"testdata/docker-two-images.tar.xz", false, 2, []string{"example.com/empty:latest", "example.com/empty/but:different"}}, {"testdata/docker-unnamed.tar.xz", false, 1, []string{"sha256:ec9293436c2e66da44edb9efb8d41f6b13baf62283ebe846468bc992d76d7951"}}, + {"testdata/buildkit-docker.tar", false, 1, []string{"github.com/buildkit/archive:docker"}}, // OCI ARCHIVE {"testdata/oci-name-only.tar.gz", false, 1, []string{"localhost/pretty-empty:latest"}}, {"testdata/oci-non-docker-name.tar.gz", true, 0, nil}, {"testdata/oci-registry-name.tar.gz", false, 1, []string{"example.com/empty:latest"}}, {"testdata/oci-unnamed.tar.gz", false, 1, []string{"sha256:5c8aca8137ac47e84c69ae93ce650ce967917cc001ba7aad5494073fac75b8b6"}}, + {"testdata/buildkit-oci.tar", false, 1, []string{"github.com/buildkit/archive:oci"}}, } { loadedImages, err := runtime.Load(ctx, test.input, loadOptions) if test.expectError { diff --git a/libimage/pull.go b/libimage/pull.go index 484b6e3fa..cddd1ecc4 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -21,6 +21,7 @@ import ( "github.com/containers/image/v5/types" "github.com/containers/storage" digest "github.com/opencontainers/go-digest" + ociSpec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -169,6 +170,20 @@ func (r *Runtime) Pull(ctx context.Context, name string, pullPolicy config.PullP return localImages, pullError } +// nameFromAnnotations returns a reference string to be used as an image name, +// or an empty string. The annotations map may be nil. +func nameFromAnnotations(annotations map[string]string) string { + if annotations == nil { + return "" + } + // buildkit/containerd are using a custom annotation see + // containers/podman/issues/12560. + if annotations["io.containerd.image.name"] != "" { + return annotations["io.containerd.image.name"] + } + return annotations[ociSpec.AnnotationRefName] +} + // copyFromDefault is the default copier for a number of transports. Other // transports require some specific dancing, sometimes Yoga. func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference, options *CopyOptions) ([]string, error) { @@ -201,15 +216,16 @@ func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference, if err != nil { return nil, err } - // if index.json has no reference name, compute the image ID instead - if manifestDescriptor.Annotations == nil || manifestDescriptor.Annotations["org.opencontainers.image.ref.name"] == "" { + storageName = nameFromAnnotations(manifestDescriptor.Annotations) + switch len(storageName) { + case 0: + // If there's no reference name in the annotations, compute an ID. storageName, err = getImageID(ctx, ref, nil) if err != nil { return nil, err } imageName = "sha256:" + storageName[1:] - } else { - storageName = manifestDescriptor.Annotations["org.opencontainers.image.ref.name"] + default: named, err := NormalizeName(storageName) if err != nil { return nil, err diff --git a/libimage/testdata/buildkit-docker.tar b/libimage/testdata/buildkit-docker.tar new file mode 100644 index 0000000000000000000000000000000000000000..3b95183f55b269e0c71481f1c8181260791e6cbb GIT binary patch literal 7168 zcmeHKON-ku5YBmjh0(QHN0MK$p@+VpheAsUEp%CmY{`kbjuiX|-G==4j_kynEF|4F z@v>~Ki#?JxGn&!&%{b0t4h$pkyn%Vbxc}UpYu*WVus^W8PQBVFuZoi8;L@qK zjOQ@g!=0+{k@m-R{)-u-p5N6qFmd_2{1b=t`G1rfu$TXUa6yPWcAOw}IT^cNEHDwV zo3NC4JQxdC#KdES#i0)v9pi9JeLoyCFAjur2ty|SuIskYFfxp-j2*Y-hGw1;KY;un z(s$Ot{O9;zA(kgIS&F2ra)Bbm=G=GD5G7ira*CoAdZ(5sxA$gOy{P){`?yrSzN`l|CAsitpb4yHz6=A(LD$&8~&Rav$Mv$&x{VqAI)mqnQy)e&9*&hDmBnY_> zVM5)I_zn*^_dMbFVIa~Fa~uSeQk-(;U_nzSOcX-tG#6a5NsS^4vBe^j2`i;mqov|D*U62@Z4OY(Wn7tgd(M8iZXXlL}1BIhzW5Np?&bC0tIy z1u%n}{v|rKel@2sstfrG7{#JAH0N43$Jp>oWEobucpj`G6&F8klftuJt371~|NfEs z@7>2gA(Xg1{`Z2|k+4GdFiVGM3vQ^NSF2ra_(y7Jfx5-%sdWZQd;?I2ctgaL(q&Co zbu%Ef-7*K?P={i3(;!S`Ijdq=o{wVFb==5uRJT7%5j89ansq+4^+KptNwHDVwkC*T zN-hb#BzQt-L`dX$-;V9fMQU$c*qB+bub&2gY;)Zu1^fyB)M@_>%%=lPNPxW_{~bYo zPb~oa&8E{wd%fW4wG@383q4^|Ln9A?9{e7BXgc_&ZNJ`gv==&wfKKEk^fkIFJ5gUN zHK0D$|6|AB@BdBxr#=3=%gCMP6{?$S03KX{wsA+q#x_PzqeR^~+rRY@=p)cappU@W GBJdkB=nORg literal 0 HcmV?d00001 diff --git a/libimage/testdata/buildkit-oci.tar b/libimage/testdata/buildkit-oci.tar new file mode 100644 index 0000000000000000000000000000000000000000..8f7455078ec6a51f743a33a0f9a3a52e5c59bb63 GIT binary patch literal 7168 zcmeHKTWi}e6z+3>h0*I$$C7-B4SU!P_AuBOVS|-2WJ^x8I#RHm&@SY^@5pw%IFO7k zNmn|HKzt<5#pmez&T*XSxENlXbvdvC&$}@Huy+5q_v&ZtP{+Tpyt78t7+(}6%ORv= zeVNE%wJ&$9y+_iItNIsHMm+yq*TCfEVfDup_w|328_=nLfVsff9Z?#jF2^I+i-m(l z>?SP59uG#s6*2Z0W^w2PMn_IKBEBDvm=_0v?!%C&zw5d!G>i;uBRkY>xuM=C*bkuo z`|O=GG5oaJa%~CQko`&hHmV09@B*5fVq@0ng*B&k1#i2G^8HelNtl+L-=<++Hyg~ z3xrTYd;IT7=SjC$mzHIrnc$L*D->CX&F7g+SShs{E)}BC zD6~R@O1ns=V#dAz_CN(i7OA&43}RJ29^j@{>^d>(Tu#6_I6(Eh5*^#G>iCW6LjD8> zu?Pvx8?EaXX!s+KhXcZUHoHAFzNB%N#dD=71}{B z8K4c!p?_Ylwz=U!)X>7UMeC8>1sZz|Qxb*)#FWxyMN-uWAhiv&Q8KAS&KtT?Y-YxS zi7cm!7>;Md*vuNYvK&@J&r(FdZD3aC6I;)PYAqCvg?1W2#3A?!lPm0uF^S;kjJ_Sx z%tdN9{*90I`sQi!$EMcjq<}x+9|xq{|1l2C_kTbCp9%C^W&z-Dnn@pZXTc-C1pyqN z#axfsq#45p=tDi9*F*WuRuKK;Kg$4+(-q7)dQqM)1cU!W^S?v=UHtd`|9s`>SYDxO kv*Lo%nm9&p@AnJ_CIQ{w@Q*0CD~D@&Et; literal 0 HcmV?d00001 From 14d59d01ff58d184c9cc5e832623d4060cd02ed8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 22 Dec 2021 10:36:02 +0100 Subject: [PATCH 7/8] v0.44.5 A series of backports for the Podman v3.5 release: * load: support buildkit archives * libimage: load: improve error messages * libimage: fix reference filters * reference filter: match exact behavior of Docker * pull: fix pulling from dir transport * libimage: speed up image filters Signed-off-by: Valentin Rothberg --- libimage/filters.go | 1 - version/version.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index 4aa94b561..40832ec2e 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -2,7 +2,6 @@ package libimage import ( "context" - "fmt" "path" "strconv" "strings" diff --git a/version/version.go b/version/version.go index 833012552..ccce81bb7 100644 --- a/version/version.go +++ b/version/version.go @@ -1,4 +1,4 @@ package version // Version is the version of the build. -const Version = "0.44.5-dev" +const Version = "0.44.5" From 2bb48da68fd01370e6c3200bb564935a81365175 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 22 Dec 2021 10:36:35 +0100 Subject: [PATCH 8/8] bump to v0.44.6-dev Signed-off-by: Valentin Rothberg --- version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/version.go b/version/version.go index ccce81bb7..3c6d42272 100644 --- a/version/version.go +++ b/version/version.go @@ -1,4 +1,4 @@ package version // Version is the version of the build. -const Version = "0.44.5" +const Version = "0.44.6-dev"