Skip to content

Commit

Permalink
Improve push AllTags logic
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@byarbrough
byarbrough committed Jul 28, 2022
1 parent 951cdd6 commit 56a7865
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 62 deletions.
95 changes: 54 additions & 41 deletions libimage/push.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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 {
Expand All @@ -99,19 +117,14 @@ 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})
}

// 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}
Expand Down
39 changes: 18 additions & 21 deletions libimage/push_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,55 +72,52 @@ 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 {
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)

require.Len(t, rmReports, 2)
}

func TestPushOtherPlatform(t *testing.T) {
Expand Down

0 comments on commit 56a7865

Please sign in to comment.