From ab5bc69f92f070723919e05d326aba7a861115aa Mon Sep 17 00:00:00 2001 From: byarbrough Date: Thu, 28 Jul 2022 09:27:15 -0600 Subject: [PATCH] Improve push AllTags logic 1. Inverts test of AllTags bool, so that false is handled first the single image is pushed. 2. Makes use of library functions to test source formats instead of flaky string parsing. 3. Ensures that all tags across mulitple images for a given repo are pushed, instead of just for the single image. 4. Make the AllTags bool and a specified destination mutually exclusive. Note that Podman currently assumes the destination is the source if no destination is provided, rather than leaving it as an empty string. This will cause an error, and I think should be fixed in a PR to that repo since libimage.PushImage already fills in resolvedSource for an empty destination. https://github.com/containers/podman/blob/v4.2/cmd/podman/images/push.go#L131 Also note that the unit test is not able to push images to a real docker registry, so I would consider them incomplete, but don't know how to improve. Signed-off-by: Brian Yarbrough --- libimage/push.go | 95 ++++++++++++++++++++++++------------------- libimage/push_test.go | 39 ++++++++---------- 2 files changed, 72 insertions(+), 62 deletions(-) diff --git a/libimage/push.go b/libimage/push.go index 94d6ca005..84f357a06 100644 --- a/libimage/push.go +++ b/libimage/push.go @@ -3,12 +3,11 @@ package libimage import ( "context" "fmt" - "strings" "time" - dockerTransport "github.com/containers/image/v5/docker" dockerArchiveTransport "github.com/containers/image/v5/docker/archive" "github.com/containers/image/v5/docker/reference" + "github.com/containers/image/v5/transports" "github.com/containers/image/v5/transports/alltransports" "github.com/sirupsen/logrus" ) @@ -33,60 +32,79 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options options = &PushOptions{} } - // Look up the local image. Note that we need to ignore the platform - // and push what the user specified (containers/podman/issues/10344). - image, resolvedSource, err := r.LookupImage(source, nil) - if err != nil { - return nil, err + // Push the single image + if !options.AllTags { + + // Look up the local image. Note that we need to ignore the platform + // and push what the user specified (containers/podman/issues/10344). + image, resolvedSource, err := r.LookupImage(source, nil) + if err != nil { + return nil, err + } + + // Make sure we have a proper destination, and parse it into an image + // reference for copying. + if destination == "" { + // Doing an ID check here is tempting but false positives (due + // to a short partial IDs) are more painful than false + // negatives. + destination = resolvedSource + } + + return pushImage(ctx, image, destination, options, resolvedSource, r) } - // Make sure we have a proper destination, and parse it into an image - // reference for copying. - if destination == "" { - // Doing an ID check here is tempting but false positives (due - // to a short partial IDs) are more painful than false - // negatives. - destination = resolvedSource + // Below handles the AllTags option, for which we have to build a list of + // all the local images that match the provided repository and then push them. + // + // Start by making sure a destination was not specified, since we'll get that from the source + if len(destination) != 0 { + return nil, fmt.Errorf("`destination` should not be specified if using AllTags") } - // If specified to push --all-tags, look them up and iterate. - if options.AllTags { + // Make sure the source repository does not have a tag + srcNamed, err := reference.ParseNormalizedNamed(source) + if err != nil { + return nil, err + } + if _, hasTag := srcNamed.(reference.NamedTagged); hasTag { + return nil, fmt.Errorf("can't push with AllTags if source tag is specified") + } - // Do not allow : for tags, other than specifying transport - d := strings.TrimPrefix(destination, "docker://") - if strings.ContainsAny(d, ":") { - return nil, fmt.Errorf("tag can't be used with --all-tags/-a") - } + // Now list every image of that source repository + listOptions := &ListImagesOptions{} + listOptions.Filters = []string{fmt.Sprintf("reference=%s:*", source)} + logrus.Debugf("Finding all images for source %s", source) + srcImages, _ := r.ListImages(ctx, nil, listOptions) - namedRepoTags, err := image.NamedTaggedRepoTags() + // Push each tag for every image in the list + var byteManifest []byte + for _, img := range srcImages { + namedTagged, err := img.NamedTaggedRepoTags() if err != nil { return nil, err } - - logrus.Debugf("Flag --all-tags true, found: %s", namedRepoTags) - - for _, tag := range namedRepoTags { - fullNamedTag := fmt.Sprintf("%s:%s", destination, tag.Tag()) - _, err = pushImage(ctx, fullNamedTag, options, image, r) + for _, n := range namedTagged { + destWithTag := fmt.Sprintf("%s:%s", source, n.Tag()) + b, err := pushImage(ctx, img, destWithTag, options, "", r) if err != nil { - return nil, err + return byteManifest, err } + byteManifest = append(byteManifest, b...) } - } else { - // No --all-tags, so just push just the single image. - return pushImage(ctx, destination, options, image, r) } - return nil, nil + return byteManifest, nil } -func pushImage(ctx context.Context, destination string, options *PushOptions, image *Image, r *Runtime) ([]byte, error) { +// pushImage sends a single image to be copied to the destination +func pushImage(ctx context.Context, image *Image, destination string, options *PushOptions, resolvedSource string, r *Runtime) ([]byte, error) { srcRef, err := image.StorageReference() if err != nil { return nil, err } - logrus.Debugf("Pushing image %s to %s", srcRef, destination) + logrus.Debugf("Pushing image %s to %s", transports.ImageName(srcRef), destination) destRef, err := alltransports.ParseImageName(destination) if err != nil { @@ -99,11 +117,6 @@ func pushImage(ctx context.Context, destination string, options *PushOptions, im destRef = dockerRef } - // If using --all-tags, must push to registry - if destRef.Transport().Name() != dockerTransport.Transport.Name() && options.AllTags { - return nil, fmt.Errorf("--all-tags can only be used with docker transport") - } - if r.eventChannel != nil { defer r.writeEvent(&Event{ID: image.ID(), Name: destination, Time: time.Now(), Type: EventTypeImagePush}) } @@ -111,7 +124,7 @@ func pushImage(ctx context.Context, destination string, options *PushOptions, im // Buildah compat: Make sure to tag the destination image if it's a // Docker archive. This way, we preserve the image name. if destRef.Transport().Name() == dockerArchiveTransport.Transport.Name() { - if named, err := reference.ParseNamed(destination); err == nil { + if named, err := reference.ParseNamed(resolvedSource); err == nil { tagged, isTagged := named.(reference.NamedTagged) if isTagged { options.dockerArchiveAdditionalTags = []reference.NamedTagged{tagged} diff --git a/libimage/push_test.go b/libimage/push_test.go index d174abc11..45d3872c5 100644 --- a/libimage/push_test.go +++ b/libimage/push_test.go @@ -72,37 +72,39 @@ func TestPushAllTags(t *testing.T) { defer cleanup() ctx := context.Background() - // Prefetch alpine. + // Prefetch two different alpine images and make some tags pullOptions := &PullOptions{} pullOptions.Writer = os.Stdout - _, err := runtime.Pull(ctx, "docker.io/library/alpine:latest", config.PullPolicyAlways, pullOptions) + _, err := runtime.Pull(ctx, "docker.io/library/alpine:3.15", config.PullPolicyAlways, pullOptions) + require.NoError(t, err) + lookupOptions := &LookupImageOptions{} + img, _, err := runtime.LookupImage("docker.io/library/alpine:3.15", lookupOptions) + require.NoError(t, err) + img.Tag("docker.io/library/alpine") // imply latest + img.Tag("docker.io/library/alpine:3.15alpha") + _, err = runtime.Pull(ctx, "docker.io/library/alpine:3.14", config.PullPolicyAlways, pullOptions) require.NoError(t, err) pushOptions := &PushOptions{} - pushOptions.AllTags = true + pushOptions.AllTags = true // primary thing being tested here pushOptions.Writer = os.Stdout workdir, err := ioutil.TempDir("", "libimagepush") require.NoError(t, err) defer os.RemoveAll(workdir) - // tag image with alternates - lookupOptions := &LookupImageOptions{} - img, _, err := runtime.LookupImage("alpine", lookupOptions) - require.NoError(t, err) - img.Tag("01") - img.Tag("02") - for _, test := range []struct { source string destination string expectError bool }{ - {"alpine", "dir:" + workdir + "/dir", true}, - {"alpine", "containers-storage:localhost/another:alpine", true}, - {"alpine", "docker://docker.io/library/alpine:latest", true}, - {"alpine", "docker://docker.io/library/alpine", false}, - {"alpine", "docker.io/library/alpine", false}, + {"alpine", "docker.io/library/alpine", true}, // fail for destination + {"docker://docker.io/library/alpine", "", true}, // fail for transport + {"docker.io/library/alpine:latest", "", true}, // fail for tag + {"alpine:latest", "", true}, // fail for tag + // These two tests require authentication to a real registry to work + // {"myregistry/alpine", "", false}, + // {"example.com/myregistry/alpine", "", false}, } { _, err := runtime.Push(ctx, test.source, test.destination, pushOptions) if test.expectError { @@ -110,17 +112,12 @@ func TestPushAllTags(t *testing.T) { continue } require.NoError(t, err, "%v", test) - pullOptions.AllTags = true - pulledImages, err := runtime.Pull(ctx, test.destination, config.PullPolicyAlways, pullOptions) - require.NoError(t, err, "%v", test) - require.Len(t, pulledImages, 2, "%v", test) } // And now remove all of them. rmReports, rmErrors := runtime.RemoveImages(ctx, nil, nil) require.Len(t, rmErrors, 0) - require.Len(t, rmReports, 3) - + require.Len(t, rmReports, 2) } func TestPushOtherPlatform(t *testing.T) {