WIP: Add and implement TryReusingOptions.PossibleManifestFormats

... so that we can limit blob reuse to acceptable conversion formats, based on relevant manifests. Implement it in CandidateMatchesTryReusingBlobOptions, covering all (in-tree) implementations. WIP: actually try in practice Signed-off-by: Miloslav Trmač <mitr@redhat.com>
containers · Jan 30, 2024 · e0eaafa · e0eaafa
1 parent d829a13
commit e0eaafa
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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.