From c438c690575ac62fed7d7082c88464f114bc03c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:22:16 +0100
Subject: [PATCH 1/9] Try all manifest formats if the destinations accepts all
 of them
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We might want to trigger a conversion to OCI if the source is not
OCI, but the destination already contains a Zstd version of a layer.

We can do that for destinations that express a restricted list of
manifest formats, so it is unexpected that completely unrestricted
destinations can't trigger a conversion, and just fail (right now),
or perhaps don't fail but unnecessarily upload (in the future), in that case.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 copy/manifest.go      | 7 -------
 copy/manifest_test.go | 6 +++---
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/copy/manifest.go b/copy/manifest.go
index 8844ac8e7e..9210a617bb 100644
--- a/copy/manifest.go
+++ b/copy/manifest.go
@@ -85,13 +85,6 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
 
 	restrictiveCompressionRequired := in.requestedCompressionFormat != nil && !internalManifest.CompressionAlgorithmIsUniversallySupported(*in.requestedCompressionFormat)
 	if len(destSupportedManifestMIMETypes) == 0 {
-		if (!in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(srcType)) &&
-			(!restrictiveCompressionRequired || internalManifest.MIMETypeSupportsCompressionAlgorithm(srcType, *in.requestedCompressionFormat)) {
-			return manifestConversionPlan{ // Anything goes; just use the original as is, do not try any conversions.
-				preferredMIMEType:       srcType,
-				otherMIMETypeCandidates: []string{},
-			}, nil
-		}
 		destSupportedManifestMIMETypes = allManifestMIMETypes
 	}
 	supportedByDest := set.New[string]()
diff --git a/copy/manifest_test.go b/copy/manifest_test.go
index 7f22c0fe31..962b80ecf7 100644
--- a/copy/manifest_test.go
+++ b/copy/manifest_test.go
@@ -56,13 +56,13 @@ func TestDetermineManifestConversion(t *testing.T) {
 		destTypes   []string
 		expected    manifestConversionPlan
 	}{
-		// Destination accepts anything — no conversion necessary
+		// Destination accepts anything — consider all options, prefer the source format
 		{
 			"s1→anything", manifest.DockerV2Schema1SignedMediaType, nil,
 			manifestConversionPlan{
 				preferredMIMEType:                manifest.DockerV2Schema1SignedMediaType,
 				preferredMIMETypeNeedsConversion: false,
-				otherMIMETypeCandidates:          []string{},
+				otherMIMETypeCandidates:          []string{manifest.DockerV2Schema2MediaType, v1.MediaTypeImageManifest, manifest.DockerV2Schema1MediaType},
 			},
 		},
 		{
@@ -70,7 +70,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 			manifestConversionPlan{
 				preferredMIMEType:                manifest.DockerV2Schema2MediaType,
 				preferredMIMETypeNeedsConversion: false,
-				otherMIMETypeCandidates:          []string{},
+				otherMIMETypeCandidates:          []string{manifest.DockerV2Schema1SignedMediaType, v1.MediaTypeImageManifest, manifest.DockerV2Schema1MediaType},
 			},
 		},
 		// Destination accepts the unmodified original

From bbaad3dd2f39059a9c3ee66ae24962c04ca908c4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:24:39 +0100
Subject: [PATCH 2/9] Beautify determineManifestConversion a bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 copy/manifest.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/copy/manifest.go b/copy/manifest.go
index 9210a617bb..60ea92aae1 100644
--- a/copy/manifest.go
+++ b/copy/manifest.go
@@ -82,11 +82,11 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
 	if in.forceManifestMIMEType != "" {
 		destSupportedManifestMIMETypes = []string{in.forceManifestMIMEType}
 	}
-
-	restrictiveCompressionRequired := in.requestedCompressionFormat != nil && !internalManifest.CompressionAlgorithmIsUniversallySupported(*in.requestedCompressionFormat)
 	if len(destSupportedManifestMIMETypes) == 0 {
 		destSupportedManifestMIMETypes = allManifestMIMETypes
 	}
+
+	restrictiveCompressionRequired := in.requestedCompressionFormat != nil && !internalManifest.CompressionAlgorithmIsUniversallySupported(*in.requestedCompressionFormat)
 	supportedByDest := set.New[string]()
 	for _, t := range destSupportedManifestMIMETypes {
 		if in.requiresOCIEncryption && !manifest.MIMETypeSupportsEncryption(t) {

From 2879f25d0118855047779d34b8eac6986710edf2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:28:54 +0100
Subject: [PATCH 3/9] Reorder TryReusingBlobOptions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... to be a bit more similar to PutBlobOptions,
to be consistent with the constructor, and to start
with identification before parameters.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 internal/private/private.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/internal/private/private.go b/internal/private/private.go
index 72b574a5bd..9a52a749f8 100644
--- a/internal/private/private.go
+++ b/internal/private/private.go
@@ -112,11 +112,11 @@ type TryReusingBlobOptions struct {
 	// Transports, OTOH, MUST support these fields being zero-valued for types.ImageDestination callers
 	// if they use internal/imagedestination/impl.Compat;
 	// in that case, they will all be consistently zero-valued.
-	RequiredCompression *compression.Algorithm // If set, reuse blobs with a matching algorithm as per implementations in internal/imagedestination/impl.helpers.go
-	OriginalCompression *compression.Algorithm // Must be set if RequiredCompression is set; can be set to nil to indicate “uncompressed” or “unknown”.
 	EmptyLayer          bool                   // True if the blob is an "empty"/"throwaway" layer, and may not necessarily be physically represented.
 	LayerIndex          *int                   // If the blob is a layer, a zero-based index of the layer within the image; nil otherwise.
 	SrcRef              reference.Named        // A reference to the source image that contains the input blob.
+	RequiredCompression *compression.Algorithm // If set, reuse blobs with a matching algorithm as per implementations in internal/imagedestination/impl.helpers.go
+	OriginalCompression *compression.Algorithm // Must be set if RequiredCompression is set; can be set to nil to indicate “uncompressed” or “unknown”.
 	TOCDigest           *digest.Digest         // If specified, the blob can be looked up in the destination also by its TOC digest.
 }
 

From ea7437ece9d909b2b07590df8a8cc737d6e85280 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:35:53 +0100
Subject: [PATCH 4/9] Make manifestConversionPlan a field of imageCopier
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We will need to access it from copyLayer.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 copy/single.go | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/copy/single.go b/copy/single.go
index 9003965c95..d407d2ba1d 100644
--- a/copy/single.go
+++ b/copy/single.go
@@ -33,6 +33,7 @@ type imageCopier struct {
 	c                             *copier
 	manifestUpdates               *types.ManifestUpdateOptions
 	src                           *image.SourcedImage
+	manifestConversionPlan        manifestConversionPlan
 	diffIDsAreNeeded              bool
 	cannotModifyManifestReason    string // The reason the manifest cannot be modified, or an empty string if it can
 	canSubstituteBlobs            bool
@@ -136,7 +137,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 		c:               c,
 		manifestUpdates: &types.ManifestUpdateOptions{InformationOnly: types.ManifestUpdateInformation{Destination: c.dest}},
 		src:             src,
-		// diffIDsAreNeeded is computed later
+		// manifestConversionPlan and diffIDsAreNeeded are computed later
 		cannotModifyManifestReason:    cannotModifyManifestReason,
 		requireCompressionFormatMatch: opts.requireCompressionFormatMatch,
 	}
@@ -164,7 +165,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 
 	destRequiresOciEncryption := (isEncrypted(src) && ic.c.options.OciDecryptConfig == nil) || c.options.OciEncryptLayers != nil
 
-	manifestConversionPlan, err := determineManifestConversion(determineManifestConversionInputs{
+	ic.manifestConversionPlan, err = determineManifestConversion(determineManifestConversionInputs{
 		srcMIMEType:                    ic.src.ManifestMIMEType,
 		destSupportedManifestMIMETypes: ic.c.dest.SupportedManifestMIMETypes(),
 		forceManifestMIMEType:          c.options.ForceManifestMIMEType,
@@ -179,8 +180,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 	// code that calls copyUpdatedConfigAndManifest, so that other parts of the copy code
 	// (e.g. the UpdatedImageNeedsLayerDiffIDs check just below) can make decisions based
 	// on the expected destination format.
-	if manifestConversionPlan.preferredMIMETypeNeedsConversion {
-		ic.manifestUpdates.ManifestMIMEType = manifestConversionPlan.preferredMIMEType
+	if ic.manifestConversionPlan.preferredMIMETypeNeedsConversion {
+		ic.manifestUpdates.ManifestMIMEType = ic.manifestConversionPlan.preferredMIMEType
 	}
 
 	// If src.UpdatedImageNeedsLayerDiffIDs(ic.manifestUpdates) will be true, it needs to be true by the time we get here.
@@ -219,11 +220,11 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 	manifestBytes, manifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance)
 	wipResult := copySingleImageResult{
 		manifest:         manifestBytes,
-		manifestMIMEType: manifestConversionPlan.preferredMIMEType,
+		manifestMIMEType: ic.manifestConversionPlan.preferredMIMEType,
 		manifestDigest:   manifestDigest,
 	}
 	if err != nil {
-		logrus.Debugf("Writing manifest using preferred type %s failed: %v", manifestConversionPlan.preferredMIMEType, err)
+		logrus.Debugf("Writing manifest using preferred type %s failed: %v", ic.manifestConversionPlan.preferredMIMEType, err)
 		// … if it fails, and the failure is either because the manifest is rejected by the registry, or
 		// because we failed to create a manifest of the specified type because the specific manifest type
 		// doesn't support the type of compression we're trying to use (e.g. docker v2s2 and zstd), we may
@@ -232,13 +233,13 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 		var manifestLayerCompressionIncompatibilityError manifest.ManifestLayerCompressionIncompatibilityError
 		isManifestRejected := errors.As(err, &manifestTypeRejectedError)
 		isCompressionIncompatible := errors.As(err, &manifestLayerCompressionIncompatibilityError)
-		if (!isManifestRejected && !isCompressionIncompatible) || len(manifestConversionPlan.otherMIMETypeCandidates) == 0 {
+		if (!isManifestRejected && !isCompressionIncompatible) || len(ic.manifestConversionPlan.otherMIMETypeCandidates) == 0 {
 			// We don’t have other options.
 			// In principle the code below would handle this as well, but the resulting  error message is fairly ugly.
 			// Don’t bother the user with MIME types if we have no choice.
 			return copySingleImageResult{}, err
 		}
-		// If the original MIME type is acceptable, determineManifestConversion always uses it as manifestConversionPlan.preferredMIMEType.
+		// If the original MIME type is acceptable, determineManifestConversion always uses it as ic.manifestConversionPlan.preferredMIMEType.
 		// So if we are here, we will definitely be trying to convert the manifest.
 		// With ic.cannotModifyManifestReason != "", that would just be a string of repeated failures for the same reason,
 		// so let’s bail out early and with a better error message.
@@ -247,8 +248,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 		}
 
 		// errs is a list of errors when trying various manifest types. Also serves as an "upload succeeded" flag when set to nil.
-		errs := []string{fmt.Sprintf("%s(%v)", manifestConversionPlan.preferredMIMEType, err)}
-		for _, manifestMIMEType := range manifestConversionPlan.otherMIMETypeCandidates {
+		errs := []string{fmt.Sprintf("%s(%v)", ic.manifestConversionPlan.preferredMIMEType, err)}
+		for _, manifestMIMEType := range ic.manifestConversionPlan.otherMIMETypeCandidates {
 			logrus.Debugf("Trying to use manifest type %s…", manifestMIMEType)
 			ic.manifestUpdates.ManifestMIMEType = manifestMIMEType
 			attemptedManifest, attemptedManifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance)

From 4f86a47b7522247a61f9230209a9e44ec3b72533 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:38:38 +0100
Subject: [PATCH 5/9] Refactor BlobMatchesRequiredCompression a bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... to allow adding more conditions

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 internal/imagedestination/impl/helpers.go | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/internal/imagedestination/impl/helpers.go b/internal/imagedestination/impl/helpers.go
index 5d28b3e73a..b6bdce5fa2 100644
--- a/internal/imagedestination/impl/helpers.go
+++ b/internal/imagedestination/impl/helpers.go
@@ -9,15 +9,18 @@ import (
 // then function performs a match against the compression requested by the caller and compression of existing blob
 // (which can be nil to represent uncompressed or unknown)
 func BlobMatchesRequiredCompression(options private.TryReusingBlobOptions, candidateCompression *compression.Algorithm) bool {
-	if options.RequiredCompression == nil {
-		return true // no requirement imposed
+	if options.RequiredCompression != nil {
+		if options.RequiredCompression.Name() == compression.ZstdChunkedAlgorithmName {
+			// HACK: Never match when the caller asks for zstd:chunked, because we don’t record the annotations required to use the chunked blobs.
+			// The caller must re-compress to build those annotations.
+			return false
+		}
+		if candidateCompression == nil || (options.RequiredCompression.Name() != candidateCompression.Name()) {
+			return false
+		}
 	}
-	if options.RequiredCompression.Name() == compression.ZstdChunkedAlgorithmName {
-		// HACK: Never match when the caller asks for zstd:chunked, because we don’t record the annotations required to use the chunked blobs.
-		// The caller must re-compress to build those annotations.
-		return false
-	}
-	return candidateCompression != nil && (options.RequiredCompression.Name() == candidateCompression.Name())
+
+	return true
 }
 
 func OriginalBlobMatchesRequiredCompression(opts private.TryReusingBlobOptions) bool {

From ffd21be77874b12ee5fa827c21852a66c17d982a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:42:51 +0100
Subject: [PATCH 6/9] Rename *BlobMatchesRequiredCompression to
 *CandidateMatchesTryReusingBlobOptions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... because we will add more reasons to reject a match.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 copy/compression.go                            | 2 +-
 directory/directory_dest.go                    | 2 +-
 docker/docker_image_dest.go                    | 4 ++--
 docker/internal/tarfile/dest.go                | 2 +-
 internal/imagedestination/impl/helpers.go      | 8 ++++----
 internal/imagedestination/impl/helpers_test.go | 4 ++--
 oci/layout/oci_dest.go                         | 2 +-
 ostree/ostree_dest.go                          | 2 +-
 pkg/blobcache/dest.go                          | 2 +-
 storage/storage_dest.go                        | 2 +-
 10 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/copy/compression.go b/copy/compression.go
index a42e3b67ab..953ef9deb2 100644
--- a/copy/compression.go
+++ b/copy/compression.go
@@ -286,7 +286,7 @@ func (d *bpCompressionStepData) recordValidatedDigestData(c *copier, uploadedInf
 	if d.uploadedCompressorName != "" && d.uploadedCompressorName != internalblobinfocache.UnknownCompression {
 		if d.uploadedCompressorName != compressiontypes.ZstdChunkedAlgorithmName {
 			// HACK: Don’t record zstd:chunked algorithms.
-			// There is already a similar hack in internal/imagedestination/impl/helpers.BlobMatchesRequiredCompression,
+			// There is already a similar hack in internal/imagedestination/impl/helpers.CandidateMatchesTryReusingBlobOptions,
 			// and that one prevents reusing zstd:chunked blobs, so recording the algorithm here would be mostly harmless.
 			//
 			// We skip that here anyway to work around the inability of blobPipelineDetectCompressionStep to differentiate
diff --git a/directory/directory_dest.go b/directory/directory_dest.go
index 222723a8f5..9d92192418 100644
--- a/directory/directory_dest.go
+++ b/directory/directory_dest.go
@@ -190,7 +190,7 @@ func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
 // If the blob has been successfully reused, returns (true, info, nil).
 // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
 func (d *dirImageDestination) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) {
-	if !impl.OriginalBlobMatchesRequiredCompression(options) {
+	if !impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		return false, private.ReusedBlob{}, nil
 	}
 	if info.Digest == "" {
diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go
index a9a36f0a34..0a623cb458 100644
--- a/docker/docker_image_dest.go
+++ b/docker/docker_image_dest.go
@@ -321,7 +321,7 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 		return false, private.ReusedBlob{}, errors.New("Can not check for a blob with unknown digest")
 	}
 
-	if impl.OriginalBlobMatchesRequiredCompression(options) {
+	if impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		// First, check whether the blob happens to already exist at the destination.
 		haveBlob, reusedInfo, err := d.tryReusingExactBlob(ctx, info, options.Cache)
 		if err != nil {
@@ -355,7 +355,7 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 				continue
 			}
 		}
-		if !impl.BlobMatchesRequiredCompression(options, compressionAlgorithm) {
+		if !impl.CandidateMatchesTryReusingBlobOptions(options, compressionAlgorithm) {
 			requiredCompression := "nil"
 			if compressionAlgorithm != nil {
 				requiredCompression = compressionAlgorithm.Name()
diff --git a/docker/internal/tarfile/dest.go b/docker/internal/tarfile/dest.go
index 7507d85595..b44f4ca1f5 100644
--- a/docker/internal/tarfile/dest.go
+++ b/docker/internal/tarfile/dest.go
@@ -129,7 +129,7 @@ func (d *Destination) PutBlobWithOptions(ctx context.Context, stream io.Reader,
 // If the blob has been successfully reused, returns (true, info, nil).
 // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
 func (d *Destination) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) {
-	if !impl.OriginalBlobMatchesRequiredCompression(options) {
+	if !impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		return false, private.ReusedBlob{}, nil
 	}
 	if err := d.archive.lock(); err != nil {
diff --git a/internal/imagedestination/impl/helpers.go b/internal/imagedestination/impl/helpers.go
index b6bdce5fa2..6d4d914382 100644
--- a/internal/imagedestination/impl/helpers.go
+++ b/internal/imagedestination/impl/helpers.go
@@ -5,10 +5,10 @@ import (
 	compression "github.com/containers/image/v5/pkg/compression/types"
 )
 
-// BlobMatchesRequiredCompression validates if compression is required by the caller while selecting a blob, if it is required
+// CandidateMatchesTryReusingBlobOptions validates if compression is required by the caller while selecting a blob, if it is required
 // then function performs a match against the compression requested by the caller and compression of existing blob
 // (which can be nil to represent uncompressed or unknown)
-func BlobMatchesRequiredCompression(options private.TryReusingBlobOptions, candidateCompression *compression.Algorithm) bool {
+func CandidateMatchesTryReusingBlobOptions(options private.TryReusingBlobOptions, candidateCompression *compression.Algorithm) bool {
 	if options.RequiredCompression != nil {
 		if options.RequiredCompression.Name() == compression.ZstdChunkedAlgorithmName {
 			// HACK: Never match when the caller asks for zstd:chunked, because we don’t record the annotations required to use the chunked blobs.
@@ -23,6 +23,6 @@ func BlobMatchesRequiredCompression(options private.TryReusingBlobOptions, candi
 	return true
 }
 
-func OriginalBlobMatchesRequiredCompression(opts private.TryReusingBlobOptions) bool {
-	return BlobMatchesRequiredCompression(opts, opts.OriginalCompression)
+func OriginalCandidateMatchesTryReusingBlobOptions(opts private.TryReusingBlobOptions) bool {
+	return CandidateMatchesTryReusingBlobOptions(opts, opts.OriginalCompression)
 }
diff --git a/internal/imagedestination/impl/helpers_test.go b/internal/imagedestination/impl/helpers_test.go
index 8a80d1d40d..db76f0de64 100644
--- a/internal/imagedestination/impl/helpers_test.go
+++ b/internal/imagedestination/impl/helpers_test.go
@@ -9,7 +9,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestBlobMatchesRequiredCompression(t *testing.T) {
+func TestCandidateMatchesTryReusingBlobOptions(t *testing.T) {
 	var opts private.TryReusingBlobOptions
 	cases := []struct {
 		requiredCompression  *compressionTypes.Algorithm
@@ -24,6 +24,6 @@ func TestBlobMatchesRequiredCompression(t *testing.T) {
 
 	for _, c := range cases {
 		opts = private.TryReusingBlobOptions{RequiredCompression: c.requiredCompression}
-		assert.Equal(t, c.result, BlobMatchesRequiredCompression(opts, c.candidateCompression))
+		assert.Equal(t, c.result, CandidateMatchesTryReusingBlobOptions(opts, c.candidateCompression))
 	}
 }
diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go
index 100d16763f..305d8c9c71 100644
--- a/oci/layout/oci_dest.go
+++ b/oci/layout/oci_dest.go
@@ -173,7 +173,7 @@ func (d *ociImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
 // If the blob has been successfully reused, returns (true, info, nil).
 // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
 func (d *ociImageDestination) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) {
-	if !impl.OriginalBlobMatchesRequiredCompression(options) {
+	if !impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		return false, private.ReusedBlob{}, nil
 	}
 	if info.Digest == "" {
diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go
index d00a0cdf86..228af90ca0 100644
--- a/ostree/ostree_dest.go
+++ b/ostree/ostree_dest.go
@@ -335,7 +335,7 @@ func (d *ostreeImageDestination) importConfig(repo *otbuiltin.Repo, blob *blobTo
 // reflected in the manifest that will be written.
 // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
 func (d *ostreeImageDestination) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) {
-	if !impl.OriginalBlobMatchesRequiredCompression(options) {
+	if !impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		return false, private.ReusedBlob{}, nil
 	}
 	if d.repo == nil {
diff --git a/pkg/blobcache/dest.go b/pkg/blobcache/dest.go
index 9bda085158..f32fccf805 100644
--- a/pkg/blobcache/dest.go
+++ b/pkg/blobcache/dest.go
@@ -237,7 +237,7 @@ func (d *blobCacheDestination) PutBlobPartial(ctx context.Context, chunkAccessor
 // If the blob has been successfully reused, returns (true, info, nil).
 // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
 func (d *blobCacheDestination) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) {
-	if !impl.OriginalBlobMatchesRequiredCompression(options) {
+	if !impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		return false, private.ReusedBlob{}, nil
 	}
 	present, reusedInfo, err := d.destination.TryReusingBlobWithOptions(ctx, info, options)
diff --git a/storage/storage_dest.go b/storage/storage_dest.go
index b2bc26fbf8..53569793e6 100644
--- a/storage/storage_dest.go
+++ b/storage/storage_dest.go
@@ -307,7 +307,7 @@ func (s *storageImageDestination) PutBlobPartial(ctx context.Context, chunkAcces
 // If the blob has been successfully reused, returns (true, info, nil).
 // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
 func (s *storageImageDestination) TryReusingBlobWithOptions(ctx context.Context, blobinfo types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) {
-	if !impl.OriginalBlobMatchesRequiredCompression(options) {
+	if !impl.OriginalCandidateMatchesTryReusingBlobOptions(options) {
 		return false, private.ReusedBlob{}, nil
 	}
 	reused, info, err := s.tryReusingBlobAsPending(blobinfo.Digest, blobinfo.Size, &options)

From 7a9148a5908bd08aa171070f63a24b9783f7616f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 6 Dec 2023 18:46:45 +0100
Subject: [PATCH 7/9] Always pass OriginalCompression to
 TryReusingBlobWithOptions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There's no reason not to, and we will need it in other cases
as well.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 copy/single.go | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/copy/single.go b/copy/single.go
index d407d2ba1d..57e6060987 100644
--- a/copy/single.go
+++ b/copy/single.go
@@ -691,10 +691,8 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 		// Fixing that will probably require passing more information to TryReusingBlob() than the current version of
 		// the ImageDestination interface lets us pass in.
 		var requiredCompression *compressiontypes.Algorithm
-		var originalCompression *compressiontypes.Algorithm
 		if ic.requireCompressionFormatMatch {
 			requiredCompression = ic.compressionFormat
-			originalCompression = srcInfo.CompressionAlgorithm
 		}
 
 		// Check if we have a chunked layer in storage that's based on that blob.  These layers are stored by their TOC digest.
@@ -710,7 +708,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 			LayerIndex:          &layerIndex,
 			SrcRef:              srcRef,
 			RequiredCompression: requiredCompression,
-			OriginalCompression: originalCompression,
+			OriginalCompression: srcInfo.CompressionAlgorithm,
 			TOCDigest:           tocDigest,
 		})
 		if err != nil {

From 4d91fdaf114935047a9fac666dfb384f86c90301 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Tue, 30 Jan 2024 22:43:53 +0100
Subject: [PATCH 8/9] Add an optionalCompressionName helper
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... and eliminate incorrectly named requiredCompression variables
by using it.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 docker/docker_image_dest.go | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go
index 0a623cb458..72f2fa5f97 100644
--- a/docker/docker_image_dest.go
+++ b/docker/docker_image_dest.go
@@ -27,6 +27,7 @@ import (
 	"github.com/containers/image/v5/internal/uploadreader"
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/pkg/blobinfocache/none"
+	compressiontypes "github.com/containers/image/v5/pkg/compression/types"
 	"github.com/containers/image/v5/types"
 	"github.com/docker/distribution/registry/api/errcode"
 	v2 "github.com/docker/distribution/registry/api/v2"
@@ -311,6 +312,13 @@ func (d *dockerImageDestination) tryReusingExactBlob(ctx context.Context, info t
 	return false, private.ReusedBlob{}, nil
 }
 
+func optionalCompressionName(algo *compressiontypes.Algorithm) string {
+	if algo != nil {
+		return algo.Name()
+	}
+	return "nil"
+}
+
 // TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination
 // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree).
 // info.Digest must not be empty.
@@ -331,11 +339,7 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 			return true, reusedInfo, nil
 		}
 	} else {
-		requiredCompression := "nil"
-		if options.OriginalCompression != nil {
-			requiredCompression = options.OriginalCompression.Name()
-		}
-		logrus.Debugf("Ignoring exact blob match case due to compression mismatch ( %s vs %s )", options.RequiredCompression.Name(), requiredCompression)
+		logrus.Debugf("Ignoring exact blob match case due to compression mismatch ( %s vs %s )", options.RequiredCompression.Name(), optionalCompressionName(options.OriginalCompression))
 	}
 
 	// Then try reusing blobs from other locations.
@@ -356,14 +360,10 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 			}
 		}
 		if !impl.CandidateMatchesTryReusingBlobOptions(options, compressionAlgorithm) {
-			requiredCompression := "nil"
-			if compressionAlgorithm != nil {
-				requiredCompression = compressionAlgorithm.Name()
-			}
 			if !candidate.UnknownLocation {
-				logrus.Debugf("Ignoring candidate blob %s as reuse candidate due to compression mismatch ( %s vs %s ) in %s", candidate.Digest.String(), options.RequiredCompression.Name(), requiredCompression, candidateRepo.Name())
+				logrus.Debugf("Ignoring candidate blob %s as reuse candidate due to compression mismatch ( %s vs %s ) in %s", candidate.Digest.String(), options.RequiredCompression.Name(), optionalCompressionName(compressionAlgorithm), candidateRepo.Name())
 			} else {
-				logrus.Debugf("Ignoring candidate blob %s as reuse candidate due to compression mismatch ( %s vs %s ) with no location match, checking current repo", candidate.Digest.String(), options.RequiredCompression.Name(), requiredCompression)
+				logrus.Debugf("Ignoring candidate blob %s as reuse candidate due to compression mismatch ( %s vs %s ) with no location match, checking current repo", candidate.Digest.String(), options.RequiredCompression.Name(), optionalCompressionName(compressionAlgorithm))
 			}
 			continue
 		}

From 8aadec3c57f8ef04e83febce628aaaaff3868498 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Tue, 30 Jan 2024 22:45:38 +0100
Subject: [PATCH 9/9] Add and implement
 TryReusingOptions.PossibleManifestFormats
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... so that we limit blob reuse to acceptable conversion formats,
based on possible manifest formats.

Implement it in CandidateMatchesTryReusingBlobOptions, covering all
(in-tree) implementations.

We still _try_ converting to schema2/schema1 (and upload the schema1
empty layer to the destination registry) before succeeding with OCI.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 copy/single.go                                | 24 ++++++-------
 docker/docker_image_dest.go                   |  9 +++--
 internal/imagedestination/impl/helpers.go     | 13 +++++++
 .../imagedestination/impl/helpers_test.go     | 36 ++++++++++++++-----
 internal/private/private.go                   | 13 +++----
 5 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/copy/single.go b/copy/single.go
index 57e6060987..c233619427 100644
--- a/copy/single.go
+++ b/copy/single.go
@@ -684,12 +684,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 		logrus.Debugf("Checking if we can reuse blob %s: general substitution = %v, compression for MIME type %q = %v",
 			srcInfo.Digest, ic.canSubstituteBlobs, srcInfo.MediaType, canChangeLayerCompression)
 		canSubstitute := ic.canSubstituteBlobs && ic.src.CanChangeLayerCompression(srcInfo.MediaType)
-		// TODO: at this point we don't know whether or not a blob we end up reusing is compressed using an algorithm
-		// that is acceptable for use on layers in the manifest that we'll be writing later, so if we end up reusing
-		// a blob that's compressed with e.g. zstd, but we're only allowed to write a v2s2 manifest, this will cause
-		// a failure when we eventually try to update the manifest with the digest and MIME type of the reused blob.
-		// Fixing that will probably require passing more information to TryReusingBlob() than the current version of
-		// the ImageDestination interface lets us pass in.
+
 		var requiredCompression *compressiontypes.Algorithm
 		if ic.requireCompressionFormatMatch {
 			requiredCompression = ic.compressionFormat
@@ -702,14 +697,15 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 		}
 
 		reused, reusedBlob, err := ic.c.dest.TryReusingBlobWithOptions(ctx, srcInfo, private.TryReusingBlobOptions{
-			Cache:               ic.c.blobInfoCache,
-			CanSubstitute:       canSubstitute,
-			EmptyLayer:          emptyLayer,
-			LayerIndex:          &layerIndex,
-			SrcRef:              srcRef,
-			RequiredCompression: requiredCompression,
-			OriginalCompression: srcInfo.CompressionAlgorithm,
-			TOCDigest:           tocDigest,
+			Cache:                   ic.c.blobInfoCache,
+			CanSubstitute:           canSubstitute,
+			EmptyLayer:              emptyLayer,
+			LayerIndex:              &layerIndex,
+			SrcRef:                  srcRef,
+			PossibleManifestFormats: append([]string{ic.manifestConversionPlan.preferredMIMEType}, ic.manifestConversionPlan.otherMIMETypeCandidates...),
+			RequiredCompression:     requiredCompression,
+			OriginalCompression:     srcInfo.CompressionAlgorithm,
+			TOCDigest:               tocDigest,
 		})
 		if err != nil {
 			return types.BlobInfo{}, "", fmt.Errorf("trying to reuse blob %s at destination: %w", srcInfo.Digest, err)
diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go
index 72f2fa5f97..877d11b738 100644
--- a/docker/docker_image_dest.go
+++ b/docker/docker_image_dest.go
@@ -339,7 +339,8 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 			return true, reusedInfo, nil
 		}
 	} else {
-		logrus.Debugf("Ignoring exact blob match case due to compression mismatch ( %s vs %s )", options.RequiredCompression.Name(), optionalCompressionName(options.OriginalCompression))
+		logrus.Debugf("Ignoring exact blob match, compression %s does not match required %s or MIME types %#v",
+			optionalCompressionName(options.OriginalCompression), optionalCompressionName(options.RequiredCompression), options.PossibleManifestFormats)
 	}
 
 	// Then try reusing blobs from other locations.
@@ -361,9 +362,11 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 		}
 		if !impl.CandidateMatchesTryReusingBlobOptions(options, compressionAlgorithm) {
 			if !candidate.UnknownLocation {
-				logrus.Debugf("Ignoring candidate blob %s as reuse candidate due to compression mismatch ( %s vs %s ) in %s", candidate.Digest.String(), options.RequiredCompression.Name(), optionalCompressionName(compressionAlgorithm), candidateRepo.Name())
+				logrus.Debugf("Ignoring candidate blob %s in %s, compression %s does not match required %s or MIME types %#v", candidate.Digest.String(), candidateRepo.Name(),
+					optionalCompressionName(compressionAlgorithm), optionalCompressionName(options.RequiredCompression), options.PossibleManifestFormats)
 			} else {
-				logrus.Debugf("Ignoring candidate blob %s as reuse candidate due to compression mismatch ( %s vs %s ) with no location match, checking current repo", candidate.Digest.String(), options.RequiredCompression.Name(), optionalCompressionName(compressionAlgorithm))
+				logrus.Debugf("Ignoring candidate blob %s with no known location, compression %s does not match required %s or MIME types %#v", candidate.Digest.String(),
+					optionalCompressionName(compressionAlgorithm), optionalCompressionName(options.RequiredCompression), options.PossibleManifestFormats)
 			}
 			continue
 		}
diff --git a/internal/imagedestination/impl/helpers.go b/internal/imagedestination/impl/helpers.go
index 6d4d914382..a8a9f51bed 100644
--- a/internal/imagedestination/impl/helpers.go
+++ b/internal/imagedestination/impl/helpers.go
@@ -1,8 +1,10 @@
 package impl
 
 import (
+	"github.com/containers/image/v5/internal/manifest"
 	"github.com/containers/image/v5/internal/private"
 	compression "github.com/containers/image/v5/pkg/compression/types"
+	"golang.org/x/exp/slices"
 )
 
 // CandidateMatchesTryReusingBlobOptions validates if compression is required by the caller while selecting a blob, if it is required
@@ -20,6 +22,17 @@ func CandidateMatchesTryReusingBlobOptions(options private.TryReusingBlobOptions
 		}
 	}
 
+	// For candidateCompression == nil, we can’t tell the difference between “uncompressed” and “unknown”;
+	// and “uncompressed” is acceptable in all known formats (well, it seems to work in practice for schema1),
+	// so don’t impose any restrictions if candidateCompression == nil
+	if options.PossibleManifestFormats != nil && candidateCompression != nil {
+		if !slices.ContainsFunc(options.PossibleManifestFormats, func(mt string) bool {
+			return manifest.MIMETypeSupportsCompressionAlgorithm(mt, *candidateCompression)
+		}) {
+			return false
+		}
+	}
+
 	return true
 }
 
diff --git a/internal/imagedestination/impl/helpers_test.go b/internal/imagedestination/impl/helpers_test.go
index db76f0de64..93749b1bd5 100644
--- a/internal/imagedestination/impl/helpers_test.go
+++ b/internal/imagedestination/impl/helpers_test.go
@@ -4,26 +4,44 @@ import (
 	"testing"
 
 	"github.com/containers/image/v5/internal/private"
+	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/pkg/compression"
 	compressionTypes "github.com/containers/image/v5/pkg/compression/types"
+	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestCandidateMatchesTryReusingBlobOptions(t *testing.T) {
-	var opts private.TryReusingBlobOptions
 	cases := []struct {
-		requiredCompression  *compressionTypes.Algorithm
-		candidateCompression *compressionTypes.Algorithm
-		result               bool
+		requiredCompression     *compressionTypes.Algorithm
+		possibleManifestFormats []string
+		candidateCompression    *compressionTypes.Algorithm
+		result                  bool
 	}{
-		{&compression.Zstd, &compression.Zstd, true},
-		{&compression.Gzip, &compression.Zstd, false},
-		{&compression.Zstd, nil, false},
-		{nil, &compression.Zstd, true},
+		// RequiredCompression restrictions
+		{&compression.Zstd, nil, &compression.Zstd, true},
+		{&compression.Gzip, nil, &compression.Zstd, false},
+		{&compression.Zstd, nil, nil, false},
+		{nil, nil, &compression.Zstd, true},
+		// PossibleManifestFormats restrictions
+		{nil, []string{imgspecv1.MediaTypeImageManifest}, &compression.Zstd, true},
+		{nil, []string{manifest.DockerV2Schema2MediaType}, &compression.Zstd, false},
+		{nil, []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest}, &compression.Zstd, true},
+		{nil, nil, &compression.Zstd, true},
+		{nil, []string{imgspecv1.MediaTypeImageManifest}, &compression.Gzip, true},
+		{nil, []string{manifest.DockerV2Schema2MediaType}, &compression.Gzip, true},
+		{nil, []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest}, &compression.Gzip, true},
+		{nil, nil, &compression.Gzip, true},
+		// Some possible combinations (always 1 constraint not matching)
+		{&compression.Zstd, []string{manifest.DockerV2Schema2MediaType}, &compression.Zstd, false},
+		{&compression.Gzip, []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest}, &compression.Zstd, false},
 	}
 
 	for _, c := range cases {
-		opts = private.TryReusingBlobOptions{RequiredCompression: c.requiredCompression}
+		opts := private.TryReusingBlobOptions{
+			RequiredCompression:     c.requiredCompression,
+			PossibleManifestFormats: c.possibleManifestFormats,
+		}
 		assert.Equal(t, c.result, CandidateMatchesTryReusingBlobOptions(opts, c.candidateCompression))
 	}
 }
diff --git a/internal/private/private.go b/internal/private/private.go
index 9a52a749f8..7037755bfc 100644
--- a/internal/private/private.go
+++ b/internal/private/private.go
@@ -112,12 +112,13 @@ type TryReusingBlobOptions struct {
 	// Transports, OTOH, MUST support these fields being zero-valued for types.ImageDestination callers
 	// if they use internal/imagedestination/impl.Compat;
 	// in that case, they will all be consistently zero-valued.
-	EmptyLayer          bool                   // True if the blob is an "empty"/"throwaway" layer, and may not necessarily be physically represented.
-	LayerIndex          *int                   // If the blob is a layer, a zero-based index of the layer within the image; nil otherwise.
-	SrcRef              reference.Named        // A reference to the source image that contains the input blob.
-	RequiredCompression *compression.Algorithm // If set, reuse blobs with a matching algorithm as per implementations in internal/imagedestination/impl.helpers.go
-	OriginalCompression *compression.Algorithm // Must be set if RequiredCompression is set; can be set to nil to indicate “uncompressed” or “unknown”.
-	TOCDigest           *digest.Digest         // If specified, the blob can be looked up in the destination also by its TOC digest.
+	EmptyLayer              bool                   // True if the blob is an "empty"/"throwaway" layer, and may not necessarily be physically represented.
+	LayerIndex              *int                   // If the blob is a layer, a zero-based index of the layer within the image; nil otherwise.
+	SrcRef                  reference.Named        // A reference to the source image that contains the input blob.
+	PossibleManifestFormats []string               // A set of possible manifest formats; at least one should support the reused layer blob.
+	RequiredCompression     *compression.Algorithm // If set, reuse blobs with a matching algorithm as per implementations in internal/imagedestination/impl.helpers.go
+	OriginalCompression     *compression.Algorithm // May be nil to indicate “uncompressed” or “unknown”.
+	TOCDigest               *digest.Digest         // If specified, the blob can be looked up in the destination also by its TOC digest.
 }
 
 // ReusedBlob is information about a blob reused in a destination.