From eb1315fcd5aee8fdb925e075c984b3c8cc63a538 Mon Sep 17 00:00:00 2001 From: byarbrough Date: Thu, 21 Jul 2022 13:51:02 -0600 Subject: [PATCH 1/3] Add option to push all tags This seeks to mirror `docker push --all-tags IMAGE`. Because tags are appended to the destination, this will only work with the docker transport. Otherwise you'd get directories overwriting or with weird names. Also note that if AllTags is true then the user must provide the name of the image only; providing a tag will crash. Docker has this behavior: ``` tag can't be used with --all-tags/-a ``` Requirement for https://github.com/containers/podman/pull/14949 Signed-off-by: Brian Yarbrough --- libimage/push.go | 55 ++++++++++++++++++++++++++++++++++++------ libimage/push_test.go | 56 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/libimage/push.go b/libimage/push.go index 7203838aa..94d6ca005 100644 --- a/libimage/push.go +++ b/libimage/push.go @@ -2,8 +2,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/alltransports" @@ -13,6 +16,7 @@ import ( // PushOptions allows for custommizing image pushes. type PushOptions struct { CopyOptions + AllTags bool } // Push pushes the specified source which must refer to an image in the local @@ -36,11 +40,6 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options return nil, err } - srcRef, err := image.StorageReference() - if err != nil { - return nil, err - } - // Make sure we have a proper destination, and parse it into an image // reference for copying. if destination == "" { @@ -50,7 +49,44 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options destination = resolvedSource } - logrus.Debugf("Pushing image %s to %s", source, destination) + // If specified to push --all-tags, look them up and iterate. + if options.AllTags { + + // 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") + } + + namedRepoTags, err := image.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) + if err != nil { + return nil, err + } + } + } else { + // No --all-tags, so just push just the single image. + return pushImage(ctx, destination, options, image, r) + } + + return nil, nil +} + +func pushImage(ctx context.Context, destination string, options *PushOptions, image *Image, r *Runtime) ([]byte, error) { + srcRef, err := image.StorageReference() + if err != nil { + return nil, err + } + + logrus.Debugf("Pushing image %s to %s", srcRef, destination) destRef, err := alltransports.ParseImageName(destination) if err != nil { @@ -63,6 +99,11 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options 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}) } @@ -70,7 +111,7 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options // 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(resolvedSource); err == nil { + if named, err := reference.ParseNamed(destination); 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 444f54d00..d174abc11 100644 --- a/libimage/push_test.go +++ b/libimage/push_test.go @@ -67,6 +67,62 @@ func TestPush(t *testing.T) { } } +func TestPushAllTags(t *testing.T) { + runtime, cleanup := testNewRuntime(t) + defer cleanup() + ctx := context.Background() + + // Prefetch alpine. + pullOptions := &PullOptions{} + pullOptions.Writer = os.Stdout + _, err := runtime.Pull(ctx, "docker.io/library/alpine:latest", config.PullPolicyAlways, pullOptions) + require.NoError(t, err) + + pushOptions := &PushOptions{} + pushOptions.AllTags = true + 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}, + } { + _, err := runtime.Push(ctx, test.source, test.destination, pushOptions) + if test.expectError { + require.Error(t, err, "%v", test) + 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) + +} + func TestPushOtherPlatform(t *testing.T) { runtime, cleanup := testNewRuntime(t) defer cleanup() From ab5bc69f92f070723919e05d326aba7a861115aa Mon Sep 17 00:00:00 2001 From: byarbrough Date: Thu, 28 Jul 2022 09:27:15 -0600 Subject: [PATCH 2/3] 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) { From ee1e826ab9a8f14903d43b138dd29e1e0c4c24aa Mon Sep 17 00:00:00 2001 From: Brian Yarbrough Date: Thu, 4 Aug 2022 15:16:47 -0600 Subject: [PATCH 3/3] Resolve conflict of same images to multiple repositories Now AllTags will filter again on the source, so you only push to the repository specified in the original source. Push() with AllTags=true returns a nil []byte Also addresses some minor comments and adds descriptions to new fields. Signed-off-by: Brian Yarbrough --- libimage/push.go | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/libimage/push.go b/libimage/push.go index 84f357a06..88caf2f57 100644 --- a/libimage/push.go +++ b/libimage/push.go @@ -15,6 +15,9 @@ import ( // PushOptions allows for custommizing image pushes. type PushOptions struct { CopyOptions + // If true then all images and tags matching a given repository + // will be pushed. Only supported for the docker transport. + // Usage of this flag will cause Push() to return a nil []byte. AllTags bool } @@ -27,6 +30,9 @@ type PushOptions struct { // // Return storage.ErrImageUnknown if source could not be found in the local // containers storage. +// Returns the bytes of the copied manifest when pushing a single tag, +// which may be used for digest computation. +// When pushing with AllTags=true then the returned []byte is always nil. func (r *Runtime) Push(ctx context.Context, source, destination string, options *PushOptions) ([]byte, error) { if options == nil { options = &PushOptions{} @@ -57,7 +63,8 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options // 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 + // For now, make sure a destination was not specified and get it from the source. + // This could change in the future, but that gets close to the Copy() functionality. if len(destination) != 0 { return nil, fmt.Errorf("`destination` should not be specified if using AllTags") } @@ -67,34 +74,39 @@ func (r *Runtime) Push(ctx context.Context, source, destination string, options if err != nil { return nil, err } - if _, hasTag := srcNamed.(reference.NamedTagged); hasTag { + if !reference.IsNameOnly(srcNamed) { return nil, fmt.Errorf("can't push with AllTags if source tag is specified") } - // Now list every image of that source repository + logrus.Debugf("Finding all images for source %s", srcNamed.Name()) 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) + srcImages, _ := r.ListImages(ctx, []string{srcNamed.Name()}, listOptions) // 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 } for _, n := range namedTagged { - destWithTag := fmt.Sprintf("%s:%s", source, n.Tag()) - b, err := pushImage(ctx, img, destWithTag, options, "", r) + // Filter on repo name again to avoid pushing an image that matches + // the source image ID but has a different repository than the source + currentNamed, err := reference.ParseNormalizedNamed(n.Name()) if err != nil { - return byteManifest, err + return nil, err + } + if reference.Path(currentNamed) == reference.Path(srcNamed) { + // Have to use Sprintf because pushImage expects a string + destWithTag := fmt.Sprintf("%s:%s", source, n.Tag()) + _, err := pushImage(ctx, img, destWithTag, options, "", r) + if err != nil { + return nil, err + } } - byteManifest = append(byteManifest, b...) } } - return byteManifest, nil + return nil, nil } // pushImage sends a single image to be copied to the destination