From c438c690575ac62fed7d7082c88464f114bc03c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= 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č --- 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 8844ac8e7..9210a617b 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 7f22c0fe3..962b80ecf 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?= 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č --- copy/manifest.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/copy/manifest.go b/copy/manifest.go index 9210a617b..60ea92aae 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?= 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č --- 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 72b574a5b..9a52a749f 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?= 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č --- copy/single.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/copy/single.go b/copy/single.go index 9003965c9..d407d2ba1 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?= 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č --- 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 5d28b3e73..b6bdce5fa 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?= 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č --- 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 a42e3b67a..953ef9deb 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 222723a8f..9d9219241 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 a9a36f0a3..0a623cb45 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 7507d8559..b44f4ca1f 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 b6bdce5fa..6d4d91438 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 8a80d1d40..db76f0de6 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 100d16763..305d8c9c7 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 d00a0cdf8..228af90ca 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 9bda08515..f32fccf80 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 b2bc26fbf..53569793e 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?= 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č --- copy/single.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/copy/single.go b/copy/single.go index d407d2ba1..57e606098 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?= 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č --- 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 0a623cb45..72f2fa5f9 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?= 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č --- 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 57e606098..c23361942 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 72f2fa5f9..877d11b73 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 6d4d91438..a8a9f51be 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 db76f0de6..93749b1bd 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 9a52a749f..7037755bf 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.