From 401fa92e36db6dd8a2f287faee2f854756705672 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 6 Jul 2023 12:31:55 +0530 Subject: [PATCH 01/51] list: ListUpdate add Platform field `c/image` uses `Instance(` API to get the required details of an instance while performing replication `Platform` of instance is needed hence `ListUpdate` must include platform. Needed by: https://github.com/containers/image/pull/1987 Signed-off-by: Aditya R --- internal/manifest/docker_schema2_list.go | 7 +++++++ internal/manifest/docker_schema2_list_test.go | 2 ++ internal/manifest/list.go | 1 + internal/manifest/oci_index.go | 1 + internal/manifest/oci_index_test.go | 2 ++ 5 files changed, 13 insertions(+) diff --git a/internal/manifest/docker_schema2_list.go b/internal/manifest/docker_schema2_list.go index 516ca7ac9..14e7f4215 100644 --- a/internal/manifest/docker_schema2_list.go +++ b/internal/manifest/docker_schema2_list.go @@ -61,6 +61,13 @@ func (list *Schema2ListPublic) Instance(instanceDigest digest.Digest) (ListUpdat Digest: manifest.Digest, Size: manifest.Size, MediaType: manifest.MediaType, + Platform: &imgspecv1.Platform{ + OS: manifest.Platform.OS, + Architecture: manifest.Platform.Architecture, + OSVersion: manifest.Platform.OSVersion, + OSFeatures: manifest.Platform.OSFeatures, + Variant: manifest.Platform.Variant, + }, }, nil } } diff --git a/internal/manifest/docker_schema2_list_test.go b/internal/manifest/docker_schema2_list_test.go index 6597bc977..a13505c19 100644 --- a/internal/manifest/docker_schema2_list_test.go +++ b/internal/manifest/docker_schema2_list_test.go @@ -55,6 +55,8 @@ func TestSchema2ListEditInstances(t *testing.T) { require.NoError(t, err) assert.Equal(t, "something", instance.MediaType) assert.Equal(t, int64(32), instance.Size) + // platform must match with instance platform set in `v2list.manifest.json` for the first instance + assert.Equal(t, &imgspecv1.Platform{Architecture: "ppc64le", OS: "linux", OSVersion: "", OSFeatures: []string(nil), Variant: ""}, instance.Platform) // Create a fresh list list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest)) diff --git a/internal/manifest/list.go b/internal/manifest/list.go index 8786324ea..58c03b8a3 100644 --- a/internal/manifest/list.go +++ b/internal/manifest/list.go @@ -68,6 +68,7 @@ type ListUpdate struct { Digest digest.Digest Size int64 MediaType string + Platform *imgspecv1.Platform // read-only field: may be set by Instance(), ignored by UpdateInstance() } type ListOp int diff --git a/internal/manifest/oci_index.go b/internal/manifest/oci_index.go index fd251d951..d5c092923 100644 --- a/internal/manifest/oci_index.go +++ b/internal/manifest/oci_index.go @@ -57,6 +57,7 @@ func (index *OCI1IndexPublic) Instance(instanceDigest digest.Digest) (ListUpdate Digest: manifest.Digest, Size: manifest.Size, MediaType: manifest.MediaType, + Platform: manifest.Platform, }, nil } } diff --git a/internal/manifest/oci_index_test.go b/internal/manifest/oci_index_test.go index 2e602c52d..ccaf68147 100644 --- a/internal/manifest/oci_index_test.go +++ b/internal/manifest/oci_index_test.go @@ -77,6 +77,8 @@ func TestOCI1EditInstances(t *testing.T) { require.NoError(t, err) assert.Equal(t, "something", instance.MediaType) assert.Equal(t, int64(32), instance.Size) + // platform must match with what was set in `ociv1.image.index.json` for the first instance + assert.Equal(t, &imgspecv1.Platform{Architecture: "ppc64le", OS: "linux", OSVersion: "", OSFeatures: []string(nil), Variant: ""}, instance.Platform) // Create a fresh list list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest)) From a62f0e57656e2af38ff3f7a885d04860b5a6d873 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sun, 2 Jul 2023 12:53:12 +0530 Subject: [PATCH 02/51] blob: TryReusingBlobWithOptions consider requiredCompression if set TryReusingBlob now contains a new option `RequiredCompression` which filters the blob by checking against a compression of the blob which is considerd to be resued, in case `RequiredCompression` is set and `info` of the blob being reused does not matches, no blob is returned. Signed-off-by: Aditya R --- directory/directory_dest.go | 3 ++ docker/docker_image_dest.go | 41 +++++++++++++------ docker/internal/tarfile/dest.go | 3 ++ internal/imagedestination/impl/helpers.go | 20 +++++++++ .../imagedestination/impl/helpers_test.go | 29 +++++++++++++ internal/imagedestination/wrapper.go | 3 ++ internal/private/private.go | 9 ++-- oci/layout/oci_dest.go | 3 ++ ostree/ostree_dest.go | 3 ++ pkg/blobcache/dest.go | 3 ++ storage/storage_dest.go | 3 ++ 11 files changed, 103 insertions(+), 17 deletions(-) create mode 100644 internal/imagedestination/impl/helpers.go create mode 100644 internal/imagedestination/impl/helpers_test.go diff --git a/directory/directory_dest.go b/directory/directory_dest.go index 974d23d5f..222723a8f 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -190,6 +190,9 @@ 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) { + return false, private.ReusedBlob{}, nil + } if info.Digest == "" { return false, private.ReusedBlob{}, fmt.Errorf("Can not check for a blob with unknown digest") } diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index 44e2aea23..63e372d67 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -321,13 +321,21 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context, return false, private.ReusedBlob{}, errors.New("Can not check for a blob with unknown digest") } - // First, check whether the blob happens to already exist at the destination. - haveBlob, reusedInfo, err := d.tryReusingExactBlob(ctx, info, options.Cache) - if err != nil { - return false, private.ReusedBlob{}, err - } - if haveBlob { - return true, reusedInfo, nil + if impl.OriginalBlobMatchesRequiredCompression(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 { + return false, private.ReusedBlob{}, err + } + if haveBlob { + 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) } // Then try reusing blobs from other locations. @@ -338,6 +346,19 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context, logrus.Debugf("Error parsing BlobInfoCache location reference: %s", err) continue } + compressionOperation, compressionAlgorithm, err := blobinfocache.OperationAndAlgorithmForCompressor(candidate.CompressorName) + if err != nil { + logrus.Debugf("OperationAndAlgorithmForCompressor Failed: %v", err) + continue + } + if !impl.BlobMatchesRequiredCompression(options, compressionAlgorithm) { + requiredCompression := "nil" + if compressionAlgorithm != nil { + requiredCompression = compressionAlgorithm.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(), requiredCompression, candidateRepo.Name()) + continue + } if candidate.CompressorName != blobinfocache.Uncompressed { logrus.Debugf("Trying to reuse cached location %s compressed with %s in %s", candidate.Digest.String(), candidate.CompressorName, candidateRepo.Name()) } else { @@ -388,12 +409,6 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context, options.Cache.RecordKnownLocation(d.ref.Transport(), bicTransportScope(d.ref), candidate.Digest, newBICLocationReference(d.ref)) - compressionOperation, compressionAlgorithm, err := blobinfocache.OperationAndAlgorithmForCompressor(candidate.CompressorName) - if err != nil { - logrus.Debugf("... Failed: %v", err) - continue - } - return true, private.ReusedBlob{ Digest: candidate.Digest, Size: size, diff --git a/docker/internal/tarfile/dest.go b/docker/internal/tarfile/dest.go index 00e25748b..7507d8559 100644 --- a/docker/internal/tarfile/dest.go +++ b/docker/internal/tarfile/dest.go @@ -129,6 +129,9 @@ 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) { + return false, private.ReusedBlob{}, nil + } if err := d.archive.lock(); err != nil { return false, private.ReusedBlob{}, err } diff --git a/internal/imagedestination/impl/helpers.go b/internal/imagedestination/impl/helpers.go new file mode 100644 index 000000000..d5de81a61 --- /dev/null +++ b/internal/imagedestination/impl/helpers.go @@ -0,0 +1,20 @@ +package impl + +import ( + "github.com/containers/image/v5/internal/private" + 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 +// 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 + } + return candidateCompression != nil && (options.RequiredCompression.Name() == candidateCompression.Name()) +} + +func OriginalBlobMatchesRequiredCompression(opts private.TryReusingBlobOptions) bool { + return BlobMatchesRequiredCompression(opts, opts.OriginalCompression) +} diff --git a/internal/imagedestination/impl/helpers_test.go b/internal/imagedestination/impl/helpers_test.go new file mode 100644 index 000000000..26af382d3 --- /dev/null +++ b/internal/imagedestination/impl/helpers_test.go @@ -0,0 +1,29 @@ +package impl + +import ( + "testing" + + "github.com/containers/image/v5/internal/private" + "github.com/containers/image/v5/pkg/compression" + compressionTypes "github.com/containers/image/v5/pkg/compression/types" + "github.com/stretchr/testify/assert" +) + +func TestBlobMatchesRequiredCompression(t *testing.T) { + var opts private.TryReusingBlobOptions + cases := []struct { + requiredCompression *compressionTypes.Algorithm + candidateCompression *compressionTypes.Algorithm + result bool + }{ + {&compression.Zstd, &compression.Zstd, true}, + {&compression.Gzip, &compression.Zstd, false}, + {&compression.Zstd, nil, false}, + {nil, &compression.Zstd, true}, + } + + for _, c := range cases { + opts = private.TryReusingBlobOptions{RequiredCompression: c.requiredCompression} + assert.Equal(t, BlobMatchesRequiredCompression(opts, c.candidateCompression), c.result) + } +} diff --git a/internal/imagedestination/wrapper.go b/internal/imagedestination/wrapper.go index 41a81628b..17e1870c1 100644 --- a/internal/imagedestination/wrapper.go +++ b/internal/imagedestination/wrapper.go @@ -64,6 +64,9 @@ func (w *wrapped) PutBlobWithOptions(ctx context.Context, stream io.Reader, inpu // 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 (w *wrapped) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, private.ReusedBlob, error) { + if options.RequiredCompression != nil { + return false, private.ReusedBlob{}, nil + } reused, blob, err := w.TryReusingBlob(ctx, info, options.Cache, options.CanSubstitute) if !reused || err != nil { return reused, private.ReusedBlob{}, err diff --git a/internal/private/private.go b/internal/private/private.go index b1dd4ceb0..95d561fcd 100644 --- a/internal/private/private.go +++ b/internal/private/private.go @@ -112,10 +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. - - 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”. + 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. } // ReusedBlob is information about a blob reused in a destination. diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index 0a9e4eab9..8ff43d448 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -172,6 +172,9 @@ 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) { + return false, private.ReusedBlob{}, nil + } if info.Digest == "" { return false, private.ReusedBlob{}, errors.New("Can not check for a blob with unknown digest") } diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index 48f3ee5a7..d00a0cdf8 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -335,6 +335,9 @@ 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) { + return false, private.ReusedBlob{}, nil + } if d.repo == nil { repo, err := openRepo(d.ref.repo) if err != nil { diff --git a/pkg/blobcache/dest.go b/pkg/blobcache/dest.go index a0e353d46..9bda08515 100644 --- a/pkg/blobcache/dest.go +++ b/pkg/blobcache/dest.go @@ -237,6 +237,9 @@ 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) { + return false, private.ReusedBlob{}, nil + } present, reusedInfo, err := d.destination.TryReusingBlobWithOptions(ctx, info, options) if err != nil || present { return present, reusedInfo, err diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 576d510cc..7bbbf1752 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -307,6 +307,9 @@ 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) { + return false, private.ReusedBlob{}, nil + } reused, info, err := s.tryReusingBlobAsPending(blobinfo.Digest, blobinfo.Size, &options) if err != nil || !reused || options.LayerIndex == nil { return reused, info, err From 49f559da2999c14333fd2a13d8309784733f17cf Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 13 Jul 2023 14:40:20 +0530 Subject: [PATCH 03/51] helpers_test,cleanup: correct argument order Fixes: https://github.com/containers/image/pull/2023#discussion_r1261581159. `assert.Equal` expects `assert.Equal(t, expected, actual)` instead of `assert.Equal(t, actual, expected)`. Ref:https://pkg.go.dev/github.com/stretchr/testify/assert#Assertions.Equal Signed-off-by: Aditya R --- internal/imagedestination/impl/helpers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/imagedestination/impl/helpers_test.go b/internal/imagedestination/impl/helpers_test.go index 26af382d3..8a80d1d40 100644 --- a/internal/imagedestination/impl/helpers_test.go +++ b/internal/imagedestination/impl/helpers_test.go @@ -24,6 +24,6 @@ func TestBlobMatchesRequiredCompression(t *testing.T) { for _, c := range cases { opts = private.TryReusingBlobOptions{RequiredCompression: c.requiredCompression} - assert.Equal(t, BlobMatchesRequiredCompression(opts, c.candidateCompression), c.result) + assert.Equal(t, c.result, BlobMatchesRequiredCompression(opts, c.candidateCompression)) } } From fbd4da021f9e15bdb576b25317ad5f21f5d663f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 15 Jul 2023 02:31:07 +0200 Subject: [PATCH 04/51] Fix TestOCI1IndexChooseInstanceByCompression on non-amd64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting SystemContext.ArchitectureChoice to "" does not mean "match any/the first platform"; it's the default behavior of SystemContext, and it means "choose for the current runtime architecture". (Originally discussed in https://github.com/containers/image/pull/1789#discussion_r1115647449 ) I.e. on amd64 these two test cases are redundant with the first two instances above, and on other architectures (notably ARM) they cause failures. So just drop them. Signed-off-by: Miloslav Trmač --- internal/manifest/oci_index_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/manifest/oci_index_test.go b/internal/manifest/oci_index_test.go index ccaf68147..865849b83 100644 --- a/internal/manifest/oci_index_test.go +++ b/internal/manifest/oci_index_test.go @@ -213,10 +213,6 @@ func TestOCI1IndexChooseInstanceByCompression(t *testing.T) { {"arm64", "", "sha256:6dc14a60d2ba724646cfbf5fccbb9a618a5978a64a352e060b17caf5e005da9d", true}, // must return first zstd even if the first entry for same platform is gzip {"arm64", "", "sha256:1c98002b30a71b08ab175915ce7c8fb8da9e9b502ae082d6f0c572bac9dee324", false}, - // must return first zstd instance agnostic of platform - {"", "", "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc", false}, - // must return first gzip instance agnostic of platform - {"", "", "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", true}, // must return first zstd instance with no platform {"matchesImageWithNoPlatform", "", "sha256:f2f5f52a2cf2c51d4cac6df0545f751c0adc3f3427eb47c59fcb32894503e18f", false}, // must return first gzip instance with no platform From 049a67544e84b167016d45165907e16459099132 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Fri, 14 Jul 2023 15:09:12 +0530 Subject: [PATCH 05/51] listupdate,oci: instance show read-only annotations and CompressionAlgorithmNames There is a need to read annotations of a particular instance to get its compression. Expose `Annotations` as a read-only field. Needed By: https://github.com/containers/image/pull/1987 Signed-off-by: Aditya R --- internal/manifest/docker_schema2_list.go | 21 +++++++++++-------- internal/manifest/docker_schema2_list_test.go | 4 +++- internal/manifest/list.go | 7 ++++++- internal/manifest/oci_index.go | 21 ++++++++++++++++--- internal/manifest/oci_index_test.go | 12 ++++++++++- 5 files changed, 50 insertions(+), 15 deletions(-) diff --git a/internal/manifest/docker_schema2_list.go b/internal/manifest/docker_schema2_list.go index 14e7f4215..14a476642 100644 --- a/internal/manifest/docker_schema2_list.go +++ b/internal/manifest/docker_schema2_list.go @@ -5,6 +5,7 @@ import ( "fmt" platform "github.com/containers/image/v5/internal/pkg/platform" + compression "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" @@ -57,18 +58,20 @@ func (list *Schema2ListPublic) Instances() []digest.Digest { func (list *Schema2ListPublic) Instance(instanceDigest digest.Digest) (ListUpdate, error) { for _, manifest := range list.Manifests { if manifest.Digest == instanceDigest { - return ListUpdate{ + ret := ListUpdate{ Digest: manifest.Digest, Size: manifest.Size, MediaType: manifest.MediaType, - Platform: &imgspecv1.Platform{ - OS: manifest.Platform.OS, - Architecture: manifest.Platform.Architecture, - OSVersion: manifest.Platform.OSVersion, - OSFeatures: manifest.Platform.OSFeatures, - Variant: manifest.Platform.Variant, - }, - }, nil + } + ret.ReadOnly.CompressionAlgorithmNames = []string{compression.GzipAlgorithmName} + ret.ReadOnly.Platform = &imgspecv1.Platform{ + OS: manifest.Platform.OS, + Architecture: manifest.Platform.Architecture, + OSVersion: manifest.Platform.OSVersion, + OSFeatures: manifest.Platform.OSFeatures, + Variant: manifest.Platform.Variant, + } + return ret, nil } } return ListUpdate{}, fmt.Errorf("unable to find instance %s passed to Schema2List.Instances", instanceDigest) diff --git a/internal/manifest/docker_schema2_list_test.go b/internal/manifest/docker_schema2_list_test.go index a13505c19..97cdb4361 100644 --- a/internal/manifest/docker_schema2_list_test.go +++ b/internal/manifest/docker_schema2_list_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + compressionTypes "github.com/containers/image/v5/pkg/compression/types" "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" @@ -56,7 +57,8 @@ func TestSchema2ListEditInstances(t *testing.T) { assert.Equal(t, "something", instance.MediaType) assert.Equal(t, int64(32), instance.Size) // platform must match with instance platform set in `v2list.manifest.json` for the first instance - assert.Equal(t, &imgspecv1.Platform{Architecture: "ppc64le", OS: "linux", OSVersion: "", OSFeatures: []string(nil), Variant: ""}, instance.Platform) + assert.Equal(t, &imgspecv1.Platform{Architecture: "ppc64le", OS: "linux", OSVersion: "", OSFeatures: []string(nil), Variant: ""}, instance.ReadOnly.Platform) + assert.Equal(t, []string{compressionTypes.GzipAlgorithmName}, instance.ReadOnly.CompressionAlgorithmNames) // Create a fresh list list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest)) diff --git a/internal/manifest/list.go b/internal/manifest/list.go index 58c03b8a3..189f1a718 100644 --- a/internal/manifest/list.go +++ b/internal/manifest/list.go @@ -68,7 +68,12 @@ type ListUpdate struct { Digest digest.Digest Size int64 MediaType string - Platform *imgspecv1.Platform // read-only field: may be set by Instance(), ignored by UpdateInstance() + // ReadOnly fields: may be set by Instance(), ignored by UpdateInstance() + ReadOnly struct { + Platform *imgspecv1.Platform + Annotations map[string]string + CompressionAlgorithmNames []string + } } type ListOp int diff --git a/internal/manifest/oci_index.go b/internal/manifest/oci_index.go index d5c092923..cebca5a3a 100644 --- a/internal/manifest/oci_index.go +++ b/internal/manifest/oci_index.go @@ -53,12 +53,15 @@ func (index *OCI1IndexPublic) Instances() []digest.Digest { func (index *OCI1IndexPublic) Instance(instanceDigest digest.Digest) (ListUpdate, error) { for _, manifest := range index.Manifests { if manifest.Digest == instanceDigest { - return ListUpdate{ + ret := ListUpdate{ Digest: manifest.Digest, Size: manifest.Size, MediaType: manifest.MediaType, - Platform: manifest.Platform, - }, nil + } + ret.ReadOnly.Platform = manifest.Platform + ret.ReadOnly.Annotations = manifest.Annotations + ret.ReadOnly.CompressionAlgorithmNames = annotationsToCompressionAlgorithmNames(manifest.Annotations) + return ret, nil } } return ListUpdate{}, fmt.Errorf("unable to find instance %s in OCI1Index", instanceDigest) @@ -79,6 +82,18 @@ func (index *OCI1IndexPublic) UpdateInstances(updates []ListUpdate) error { return index.editInstances(editInstances) } +func annotationsToCompressionAlgorithmNames(annotations map[string]string) []string { + result := make([]string, 0, 1) + if annotations[OCI1InstanceAnnotationCompressionZSTD] == OCI1InstanceAnnotationCompressionZSTDValue { + result = append(result, compression.ZstdAlgorithmName) + } + // No compression was detected, hence assume instance has default compression `Gzip` + if len(result) == 0 { + result = append(result, compression.GzipAlgorithmName) + } + return result +} + func addCompressionAnnotations(compressionAlgorithms []compression.Algorithm, annotationsMap map[string]string) { // TODO: This should also delete the algorithm if map already contains an algorithm and compressionAlgorithm // list has a different algorithm. To do that, we would need to modify the callers to always provide a reliable diff --git a/internal/manifest/oci_index_test.go b/internal/manifest/oci_index_test.go index 865849b83..a856290cf 100644 --- a/internal/manifest/oci_index_test.go +++ b/internal/manifest/oci_index_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/containers/image/v5/pkg/compression" + compressionTypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" @@ -78,7 +79,8 @@ func TestOCI1EditInstances(t *testing.T) { assert.Equal(t, "something", instance.MediaType) assert.Equal(t, int64(32), instance.Size) // platform must match with what was set in `ociv1.image.index.json` for the first instance - assert.Equal(t, &imgspecv1.Platform{Architecture: "ppc64le", OS: "linux", OSVersion: "", OSFeatures: []string(nil), Variant: ""}, instance.Platform) + assert.Equal(t, &imgspecv1.Platform{Architecture: "ppc64le", OS: "linux", OSVersion: "", OSFeatures: []string(nil), Variant: ""}, instance.ReadOnly.Platform) + assert.Equal(t, []string{compressionTypes.GzipAlgorithmName}, instance.ReadOnly.CompressionAlgorithmNames) // Create a fresh list list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest)) @@ -132,6 +134,14 @@ func TestOCI1EditInstances(t *testing.T) { // Zstd should be kept on lowest priority as compared to the default gzip ones and order of prior elements must be preserved. assert.Equal(t, list.Instances(), []digest.Digest{digest.Digest("sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f"), digest.Digest("sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"), digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), digest.Digest("sha256:eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"), digest.Digest("sha256:hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh"), digest.Digest("sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")}) + instance, err = list.Instance(digest.Digest("sha256:eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee")) + require.NoError(t, err) + // Verify if annotations are preserved and correctly set in ReadOnly field. + assert.Equal(t, annotations, instance.ReadOnly.Annotations) + // Verify compression of an instance is added to the ReadOnly CompressionAlgorithmNames where compression name + // is internally derived from the appropriate annotations. + assert.Equal(t, []string{compressionTypes.ZstdAlgorithmName}, instance.ReadOnly.CompressionAlgorithmNames) + // Update list and remove zstd annotation from existing instance, and verify if resorting works editInstances = []ListEdit{} editInstances = append(editInstances, ListEdit{ From 1a7156c7a84685507ece8f89146f111cbd220dd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:20:44 +0200 Subject: [PATCH 06/51] Remove a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Generally I discourage unsing named return values, it's easy to miss that it wasn't set - I have no idea what the "in the middle of a multi-streamed copy" paragraph refers to. Signed-off-by: Miloslav Trmač --- copy/copy.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index bf8f4015b..a660c1c0a 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -151,11 +151,6 @@ type copier struct { // source image admissibility. It returns the manifest which was written to // the new copy of the image. func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, srcRef types.ImageReference, options *Options) (copiedManifest []byte, retErr error) { - // NOTE this function uses an output parameter for the error return value. - // Setting this and returning is the ideal way to return an error. - // - // the defers in this routine will wrap the error return with its own errors - // which can be valuable context in the middle of a multi-streamed copy. if options == nil { options = &Options{} } From 88a7bb94e42be83065e895d1a0810ceba1d80800 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:31:24 +0200 Subject: [PATCH 07/51] Add *copy.Options to copier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we don't have to carry it around in extra parameters. Migrating individual functions will follow. Signed-off-by: Miloslav Trmač --- copy/copy.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index a660c1c0a..8dea732cd 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -132,8 +132,10 @@ type Options struct { // data shared across one or more images in a possible manifest list. // The owner must call close() when done. type copier struct { - dest private.ImageDestination - rawSource private.ImageSource + dest private.ImageDestination + rawSource private.ImageSource + options *Options // never nil + reportWriter io.Writer progressOutput io.Writer progressInterval time.Duration @@ -204,8 +206,10 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } c := &copier{ - dest: dest, - rawSource: rawSource, + dest: dest, + rawSource: rawSource, + options: options, + reportWriter: reportWriter, progressOutput: progressOutput, progressInterval: options.ProgressInterval, From 8cce37bdde0b88d28aa5c21f3db05db3c0efa720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:33:04 +0200 Subject: [PATCH 08/51] Use copier.options in setupSigners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 2 +- copy/sign.go | 20 ++++++++++---------- copy/sign_test.go | 3 ++- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 8dea732cd..ce4c78ab6 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -244,7 +244,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } } - if err := c.setupSigners(options); err != nil { + if err := c.setupSigners(); err != nil { return nil, err } diff --git a/copy/sign.go b/copy/sign.go index fd19e18cd..31c69ff8b 100644 --- a/copy/sign.go +++ b/copy/sign.go @@ -13,20 +13,20 @@ import ( "github.com/containers/image/v5/transports" ) -// setupSigners initializes c.signers based on options. -func (c *copier) setupSigners(options *Options) error { - c.signers = append(c.signers, options.Signers...) - // c.signersToClose is intentionally not updated with options.Signers. +// setupSigners initializes c.signers. +func (c *copier) setupSigners() error { + c.signers = append(c.signers, c.options.Signers...) + // c.signersToClose is intentionally not updated with c.options.Signers. // We immediately append created signers to c.signers, and we rely on c.close() to clean them up; so we don’t need // to clean up any created signers on failure. - if options.SignBy != "" { + if c.options.SignBy != "" { opts := []simplesigning.Option{ - simplesigning.WithKeyFingerprint(options.SignBy), + simplesigning.WithKeyFingerprint(c.options.SignBy), } - if options.SignPassphrase != "" { - opts = append(opts, simplesigning.WithPassphrase(options.SignPassphrase)) + if c.options.SignPassphrase != "" { + opts = append(opts, simplesigning.WithPassphrase(c.options.SignPassphrase)) } signer, err := simplesigning.NewSigner(opts...) if err != nil { @@ -36,9 +36,9 @@ func (c *copier) setupSigners(options *Options) error { c.signersToClose = append(c.signersToClose, signer) } - if options.SignBySigstorePrivateKeyFile != "" { + if c.options.SignBySigstorePrivateKeyFile != "" { signer, err := sigstore.NewSigner( - sigstore.WithPrivateKeyFile(options.SignBySigstorePrivateKeyFile, options.SignSigstorePrivateKeyPassphrase), + sigstore.WithPrivateKeyFile(c.options.SignBySigstorePrivateKeyFile, c.options.SignSigstorePrivateKeyPassphrase), ) if err != nil { return err diff --git a/copy/sign_test.go b/copy/sign_test.go index c0733dee8..49f3fce5f 100644 --- a/copy/sign_test.go +++ b/copy/sign_test.go @@ -135,10 +135,11 @@ func TestCreateSignatures(t *testing.T) { c := &copier{ dest: imagedestination.FromPublic(cc.dest), + options: options, reportWriter: io.Discard, } defer c.close() - err := c.setupSigners(options) + err := c.setupSigners() require.NoError(t, err, cc.name) sigs, err := c.createSignatures(context.Background(), manifestBlob, identity) switch { From d2e20d9009187f4d25fe67e1ed3eac0a86906da1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:41:07 +0200 Subject: [PATCH 09/51] Use copier.options in sourceSignatures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/multiple.go | 2 +- copy/sign.go | 6 +++--- copy/single.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/copy/multiple.go b/copy/multiple.go index 41ea1b11b..d9b06695b 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -61,7 +61,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur } updatedList := originalList.CloneInternal() - sigs, err := c.sourceSignatures(ctx, unparsedToplevel, options, + sigs, err := c.sourceSignatures(ctx, unparsedToplevel, "Getting image list signatures", "Checking if image list destination supports signatures") if err != nil { diff --git a/copy/sign.go b/copy/sign.go index 31c69ff8b..0ec54ded2 100644 --- a/copy/sign.go +++ b/copy/sign.go @@ -50,13 +50,13 @@ func (c *copier) setupSigners() error { return nil } -// sourceSignatures returns signatures from unparsedSource based on options, +// sourceSignatures returns signatures from unparsedSource, // and verifies that they can be used (to avoid copying a large image when we // can tell in advance that it would ultimately fail) -func (c *copier) sourceSignatures(ctx context.Context, unparsed private.UnparsedImage, options *Options, +func (c *copier) sourceSignatures(ctx context.Context, unparsed private.UnparsedImage, gettingSignaturesMessage, checkingDestMessage string) ([]internalsig.Signature, error) { var sigs []internalsig.Signature - if options.RemoveSignatures { + if c.options.RemoveSignatures { sigs = []internalsig.Signature{} } else { c.Printf("%s\n", gettingSignaturesMessage) diff --git a/copy/single.go b/copy/single.go index b8569a70c..3352000fc 100644 --- a/copy/single.go +++ b/copy/single.go @@ -97,7 +97,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P return nil, "", "", err } - sigs, err := c.sourceSignatures(ctx, src, options, + sigs, err := c.sourceSignatures(ctx, src, "Getting image source signatures", "Checking if image destination supports signatures") if err != nil { From ca080b0115280eac632d28c0d66861244e574c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:52:30 +0200 Subject: [PATCH 10/51] Make compareImageDestinationManifestEqual a method on imageCopier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we can eliminate three parameters. Signed-off-by: Miloslav Trmač --- copy/single.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/copy/single.go b/copy/single.go index 3352000fc..851f9e675 100644 --- a/copy/single.go +++ b/copy/single.go @@ -175,7 +175,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P logrus.Debugf("Checking if we can skip copying: has signatures=%t, OCI encryption=%t, no manifest updates=%t", shouldUpdateSigs, destRequiresOciEncryption, noPendingManifestUpdates) if !shouldUpdateSigs && !destRequiresOciEncryption && noPendingManifestUpdates { - isSrcDestManifestEqual, retManifest, retManifestType, retManifestDigest, err := compareImageDestinationManifestEqual(ctx, options, src, targetInstance, c.dest) + isSrcDestManifestEqual, retManifest, retManifestType, retManifestDigest, err := ic.compareImageDestinationManifestEqual(ctx, targetInstance) if err != nil { logrus.Warnf("Failed to compare destination image manifest: %v", err) return nil, "", "", err @@ -323,17 +323,17 @@ func (ic *imageCopier) noPendingManifestUpdates() bool { return reflect.DeepEqual(*ic.manifestUpdates, types.ManifestUpdateOptions{InformationOnly: ic.manifestUpdates.InformationOnly}) } -// compareImageDestinationManifestEqual compares the `src` and `dest` image manifests (reading the manifest from the +// compareImageDestinationManifestEqual compares the source and destination image manifests (reading the manifest from the // (possibly remote) destination). Returning true and the destination's manifest, type and digest if they compare equal. -func compareImageDestinationManifestEqual(ctx context.Context, options *Options, src *image.SourcedImage, targetInstance *digest.Digest, dest types.ImageDestination) (bool, []byte, string, digest.Digest, error) { - srcManifestDigest, err := manifest.Digest(src.ManifestBlob) +func (ic *imageCopier) compareImageDestinationManifestEqual(ctx context.Context, targetInstance *digest.Digest) (bool, []byte, string, digest.Digest, error) { + srcManifestDigest, err := manifest.Digest(ic.src.ManifestBlob) if err != nil { return false, nil, "", "", fmt.Errorf("calculating manifest digest: %w", err) } - destImageSource, err := dest.Reference().NewImageSource(ctx, options.DestinationCtx) + destImageSource, err := ic.c.dest.Reference().NewImageSource(ctx, ic.c.options.DestinationCtx) if err != nil { - logrus.Debugf("Unable to create destination image %s source: %v", dest.Reference(), err) + logrus.Debugf("Unable to create destination image %s source: %v", ic.c.dest.Reference(), err) return false, nil, "", "", nil } From 0b2862def5cd63e8e2020c4ce2bd1c64975e5a8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:53:43 +0200 Subject: [PATCH 11/51] Use copier.options in copySingleImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 4 ++-- copy/multiple.go | 2 +- copy/single.go | 24 ++++++++++++------------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index ce4c78ab6..1070c2439 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -256,7 +256,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, options, unparsedToplevel, unparsedToplevel, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedToplevel, nil); err != nil { return nil, err } } else if options.ImageListSelection == CopySystemImage { @@ -277,7 +277,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, options, unparsedToplevel, unparsedInstance, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedInstance, nil); err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } } else { /* options.ImageListSelection == CopyAllImages or options.ImageListSelection == CopySpecificImages, */ diff --git a/copy/multiple.go b/copy/multiple.go index d9b06695b..8655f1777 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -129,7 +129,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, policyContext, options, unparsedToplevel, unparsedInstance, &instanceCopyList[i].sourceDigest) + updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedInstance, &instanceCopyList[i].sourceDigest) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } diff --git a/copy/single.go b/copy/single.go index 851f9e675..12602028f 100644 --- a/copy/single.go +++ b/copy/single.go @@ -43,7 +43,7 @@ type imageCopier struct { // copySingleImage copies a single (non-manifest-list) image unparsedImage, using policyContext to validate // source image admissibility. -func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.PolicyContext, options *Options, unparsedToplevel, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { +func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.PolicyContext, unparsedToplevel, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { // The caller is handling manifest lists; this could happen only if a manifest list contains a manifest list. // Make sure we fail cleanly in such cases. multiImage, err := isMultiImage(ctx, unparsedImage) @@ -61,7 +61,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P if allowed, err := policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so. return nil, "", "", fmt.Errorf("Source image rejected: %w", err) } - src, err := image.FromUnparsedImage(ctx, options.SourceCtx, unparsedImage) + src, err := image.FromUnparsedImage(ctx, c.options.SourceCtx, unparsedImage) if err != nil { return nil, "", "", fmt.Errorf("initializing image from source %s: %w", transports.ImageName(c.rawSource.Reference()), err) } @@ -93,7 +93,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P } } - if err := checkImageDestinationForCurrentRuntime(ctx, options.DestinationCtx, src, c.dest); err != nil { + if err := checkImageDestinationForCurrentRuntime(ctx, c.options.DestinationCtx, src, c.dest); err != nil { return nil, "", "", err } @@ -114,7 +114,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P if destIsDigestedReference { cannotModifyManifestReason = "Destination specifies a digest" } - if options.PreserveDigests { + if c.options.PreserveDigests { cannotModifyManifestReason = "Instructed to preserve digests" } @@ -124,12 +124,12 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P src: src, // diffIDsAreNeeded is computed later cannotModifyManifestReason: cannotModifyManifestReason, - ociEncryptLayers: options.OciEncryptLayers, + ociEncryptLayers: c.options.OciEncryptLayers, } - if options.DestinationCtx != nil { + if c.options.DestinationCtx != nil { // Note that compressionFormat and compressionLevel can be nil. - ic.compressionFormat = options.DestinationCtx.CompressionFormat - ic.compressionLevel = options.DestinationCtx.CompressionLevel + ic.compressionFormat = c.options.DestinationCtx.CompressionFormat + ic.compressionLevel = c.options.DestinationCtx.CompressionLevel } // Decide whether we can substitute blobs with semantic equivalents: // - Don’t do that if we can’t modify the manifest at all @@ -145,12 +145,12 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P return nil, "", "", err } - destRequiresOciEncryption := (isEncrypted(src) && ic.c.ociDecryptConfig != nil) || options.OciEncryptLayers != nil + destRequiresOciEncryption := (isEncrypted(src) && ic.c.ociDecryptConfig != nil) || c.options.OciEncryptLayers != nil manifestConversionPlan, err := determineManifestConversion(determineManifestConversionInputs{ srcMIMEType: ic.src.ManifestMIMEType, destSupportedManifestMIMETypes: ic.c.dest.SupportedManifestMIMETypes(), - forceManifestMIMEType: options.ForceManifestMIMEType, + forceManifestMIMEType: c.options.ForceManifestMIMEType, requiresOCIEncryption: destRequiresOciEncryption, cannotModifyManifestReason: ic.cannotModifyManifestReason, }) @@ -169,7 +169,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P ic.diffIDsAreNeeded = src.UpdatedImageNeedsLayerDiffIDs(*ic.manifestUpdates) // If enabled, fetch and compare the destination's manifest. And as an optimization skip updating the destination iff equal - if options.OptimizeDestinationImageAlreadyExists { + if c.options.OptimizeDestinationImageAlreadyExists { shouldUpdateSigs := len(sigs) > 0 || len(c.signers) != 0 // TODO: Consider allowing signatures updates only and skipping the image's layers/manifest copy if possible noPendingManifestUpdates := ic.noPendingManifestUpdates() @@ -250,7 +250,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P targetInstance = &retManifestDigest } - newSigs, err := c.createSignatures(ctx, manifestBytes, options.SignIdentity) + newSigs, err := c.createSignatures(ctx, manifestBytes, c.options.SignIdentity) if err != nil { return nil, "", "", err } From 68ed088b9284ea02f7467079bd22946520ca2d3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:57:49 +0200 Subject: [PATCH 12/51] Use copier.options in copyMultipleImages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 2 +- copy/multiple.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 1070c2439..003bb9ce9 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -292,7 +292,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, case CopySpecificImages: logrus.Debugf("Source is a manifest list; copying some instances") } - if copiedManifest, err = c.copyMultipleImages(ctx, policyContext, options, unparsedToplevel); err != nil { + if copiedManifest, err = c.copyMultipleImages(ctx, policyContext, unparsedToplevel); err != nil { return nil, err } } diff --git a/copy/multiple.go b/copy/multiple.go index 8655f1777..d96c3358a 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -49,7 +49,7 @@ func prepareInstanceCopies(instanceDigests []digest.Digest, options *Options) [] // copyMultipleImages copies some or all of an image list's instances, using // policyContext to validate source image admissibility. -func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext, options *Options, unparsedToplevel *image.UnparsedImage) (copiedManifest []byte, retErr error) { +func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext, unparsedToplevel *image.UnparsedImage) (copiedManifest []byte, retErr error) { // Parse the list and get a copy of the original value after it's re-encoded. manifestList, manifestType, err := unparsedToplevel.Manifest(ctx) if err != nil { @@ -94,12 +94,12 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur if destIsDigestedReference { cannotModifyManifestListReason = "Destination specifies a digest" } - if options.PreserveDigests { + if c.options.PreserveDigests { cannotModifyManifestListReason = "Instructed to preserve digests" } // Determine if we'll need to convert the manifest list to a different format. - forceListMIMEType := options.ForceManifestMIMEType + forceListMIMEType := c.options.ForceManifestMIMEType switch forceListMIMEType { case manifest.DockerV2Schema1MediaType, manifest.DockerV2Schema1SignedMediaType, manifest.DockerV2Schema2MediaType: forceListMIMEType = manifest.DockerV2ListMediaType @@ -119,7 +119,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur // Copy each image, or just the ones we want to copy, in turn. instanceDigests := updatedList.Instances() instanceEdits := []internalManifest.ListEdit{} - instanceCopyList := prepareInstanceCopies(instanceDigests, options) + instanceCopyList := prepareInstanceCopies(instanceDigests, c.options) c.Printf("Copying %d of %d images in list\n", len(instanceCopyList), len(instanceDigests)) for i, instance := range instanceCopyList { // Update instances to be edited by their `ListOperation` and @@ -204,7 +204,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur } // Sign the manifest list. - newSigs, err := c.createSignatures(ctx, manifestList, options.SignIdentity) + newSigs, err := c.createSignatures(ctx, manifestList, c.options.SignIdentity) if err != nil { return nil, err } From 4033bbbf7ba0f2688db0f42fa6019ffd2209ab19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 01:58:43 +0200 Subject: [PATCH 13/51] Consistently use c.options in copy.Image after it is set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that the code looks the same here and in possible callees. Signed-off-by: Miloslav Trmač --- copy/copy.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 003bb9ce9..14c95cda8 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -226,9 +226,9 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, // Set the concurrentBlobCopiesSemaphore if we can copy layers in parallel. if dest.HasThreadSafePutBlob() && rawSource.HasThreadSafeGetBlob() { - c.concurrentBlobCopiesSemaphore = options.ConcurrentBlobCopiesSemaphore + c.concurrentBlobCopiesSemaphore = c.options.ConcurrentBlobCopiesSemaphore if c.concurrentBlobCopiesSemaphore == nil { - max := options.MaxParallelDownloads + max := c.options.MaxParallelDownloads if max == 0 { max = maxParallelDownloads } @@ -236,11 +236,11 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } } else { c.concurrentBlobCopiesSemaphore = semaphore.NewWeighted(int64(1)) - if options.ConcurrentBlobCopiesSemaphore != nil { - if err := options.ConcurrentBlobCopiesSemaphore.Acquire(ctx, 1); err != nil { + if c.options.ConcurrentBlobCopiesSemaphore != nil { + if err := c.options.ConcurrentBlobCopiesSemaphore.Acquire(ctx, 1); err != nil { return nil, fmt.Errorf("acquiring semaphore for concurrent blob copies: %w", err) } - defer options.ConcurrentBlobCopiesSemaphore.Release(1) + defer c.options.ConcurrentBlobCopiesSemaphore.Release(1) } } @@ -259,7 +259,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedToplevel, nil); err != nil { return nil, err } - } else if options.ImageListSelection == CopySystemImage { + } else if c.options.ImageListSelection == CopySystemImage { // This is a manifest list, and we weren't asked to copy multiple images. Choose a single image that // matches the current system to copy, and copy it. mfest, manifestType, err := unparsedToplevel.Manifest(ctx) @@ -270,7 +270,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if err != nil { return nil, fmt.Errorf("parsing primary manifest as list for %s: %w", transports.ImageName(srcRef), err) } - instanceDigest, err := manifestList.ChooseInstanceByCompression(options.SourceCtx, options.PreferGzipInstances) // try to pick one that matches options.SourceCtx + instanceDigest, err := manifestList.ChooseInstanceByCompression(c.options.SourceCtx, c.options.PreferGzipInstances) // try to pick one that matches c.options.SourceCtx if err != nil { return nil, fmt.Errorf("choosing an image from manifest list %s: %w", transports.ImageName(srcRef), err) } @@ -280,13 +280,13 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedInstance, nil); err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } - } else { /* options.ImageListSelection == CopyAllImages or options.ImageListSelection == CopySpecificImages, */ + } else { /* c.options.ImageListSelection == CopyAllImages or c.options.ImageListSelection == CopySpecificImages, */ // If we were asked to copy multiple images and can't, that's an error. if !supportsMultipleImages(c.dest) { return nil, fmt.Errorf("copying multiple images: destination transport %q does not support copying multiple images as a group", destRef.Transport().Name()) } // Copy some or all of the images. - switch options.ImageListSelection { + switch c.options.ImageListSelection { case CopyAllImages: logrus.Debugf("Source is a manifest list; copying all instances") case CopySpecificImages: From c31a914fcc5642f807e5c7195e477f063f487ce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:01:47 +0200 Subject: [PATCH 14/51] Eliminate copier.progress and progressInterval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use options.Progress and ProgressInterval directly. Signed-off-by: Miloslav Trmač --- copy/blob.go | 8 ++++---- copy/copy.go | 8 ++------ copy/single.go | 4 ++-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index f45b97f56..8d5580d7c 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -83,12 +83,12 @@ func (ic *imageCopier) copyBlobFromStream(ctx context.Context, srcReader io.Read return types.BlobInfo{}, err } - // === Report progress using the ic.c.progress channel, if required. - if ic.c.progress != nil && ic.c.progressInterval > 0 { + // === Report progress using the ic.c.options.Progress channel, if required. + if ic.c.options.Progress != nil && ic.c.options.ProgressInterval > 0 { progressReader := newProgressReader( stream.reader, - ic.c.progress, - ic.c.progressInterval, + ic.c.options.Progress, + ic.c.options.ProgressInterval, srcInfo, ) defer progressReader.reportDone() diff --git a/copy/copy.go b/copy/copy.go index 14c95cda8..e0259ea9c 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -138,8 +138,6 @@ type copier struct { reportWriter io.Writer progressOutput io.Writer - progressInterval time.Duration - progress chan types.ProgressProperties blobInfoCache internalblobinfocache.BlobInfoCache2 ociDecryptConfig *encconfig.DecryptConfig ociEncryptConfig *encconfig.EncryptConfig @@ -210,10 +208,8 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, rawSource: rawSource, options: options, - reportWriter: reportWriter, - progressOutput: progressOutput, - progressInterval: options.ProgressInterval, - progress: options.Progress, + reportWriter: reportWriter, + progressOutput: progressOutput, // FIXME? The cache is used for sources and destinations equally, but we only have a SourceCtx and DestinationCtx. // For now, use DestinationCtx (because blob reuse changes the behavior of the destination side more); eventually // we might want to add a separate CommonCtx — or would that be too confusing? diff --git a/copy/single.go b/copy/single.go index 12602028f..fa4ac9896 100644 --- a/copy/single.go +++ b/copy/single.go @@ -642,8 +642,8 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to }() // Throw an event that the layer has been skipped - if ic.c.progress != nil && ic.c.progressInterval > 0 { - ic.c.progress <- types.ProgressProperties{ + if ic.c.options.Progress != nil && ic.c.options.ProgressInterval > 0 { + ic.c.options.Progress <- types.ProgressProperties{ Event: types.ProgressEventSkipped, Artifact: srcInfo, } From d88ba008ce8dee0f90d84f3bd77cdd16e1fad1b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:07:02 +0200 Subject: [PATCH 15/51] Eliminate copier.ociDecryptConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use c.options.OciDecryptConfig directly. Signed-off-by: Miloslav Trmač --- copy/copy.go | 2 -- copy/encryption.go | 4 ++-- copy/single.go | 4 ++-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index e0259ea9c..8bcdaf85e 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -139,7 +139,6 @@ type copier struct { reportWriter io.Writer progressOutput io.Writer blobInfoCache internalblobinfocache.BlobInfoCache2 - ociDecryptConfig *encconfig.DecryptConfig ociEncryptConfig *encconfig.EncryptConfig concurrentBlobCopiesSemaphore *semaphore.Weighted // Limits the amount of concurrently copied blobs downloadForeignLayers bool @@ -214,7 +213,6 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, // For now, use DestinationCtx (because blob reuse changes the behavior of the destination side more); eventually // we might want to add a separate CommonCtx — or would that be too confusing? blobInfoCache: internalblobinfocache.FromBlobInfoCache(blobinfocache.DefaultCache(options.DestinationCtx)), - ociDecryptConfig: options.OciDecryptConfig, ociEncryptConfig: options.OciEncryptConfig, downloadForeignLayers: options.DownloadForeignLayers, } diff --git a/copy/encryption.go b/copy/encryption.go index 86fadff66..bd48a94b3 100644 --- a/copy/encryption.go +++ b/copy/encryption.go @@ -34,7 +34,7 @@ type bpDecryptionStepData struct { // srcInfo is only used for error messages. // Returns data for other steps; the caller should eventually use updateCryptoOperation. func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) { - if !isOciEncrypted(stream.info.MediaType) || ic.c.ociDecryptConfig == nil { + if !isOciEncrypted(stream.info.MediaType) || ic.c.options.OciDecryptConfig == nil { return &bpDecryptionStepData{ decrypting: false, }, nil @@ -47,7 +47,7 @@ func (ic *imageCopier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo desc := imgspecv1.Descriptor{ Annotations: stream.info.Annotations, } - reader, decryptedDigest, err := ocicrypt.DecryptLayer(ic.c.ociDecryptConfig, stream.reader, desc, false) + reader, decryptedDigest, err := ocicrypt.DecryptLayer(ic.c.options.OciDecryptConfig, stream.reader, desc, false) if err != nil { return nil, fmt.Errorf("decrypting layer %s: %w", srcInfo.Digest, err) } diff --git a/copy/single.go b/copy/single.go index fa4ac9896..f3cbc1509 100644 --- a/copy/single.go +++ b/copy/single.go @@ -145,7 +145,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P return nil, "", "", err } - destRequiresOciEncryption := (isEncrypted(src) && ic.c.ociDecryptConfig != nil) || c.options.OciEncryptLayers != nil + destRequiresOciEncryption := (isEncrypted(src) && ic.c.options.OciDecryptConfig != nil) || c.options.OciEncryptLayers != nil manifestConversionPlan, err := determineManifestConversion(determineManifestConversionInputs{ srcMIMEType: ic.src.ManifestMIMEType, @@ -608,7 +608,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to // When encrypting to decrypting, only use the simple code path. We might be able to optimize more // (e.g. if we know the DiffID of an encrypted compressed layer, it might not be necessary to pull, decrypt and decompress again), // but it’s not trivially safe to do such things, so until someone takes the effort to make a comprehensive argument, let’s not. - encryptingOrDecrypting := toEncrypt || (isOciEncrypted(srcInfo.MediaType) && ic.c.ociDecryptConfig != nil) + encryptingOrDecrypting := toEncrypt || (isOciEncrypted(srcInfo.MediaType) && ic.c.options.OciDecryptConfig != nil) canAvoidProcessingCompleteLayer := !diffIDIsNeeded && !encryptingOrDecrypting // Don’t read the layer from the source if we already have the blob, and optimizations are acceptable. From 5e207880709990f4e6a83704588008f98da22a2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:07:46 +0200 Subject: [PATCH 16/51] Eliminate copier.ociEncryptConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use c.options.OciEncryptConfig directly. Signed-off-by: Miloslav Trmač --- copy/copy.go | 2 -- copy/encryption.go | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 8bcdaf85e..0284e535f 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -139,7 +139,6 @@ type copier struct { reportWriter io.Writer progressOutput io.Writer blobInfoCache internalblobinfocache.BlobInfoCache2 - ociEncryptConfig *encconfig.EncryptConfig concurrentBlobCopiesSemaphore *semaphore.Weighted // Limits the amount of concurrently copied blobs downloadForeignLayers bool signers []*signer.Signer // Signers to use to create new signatures for the image @@ -213,7 +212,6 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, // For now, use DestinationCtx (because blob reuse changes the behavior of the destination side more); eventually // we might want to add a separate CommonCtx — or would that be too confusing? blobInfoCache: internalblobinfocache.FromBlobInfoCache(blobinfocache.DefaultCache(options.DestinationCtx)), - ociEncryptConfig: options.OciEncryptConfig, downloadForeignLayers: options.DownloadForeignLayers, } defer c.close() diff --git a/copy/encryption.go b/copy/encryption.go index bd48a94b3..b406b0c31 100644 --- a/copy/encryption.go +++ b/copy/encryption.go @@ -81,7 +81,7 @@ type bpEncryptionStepData struct { // Returns data for other steps; the caller should eventually call updateCryptoOperationAndAnnotations. func (ic *imageCopier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool, srcInfo types.BlobInfo, decryptionStep *bpDecryptionStepData) (*bpEncryptionStepData, error) { - if !toEncrypt || isOciEncrypted(srcInfo.MediaType) || ic.c.ociEncryptConfig == nil { + if !toEncrypt || isOciEncrypted(srcInfo.MediaType) || ic.c.options.OciEncryptConfig == nil { return &bpEncryptionStepData{ encrypting: false, }, nil @@ -101,7 +101,7 @@ func (ic *imageCopier) blobPipelineEncryptionStep(stream *sourceStream, toEncryp Size: srcInfo.Size, Annotations: annotations, } - reader, finalizer, err := ocicrypt.EncryptLayer(ic.c.ociEncryptConfig, stream.reader, desc) + reader, finalizer, err := ocicrypt.EncryptLayer(ic.c.options.OciEncryptConfig, stream.reader, desc) if err != nil { return nil, fmt.Errorf("encrypting blob %s: %w", srcInfo.Digest, err) } From 8b4f64c16fe621fc94c07d46f3e6ffd550d3e639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:08:51 +0200 Subject: [PATCH 17/51] Eliminate copier.downloadForeignLayers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use c.options.DownloadForeignLayers directly. Signed-off-by: Miloslav Trmač --- copy/copy.go | 8 +++----- copy/single.go | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 0284e535f..4aa171582 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -140,9 +140,8 @@ type copier struct { progressOutput io.Writer blobInfoCache internalblobinfocache.BlobInfoCache2 concurrentBlobCopiesSemaphore *semaphore.Weighted // Limits the amount of concurrently copied blobs - downloadForeignLayers bool - signers []*signer.Signer // Signers to use to create new signatures for the image - signersToClose []*signer.Signer // Signers that should be closed when this copier is destroyed. + signers []*signer.Signer // Signers to use to create new signatures for the image + signersToClose []*signer.Signer // Signers that should be closed when this copier is destroyed. } // Image copies image from srcRef to destRef, using policyContext to validate @@ -211,8 +210,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, // FIXME? The cache is used for sources and destinations equally, but we only have a SourceCtx and DestinationCtx. // For now, use DestinationCtx (because blob reuse changes the behavior of the destination side more); eventually // we might want to add a separate CommonCtx — or would that be too confusing? - blobInfoCache: internalblobinfocache.FromBlobInfoCache(blobinfocache.DefaultCache(options.DestinationCtx)), - downloadForeignLayers: options.DownloadForeignLayers, + blobInfoCache: internalblobinfocache.FromBlobInfoCache(blobinfocache.DefaultCache(options.DestinationCtx)), } defer c.close() diff --git a/copy/single.go b/copy/single.go index f3cbc1509..6ffff222d 100644 --- a/copy/single.go +++ b/copy/single.go @@ -396,7 +396,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { defer ic.c.concurrentBlobCopiesSemaphore.Release(1) defer copyGroup.Done() cld := copyLayerData{} - if !ic.c.downloadForeignLayers && ic.c.dest.AcceptsForeignLayerURLs() && len(srcLayer.URLs) != 0 { + if !ic.c.options.DownloadForeignLayers && ic.c.dest.AcceptsForeignLayerURLs() && len(srcLayer.URLs) != 0 { // DiffIDs are, currently, needed only when converting from schema1. // In which case src.LayerInfos will not have URLs because schema1 // does not support them. From d1d3d633f4c9017d7769ba247ee0fe1582983814 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:12:58 +0200 Subject: [PATCH 18/51] Eliminate imageCopier.ociEncryptLayers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use ic.c.options.OciEncryptLayers directly. Signed-off-by: Miloslav Trmač --- copy/single.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/copy/single.go b/copy/single.go index 6ffff222d..843d0f18f 100644 --- a/copy/single.go +++ b/copy/single.go @@ -38,7 +38,6 @@ type imageCopier struct { canSubstituteBlobs bool compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil. compressionLevel *int - ociEncryptLayers *[]int } // copySingleImage copies a single (non-manifest-list) image unparsedImage, using policyContext to validate @@ -124,7 +123,6 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P src: src, // diffIDsAreNeeded is computed later cannotModifyManifestReason: cannotModifyManifestReason, - ociEncryptLayers: c.options.OciEncryptLayers, } if c.options.DestinationCtx != nil { // Note that compressionFormat and compressionLevel can be nil. @@ -415,10 +413,10 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // Decide which layers to encrypt layersToEncrypt := set.New[int]() var encryptAll bool - if ic.ociEncryptLayers != nil { - encryptAll = len(*ic.ociEncryptLayers) == 0 + if ic.c.options.OciEncryptLayers != nil { + encryptAll = len(*ic.c.options.OciEncryptLayers) == 0 totalLayers := len(srcInfos) - for _, l := range *ic.ociEncryptLayers { + for _, l := range *ic.c.options.OciEncryptLayers { // if layer is negative, it is reverse indexed. layersToEncrypt.Add((totalLayers + l) % totalLayers) } From 4d791ba6830aaf7e66797df22bd44d614c7f2fbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:18:59 +0200 Subject: [PATCH 19/51] Introduce copier.unparsedToplevel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So that we don't need to pass it around in a parameter. Signed-off-by: Miloslav Trmač --- copy/copy.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 4aa171582..9601bb0d0 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -136,8 +136,10 @@ type copier struct { rawSource private.ImageSource options *Options // never nil - reportWriter io.Writer - progressOutput io.Writer + reportWriter io.Writer + progressOutput io.Writer + + unparsedToplevel *image.UnparsedImage // for rawSource blobInfoCache internalblobinfocache.BlobInfoCache2 concurrentBlobCopiesSemaphore *semaphore.Weighted // Limits the amount of concurrently copied blobs signers []*signer.Signer // Signers to use to create new signatures for the image @@ -207,6 +209,8 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, reportWriter: reportWriter, progressOutput: progressOutput, + + unparsedToplevel: image.UnparsedInstance(rawSource, nil), // FIXME? The cache is used for sources and destinations equally, but we only have a SourceCtx and DestinationCtx. // For now, use DestinationCtx (because blob reuse changes the behavior of the destination side more); eventually // we might want to add a separate CommonCtx — or would that be too confusing? @@ -238,7 +242,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, return nil, err } - unparsedToplevel := image.UnparsedInstance(rawSource, nil) + unparsedToplevel := c.unparsedToplevel multiImage, err := isMultiImage(ctx, unparsedToplevel) if err != nil { return nil, fmt.Errorf("determining manifest MIME type for %s: %w", transports.ImageName(srcRef), err) From 455e5dce8b2784a1ab2c00d57560c51c58b72333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:25:29 +0200 Subject: [PATCH 20/51] Use copier.unparsedToplevel in copySingleImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 4 ++-- copy/multiple.go | 2 +- copy/single.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 9601bb0d0..0785f6e40 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -250,7 +250,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedToplevel, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, nil); err != nil { return nil, err } } else if c.options.ImageListSelection == CopySystemImage { @@ -271,7 +271,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedInstance, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedInstance, nil); err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } } else { /* c.options.ImageListSelection == CopyAllImages or c.options.ImageListSelection == CopySpecificImages, */ diff --git a/copy/multiple.go b/copy/multiple.go index d96c3358a..fb1f75272 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -129,7 +129,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, policyContext, unparsedToplevel, unparsedInstance, &instanceCopyList[i].sourceDigest) + updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, policyContext, unparsedInstance, &instanceCopyList[i].sourceDigest) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } diff --git a/copy/single.go b/copy/single.go index 843d0f18f..8d69c4bcf 100644 --- a/copy/single.go +++ b/copy/single.go @@ -42,7 +42,7 @@ type imageCopier struct { // copySingleImage copies a single (non-manifest-list) image unparsedImage, using policyContext to validate // source image admissibility. -func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.PolicyContext, unparsedToplevel, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { +func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.PolicyContext, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { // The caller is handling manifest lists; this could happen only if a manifest list contains a manifest list. // Make sure we fail cleanly in such cases. multiImage, err := isMultiImage(ctx, unparsedImage) @@ -77,7 +77,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P return nil, "", "", fmt.Errorf("computing digest of source image's manifest: %w", err) } if !matches { - manifestList, _, err := unparsedToplevel.Manifest(ctx) + manifestList, _, err := c.unparsedToplevel.Manifest(ctx) if err != nil { return nil, "", "", fmt.Errorf("reading manifest from source image: %w", err) } From d6d389c2733b1b6ecdf502fba9d92ccef21b9fad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:26:55 +0200 Subject: [PATCH 21/51] Use c.unparsedToplevel in copyMultipleImages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 2 +- copy/multiple.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 0785f6e40..7d152629b 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -286,7 +286,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, case CopySpecificImages: logrus.Debugf("Source is a manifest list; copying some instances") } - if copiedManifest, err = c.copyMultipleImages(ctx, policyContext, unparsedToplevel); err != nil { + if copiedManifest, err = c.copyMultipleImages(ctx, policyContext); err != nil { return nil, err } } diff --git a/copy/multiple.go b/copy/multiple.go index fb1f75272..d1d6a35fc 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -49,9 +49,9 @@ func prepareInstanceCopies(instanceDigests []digest.Digest, options *Options) [] // copyMultipleImages copies some or all of an image list's instances, using // policyContext to validate source image admissibility. -func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext, unparsedToplevel *image.UnparsedImage) (copiedManifest []byte, retErr error) { +func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext) (copiedManifest []byte, retErr error) { // Parse the list and get a copy of the original value after it's re-encoded. - manifestList, manifestType, err := unparsedToplevel.Manifest(ctx) + manifestList, manifestType, err := c.unparsedToplevel.Manifest(ctx) if err != nil { return nil, fmt.Errorf("reading manifest list: %w", err) } @@ -61,7 +61,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur } updatedList := originalList.CloneInternal() - sigs, err := c.sourceSignatures(ctx, unparsedToplevel, + sigs, err := c.sourceSignatures(ctx, c.unparsedToplevel, "Getting image list signatures", "Checking if image list destination supports signatures") if err != nil { From 8fcd8cee8925d730d7d7a375104dff825938001a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:27:20 +0200 Subject: [PATCH 22/51] Eliminate the unparsedToplevel variable in copy.Image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use copier.unparsedToplevel now that it exists. Signed-off-by: Miloslav Trmač --- copy/copy.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 7d152629b..00dff91b8 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -242,21 +242,20 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, return nil, err } - unparsedToplevel := c.unparsedToplevel - multiImage, err := isMultiImage(ctx, unparsedToplevel) + multiImage, err := isMultiImage(ctx, c.unparsedToplevel) if err != nil { return nil, fmt.Errorf("determining manifest MIME type for %s: %w", transports.ImageName(srcRef), err) } if !multiImage { // The simple case: just copy a single image. - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedToplevel, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, c.unparsedToplevel, nil); err != nil { return nil, err } } else if c.options.ImageListSelection == CopySystemImage { // This is a manifest list, and we weren't asked to copy multiple images. Choose a single image that // matches the current system to copy, and copy it. - mfest, manifestType, err := unparsedToplevel.Manifest(ctx) + mfest, manifestType, err := c.unparsedToplevel.Manifest(ctx) if err != nil { return nil, fmt.Errorf("reading manifest for %s: %w", transports.ImageName(srcRef), err) } @@ -291,7 +290,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } } - if err := c.dest.Commit(ctx, unparsedToplevel); err != nil { + if err := c.dest.Commit(ctx, c.unparsedToplevel); err != nil { return nil, fmt.Errorf("committing the finished image: %w", err) } From fa1dfbcb96e904d7282e82d7ba516b2492c006ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:36:33 +0200 Subject: [PATCH 23/51] Add policyContext to copier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we don't need to carry it around in parameters. Signed-off-by: Miloslav Trmač --- copy/copy.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 00dff91b8..4ae247eb7 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -132,9 +132,10 @@ type Options struct { // data shared across one or more images in a possible manifest list. // The owner must call close() when done. type copier struct { - dest private.ImageDestination - rawSource private.ImageSource - options *Options // never nil + policyContext *signature.PolicyContext + dest private.ImageDestination + rawSource private.ImageSource + options *Options // never nil reportWriter io.Writer progressOutput io.Writer @@ -203,9 +204,10 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } c := &copier{ - dest: dest, - rawSource: rawSource, - options: options, + policyContext: policyContext, + dest: dest, + rawSource: rawSource, + options: options, reportWriter: reportWriter, progressOutput: progressOutput, From c441f52ff2ca2311ddea19a7df8d180ba9e9124f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:37:44 +0200 Subject: [PATCH 24/51] Use copier.policyContext in copySingleImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 4 ++-- copy/multiple.go | 2 +- copy/single.go | 7 +++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 4ae247eb7..afa00c96b 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -251,7 +251,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, c.unparsedToplevel, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, c.unparsedToplevel, nil); err != nil { return nil, err } } else if c.options.ImageListSelection == CopySystemImage { @@ -272,7 +272,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - if copiedManifest, _, _, err = c.copySingleImage(ctx, policyContext, unparsedInstance, nil); err != nil { + if copiedManifest, _, _, err = c.copySingleImage(ctx, unparsedInstance, nil); err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } } else { /* c.options.ImageListSelection == CopyAllImages or c.options.ImageListSelection == CopySpecificImages, */ diff --git a/copy/multiple.go b/copy/multiple.go index d1d6a35fc..d741c1be7 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -129,7 +129,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, policyContext, unparsedInstance, &instanceCopyList[i].sourceDigest) + updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } diff --git a/copy/single.go b/copy/single.go index 8d69c4bcf..f095d0f0b 100644 --- a/copy/single.go +++ b/copy/single.go @@ -18,7 +18,6 @@ import ( "github.com/containers/image/v5/manifest" "github.com/containers/image/v5/pkg/compression" compressiontypes "github.com/containers/image/v5/pkg/compression/types" - "github.com/containers/image/v5/signature" "github.com/containers/image/v5/transports" "github.com/containers/image/v5/types" digest "github.com/opencontainers/go-digest" @@ -40,9 +39,9 @@ type imageCopier struct { compressionLevel *int } -// copySingleImage copies a single (non-manifest-list) image unparsedImage, using policyContext to validate +// copySingleImage copies a single (non-manifest-list) image unparsedImage, using c.policyContext to validate // source image admissibility. -func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.PolicyContext, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { +func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { // The caller is handling manifest lists; this could happen only if a manifest list contains a manifest list. // Make sure we fail cleanly in such cases. multiImage, err := isMultiImage(ctx, unparsedImage) @@ -57,7 +56,7 @@ func (c *copier) copySingleImage(ctx context.Context, policyContext *signature.P // Please keep this policy check BEFORE reading any other information about the image. // (The multiImage check above only matches the MIME type, which we have received anyway. // Actual parsing of anything should be deferred.) - if allowed, err := policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so. + if allowed, err := c.policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so. return nil, "", "", fmt.Errorf("Source image rejected: %w", err) } src, err := image.FromUnparsedImage(ctx, c.options.SourceCtx, unparsedImage) From f367a5f8b8e865166d2000a16326a3c8aa4609f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:38:45 +0200 Subject: [PATCH 25/51] Remove the policyContext parameter from copyMultipleImages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is no longer used. Signed-off-by: Miloslav Trmač --- copy/copy.go | 2 +- copy/multiple.go | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index afa00c96b..e91ad189b 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -287,7 +287,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, case CopySpecificImages: logrus.Debugf("Source is a manifest list; copying some instances") } - if copiedManifest, err = c.copyMultipleImages(ctx, policyContext); err != nil { + if copiedManifest, err = c.copyMultipleImages(ctx); err != nil { return nil, err } } diff --git a/copy/multiple.go b/copy/multiple.go index d741c1be7..c173d3e63 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -11,7 +11,6 @@ import ( "github.com/containers/image/v5/internal/image" internalManifest "github.com/containers/image/v5/internal/manifest" "github.com/containers/image/v5/manifest" - "github.com/containers/image/v5/signature" digest "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" @@ -48,8 +47,8 @@ func prepareInstanceCopies(instanceDigests []digest.Digest, options *Options) [] } // copyMultipleImages copies some or all of an image list's instances, using -// policyContext to validate source image admissibility. -func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signature.PolicyContext) (copiedManifest []byte, retErr error) { +// c.policyContext to validate source image admissibility. +func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, retErr error) { // Parse the list and get a copy of the original value after it's re-encoded. manifestList, manifestType, err := c.unparsedToplevel.Manifest(ctx) if err != nil { From 0cdeba7710af0fbe2357ec6d03db0d845968eb09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 02:53:13 +0200 Subject: [PATCH 26/51] Return a new struct copySingleImageResult from copySingleImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we don't have so many unnamed return values, and we can manage the return values as a batch. Signed-off-by: Miloslav Trmač --- copy/copy.go | 8 +++++-- copy/multiple.go | 8 +++---- copy/single.go | 61 ++++++++++++++++++++++++++++++------------------ 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index e91ad189b..fb99161d7 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -251,9 +251,11 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - if copiedManifest, _, _, err = c.copySingleImage(ctx, c.unparsedToplevel, nil); err != nil { + single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil) + if err != nil { return nil, err } + copiedManifest = single.manifest } else if c.options.ImageListSelection == CopySystemImage { // This is a manifest list, and we weren't asked to copy multiple images. Choose a single image that // matches the current system to copy, and copy it. @@ -272,9 +274,11 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - if copiedManifest, _, _, err = c.copySingleImage(ctx, unparsedInstance, nil); err != nil { + single, err := c.copySingleImage(ctx, unparsedInstance, nil) + if err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } + copiedManifest = single.manifest } else { /* c.options.ImageListSelection == CopyAllImages or c.options.ImageListSelection == CopySpecificImages, */ // If we were asked to copy multiple images and can't, that's an error. if !supportsMultipleImages(c.dest) { diff --git a/copy/multiple.go b/copy/multiple.go index c173d3e63..e221f0028 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -128,7 +128,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updatedManifest, updatedManifestType, updatedManifestDigest, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } @@ -136,9 +136,9 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, instanceEdits = append(instanceEdits, internalManifest.ListEdit{ ListOperation: internalManifest.ListOpUpdate, UpdateOldDigest: instance.sourceDigest, - UpdateDigest: updatedManifestDigest, - UpdateSize: int64(len(updatedManifest)), - UpdateMediaType: updatedManifestType}) + UpdateDigest: updated.manifestDigest, + UpdateSize: int64(len(updated.manifest)), + UpdateMediaType: updated.manifestMIMEType}) default: return nil, fmt.Errorf("copying image: invalid copy operation %d", instance.op) } diff --git a/copy/single.go b/copy/single.go index f095d0f0b..d51a8a271 100644 --- a/copy/single.go +++ b/copy/single.go @@ -39,29 +39,36 @@ type imageCopier struct { compressionLevel *int } +// copySingleImageResult carries data produced by copySingleImage +type copySingleImageResult struct { + manifest []byte + manifestMIMEType string + manifestDigest digest.Digest +} + // copySingleImage copies a single (non-manifest-list) image unparsedImage, using c.policyContext to validate // source image admissibility. -func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (retManifest []byte, retManifestType string, retManifestDigest digest.Digest, retErr error) { +func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (copySingleImageResult, error) { // The caller is handling manifest lists; this could happen only if a manifest list contains a manifest list. // Make sure we fail cleanly in such cases. multiImage, err := isMultiImage(ctx, unparsedImage) if err != nil { // FIXME FIXME: How to name a reference for the sub-image? - return nil, "", "", fmt.Errorf("determining manifest MIME type for %s: %w", transports.ImageName(unparsedImage.Reference()), err) + return copySingleImageResult{}, fmt.Errorf("determining manifest MIME type for %s: %w", transports.ImageName(unparsedImage.Reference()), err) } if multiImage { - return nil, "", "", fmt.Errorf("Unexpectedly received a manifest list instead of a manifest for a single image") + return copySingleImageResult{}, fmt.Errorf("Unexpectedly received a manifest list instead of a manifest for a single image") } // Please keep this policy check BEFORE reading any other information about the image. // (The multiImage check above only matches the MIME type, which we have received anyway. // Actual parsing of anything should be deferred.) if allowed, err := c.policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so. - return nil, "", "", fmt.Errorf("Source image rejected: %w", err) + return copySingleImageResult{}, fmt.Errorf("Source image rejected: %w", err) } src, err := image.FromUnparsedImage(ctx, c.options.SourceCtx, unparsedImage) if err != nil { - return nil, "", "", fmt.Errorf("initializing image from source %s: %w", transports.ImageName(c.rawSource.Reference()), err) + return copySingleImageResult{}, fmt.Errorf("initializing image from source %s: %w", transports.ImageName(c.rawSource.Reference()), err) } // If the destination is a digested reference, make a note of that, determine what digest value we're @@ -73,33 +80,33 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar destIsDigestedReference = true matches, err := manifest.MatchesDigest(src.ManifestBlob, digested.Digest()) if err != nil { - return nil, "", "", fmt.Errorf("computing digest of source image's manifest: %w", err) + return copySingleImageResult{}, fmt.Errorf("computing digest of source image's manifest: %w", err) } if !matches { manifestList, _, err := c.unparsedToplevel.Manifest(ctx) if err != nil { - return nil, "", "", fmt.Errorf("reading manifest from source image: %w", err) + return copySingleImageResult{}, fmt.Errorf("reading manifest from source image: %w", err) } matches, err = manifest.MatchesDigest(manifestList, digested.Digest()) if err != nil { - return nil, "", "", fmt.Errorf("computing digest of source image's manifest: %w", err) + return copySingleImageResult{}, fmt.Errorf("computing digest of source image's manifest: %w", err) } if !matches { - return nil, "", "", errors.New("Digest of source image's manifest would not match destination reference") + return copySingleImageResult{}, errors.New("Digest of source image's manifest would not match destination reference") } } } } if err := checkImageDestinationForCurrentRuntime(ctx, c.options.DestinationCtx, src, c.dest); err != nil { - return nil, "", "", err + return copySingleImageResult{}, err } sigs, err := c.sourceSignatures(ctx, src, "Getting image source signatures", "Checking if image destination supports signatures") if err != nil { - return nil, "", "", err + return copySingleImageResult{}, err } // Determine if we're allowed to modify the manifest. @@ -139,7 +146,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar ic.canSubstituteBlobs = ic.cannotModifyManifestReason == "" && len(c.signers) == 0 if err := ic.updateEmbeddedDockerReference(); err != nil { - return nil, "", "", err + return copySingleImageResult{}, err } destRequiresOciEncryption := (isEncrypted(src) && ic.c.options.OciDecryptConfig != nil) || c.options.OciEncryptLayers != nil @@ -152,7 +159,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar cannotModifyManifestReason: ic.cannotModifyManifestReason, }) if err != nil { - return nil, "", "", err + return copySingleImageResult{}, err } // We set up this part of ic.manifestUpdates quite early, not just around the // code that calls copyUpdatedConfigAndManifest, so that other parts of the copy code @@ -175,18 +182,22 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar isSrcDestManifestEqual, retManifest, retManifestType, retManifestDigest, err := ic.compareImageDestinationManifestEqual(ctx, targetInstance) if err != nil { logrus.Warnf("Failed to compare destination image manifest: %v", err) - return nil, "", "", err + return copySingleImageResult{}, err } if isSrcDestManifestEqual { c.Printf("Skipping: image already present at destination\n") - return retManifest, retManifestType, retManifestDigest, nil + return copySingleImageResult{ + manifest: retManifest, + manifestMIMEType: retManifestType, + manifestDigest: retManifestDigest, + }, nil } } } if err := ic.copyLayers(ctx); err != nil { - return nil, "", "", err + return copySingleImageResult{}, err } // With docker/distribution registries we do not know whether the registry accepts schema2 or schema1 only; @@ -195,7 +206,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar // So, try the preferred manifest MIME type with possibly-updated blob digests, media types, and sizes if // we're altering how they're compressed. If the process succeeds, fine… manifestBytes, retManifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance) - retManifestType = manifestConversionPlan.preferredMIMEType + retManifestType := manifestConversionPlan.preferredMIMEType if err != nil { logrus.Debugf("Writing manifest using preferred type %s failed: %v", manifestConversionPlan.preferredMIMEType, err) // … if it fails, and the failure is either because the manifest is rejected by the registry, or @@ -210,14 +221,14 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar // 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 nil, "", "", err + return copySingleImageResult{}, err } // If the original MIME type is acceptable, determineManifestConversion always uses it as 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. if ic.cannotModifyManifestReason != "" { - return nil, "", "", fmt.Errorf("writing manifest failed and we cannot try conversions: %q: %w", cannotModifyManifestReason, err) + return copySingleImageResult{}, fmt.Errorf("writing manifest failed and we cannot try conversions: %q: %w", cannotModifyManifestReason, err) } // errs is a list of errors when trying various manifest types. Also serves as an "upload succeeded" flag when set to nil. @@ -240,7 +251,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar break } if errs != nil { - return nil, "", "", fmt.Errorf("Uploading manifest failed, attempted the following formats: %s", strings.Join(errs, ", ")) + return copySingleImageResult{}, fmt.Errorf("Uploading manifest failed, attempted the following formats: %s", strings.Join(errs, ", ")) } } if targetInstance != nil { @@ -249,18 +260,22 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar newSigs, err := c.createSignatures(ctx, manifestBytes, c.options.SignIdentity) if err != nil { - return nil, "", "", err + return copySingleImageResult{}, err } sigs = append(sigs, newSigs...) if len(sigs) > 0 { c.Printf("Storing signatures\n") if err := c.dest.PutSignaturesWithFormat(ctx, sigs, targetInstance); err != nil { - return nil, "", "", fmt.Errorf("writing signatures: %w", err) + return copySingleImageResult{}, fmt.Errorf("writing signatures: %w", err) } } - return manifestBytes, retManifestType, retManifestDigest, nil + return copySingleImageResult{ + manifest: manifestBytes, + manifestMIMEType: retManifestType, + manifestDigest: retManifestDigest, + }, nil } // checkImageDestinationForCurrentRuntime enforces dest.MustMatchRuntimeOS, if necessary. From 0517bc42f25bdef39fd092be92004298f381e378 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 03:01:10 +0200 Subject: [PATCH 27/51] Have compareImageDestinationManifestEqual return a *copySingleImageResult on a match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that the caller doesn't have to assemble it. Using a pointer-or-nil eliminates a separate boolean. Signed-off-by: Miloslav Trmač --- copy/single.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/copy/single.go b/copy/single.go index d51a8a271..9785f1eff 100644 --- a/copy/single.go +++ b/copy/single.go @@ -179,19 +179,15 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar logrus.Debugf("Checking if we can skip copying: has signatures=%t, OCI encryption=%t, no manifest updates=%t", shouldUpdateSigs, destRequiresOciEncryption, noPendingManifestUpdates) if !shouldUpdateSigs && !destRequiresOciEncryption && noPendingManifestUpdates { - isSrcDestManifestEqual, retManifest, retManifestType, retManifestDigest, err := ic.compareImageDestinationManifestEqual(ctx, targetInstance) + matchedResult, err := ic.compareImageDestinationManifestEqual(ctx, targetInstance) if err != nil { logrus.Warnf("Failed to compare destination image manifest: %v", err) return copySingleImageResult{}, err } - if isSrcDestManifestEqual { + if matchedResult != nil { c.Printf("Skipping: image already present at destination\n") - return copySingleImageResult{ - manifest: retManifest, - manifestMIMEType: retManifestType, - manifestDigest: retManifestDigest, - }, nil + return *matchedResult, nil } } } @@ -336,37 +332,41 @@ func (ic *imageCopier) noPendingManifestUpdates() bool { } // compareImageDestinationManifestEqual compares the source and destination image manifests (reading the manifest from the -// (possibly remote) destination). Returning true and the destination's manifest, type and digest if they compare equal. -func (ic *imageCopier) compareImageDestinationManifestEqual(ctx context.Context, targetInstance *digest.Digest) (bool, []byte, string, digest.Digest, error) { +// (possibly remote) destination). If they are equal, it returns a full copySingleImageResult, nil otherwise. +func (ic *imageCopier) compareImageDestinationManifestEqual(ctx context.Context, targetInstance *digest.Digest) (*copySingleImageResult, error) { srcManifestDigest, err := manifest.Digest(ic.src.ManifestBlob) if err != nil { - return false, nil, "", "", fmt.Errorf("calculating manifest digest: %w", err) + return nil, fmt.Errorf("calculating manifest digest: %w", err) } destImageSource, err := ic.c.dest.Reference().NewImageSource(ctx, ic.c.options.DestinationCtx) if err != nil { logrus.Debugf("Unable to create destination image %s source: %v", ic.c.dest.Reference(), err) - return false, nil, "", "", nil + return nil, nil } destManifest, destManifestType, err := destImageSource.GetManifest(ctx, targetInstance) if err != nil { logrus.Debugf("Unable to get destination image %s/%s manifest: %v", destImageSource, targetInstance, err) - return false, nil, "", "", nil + return nil, nil } destManifestDigest, err := manifest.Digest(destManifest) if err != nil { - return false, nil, "", "", fmt.Errorf("calculating manifest digest: %w", err) + return nil, fmt.Errorf("calculating manifest digest: %w", err) } logrus.Debugf("Comparing source and destination manifest digests: %v vs. %v", srcManifestDigest, destManifestDigest) if srcManifestDigest != destManifestDigest { - return false, nil, "", "", nil + return nil, nil } // Destination and source manifests, types and digests should all be equivalent - return true, destManifest, destManifestType, destManifestDigest, nil + return ©SingleImageResult{ + manifest: destManifest, + manifestMIMEType: destManifestType, + manifestDigest: srcManifestDigest, + }, nil } // copyLayers copies layers from ic.src/ic.c.rawSource to dest, using and updating ic.manifestUpdates if necessary and ic.cannotModifyManifestReason == "". From 1e16fb93a0cebe214b3f4b1710fa33e8c345d7a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 19 Jul 2023 03:26:28 +0200 Subject: [PATCH 28/51] Track the manifest upload data in a wipResult variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... instead of three separate ones. Signed-off-by: Miloslav Trmač --- copy/single.go | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/copy/single.go b/copy/single.go index 9785f1eff..a0fa78c56 100644 --- a/copy/single.go +++ b/copy/single.go @@ -201,8 +201,12 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar // without actually trying to upload something and getting a types.ManifestTypeRejectedError. // So, try the preferred manifest MIME type with possibly-updated blob digests, media types, and sizes if // we're altering how they're compressed. If the process succeeds, fine… - manifestBytes, retManifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance) - retManifestType := manifestConversionPlan.preferredMIMEType + manifestBytes, manifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance) + wipResult := copySingleImageResult{ + manifest: manifestBytes, + manifestMIMEType: manifestConversionPlan.preferredMIMEType, + manifestDigest: manifestDigest, + } if err != nil { logrus.Debugf("Writing manifest using preferred type %s failed: %v", manifestConversionPlan.preferredMIMEType, err) // … if it fails, and the failure is either because the manifest is rejected by the registry, or @@ -240,9 +244,11 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar } // We have successfully uploaded a manifest. - manifestBytes = attemptedManifest - retManifestDigest = attemptedManifestDigest - retManifestType = manifestMIMEType + wipResult = copySingleImageResult{ + manifest: attemptedManifest, + manifestMIMEType: manifestMIMEType, + manifestDigest: attemptedManifestDigest, + } errs = nil // Mark this as a success so that we don't abort below. break } @@ -251,10 +257,10 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar } } if targetInstance != nil { - targetInstance = &retManifestDigest + targetInstance = &wipResult.manifestDigest } - newSigs, err := c.createSignatures(ctx, manifestBytes, c.options.SignIdentity) + newSigs, err := c.createSignatures(ctx, wipResult.manifest, c.options.SignIdentity) if err != nil { return copySingleImageResult{}, err } @@ -267,11 +273,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar } } - return copySingleImageResult{ - manifest: manifestBytes, - manifestMIMEType: retManifestType, - manifestDigest: retManifestDigest, - }, nil + res := wipResult // We are done + return res, nil } // checkImageDestinationForCurrentRuntime enforces dest.MustMatchRuntimeOS, if necessary. From 94b2c0981ead8f961d99f3cc47b4fcfbd260ce07 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Tue, 18 Jul 2023 12:46:23 +0530 Subject: [PATCH 29/51] oci_index: init Annotations if nil and required If `UpdateCompressionAlgorithms` is set then `Annotations` map must be initialized if its not set. Signed-off-by: Aditya R --- internal/manifest/oci_index.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/manifest/oci_index.go b/internal/manifest/oci_index.go index cebca5a3a..3038d8124 100644 --- a/internal/manifest/oci_index.go +++ b/internal/manifest/oci_index.go @@ -94,14 +94,17 @@ func annotationsToCompressionAlgorithmNames(annotations map[string]string) []str return result } -func addCompressionAnnotations(compressionAlgorithms []compression.Algorithm, annotationsMap map[string]string) { +func addCompressionAnnotations(compressionAlgorithms []compression.Algorithm, annotationsMap *map[string]string) { // TODO: This should also delete the algorithm if map already contains an algorithm and compressionAlgorithm // list has a different algorithm. To do that, we would need to modify the callers to always provide a reliable // and full compressionAlghorithms list. + if *annotationsMap == nil && len(compressionAlgorithms) > 0 { + *annotationsMap = map[string]string{} + } for _, algo := range compressionAlgorithms { switch algo.Name() { case compression.ZstdAlgorithmName: - annotationsMap[OCI1InstanceAnnotationCompressionZSTD] = OCI1InstanceAnnotationCompressionZSTDValue + (*annotationsMap)[OCI1InstanceAnnotationCompressionZSTD] = OCI1InstanceAnnotationCompressionZSTDValue default: continue } @@ -146,13 +149,13 @@ func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit) error { maps.Copy(index.Manifests[targetIndex].Annotations, editInstance.UpdateAnnotations) } } - addCompressionAnnotations(editInstance.UpdateCompressionAlgorithms, index.Manifests[targetIndex].Annotations) + addCompressionAnnotations(editInstance.UpdateCompressionAlgorithms, &index.Manifests[targetIndex].Annotations) case ListOpAdd: annotations := map[string]string{} if editInstance.AddAnnotations != nil { annotations = maps.Clone(editInstance.AddAnnotations) } - addCompressionAnnotations(editInstance.AddCompressionAlgorithms, annotations) + addCompressionAnnotations(editInstance.AddCompressionAlgorithms, &annotations) addedEntries = append(addedEntries, imgspecv1.Descriptor{ MediaType: editInstance.AddMediaType, Size: editInstance.AddSize, From e93a2814eb0e0bb28f597e339d9eb293d2dc8f99 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Tue, 18 Jul 2023 12:48:38 +0530 Subject: [PATCH 30/51] copy/single: set requiredCompression if compressionFormat is changed Modifies `copy/single` to correctly use the feature from https://github.com/containers/image/pull/2023, that is if compression is changed blob must be resued only if it matches required compression. Signed-off-by: Aditya R --- copy/single.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/copy/single.go b/copy/single.go index a0fa78c56..1aa5a7263 100644 --- a/copy/single.go +++ b/copy/single.go @@ -37,6 +37,7 @@ type imageCopier struct { canSubstituteBlobs bool compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil. compressionLevel *int + requireCompressionFormatMatch bool } // copySingleImageResult carries data produced by copySingleImage @@ -177,8 +178,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar shouldUpdateSigs := len(sigs) > 0 || len(c.signers) != 0 // TODO: Consider allowing signatures updates only and skipping the image's layers/manifest copy if possible noPendingManifestUpdates := ic.noPendingManifestUpdates() - logrus.Debugf("Checking if we can skip copying: has signatures=%t, OCI encryption=%t, no manifest updates=%t", shouldUpdateSigs, destRequiresOciEncryption, noPendingManifestUpdates) - if !shouldUpdateSigs && !destRequiresOciEncryption && noPendingManifestUpdates { + logrus.Debugf("Checking if we can skip copying: has signatures=%t, OCI encryption=%t, no manifest updates=%t, compression match required for resuing blobs=%t", shouldUpdateSigs, destRequiresOciEncryption, noPendingManifestUpdates, requireCompressionFormatMatch) + if !shouldUpdateSigs && !destRequiresOciEncryption && noPendingManifestUpdates && !ic.requireCompressionFormatMatch { matchedResult, err := ic.compareImageDestinationManifestEqual(ctx, targetInstance) if err != nil { logrus.Warnf("Failed to compare destination image manifest: %v", err) @@ -638,12 +639,20 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to // 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 + var originalCompression *compressiontypes.Algorithm + if ic.requireCompressionFormatMatch { + requiredCompression = ic.compressionFormat + originalCompression = srcInfo.CompressionAlgorithm + } reused, reusedBlob, err := ic.c.dest.TryReusingBlobWithOptions(ctx, srcInfo, private.TryReusingBlobOptions{ - Cache: ic.c.blobInfoCache, - CanSubstitute: canSubstitute, - EmptyLayer: emptyLayer, - LayerIndex: &layerIndex, - SrcRef: srcRef, + Cache: ic.c.blobInfoCache, + CanSubstitute: canSubstitute, + EmptyLayer: emptyLayer, + LayerIndex: &layerIndex, + SrcRef: srcRef, + RequiredCompression: requiredCompression, + OriginalCompression: originalCompression, }) if err != nil { return types.BlobInfo{}, "", fmt.Errorf("trying to reuse blob %s at destination: %w", srcInfo.Digest, err) From 54d86a5215a2b1467c008a3d84e7399a665b67f9 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 20 Jul 2023 11:11:23 +0530 Subject: [PATCH 31/51] copy/single: accept requireCompressionFormatMatch for imagecopier imagecopier implementes feature for requireCompressionFormatMatch refactor `copySingleImage` to accept that as an argument. Signed-off-by: Aditya R --- copy/copy.go | 4 ++-- copy/multiple.go | 2 +- copy/single.go | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index fb99161d7..53ae20cc7 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -251,7 +251,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil) + single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, false) if err != nil { return nil, err } @@ -274,7 +274,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - single, err := c.copySingleImage(ctx, unparsedInstance, nil) + single, err := c.copySingleImage(ctx, unparsedInstance, nil, false) if err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } diff --git a/copy/multiple.go b/copy/multiple.go index e221f0028..eaba7f79a 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -128,7 +128,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, false) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } diff --git a/copy/single.go b/copy/single.go index 1aa5a7263..9b8c4d76c 100644 --- a/copy/single.go +++ b/copy/single.go @@ -49,7 +49,7 @@ type copySingleImageResult struct { // copySingleImage copies a single (non-manifest-list) image unparsedImage, using c.policyContext to validate // source image admissibility. -func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest) (copySingleImageResult, error) { +func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest, requireCompressionFormatMatch bool) (copySingleImageResult, error) { // The caller is handling manifest lists; this could happen only if a manifest list contains a manifest list. // Make sure we fail cleanly in such cases. multiImage, err := isMultiImage(ctx, unparsedImage) @@ -129,7 +129,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar manifestUpdates: &types.ManifestUpdateOptions{InformationOnly: types.ManifestUpdateInformation{Destination: c.dest}}, src: src, // diffIDsAreNeeded is computed later - cannotModifyManifestReason: cannotModifyManifestReason, + cannotModifyManifestReason: cannotModifyManifestReason, + requireCompressionFormatMatch: requireCompressionFormatMatch, } if c.options.DestinationCtx != nil { // Note that compressionFormat and compressionLevel can be nil. From 98cd2aa768a3cf1c7baa8d177abd8e41a3c86b6e Mon Sep 17 00:00:00 2001 From: Aditya R Date: Tue, 18 Jul 2023 12:54:43 +0530 Subject: [PATCH 32/51] copy/multiple: instanceCopyCopy honor UpdateCompressionAlgorithms Users can set compression formats while performing `copy` in such cases `instanceCopyCopy` must honor UpdateCompressionAlgorithms. Signed-off-by: Aditya R --- copy/multiple.go | 11 ++--- copy/single.go | 105 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 78 insertions(+), 38 deletions(-) diff --git a/copy/multiple.go b/copy/multiple.go index eaba7f79a..521383cda 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -134,11 +134,12 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, } // Record the result of a possible conversion here. instanceEdits = append(instanceEdits, internalManifest.ListEdit{ - ListOperation: internalManifest.ListOpUpdate, - UpdateOldDigest: instance.sourceDigest, - UpdateDigest: updated.manifestDigest, - UpdateSize: int64(len(updated.manifest)), - UpdateMediaType: updated.manifestMIMEType}) + ListOperation: internalManifest.ListOpUpdate, + UpdateOldDigest: instance.sourceDigest, + UpdateDigest: updated.manifestDigest, + UpdateSize: int64(len(updated.manifest)), + UpdateCompressionAlgorithms: updated.compressionAlgorithms, + UpdateMediaType: updated.manifestMIMEType}) default: return nil, fmt.Errorf("copying image: invalid copy operation %d", instance.op) } diff --git a/copy/single.go b/copy/single.go index 9b8c4d76c..d00b37da3 100644 --- a/copy/single.go +++ b/copy/single.go @@ -29,22 +29,23 @@ import ( // imageCopier tracks state specific to a single image (possibly an item of a manifest list) type imageCopier struct { - c *copier - manifestUpdates *types.ManifestUpdateOptions - src *image.SourcedImage - diffIDsAreNeeded bool - cannotModifyManifestReason string // The reason the manifest cannot be modified, or an empty string if it can - canSubstituteBlobs bool - compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil. - compressionLevel *int + c *copier + manifestUpdates *types.ManifestUpdateOptions + src *image.SourcedImage + diffIDsAreNeeded bool + cannotModifyManifestReason string // The reason the manifest cannot be modified, or an empty string if it can + canSubstituteBlobs bool + compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil. + compressionLevel *int requireCompressionFormatMatch bool } // copySingleImageResult carries data produced by copySingleImage type copySingleImageResult struct { - manifest []byte - manifestMIMEType string - manifestDigest digest.Digest + manifest []byte + manifestMIMEType string + manifestDigest digest.Digest + compressionAlgorithms []compressiontypes.Algorithm } // copySingleImage copies a single (non-manifest-list) image unparsedImage, using c.policyContext to validate @@ -194,7 +195,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar } } - if err := ic.copyLayers(ctx); err != nil { + compressionAlgos, err := ic.copyLayers(ctx) + if err != nil { return copySingleImageResult{}, err } @@ -274,7 +276,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar return copySingleImageResult{}, fmt.Errorf("writing signatures: %w", err) } } - + wipResult.compressionAlgorithms = compressionAlgos res := wipResult // We are done return res, nil } @@ -366,26 +368,38 @@ func (ic *imageCopier) compareImageDestinationManifestEqual(ctx context.Context, return nil, nil } + compressionAlgos := set.New[string]() + for _, srcInfo := range ic.src.LayerInfos() { + compression := compressionAlgorithmFromMIMEType(srcInfo) + compressionAlgos.Add(compression.Name()) + } + + algos, err := algorithmsByNames(compressionAlgos.Values()) + if err != nil { + return nil, err + } + // Destination and source manifests, types and digests should all be equivalent return ©SingleImageResult{ - manifest: destManifest, - manifestMIMEType: destManifestType, - manifestDigest: srcManifestDigest, + manifest: destManifest, + manifestMIMEType: destManifestType, + manifestDigest: srcManifestDigest, + compressionAlgorithms: algos, }, nil } // copyLayers copies layers from ic.src/ic.c.rawSource to dest, using and updating ic.manifestUpdates if necessary and ic.cannotModifyManifestReason == "". -func (ic *imageCopier) copyLayers(ctx context.Context) error { +func (ic *imageCopier) copyLayers(ctx context.Context) ([]compressiontypes.Algorithm, error) { srcInfos := ic.src.LayerInfos() numLayers := len(srcInfos) updatedSrcInfos, err := ic.src.LayerInfosForCopy(ctx) if err != nil { - return err + return nil, err } srcInfosUpdated := false if updatedSrcInfos != nil && !reflect.DeepEqual(srcInfos, updatedSrcInfos) { if ic.cannotModifyManifestReason != "" { - return fmt.Errorf("Copying this image would require changing layer representation, which we cannot do: %q", ic.cannotModifyManifestReason) + return nil, fmt.Errorf("Copying this image would require changing layer representation, which we cannot do: %q", ic.cannotModifyManifestReason) } srcInfos = updatedSrcInfos srcInfosUpdated = true @@ -401,7 +415,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // layer is empty. man, err := manifest.FromBlob(ic.src.ManifestBlob, ic.src.ManifestMIMEType) if err != nil { - return err + return nil, err } manifestLayerInfos := man.LayerInfos() @@ -467,14 +481,18 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // A call to copyGroup.Wait() is done at this point by the defer above. return nil }(); err != nil { - return err + return nil, err } + compressionAlgos := set.New[string]() destInfos := make([]types.BlobInfo, numLayers) diffIDs := make([]digest.Digest, numLayers) for i, cld := range data { if cld.err != nil { - return cld.err + return nil, cld.err + } + if cld.destInfo.CompressionAlgorithm != nil { + compressionAlgos.Add(cld.destInfo.CompressionAlgorithm.Name()) } destInfos[i] = cld.destInfo diffIDs[i] = cld.diffID @@ -489,7 +507,11 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { if srcInfosUpdated || layerDigestsDiffer(srcInfos, destInfos) { ic.manifestUpdates.LayerInfos = destInfos } - return nil + algos, err := algorithmsByNames(compressionAlgos.Values()) + if err != nil { + return nil, err + } + return algos, nil } // layerDigestsDiffer returns true iff the digests in a and b differ (ignoring sizes and possible other fields) @@ -594,6 +616,19 @@ type diffIDResult struct { err error } +func compressionAlgorithmFromMIMEType(srcInfo types.BlobInfo) *compressiontypes.Algorithm { + // This MIME type → compression mapping belongs in manifest-specific code in our manifest + // package (but we should preferably replace/change UpdatedImage instead of productizing + // this workaround). + switch srcInfo.MediaType { + case manifest.DockerV2Schema2LayerMediaType, imgspecv1.MediaTypeImageLayerGzip: + return &compression.Gzip + case imgspecv1.MediaTypeImageLayerZstd: + return &compression.Zstd + } + return nil +} + // copyLayer copies a layer with srcInfo (with known Digest and Annotations and possibly known Size) in src to dest, perhaps (de/re/)compressing it, // and returns a complete blobInfo of the copied layer, and a value for LayerDiffIDs if diffIDIsNeeded // srcRef can be used as an additional hint to the destination during checking whether a layer can be reused but srcRef can be nil. @@ -605,17 +640,8 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to // which uses the compression information to compute the updated MediaType values. // (Sadly UpdatedImage() is documented to not update MediaTypes from // ManifestUpdateOptions.LayerInfos[].MediaType, so we are doing it indirectly.) - // - // This MIME type → compression mapping belongs in manifest-specific code in our manifest - // package (but we should preferably replace/change UpdatedImage instead of productizing - // this workaround). if srcInfo.CompressionAlgorithm == nil { - switch srcInfo.MediaType { - case manifest.DockerV2Schema2LayerMediaType, imgspecv1.MediaTypeImageLayerGzip: - srcInfo.CompressionAlgorithm = &compression.Gzip - case imgspecv1.MediaTypeImageLayerZstd: - srcInfo.CompressionAlgorithm = &compression.Zstd - } + srcInfo.CompressionAlgorithm = compressionAlgorithmFromMIMEType(srcInfo) } ic.c.printCopyInfo("blob", srcInfo) @@ -843,3 +869,16 @@ func computeDiffID(stream io.Reader, decompressor compressiontypes.DecompressorF return digest.Canonical.FromReader(stream) } + +// algorithmsByNames returns slice of Algorithms from slice of Algorithm Names +func algorithmsByNames(names []string) ([]compressiontypes.Algorithm, error) { + result := []compressiontypes.Algorithm{} + for _, name := range names { + algo, err := compression.AlgorithmByName(name) + if err != nil { + return nil, err + } + result = append(result, algo) + } + return result, nil +} From a650a204ab1adb1211aa598f7e500a44bd3d609d Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 20 Jul 2023 10:55:55 +0530 Subject: [PATCH 33/51] internal/set: verify if no duplicates in set Verifies if no duplicates are kept when `.Values()` is used to return slice. Signed-off-by: Aditya R --- internal/set/set_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/set/set_test.go b/internal/set/set_test.go index f56873070..82a245052 100644 --- a/internal/set/set_test.go +++ b/internal/set/set_test.go @@ -25,6 +25,8 @@ func TestAdd(t *testing.T) { assert.True(t, s.Contains(2)) s.Add(2) // Adding an already-present element assert.True(t, s.Contains(2)) + // should not contain duplicate value of `2` + assert.ElementsMatch(t, []int{1, 2}, s.Values()) // Unrelated elements are unaffected assert.True(t, s.Contains(1)) assert.False(t, s.Contains(3)) @@ -62,5 +64,7 @@ func TestValues(t *testing.T) { assert.Empty(t, s.Values()) s.Add(1) s.Add(2) + // ignore duplicate + s.Add(2) assert.ElementsMatch(t, []int{1, 2}, s.Values()) } From 7521a10e12f482065536d695ff09c3fea462bb1e Mon Sep 17 00:00:00 2001 From: Aditya R Date: Fri, 21 Jul 2023 23:42:20 +0530 Subject: [PATCH 34/51] copy/single: wrap arguments in copySingleImageOptions Create a wrapper around arguments of `copySingleImage` Signed-off-by: Aditya R --- copy/copy.go | 7 ++++--- copy/multiple.go | 3 ++- copy/single.go | 10 +++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 53ae20cc7..cef058294 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -251,7 +251,8 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, false) + singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false} + single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, singleImageOpts) if err != nil { return nil, err } @@ -273,8 +274,8 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - - single, err := c.copySingleImage(ctx, unparsedInstance, nil, false) + singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false} + single, err := c.copySingleImage(ctx, unparsedInstance, nil, singleImageOpts) if err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } diff --git a/copy/multiple.go b/copy/multiple.go index 521383cda..45d9c6d16 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -128,7 +128,8 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, false) + singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false} + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, singleImageOpts) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } diff --git a/copy/single.go b/copy/single.go index d00b37da3..9bea50e1e 100644 --- a/copy/single.go +++ b/copy/single.go @@ -40,6 +40,10 @@ type imageCopier struct { requireCompressionFormatMatch bool } +type copySingleImageOptions struct { + requireCompressionFormatMatch bool +} + // copySingleImageResult carries data produced by copySingleImage type copySingleImageResult struct { manifest []byte @@ -50,7 +54,7 @@ type copySingleImageResult struct { // copySingleImage copies a single (non-manifest-list) image unparsedImage, using c.policyContext to validate // source image admissibility. -func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest, requireCompressionFormatMatch bool) (copySingleImageResult, error) { +func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.UnparsedImage, targetInstance *digest.Digest, opts copySingleImageOptions) (copySingleImageResult, error) { // The caller is handling manifest lists; this could happen only if a manifest list contains a manifest list. // Make sure we fail cleanly in such cases. multiImage, err := isMultiImage(ctx, unparsedImage) @@ -131,7 +135,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar src: src, // diffIDsAreNeeded is computed later cannotModifyManifestReason: cannotModifyManifestReason, - requireCompressionFormatMatch: requireCompressionFormatMatch, + requireCompressionFormatMatch: opts.requireCompressionFormatMatch, } if c.options.DestinationCtx != nil { // Note that compressionFormat and compressionLevel can be nil. @@ -180,7 +184,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar shouldUpdateSigs := len(sigs) > 0 || len(c.signers) != 0 // TODO: Consider allowing signatures updates only and skipping the image's layers/manifest copy if possible noPendingManifestUpdates := ic.noPendingManifestUpdates() - logrus.Debugf("Checking if we can skip copying: has signatures=%t, OCI encryption=%t, no manifest updates=%t, compression match required for resuing blobs=%t", shouldUpdateSigs, destRequiresOciEncryption, noPendingManifestUpdates, requireCompressionFormatMatch) + logrus.Debugf("Checking if we can skip copying: has signatures=%t, OCI encryption=%t, no manifest updates=%t, compression match required for resuing blobs=%t", shouldUpdateSigs, destRequiresOciEncryption, noPendingManifestUpdates, opts.requireCompressionFormatMatch) if !shouldUpdateSigs && !destRequiresOciEncryption && noPendingManifestUpdates && !ic.requireCompressionFormatMatch { matchedResult, err := ic.compareImageDestinationManifestEqual(ctx, targetInstance) if err != nil { From 2a961fd697c74031d1ca27cd10d3646812f06532 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 00:11:14 +0530 Subject: [PATCH 35/51] copy/single: add custom compressionFormat, compressionLevel fields After #2048 there is no room for copy/multiple to pass custom compressionFormat to copySingleImage while processing each instance, following PR introduces that functionality again and wraps options to simpler struct. Signed-off-by: Aditya R --- copy/copy.go | 6 ++---- copy/multiple.go | 3 +-- copy/single.go | 6 ++++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index cef058294..f36883ad9 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -251,8 +251,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, if !multiImage { // The simple case: just copy a single image. - singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false} - single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, singleImageOpts) + single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, copySingleImageOptions{requireCompressionFormatMatch: false}) if err != nil { return nil, err } @@ -274,8 +273,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } logrus.Debugf("Source is a manifest list; copying (only) instance %s for current system", instanceDigest) unparsedInstance := image.UnparsedInstance(rawSource, &instanceDigest) - singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false} - single, err := c.copySingleImage(ctx, unparsedInstance, nil, singleImageOpts) + single, err := c.copySingleImage(ctx, unparsedInstance, nil, copySingleImageOptions{requireCompressionFormatMatch: false}) if err != nil { return nil, fmt.Errorf("copying system image from manifest list: %w", err) } diff --git a/copy/multiple.go b/copy/multiple.go index 45d9c6d16..0482e0a9c 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -128,8 +128,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false} - updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, singleImageOpts) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, copySingleImageOptions{requireCompressionFormatMatch: false}) if err != nil { return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } diff --git a/copy/single.go b/copy/single.go index 9bea50e1e..2bd479038 100644 --- a/copy/single.go +++ b/copy/single.go @@ -42,6 +42,8 @@ type imageCopier struct { type copySingleImageOptions struct { requireCompressionFormatMatch bool + compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil. + compressionLevel *int } // copySingleImageResult carries data produced by copySingleImage @@ -139,8 +141,8 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar } if c.options.DestinationCtx != nil { // Note that compressionFormat and compressionLevel can be nil. - ic.compressionFormat = c.options.DestinationCtx.CompressionFormat - ic.compressionLevel = c.options.DestinationCtx.CompressionLevel + ic.compressionFormat = opts.compressionFormat + ic.compressionLevel = opts.compressionLevel } // Decide whether we can substitute blobs with semantic equivalents: // - Don’t do that if we can’t modify the manifest at all From 033a141ea731ed032654acfc088e06f818e7125f Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 00:31:17 +0530 Subject: [PATCH 36/51] copy/single: honor c.options.DestCtx for regular copy If its a regular copy callers might not set `compressionFormat` and `compressionLevel` in such case still honor compression which is set in DestinationCtx from copier. Signed-off-by: Aditya R --- copy/single.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/copy/single.go b/copy/single.go index 2bd479038..f40b5f2f7 100644 --- a/copy/single.go +++ b/copy/single.go @@ -139,10 +139,13 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar cannotModifyManifestReason: cannotModifyManifestReason, requireCompressionFormatMatch: opts.requireCompressionFormatMatch, } - if c.options.DestinationCtx != nil { - // Note that compressionFormat and compressionLevel can be nil. + if opts.compressionFormat != nil { ic.compressionFormat = opts.compressionFormat ic.compressionLevel = opts.compressionLevel + } else if c.options.DestinationCtx != nil { + // Note that compressionFormat and compressionLevel can be nil. + ic.compressionFormat = c.options.DestinationCtx.CompressionFormat + ic.compressionLevel = c.options.DestinationCtx.CompressionLevel } // Decide whether we can substitute blobs with semantic equivalents: // - Don’t do that if we can’t modify the manifest at all From 2d781e2efc5a4bbbc4ef4d482cf307e99ad09ca9 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Fri, 21 Jul 2023 10:15:34 +0530 Subject: [PATCH 37/51] copy: add EnsureCompressionVariantExist for instanceCopyClone * copy.Options now contains a new field `EnsureCompressionVariantExists map[string]int` which allows users to specify if they want to clone images with specified `compression`. Signed-off-by: Aditya R --- copy/copy.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/copy/copy.go b/copy/copy.go index f36883ad9..f11dee5c9 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -17,6 +17,7 @@ import ( "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/manifest" "github.com/containers/image/v5/pkg/blobinfocache" + compression "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/signature" "github.com/containers/image/v5/signature/signer" "github.com/containers/image/v5/transports" @@ -126,6 +127,21 @@ type Options struct { // Download layer contents with "nondistributable" media types ("foreign" layers) and translate the layer media type // to not indicate "nondistributable". DownloadForeignLayers bool + + // Contains slice of OptionCompressionVariant, where copy will ensure that for each platform + // in the manifest list, a variant with the requested compression will exist. + // Invalid when copying a non-multi-architecture image. That will probably + // change in the future. + EnsureCompressionVariantsExist []OptionCompressionVariant +} + +// OptionCompressionVariant allows to supply information about +// selected compression algorithm and compression level by the +// end-user. Refer to EnsureCompressionVariantsExist to know +// more about its usage. +type OptionCompressionVariant struct { + Algorithm compression.Algorithm + Level *int // Only used when we are creating a new image instance using the specified algorithm, not when the image already contains such an instance } // copier allows us to keep track of diffID values for blobs, and other From 01a09cc94eeeee77b95699afa1a3143799d23403 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Fri, 21 Jul 2023 14:44:06 +0530 Subject: [PATCH 38/51] internal/set: implement AddSlice for easier syntax Signed-off-by: Aditya R --- internal/set/set.go | 6 ++++++ internal/set/set_test.go | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/internal/set/set.go b/internal/set/set.go index 3e777fe12..acf30343e 100644 --- a/internal/set/set.go +++ b/internal/set/set.go @@ -28,6 +28,12 @@ func (s *Set[E]) Add(v E) { s.m[v] = struct{}{} // Possibly writing the same struct{}{} presence marker again. } +func (s *Set[E]) AddSlice(slice []E) { + for _, v := range slice { + s.Add(v) + } +} + func (s *Set[E]) Delete(v E) { delete(s.m, v) } diff --git a/internal/set/set_test.go b/internal/set/set_test.go index 82a245052..3e704a95b 100644 --- a/internal/set/set_test.go +++ b/internal/set/set_test.go @@ -32,6 +32,13 @@ func TestAdd(t *testing.T) { assert.False(t, s.Contains(3)) } +func TestAddSlice(t *testing.T) { + s := NewWithValues(1) + s.Add(2) + s.AddSlice([]int{3, 4}) + assert.ElementsMatch(t, []int{1, 2, 3, 4}, s.Values()) +} + func TestDelete(t *testing.T) { s := NewWithValues(1, 2) assert.True(t, s.Contains(2)) From 4b86fae1a59c1970fc58444359a1b1cfe134002c Mon Sep 17 00:00:00 2001 From: Aditya R Date: Fri, 21 Jul 2023 14:41:45 +0530 Subject: [PATCH 39/51] copy/multiple: implement instanceCopyClone Implement `instanceCopyClone` for multiple compression. Signed-off-by: Aditya R --- copy/multiple.go | 104 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 101 insertions(+), 3 deletions(-) diff --git a/copy/multiple.go b/copy/multiple.go index 0482e0a9c..b179b763f 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -5,15 +5,18 @@ import ( "context" "errors" "fmt" + "sort" "strings" "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/internal/image" internalManifest "github.com/containers/image/v5/internal/manifest" + "github.com/containers/image/v5/internal/set" "github.com/containers/image/v5/manifest" digest "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" + "golang.org/x/exp/maps" "golang.org/x/exp/slices" ) @@ -27,23 +30,97 @@ const ( type instanceCopy struct { op instanceCopyKind sourceDigest digest.Digest + + // Fields which can be used by callers when operation + // is `instanceCopyClone` + cloneCompressionVariant OptionCompressionVariant + clonePlatform *imgspecv1.Platform + cloneAnnotations map[string]string +} + +// internal type only to make imgspecv1.Platform comparable +type platformComparable struct { + architecture string + os string + osVersion string + osFeatures string + variant string +} + +// Converts imgspecv1.Platform to a comparable format. +func platformV1ToPlatformComparable(platform *imgspecv1.Platform) platformComparable { + if platform == nil { + return platformComparable{} + } + osFeatures := slices.Clone(platform.OSFeatures) + sort.Strings(osFeatures) + return platformComparable{architecture: platform.Architecture, + os: platform.OS, + // This is strictly speaking ambiguous, fields of OSFeatures can contain a ','. Probably good enough for now. + osFeatures: strings.Join(osFeatures, ","), + osVersion: platform.OSVersion, + variant: platform.Variant, + } +} + +// platformCompressionMap prepares a mapping of platformComparable -> CompressionAlgorithmNames for given digests +func platformCompressionMap(list internalManifest.List, instanceDigests []digest.Digest) (map[platformComparable]*set.Set[string], error) { + res := make(map[platformComparable]*set.Set[string]) + for _, instanceDigest := range instanceDigests { + instanceDetails, err := list.Instance(instanceDigest) + if err != nil { + return nil, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err) + } + platform := platformV1ToPlatformComparable(instanceDetails.ReadOnly.Platform) + platformSet, ok := res[platform] + if !ok { + platformSet = set.New[string]() + res[platform] = platformSet + } + platformSet.AddSlice(instanceDetails.ReadOnly.CompressionAlgorithmNames) + } + return res, nil } // prepareInstanceCopies prepares a list of instances which needs to copied to the manifest list. -func prepareInstanceCopies(instanceDigests []digest.Digest, options *Options) []instanceCopy { +func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, error) { res := []instanceCopy{} + compressionsByPlatform, err := platformCompressionMap(list, instanceDigests) + if err != nil { + return nil, err + } for i, instanceDigest := range instanceDigests { if options.ImageListSelection == CopySpecificImages && !slices.Contains(options.Instances, instanceDigest) { logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests)) continue } + instanceDetails, err := list.Instance(instanceDigest) + if err != nil { + return res, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err) + } + platform := platformV1ToPlatformComparable(instanceDetails.ReadOnly.Platform) + compressionList := compressionsByPlatform[platform] + for _, compressionVariant := range options.EnsureCompressionVariantsExist { + if !compressionList.Contains(compressionVariant.Algorithm.Name()) { + res = append(res, instanceCopy{ + op: instanceCopyClone, + sourceDigest: instanceDigest, + cloneCompressionVariant: compressionVariant, + clonePlatform: instanceDetails.ReadOnly.Platform, + cloneAnnotations: maps.Clone(instanceDetails.ReadOnly.Annotations), + }) + // add current compression to the list so logic acks any + // duplicates while processing other instances + compressionList.Add(compressionVariant.Algorithm.Name()) + } + } res = append(res, instanceCopy{ op: instanceCopyCopy, sourceDigest: instanceDigest, }) } - return res + return res, nil } // copyMultipleImages copies some or all of an image list's instances, using @@ -118,7 +195,10 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, // Copy each image, or just the ones we want to copy, in turn. instanceDigests := updatedList.Instances() instanceEdits := []internalManifest.ListEdit{} - instanceCopyList := prepareInstanceCopies(instanceDigests, c.options) + instanceCopyList, err := prepareInstanceCopies(updatedList, instanceDigests, c.options) + if err != nil { + return nil, fmt.Errorf("preparing instances for copy: %w", err) + } c.Printf("Copying %d of %d images in list\n", len(instanceCopyList), len(instanceDigests)) for i, instance := range instanceCopyList { // Update instances to be edited by their `ListOperation` and @@ -140,6 +220,24 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, UpdateSize: int64(len(updated.manifest)), UpdateCompressionAlgorithms: updated.compressionAlgorithms, UpdateMediaType: updated.manifestMIMEType}) + case instanceCopyClone: + logrus.Debugf("Replicating instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) + c.Printf("Replicating image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) + unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, copySingleImageOptions{requireCompressionFormatMatch: true}) + if err != nil { + return nil, fmt.Errorf("replicating image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) + } + // Record the result of a possible conversion here. + instanceEdits = append(instanceEdits, internalManifest.ListEdit{ + ListOperation: internalManifest.ListOpAdd, + AddDigest: updated.manifestDigest, + AddSize: int64(len(updated.manifest)), + AddMediaType: updated.manifestMIMEType, + AddPlatform: instance.clonePlatform, + AddAnnotations: instance.cloneAnnotations, + AddCompressionAlgorithms: updated.compressionAlgorithms, + }) default: return nil, fmt.Errorf("copying image: invalid copy operation %d", instance.op) } From a269ca9088cf43a27aad9b12fdc73deee3cbb5f6 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 12:51:19 +0530 Subject: [PATCH 40/51] copy/multiple: instanceCopyClone set custom compression While performing copy, set a custom compression passed generated from prepareInstanceCopies. Signed-off-by: Aditya R --- copy/multiple.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/copy/multiple.go b/copy/multiple.go index b179b763f..937f89263 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -110,8 +110,7 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest. clonePlatform: instanceDetails.ReadOnly.Platform, cloneAnnotations: maps.Clone(instanceDetails.ReadOnly.Annotations), }) - // add current compression to the list so logic acks any - // duplicates while processing other instances + // add current compression to the list so that we don’t create duplicate clones compressionList.Add(compressionVariant.Algorithm.Name()) } } @@ -224,7 +223,10 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, logrus.Debugf("Replicating instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList)) c.Printf("Replicating image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList)) unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest) - updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, copySingleImageOptions{requireCompressionFormatMatch: true}) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, copySingleImageOptions{ + requireCompressionFormatMatch: true, + compressionFormat: &instance.cloneCompressionVariant.Algorithm, + compressionLevel: instance.cloneCompressionVariant.Level}) if err != nil { return nil, fmt.Errorf("replicating image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) } From 174c46f18d6611b91fbc136e5680beddbdc4efa6 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 14:17:46 +0530 Subject: [PATCH 41/51] copy/multiple: error on EnsureCompressionVariant and CopySpecificImages Following option does not provides a way to detect and exclude compression if it already exists, this feature may be implemented in future. See: https://github.com/containers/image/pull/1987#discussion_r1271166856 Signed-off-by: Aditya R --- copy/multiple.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/copy/multiple.go b/copy/multiple.go index 937f89263..443aed566 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -13,6 +13,7 @@ import ( internalManifest "github.com/containers/image/v5/internal/manifest" "github.com/containers/image/v5/internal/set" "github.com/containers/image/v5/manifest" + "github.com/containers/image/v5/pkg/compression" digest "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" @@ -82,9 +83,31 @@ func platformCompressionMap(list internalManifest.List, instanceDigests []digest return res, nil } +func validateCompressionVariantExists(input []OptionCompressionVariant) error { + for _, option := range input { + _, err := compression.AlgorithmByName(option.Algorithm.Name()) + if err != nil { + return fmt.Errorf("invalid algorithm %q in option.EnsureCompressionVariantsExist: %w", option.Algorithm.Name(), err) + } + } + return nil +} + // prepareInstanceCopies prepares a list of instances which needs to copied to the manifest list. func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, error) { res := []instanceCopy{} + if options.ImageListSelection == CopySpecificImages && len(options.EnsureCompressionVariantsExist) > 0 { + // List can already contain compressed instance for a compression selected in `EnsureCompressionVariantsExist` + // It’s unclear what it means when `CopySpecificImages` includes an instance in options.Instances, + // EnsureCompressionVariantsExist asks for an instance with some compression, + // an instance with that compression already exists, but is not included in options.Instances. + // We might define the semantics and implement this in the future. + return res, fmt.Errorf("EnsureCompressionVariantsExist is not implemented for CopySpecificImages") + } + err := validateCompressionVariantExists(options.EnsureCompressionVariantsExist) + if err != nil { + return res, err + } compressionsByPlatform, err := platformCompressionMap(list, instanceDigests) if err != nil { return nil, err From 3a40cd23a0905c9adf978317eb49d69f258681ad Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 17:25:07 +0530 Subject: [PATCH 42/51] copy/multiple: report more meaningful copy msg After implementing `instanceCopyClone` now instance to be copied can exceed the original number of instances in the source, so amend report message to make it more meaningful. Signed-off-by: Aditya R --- copy/multiple.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/copy/multiple.go b/copy/multiple.go index 443aed566..751bb848f 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -221,7 +221,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, if err != nil { return nil, fmt.Errorf("preparing instances for copy: %w", err) } - c.Printf("Copying %d of %d images in list\n", len(instanceCopyList), len(instanceDigests)) + c.Printf("Copying %d images generated from %d images in list\n", len(instanceCopyList), len(instanceDigests)) for i, instance := range instanceCopyList { // Update instances to be edited by their `ListOperation` and // populate necessary fields. From cfc0078ad842fb077eb0f4a8f00526dbb4bedff6 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 12:34:37 +0530 Subject: [PATCH 43/51] copy/multiple_test: retrofit tests for instanceCopyCopy Signed-off-by: Aditya R --- copy/multiple_test.go | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/copy/multiple_test.go b/copy/multiple_test.go index 99b74f237..f76456d51 100644 --- a/copy/multiple_test.go +++ b/copy/multiple_test.go @@ -1,36 +1,44 @@ package copy import ( + "os" + "path/filepath" "testing" + internalManifest "github.com/containers/image/v5/internal/manifest" digest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestPrepareCopyInstances(t *testing.T) { +// Test `instanceCopyCopy` cases. +func TestPrepareCopyInstancesforInstanceCopyCopy(t *testing.T) { + validManifest, err := os.ReadFile(filepath.Join("..", "internal", "manifest", "testdata", "oci1.index.zstd-selection.json")) + require.NoError(t, err) + list, err := internalManifest.ListFromBlob(validManifest, internalManifest.GuessMIMEType(validManifest)) + require.NoError(t, err) + // Test CopyAllImages sourceInstances := []digest.Digest{ - digest.Digest("sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"), + digest.Digest("sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"), digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), } - instancesToCopy := prepareInstanceCopies(sourceInstances, &Options{}) + instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{}) + require.NoError(t, err) compare := []instanceCopy{} + for _, instance := range sourceInstances { compare = append(compare, instanceCopy{op: instanceCopyCopy, sourceDigest: instance}) } assert.Equal(t, instancesToCopy, compare) - // Test with CopySpecific Images - copyOnly := []digest.Digest{ - digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), - } - instancesToCopy = prepareInstanceCopies(sourceInstances, &Options{ - Instances: copyOnly, - ImageListSelection: CopySpecificImages}) - assert.Equal(t, instancesToCopy, []instanceCopy{{ - op: instanceCopyCopy, - sourceDigest: copyOnly[0]}}) + // Test CopySpecificImages where selected instance is sourceInstances[1] + instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages}) + require.NoError(t, err) + compare = []instanceCopy{{op: instanceCopyCopy, + sourceDigest: sourceInstances[1]}} + assert.Equal(t, instancesToCopy, compare) } From 70a5292ca3466103cc08c9dcf1c82096d60f180d Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 22 Jul 2023 16:40:12 +0530 Subject: [PATCH 44/51] copy/multiple_test: implement test for instanceCopyClone with helpers Signed-off-by: Aditya R --- copy/multiple_test.go | 83 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/copy/multiple_test.go b/copy/multiple_test.go index f76456d51..48b3c003a 100644 --- a/copy/multiple_test.go +++ b/copy/multiple_test.go @@ -6,6 +6,7 @@ import ( "testing" internalManifest "github.com/containers/image/v5/internal/manifest" + "github.com/containers/image/v5/pkg/compression" digest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,3 +43,85 @@ func TestPrepareCopyInstancesforInstanceCopyCopy(t *testing.T) { sourceDigest: sourceInstances[1]}} assert.Equal(t, instancesToCopy, compare) } + +// Test `instanceCopyClone` cases. +func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { + validManifest, err := os.ReadFile(filepath.Join("..", "internal", "manifest", "testdata", "oci1.index.zstd-selection.json")) + require.NoError(t, err) + list, err := internalManifest.ListFromBlob(validManifest, internalManifest.GuessMIMEType(validManifest)) + require.NoError(t, err) + + // Prepare option for `instanceCopyClone` case. + ensureCompressionVariantsExist := []OptionCompressionVariant{{Algorithm: compression.Zstd}} + + sourceInstances := []digest.Digest{ + digest.Digest("sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"), + digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), + } + + // CopySpecificImage must fail with error + _, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist, + Instances: []digest.Digest{sourceInstances[1]}, + ImageListSelection: CopySpecificImages}) + require.EqualError(t, err, "EnsureCompressionVariantsExist is not implemented for CopySpecificImages") + + // Test copying all images with replication + instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}) + require.NoError(t, err) + + // Following test ensures + // * Still copy gzip variants if they exist in the original + // * Not create new Zstd variants if they exist in the original. + + // We crated a list of three instances `sourceInstances` and since in oci1.index.zstd-selection.json + // amd64 already has a zstd instance i.e sourceInstance[1] so it should not create replication for + // `sourceInstance[0]` and `sourceInstance[1]` but should do it for `sourceInstance[2]` for `arm64` + // and still copy `sourceInstance[2]`. + assert.Equal(t, 4, len(instancesToCopy)) + expectedResponse := []simplerInstanceCopy{} + for _, instance := range sourceInstances { + // If its `arm64` and sourceDigest[2] , expect a clone to happen + if instance == sourceInstances[2] { + expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyClone, sourceDigest: instance, cloneCompressionVariant: "zstd", clonePlatform: "arm64-linux-"}) + } + expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyCopy, + sourceDigest: instance}) + } + actualResponse := convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) + assert.Equal(t, expectedResponse, actualResponse) + +} + +// simpler version of `instanceCopy` for testing where fields are string +// instead of pointer +type simplerInstanceCopy struct { + op instanceCopyKind + sourceDigest digest.Digest + + // Fields which can be used by callers when operation + // is `instanceCopyClone` + cloneCompressionVariant string + clonePlatform string + cloneAnnotations map[string]string +} + +func convertInstanceCopyToSimplerInstanceCopy(copies []instanceCopy) []simplerInstanceCopy { + res := []simplerInstanceCopy{} + for _, instance := range copies { + compression := "" + platform := "" + compression = instance.cloneCompressionVariant.Algorithm.Name() + if instance.clonePlatform != nil { + platform = instance.clonePlatform.Architecture + "-" + instance.clonePlatform.OS + "-" + instance.clonePlatform.Variant + } + res = append(res, simplerInstanceCopy{ + op: instance.op, + sourceDigest: instance.sourceDigest, + cloneCompressionVariant: compression, + clonePlatform: platform, + cloneAnnotations: instance.cloneAnnotations, + }) + } + return res +} From 7f3eeec569ed978726c2d531173fee35e5aa4310 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Tue, 25 Jul 2023 20:52:51 +0530 Subject: [PATCH 45/51] copy/multiple_test: multiple copy requests of same compression * test multiple copy requests of same compression Signed-off-by: Aditya R --- copy/multiple_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/copy/multiple_test.go b/copy/multiple_test.go index 48b3c003a..e208f3964 100644 --- a/copy/multiple_test.go +++ b/copy/multiple_test.go @@ -78,7 +78,6 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { // amd64 already has a zstd instance i.e sourceInstance[1] so it should not create replication for // `sourceInstance[0]` and `sourceInstance[1]` but should do it for `sourceInstance[2]` for `arm64` // and still copy `sourceInstance[2]`. - assert.Equal(t, 4, len(instancesToCopy)) expectedResponse := []simplerInstanceCopy{} for _, instance := range sourceInstances { // If its `arm64` and sourceDigest[2] , expect a clone to happen @@ -91,6 +90,22 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { actualResponse := convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) assert.Equal(t, expectedResponse, actualResponse) + // Test option with multiple copy request for same compression format + // above expection should stay same, if out ensureCompressionVariantsExist requests zstd twice + ensureCompressionVariantsExist = []OptionCompressionVariant{{Algorithm: compression.Zstd}, {Algorithm: compression.Zstd}} + instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}) + require.NoError(t, err) + expectedResponse = []simplerInstanceCopy{} + for _, instance := range sourceInstances { + // If its `arm64` and sourceDigest[2] , expect a clone to happen + if instance == sourceInstances[2] { + expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyClone, sourceDigest: instance, cloneCompressionVariant: "zstd", clonePlatform: "arm64-linux-"}) + } + expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyCopy, + sourceDigest: instance}) + } + actualResponse = convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) + assert.Equal(t, expectedResponse, actualResponse) } // simpler version of `instanceCopy` for testing where fields are string From 5defa5132b24ae25829da7563795faa0529ae963 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Wed, 26 Jul 2023 00:21:17 +0530 Subject: [PATCH 46/51] copy/multiple_test: clone must happen once for duplicate entries Signed-off-by: Aditya R --- copy/multiple_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/copy/multiple_test.go b/copy/multiple_test.go index e208f3964..4dad5cd84 100644 --- a/copy/multiple_test.go +++ b/copy/multiple_test.go @@ -106,6 +106,23 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { } actualResponse = convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) assert.Equal(t, expectedResponse, actualResponse) + + // Add same instance twice but clone must appear only once. + ensureCompressionVariantsExist = []OptionCompressionVariant{{Algorithm: compression.Zstd}} + sourceInstances = []digest.Digest{ + digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), + digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), + } + instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}) + require.NoError(t, err) + // two copies but clone should happen only once + numberOfCopyClone := 0 + for _, instance := range instancesToCopy { + if instance.op == instanceCopyClone { + numberOfCopyClone++ + } + } + assert.Equal(t, 1, numberOfCopyClone) } // simpler version of `instanceCopy` for testing where fields are string From e3cf0f2d3147a0a2290124d7d2276d66ca550f19 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Wed, 26 Jul 2023 01:43:47 +0530 Subject: [PATCH 47/51] copy/copy: fail copySingleImage cases on EnsureCompressionVariantsExist EnsureCompressionVariantsExist is only valid when working with a manifest list. Signed-off-by: Aditya R --- copy/copy.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/copy/copy.go b/copy/copy.go index f11dee5c9..ac0e6f2fa 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -266,6 +266,9 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } if !multiImage { + if len(options.EnsureCompressionVariantsExist) > 0 { + return nil, fmt.Errorf("EnsureCompressionVariantsExist is not implemented when not creating a multi-architecture image") + } // The simple case: just copy a single image. single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, copySingleImageOptions{requireCompressionFormatMatch: false}) if err != nil { @@ -273,6 +276,9 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } copiedManifest = single.manifest } else if c.options.ImageListSelection == CopySystemImage { + if len(options.EnsureCompressionVariantsExist) > 0 { + return nil, fmt.Errorf("EnsureCompressionVariantsExist is not implemented when not creating a multi-architecture image") + } // This is a manifest list, and we weren't asked to copy multiple images. Choose a single image that // matches the current system to copy, and copy it. mfest, manifestType, err := c.unparsedToplevel.Manifest(ctx) From 9eff79206f26b342ad67aa84f34210b0cafa2a03 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 27 Jul 2023 15:45:39 +0530 Subject: [PATCH 48/51] copy/multiple: priority of instanceCopyCopy must be higher When copying multiple images i.e `instanceCopyClone` and no image exists in registry in such case if clones are prepared and copied first then original copies will reuse blobs from the clone which is unexpected since argument `requireCompressionFormatMatch` is by default false for `instanceCopyCopy` case. Problem described in following PR is easily reproduceable when working with tools such as buildah, because without this PR buildah will push `clones` first instead of originals and later on `originals` will reuse blobs from `clones`. Signed-off-by: Aditya R --- copy/multiple.go | 8 ++++---- copy/multiple_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/copy/multiple.go b/copy/multiple.go index 751bb848f..34f2129d6 100644 --- a/copy/multiple.go +++ b/copy/multiple.go @@ -122,6 +122,10 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest. if err != nil { return res, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err) } + res = append(res, instanceCopy{ + op: instanceCopyCopy, + sourceDigest: instanceDigest, + }) platform := platformV1ToPlatformComparable(instanceDetails.ReadOnly.Platform) compressionList := compressionsByPlatform[platform] for _, compressionVariant := range options.EnsureCompressionVariantsExist { @@ -137,10 +141,6 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest. compressionList.Add(compressionVariant.Algorithm.Name()) } } - res = append(res, instanceCopy{ - op: instanceCopyCopy, - sourceDigest: instanceDigest, - }) } return res, nil } diff --git a/copy/multiple_test.go b/copy/multiple_test.go index 4dad5cd84..223dd274d 100644 --- a/copy/multiple_test.go +++ b/copy/multiple_test.go @@ -80,12 +80,12 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { // and still copy `sourceInstance[2]`. expectedResponse := []simplerInstanceCopy{} for _, instance := range sourceInstances { + expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyCopy, + sourceDigest: instance}) // If its `arm64` and sourceDigest[2] , expect a clone to happen if instance == sourceInstances[2] { expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyClone, sourceDigest: instance, cloneCompressionVariant: "zstd", clonePlatform: "arm64-linux-"}) } - expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyCopy, - sourceDigest: instance}) } actualResponse := convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) assert.Equal(t, expectedResponse, actualResponse) @@ -97,12 +97,12 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { require.NoError(t, err) expectedResponse = []simplerInstanceCopy{} for _, instance := range sourceInstances { + expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyCopy, + sourceDigest: instance}) // If its `arm64` and sourceDigest[2] , expect a clone to happen if instance == sourceInstances[2] { expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyClone, sourceDigest: instance, cloneCompressionVariant: "zstd", clonePlatform: "arm64-linux-"}) } - expectedResponse = append(expectedResponse, simplerInstanceCopy{op: instanceCopyCopy, - sourceDigest: instance}) } actualResponse = convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) assert.Equal(t, expectedResponse, actualResponse) From e907cf1e04dff1679358ad723ba7750fcefe00e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 5 Aug 2023 18:14:06 +0200 Subject: [PATCH 49/51] .cirrus: lock skopeo to stable version and correct dest branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Aditya R Signed-off-by: Miloslav Trmač --- .cirrus.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index af79c3f52..7116742ef 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -6,7 +6,7 @@ env: #### Global variables used for all tasks #### # Name of the ultimate destination branch for this CI run - DEST_BRANCH: "main" + DEST_BRANCH: "release-5.26" # CI container image tag (c/skopeo branch name) SKOPEO_CI_TAG: "release-1.13" # Use GO module mirror (reason unknown, travis did it this way) From 57ab99a02fb5aa65636d151a8043baa1f414ff0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 4 Aug 2023 23:06:45 +0200 Subject: [PATCH 50/51] Release 5.27.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/version.go b/version/version.go index c270910b0..584764c9a 100644 --- a/version/version.go +++ b/version/version.go @@ -11,7 +11,7 @@ const ( VersionPatch = 0 // VersionDev indicates development branch. Releases will be empty string. - VersionDev = "-dev" + VersionDev = "" ) // Version is the specification version that the package types support. From 22f70e8f263c890355fee9598c14337cabe22e2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 4 Aug 2023 23:07:16 +0200 Subject: [PATCH 51/51] Bump to v5.27.1-dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- version/version.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/version/version.go b/version/version.go index 584764c9a..638129d6f 100644 --- a/version/version.go +++ b/version/version.go @@ -8,10 +8,10 @@ const ( // VersionMinor is for functionality in a backwards-compatible manner VersionMinor = 27 // VersionPatch is for backwards-compatible bug fixes - VersionPatch = 0 + VersionPatch = 1 // VersionDev indicates development branch. Releases will be empty string. - VersionDev = "" + VersionDev = "-dev" ) // Version is the specification version that the package types support.