From e7f6b11ad577d34931352c8da46f88d1f6ff2c83 Mon Sep 17 00:00:00 2001 From: Satwik Sai Prakash Sahoo Date: Tue, 28 Jul 2026 12:09:56 +0530 Subject: [PATCH] copy: add OmitPrimaryManifestUpdateIfUnchanged option Copying an image that is already at the destination still writes the image's primary manifest unconditionally. Registries which enforce tag immutability reject that write, so an otherwise complete no-op copy fails against them. Add an option that makes the copy read the manifest currently stored at the destination and skip the write when it is byte-for-byte identical. The check lives in a new (*copier).putManifest helper used by both the single-image and the manifest-list paths, so the two cannot drift apart. The helper only does this for the primary manifest: instances of a manifest list are written by digest, which such registries do not restrict, and checking each of them would cost one extra read per instance for nothing. The option name says so, since the behaviour is not "omit any unchanged manifest update". Fixes: #918 Signed-off-by: Satwik Sai Prakash Sahoo --- image/copy/copy.go | 46 ++++++++++++++++++++++ image/copy/copy_test.go | 84 +++++++++++++++++++++++++++++++++++++++++ image/copy/multiple.go | 2 +- image/copy/single.go | 2 +- 4 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 image/copy/copy_test.go diff --git a/image/copy/copy.go b/image/copy/copy.go index 577473718b..a17c6f5e1b 100644 --- a/image/copy/copy.go +++ b/image/copy/copy.go @@ -1,6 +1,7 @@ package copy import ( + "bytes" "context" "errors" "fmt" @@ -154,6 +155,13 @@ type Options struct { // is slightly pessimistic if the destination image doesn't exist, or is not equivalent. OptimizeDestinationImageAlreadyExists bool + // When OmitPrimaryManifestUpdateIfUnchanged is set, the image's primary manifest is not + // written to the destination if the destination already contains a byte-for-byte identical + // one. That way a copy which changes nothing does not fail against destinations which reject + // writing to a name that already exists, e.g. registries enforcing tag immutability. + // Manifest instances of a manifest list are not affected, see (*copier).putManifest. + OmitPrimaryManifestUpdateIfUnchanged bool + // Download layer contents with "nondistributable" media types ("foreign" layers) and translate the layer media type // to not indicate "nondistributable". DownloadForeignLayers bool @@ -425,6 +433,44 @@ func (c *copier) Printf(format string, a ...any) { fmt.Fprintf(c.reportWriter, format, a...) } +// putManifest writes man to c.dest, with instanceDigest interpreted as in +// private.ImageDestination.PutManifest. +// +// If c.options.OmitPrimaryManifestUpdateIfUnchanged is set, the write is skipped when the +// destination already contains a byte-for-byte identical primary manifest; some destinations +// reject writing to a name that already exists, so an otherwise no-op copy would fail there. +// +// That check costs an extra read from the destination, so it is deliberately limited to the +// primary manifest (instanceDigest == nil): instances of a manifest list are written by digest, +// which such destinations don't restrict, and there can be arbitrarily many of them. +func (c *copier) putManifest(ctx context.Context, man []byte, instanceDigest *digest.Digest) error { + if c.options.OmitPrimaryManifestUpdateIfUnchanged && instanceDigest == nil { + if unchanged, err := c.destinationManifestEqual(ctx, man); err != nil { + // Not being able to read the destination is not fatal, we can still just write. + logrus.Debugf("Error reading manifest from destination, writing unconditionally: %v", err) + } else if unchanged { + logrus.Debugf("Skipping manifest write, destination already contains an identical manifest") + return nil + } + } + return c.dest.PutManifest(ctx, man, instanceDigest) +} + +// destinationManifestEqual reports whether the primary manifest currently stored at the +// destination is byte-for-byte identical to man. +func (c *copier) destinationManifestEqual(ctx context.Context, man []byte) (bool, error) { + src, err := c.dest.Reference().NewImageSource(ctx, c.options.DestinationCtx) + if err != nil { + return false, err + } + defer src.Close() + destManifest, _, err := src.GetManifest(ctx, nil) + if err != nil { + return false, err + } + return bytes.Equal(man, destManifest), nil +} + // close tears down state owned by copier. func (c *copier) close() { for i, s := range c.signersToClose { diff --git a/image/copy/copy_test.go b/image/copy/copy_test.go new file mode 100644 index 0000000000..eceb1524f2 --- /dev/null +++ b/image/copy/copy_test.go @@ -0,0 +1,84 @@ +package copy + +import ( + "context" + "testing" + + digest "github.com/opencontainers/go-digest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.podman.io/image/v5/directory" + "go.podman.io/image/v5/internal/imagedestination" + "go.podman.io/image/v5/internal/private" +) + +// putManifestCountingDestination counts the PutManifest calls that reach the underlying destination. +type putManifestCountingDestination struct { + private.ImageDestination + putManifestCalls int +} + +func (d *putManifestCountingDestination) PutManifest(ctx context.Context, manifest []byte, instanceDigest *digest.Digest) error { + d.putManifestCalls++ + return d.ImageDestination.PutManifest(ctx, manifest, instanceDigest) +} + +func TestCopierPutManifest(t *testing.T) { + const manifestContents = `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json"}` + instance := digest.FromString("instance") + + for _, c := range []struct { + name string + // omitIfUnchanged is the value of Options.OmitPrimaryManifestUpdateIfUnchanged. + omitIfUnchanged bool + // instanceDigest is passed to putManifest; nil writes the primary manifest. + instanceDigest *digest.Digest + // existing is what the destination already contains, "" for nothing at all. + existing string + // expectWrite is whether the destination is expected to be written to. + expectWrite bool + }{ + {name: "option not set", omitIfUnchanged: false, existing: manifestContents, expectWrite: true}, + {name: "unchanged primary manifest", omitIfUnchanged: true, existing: manifestContents, expectWrite: false}, + {name: "changed primary manifest", omitIfUnchanged: true, existing: `{"schemaVersion":2}`, expectWrite: true}, + {name: "nothing at the destination", omitIfUnchanged: true, existing: "", expectWrite: true}, + // Instances are written by digest, so they are never skipped, even if unchanged. + {name: "unchanged instance", omitIfUnchanged: true, instanceDigest: &instance, existing: manifestContents, expectWrite: true}, + } { + t.Run(c.name, func(t *testing.T) { + ctx := context.Background() + ref, err := directory.NewReference(t.TempDir()) + require.NoError(t, err) + publicDest, err := ref.NewImageDestination(ctx, nil) + require.NoError(t, err) + defer publicDest.Close() + dest := &putManifestCountingDestination{ImageDestination: imagedestination.FromPublic(publicDest)} + + if c.existing != "" { + err := dest.PutManifest(ctx, []byte(c.existing), c.instanceDigest) + require.NoError(t, err) + dest.putManifestCalls = 0 + } + + copier := &copier{ + dest: dest, + options: &Options{OmitPrimaryManifestUpdateIfUnchanged: c.omitIfUnchanged}, + } + err = copier.putManifest(ctx, []byte(manifestContents), c.instanceDigest) + require.NoError(t, err) + if c.expectWrite { + assert.Equal(t, 1, dest.putManifestCalls) + } else { + assert.Equal(t, 0, dest.putManifestCalls) + } + + // Either way, the destination ends up containing the manifest. + src, err := ref.NewImageSource(ctx, nil) + require.NoError(t, err) + defer src.Close() + m, _, err := src.GetManifest(ctx, c.instanceDigest) + require.NoError(t, err) + assert.Equal(t, manifestContents, string(m)) + }) + } +} diff --git a/image/copy/multiple.go b/image/copy/multiple.go index cb43ea7186..1430328956 100644 --- a/image/copy/multiple.go +++ b/image/copy/multiple.go @@ -413,7 +413,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, } // Save the manifest list. - err = c.dest.PutManifest(ctx, attemptedManifestList, nil) + err = c.putManifest(ctx, attemptedManifestList, nil) if err != nil { logrus.Debugf("Upload of manifest list type %s failed: %v", thisListType, err) errs = append(errs, fmt.Sprintf("%s(%v)", thisListType, err)) diff --git a/image/copy/single.go b/image/copy/single.go index 8a2af47af7..32157b64d9 100644 --- a/image/copy/single.go +++ b/image/copy/single.go @@ -602,7 +602,7 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanc if instanceDigest != nil { instanceDigest = &manifestDigest } - if err := ic.c.dest.PutManifest(ctx, man, instanceDigest); err != nil { + if err := ic.c.putManifest(ctx, man, instanceDigest); err != nil { logrus.Debugf("Error %v while writing manifest %q", err, string(man)) return nil, "", fmt.Errorf("writing manifest: %w", err) }