From f2adbc9685b4a7b4cab11e7b343be569f530368d Mon Sep 17 00:00:00 2001 From: Fabian Sommerauer Date: Wed, 4 Nov 2026 17:16:51 +0000 Subject: [PATCH] Fix image load response parsing in the docker manager ImagePush decoded only the first message of the docker ImageLoad response and used its stream text as the image reference. When an image with the same name:tag already exists, docker first sends "The image X already exists, renaming the old one with ID ... to empty string" and only then "Loaded image: X", so the warning was passed to ImageTag and the push failed with "Error parsing reference". Without a target name the reference was split with strings.SplitN and parts[1] was read unchecked, which panics when the stream has no ':' (empty stream, or "Loaded image ID: ..." for untagged images). - Read all messages of the response and use the first "Loaded image: " (or "Loaded image ID: ") line; ignore other messages. - Return errors reported in the stream (errorDetail / error) as Internal. - Return Internal if no loaded image is reported instead of panicking. - Untagged images ("Loaded image ID: "): tag the ID when a target name is given, otherwise return InvalidArgument. - Split name and tag at the last ':' that is not part of a registry host:port; default the tag to "latest". --- containers/docker/image_push.go | 105 +++++++++++++++++--- containers/docker/image_push_test.go | 143 +++++++++++++++++++++++++-- 2 files changed, 227 insertions(+), 21 deletions(-) diff --git a/containers/docker/image_push.go b/containers/docker/image_push.go index 1700d52..2bf946b 100644 --- a/containers/docker/image_push.go +++ b/containers/docker/image_push.go @@ -4,14 +4,15 @@ import ( "context" "encoding/json" "fmt" + "io" "os" - "github.com/docker/docker/client" "strings" + "github.com/docker/docker/client" "github.com/moby/moby/pkg/jsonmessage" + "github.com/openconfig/containerz/containers" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "github.com/openconfig/containerz/containers" ) // ImagePush pushes the container file to the containerz server. It can optionally tag the @@ -30,31 +31,105 @@ func (m *Manager) ImagePush(ctx context.Context, file *os.File, opts ...options. defer resp.Body.Close() if resp.Body != nil && resp.JSON { - var jm jsonmessage.JSONMessage - dec := json.NewDecoder(resp.Body) - if err := dec.Decode(&jm); err != nil { - return "", "", status.Convert(err).Err() + loaded, err := loadedImageFromStream(json.NewDecoder(resp.Body)) + if err != nil { + return "", "", err } - imageNameAndTag := extractImageNameFromStream(jm.Stream) - if options.TargetName != "" && imageNameAndTag != "" { - if err := m.client.ImageTag(ctx, imageNameAndTag, fmt.Sprintf("%s:%s", options.TargetName, options.TargetTag)); err != nil { + if options.TargetName != "" { + if err := m.client.ImageTag(ctx, loaded.ref(), fmt.Sprintf("%s:%s", options.TargetName, options.TargetTag)); err != nil { return "", "", status.Convert(err).Err() } return options.TargetName, options.TargetTag, nil } - parts := strings.SplitN(imageNameAndTag, ":", 2) - return parts[0], parts[1], nil + if loaded.name == "" { + return "", "", status.Errorf(codes.InvalidArgument, + "image was loaded as %s without a name and tag; a target name must be provided", loaded.id) + } + return loaded.name, loaded.tag, nil } return options.TargetName, options.TargetTag, nil } -func extractImageNameFromStream(stream string) string { - if len(stream) == 0 { - return "" +const ( + // loadedImagePrefix is the prefix docker uses to report a loaded image that + // has a name and tag. + loadedImagePrefix = "Loaded image: " + // loadedImageIDPrefix is the prefix docker uses to report a loaded image + // that has no name or tag. + loadedImageIDPrefix = "Loaded image ID: " +) + +// loadedImage describes the image reported by docker after an image load. +// Either name (and tag) or id is set. +type loadedImage struct { + name, tag string + id string +} + +// ref returns a reference to the loaded image that can be used as the source +// of an image tag operation. +func (l loadedImage) ref() string { + if l.name == "" { + return l.id + } + return l.name + ":" + l.tag +} + +// loadedImageFromStream reads all messages docker sends in response to an +// image load and returns the first image it reports as loaded. Other messages, +// such as the warning that an existing image with the same tag was renamed, +// are ignored. Errors reported in the stream are returned. +func loadedImageFromStream(dec *json.Decoder) (loadedImage, error) { + var loaded loadedImage + for { + var jm jsonmessage.JSONMessage + if err := dec.Decode(&jm); err != nil { + if err == io.EOF { + break + } + return loadedImage{}, status.Errorf(codes.Internal, "unable to decode image load response: %v", err) + } + + if jm.Error != nil { + return loadedImage{}, status.Errorf(codes.Internal, "unable to load image: %s", jm.Error.Message) + } + if jm.ErrorMessage != "" { + return loadedImage{}, status.Errorf(codes.Internal, "unable to load image: %s", jm.ErrorMessage) + } + + if loaded.name != "" || loaded.id != "" { + // Keep reading to surface errors reported after the image was loaded. + continue + } + for _, line := range strings.Split(jm.Stream, "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, loadedImageIDPrefix) { + loaded.id = strings.TrimPrefix(line, loadedImageIDPrefix) + break + } + if strings.HasPrefix(line, loadedImagePrefix) { + loaded.name, loaded.tag = splitImageReference(strings.TrimPrefix(line, loadedImagePrefix)) + break + } + } } - return strings.Trim(strings.Replace(stream, "Loaded image: ", "", 1), "\n") + if loaded.name == "" && loaded.id == "" { + return loadedImage{}, status.Error(codes.Internal, "image load response did not report a loaded image") + } + return loaded, nil +} + +// splitImageReference splits an image reference of the form name[:tag] into +// its name and tag. The tag defaults to "latest". A colon that is part of a +// registry host:port is not treated as the tag separator. +func splitImageReference(ref string) (string, string) { + i := strings.LastIndex(ref, ":") + if i < 0 || strings.Contains(ref[i+1:], "/") { + return ref, "latest" + } + return ref[:i], ref[i+1:] } diff --git a/containers/docker/image_push_test.go b/containers/docker/image_push_test.go index a59e908..b479a61 100644 --- a/containers/docker/image_push_test.go +++ b/containers/docker/image_push_test.go @@ -23,22 +23,30 @@ type fakePushingDocker struct { fakeDocker image, tag string isJSON bool + // msgs, if set, replaces the default "Loaded image" message. + msgs []*jsonmessage.JSONMessage Source, Target string } func (f fakePushingDocker) ImageLoad(ctx context.Context, input io.Reader, options ...client.ImageLoadOption) (image.LoadResponse, error) { - jm := &jsonmessage.JSONMessage{ - Stream: fmt.Sprintf("Loaded image: %s\n", f.image+":"+f.tag), + msgs := f.msgs + if msgs == nil { + msgs = []*jsonmessage.JSONMessage{{ + Stream: fmt.Sprintf("Loaded image: %s\n", f.image+":"+f.tag), + }} } - data, err := json.Marshal(jm) - if err != nil { - return image.LoadResponse{}, err + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + for _, jm := range msgs { + if err := enc.Encode(jm); err != nil { + return image.LoadResponse{}, err + } } return image.LoadResponse{ - Body: io.NopCloser(bytes.NewBuffer(data)), + Body: io.NopCloser(&buf), JSON: f.isJSON, }, nil } @@ -55,6 +63,7 @@ func TestImagePush(t *testing.T) { inOpts []options.Option inFile *os.File inImage, inTag string + inMsgs []*jsonmessage.JSONMessage isJSON bool wantState *fakePushingDocker wantImage, wantTag string @@ -87,6 +96,123 @@ func TestImagePush(t *testing.T) { wantImage: "another-image", wantTag: "another-tag", }, + { + name: "existing-image-warning-then-loaded", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "The image some-image:latest already exists, renaming the old one with ID sha256:1234 to empty string\n"}, + {Stream: "Loaded image: some-image:latest\n"}, + }, + wantImage: "some-image", + wantTag: "latest", + }, + { + name: "existing-image-warning-then-loaded-with-tagging", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "The image some-image:latest already exists, renaming the old one with ID sha256:1234 to empty string\n"}, + {Stream: "Loaded image: some-image:latest\n"}, + }, + inOpts: []options.Option{options.WithTarget("another-image", "another-tag")}, + wantState: &fakePushingDocker{ + Source: "some-image:latest", + Target: "another-image:another-tag", + }, + wantImage: "another-image", + wantTag: "another-tag", + }, + { + name: "warning-and-loaded-in-one-message", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "The image some-image:latest already exists, renaming the old one with ID sha256:1234 to empty string\nLoaded image: some-image:latest\n"}, + }, + wantImage: "some-image", + wantTag: "latest", + }, + { + name: "registry-with-port", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "Loaded image: registry:5000/some-image:some-tag\n"}, + }, + wantImage: "registry:5000/some-image", + wantTag: "some-tag", + }, + { + name: "no-tag-defaults-to-latest", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "Loaded image: registry:5000/some-image\n"}, + }, + wantImage: "registry:5000/some-image", + wantTag: "latest", + }, + { + name: "image-id-only-with-tagging", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "Loaded image ID: sha256:1234\n"}, + }, + inOpts: []options.Option{options.WithTarget("another-image", "another-tag")}, + wantState: &fakePushingDocker{ + Source: "sha256:1234", + Target: "another-image:another-tag", + }, + wantImage: "another-image", + wantTag: "another-tag", + }, + { + name: "image-id-only-without-target", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "Loaded image ID: sha256:1234\n"}, + }, + wantErr: status.Error(codes.InvalidArgument, "image was loaded as sha256:1234 without a name and tag; a target name must be provided"), + }, + { + name: "error-detail", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Error: &jsonmessage.JSONError{Message: "archive/tar: invalid tar header"}}, + }, + wantErr: status.Error(codes.Internal, "unable to load image: archive/tar: invalid tar header"), + }, + { + name: "error-after-loaded", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "Loaded image: some-image:latest\n"}, + {ErrorMessage: "something went wrong"}, + }, + wantErr: status.Error(codes.Internal, "unable to load image: something went wrong"), + }, + { + name: "empty-stream", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{}, + wantErr: status.Error(codes.Internal, "image load response did not report a loaded image"), + }, + { + name: "empty-stream-with-tagging", + isJSON: true, + inFile: os.Stdin, + inMsgs: []*jsonmessage.JSONMessage{ + {Stream: "\n"}, + }, + inOpts: []options.Option{options.WithTarget("another-image", "another-tag")}, + wantErr: status.Error(codes.Internal, "image load response did not report a loaded image"), + }, } for _, tc := range tests { @@ -95,6 +221,7 @@ func TestImagePush(t *testing.T) { isJSON: tc.isJSON, tag: tc.inTag, image: tc.inImage, + msgs: tc.inMsgs, } mgr := New(fd) @@ -109,6 +236,10 @@ func TestImagePush(t *testing.T) { t.Errorf("ImagePush(file) returned error: %v", err) } + if tc.wantErr != nil { + t.Fatalf("ImagePush(file) returned no error, want %v", tc.wantErr) + } + if gotImage != tc.wantImage || gotTag != tc.wantTag { t.Errorf("ImagePush(file) returned wrong info; want %s/%s, got %s/%s", tc.wantImage, tc.wantTag, gotImage, gotTag) }