Merge pull request #2346 from mtrmac/bic-filter-first

Filter BlobInfoCache candidates before prioritization, not in transports

docker/docker_image_dest.go

@@ -344,35 +344,24 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 	}
 
 	// Then try reusing blobs from other locations.
-	candidates := options.Cache.CandidateLocations2(d.ref.Transport(), bicTransportScope(d.ref), info.Digest, options.CanSubstitute)
+	candidates := options.Cache.CandidateLocations2(d.ref.Transport(), bicTransportScope(d.ref), info.Digest, blobinfocache.CandidateLocations2Options{
+		CanSubstitute:           options.CanSubstitute,
+		PossibleManifestFormats: options.PossibleManifestFormats,
+		RequiredCompression:     options.RequiredCompression,
+	})
 	for _, candidate := range candidates {
-		var err error
-		compressionOperation, compressionAlgorithm, err := blobinfocache.OperationAndAlgorithmForCompressor(candidate.CompressorName)
-		if err != nil {
-			logrus.Debugf("OperationAndAlgorithmForCompressor Failed: %v", err)
-			continue
-		}
 		var candidateRepo reference.Named
 		if !candidate.UnknownLocation {
+			var err error
 			candidateRepo, err = parseBICLocationReference(candidate.Location)
 			if err != nil {
 				logrus.Debugf("Error parsing BlobInfoCache location reference: %s", err)
 				continue
 			}
 		}
-		if !impl.CandidateMatchesTryReusingBlobOptions(options, compressionAlgorithm) {
-			if !candidate.UnknownLocation {
-				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 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
-		}
 		if !candidate.UnknownLocation {
-			if candidate.CompressorName != blobinfocache.Uncompressed {
-				logrus.Debugf("Trying to reuse blob with cached digest %s compressed with %s in destination repo %s", candidate.Digest.String(), candidate.CompressorName, candidateRepo.Name())
+			if candidate.CompressionAlgorithm != nil {
+				logrus.Debugf("Trying to reuse blob with cached digest %s compressed with %s in destination repo %s", candidate.Digest.String(), candidate.CompressionAlgorithm.Name(), candidateRepo.Name())
 			} else {
 				logrus.Debugf("Trying to reuse blob with cached digest %s in destination repo %s", candidate.Digest.String(), candidateRepo.Name())
 			}
@@ -387,8 +376,8 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 				continue
 			}
 		} else {
-			if candidate.CompressorName != blobinfocache.Uncompressed {
-				logrus.Debugf("Trying to reuse blob with cached digest %s compressed with %s with no location match, checking current repo", candidate.Digest.String(), candidate.CompressorName)
+			if candidate.CompressionAlgorithm != nil {
+				logrus.Debugf("Trying to reuse blob with cached digest %s compressed with %s with no location match, checking current repo", candidate.Digest.String(), candidate.CompressionAlgorithm.Name())
 			} else {
 				logrus.Debugf("Trying to reuse blob with cached digest %s in destination repo with no location match, checking current repo", candidate.Digest.String())
 			}
@@ -439,8 +428,8 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,
 		return true, private.ReusedBlob{
 			Digest:               candidate.Digest,
 			Size:                 size,
-			CompressionOperation: compressionOperation,
-			CompressionAlgorithm: compressionAlgorithm}, nil
+			CompressionOperation: candidate.CompressionOperation,
+			CompressionAlgorithm: candidate.CompressionAlgorithm}, nil
 	}
 
 	return false, private.ReusedBlob{}, nil

internal/blobinfocache/blobinfocache.go

@@ -1,8 +1,6 @@
 package blobinfocache
 
 import (
-	"github.com/containers/image/v5/pkg/compression"
-	compressiontypes "github.com/containers/image/v5/pkg/compression/types"
 	"github.com/containers/image/v5/types"
 	digest "github.com/opencontainers/go-digest"
 )
@@ -32,7 +30,7 @@ func (bic *v1OnlyBlobInfoCache) Close() {
 func (bic *v1OnlyBlobInfoCache) RecordDigestCompressorName(anyDigest digest.Digest, compressorName string) {
 }
 
-func (bic *v1OnlyBlobInfoCache) CandidateLocations2(transport types.ImageTransport, scope types.BICTransportScope, digest digest.Digest, canSubstitute bool) []BICReplacementCandidate2 {
+func (bic *v1OnlyBlobInfoCache) CandidateLocations2(transport types.ImageTransport, scope types.BICTransportScope, digest digest.Digest, options CandidateLocations2Options) []BICReplacementCandidate2 {
 	return nil
 }
 
@@ -48,23 +46,3 @@ func CandidateLocationsFromV2(v2candidates []BICReplacementCandidate2) []types.B
 	}
 	return candidates
 }
-
-// OperationAndAlgorithmForCompressor returns CompressionOperation and CompressionAlgorithm
-// values suitable for inclusion in a types.BlobInfo structure, based on the name of the
-// compression algorithm, or Uncompressed, or UnknownCompression.  This is typically used by
-// TryReusingBlob() implementations to set values in the BlobInfo structure that they return
-// upon success.
-func OperationAndAlgorithmForCompressor(compressorName string) (types.LayerCompression, *compressiontypes.Algorithm, error) {
-	switch compressorName {
-	case Uncompressed:
-		return types.Decompress, nil, nil
-	case UnknownCompression:
-		return types.PreserveOriginal, nil, nil
-	default:
-		algo, err := compression.AlgorithmByName(compressorName)
-		if err == nil {
-			return types.Compress, &algo, nil
-		}
-		return types.PreserveOriginal, nil, err
-	}
-}

internal/blobinfocache/types.go

@@ -1,6 +1,7 @@
 package blobinfocache
 
 import (
+	compressiontypes "github.com/containers/image/v5/pkg/compression/types"
 	"github.com/containers/image/v5/types"
 	digest "github.com/opencontainers/go-digest"
 )
@@ -35,19 +36,24 @@ type BlobInfoCache2 interface {
 	// CandidateLocations2 returns a prioritized, limited, number of blobs and their locations (if known)
 	// that could possibly be reused within the specified (transport scope) (if they still
 	// exist, which is not guaranteed).
-	//
-	// If !canSubstitute, the returned candidates will match the submitted digest exactly; if
-	// canSubstitute, data from previous RecordDigestUncompressedPair calls is used to also look
+	CandidateLocations2(transport types.ImageTransport, scope types.BICTransportScope, digest digest.Digest, options CandidateLocations2Options) []BICReplacementCandidate2
+}
+
+// CandidateLocations2Options are used in CandidateLocations2.
+type CandidateLocations2Options struct {
+	// If !CanSubstitute, the returned candidates will match the submitted digest exactly; if
+	// CanSubstitute, data from previous RecordDigestUncompressedPair calls is used to also look
 	// up variants of the blob which have the same uncompressed digest.
-	//
-	// The CompressorName fields in returned data must never be UnknownCompression.
-	CandidateLocations2(transport types.ImageTransport, scope types.BICTransportScope, digest digest.Digest, canSubstitute bool) []BICReplacementCandidate2
+	CanSubstitute           bool
+	PossibleManifestFormats []string                    // If set, a set of possible manifest formats; at least one should support the reused layer
+	RequiredCompression     *compressiontypes.Algorithm // If set, only reuse layers with a matching algorithm
 }
 
 // BICReplacementCandidate2 is an item returned by BlobInfoCache2.CandidateLocations2.
 type BICReplacementCandidate2 struct {
-	Digest          digest.Digest
-	CompressorName  string                     // either the Name() of a known pkg/compression.Algorithm, or Uncompressed or UnknownCompression
-	UnknownLocation bool                       // is true when `Location` for this blob is not set
-	Location        types.BICLocationReference // not set if UnknownLocation is set to `true`
+	Digest               digest.Digest
+	CompressionOperation types.LayerCompression      // Either types.Decompress for uncompressed, or types.Compress for compressed
+	CompressionAlgorithm *compressiontypes.Algorithm // An algorithm when the candidate is compressed, or nil when it is uncompressed
+	UnknownLocation      bool                        // is true when `Location` for this blob is not set
+	Location             types.BICLocationReference  // not set if UnknownLocation is set to `true`
 }

internal/imagedestination/impl/helpers.go

@@ -3,40 +3,13 @@ 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
-// 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 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.
-			// The caller must re-compress to build those annotations.
-			return false
-		}
-		if candidateCompression == nil ||
-			(options.RequiredCompression.Name() != candidateCompression.Name() && options.RequiredCompression.Name() != candidateCompression.BaseVariantName()) {
-			return false
-		}
-	}
-
-	// 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
-}
-
+// OriginalCandidateMatchesTryReusingBlobOptions returns true if the original blob passed to TryReusingBlobWithOptions
+// is acceptable based on opts.
 func OriginalCandidateMatchesTryReusingBlobOptions(opts private.TryReusingBlobOptions) bool {
-	return CandidateMatchesTryReusingBlobOptions(opts, opts.OriginalCompression)
+	return manifest.CandidateCompressionMatchesReuseConditions(manifest.ReuseConditions{
+		PossibleManifestFormats: opts.PossibleManifestFormats,
+		RequiredCompression:     opts.RequiredCompression,
+	}, opts.OriginalCompression)
 }

internal/manifest/manifest.go

@@ -7,6 +7,7 @@ import (
 	"github.com/containers/libtrust"
 	digest "github.com/opencontainers/go-digest"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
+	"golang.org/x/exp/slices"
 )
 
 // FIXME: Should we just use docker/distribution and docker/docker implementations directly?
@@ -192,3 +193,39 @@ func MIMETypeSupportsCompressionAlgorithm(mimeType string, algo compressiontypes
 		return false
 	}
 }
+
+// ReuseConditions are an input to CandidateCompressionMatchesReuseConditions;
+// it is a struct to allow longer and better-documented field names.
+type ReuseConditions struct {
+	PossibleManifestFormats []string                    // If set, a set of possible manifest formats; at least one should support the reused layer
+	RequiredCompression     *compressiontypes.Algorithm // If set, only reuse layers with a matching algorithm
+}
+
+// CandidateCompressionMatchesReuseConditions returns true if a layer with candidateCompression
+// (which can be nil to represent uncompressed or unknown) matches reuseConditions.
+func CandidateCompressionMatchesReuseConditions(c ReuseConditions, candidateCompression *compressiontypes.Algorithm) bool {
+	if c.RequiredCompression != nil {
+		if c.RequiredCompression.Name() == compressiontypes.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 ||
+			(c.RequiredCompression.Name() != candidateCompression.Name() && c.RequiredCompression.Name() != candidateCompression.BaseVariantName()) {
+			return false
+		}
+	}
+
+	// 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 c.PossibleManifestFormats != nil && candidateCompression != nil {
+		if !slices.ContainsFunc(c.PossibleManifestFormats, func(mt string) bool {
+			return MIMETypeSupportsCompressionAlgorithm(mt, *candidateCompression)
+		}) {
+			return false
+		}
+	}
+
+	return true
+}

internal/manifest/manifest_test.go

@@ -182,3 +182,38 @@ func TestMIMETypeSupportsCompressionAlgorithm(t *testing.T) {
 		}
 	}
 }
+
+func TestCandidateCompressionMatchesReuseConditions(t *testing.T) {
+	cases := []struct {
+		requiredCompression     *compression.Algorithm
+		possibleManifestFormats []string
+		candidateCompression    *compression.Algorithm
+		result                  bool
+	}{
+		// 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{DockerV2Schema2MediaType}, &compression.Zstd, false},
+		{nil, []string{DockerV2Schema2MediaType, DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest}, &compression.Zstd, true},
+		{nil, nil, &compression.Zstd, true},
+		{nil, []string{imgspecv1.MediaTypeImageManifest}, &compression.Gzip, true},
+		{nil, []string{DockerV2Schema2MediaType}, &compression.Gzip, true},
+		{nil, []string{DockerV2Schema2MediaType, DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest}, &compression.Gzip, true},
+		{nil, nil, &compression.Gzip, true},
+		// Some possible combinations (always 1 constraint not matching)
+		{&compression.Zstd, []string{DockerV2Schema2MediaType}, &compression.Zstd, false},
+		{&compression.Gzip, []string{DockerV2Schema2MediaType, DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest}, &compression.Zstd, false},
+	}
+
+	for _, c := range cases {
+		conds := ReuseConditions{
+			RequiredCompression:     c.requiredCompression,
+			PossibleManifestFormats: c.possibleManifestFormats,
+		}
+		assert.Equal(t, c.result, CandidateCompressionMatchesReuseConditions(conds, c.candidateCompression))
+	}
+}