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) }