diff --git a/image/copy/copy.go b/image/copy/copy.go index 5d70482519..577473718b 100644 --- a/image/copy/copy.go +++ b/image/copy/copy.go @@ -77,9 +77,28 @@ const ( // specific images from the source reference. type ImageListSelection int +const ( + // KeepSparseManifestList is the default value which, when set in + // Options.SparseManifestListAction, indicates that the manifest is kept + // as is even though some images from the list may be missing. Some + // registries may not support this. + KeepSparseManifestList SparseManifestListAction = iota + + // StripSparseManifestList will strip missing images from the manifest + // list. When images are stripped the digest will differ from the original. + StripSparseManifestList +) + +// SparseManifestListAction is one of KeepSparseManifestList or StripSparseManifestList +// to control the behavior when only a subset of images from a manifest list is copied +type SparseManifestListAction int + // Options allows supplying non-default configuration modifying the behavior of CopyImage. type Options struct { RemoveSignatures bool // Remove any pre-existing signatures. Signers and SignBy… will still add a new signature. + // RemoveListSignatures removes the manifest list signature while preserving per-instance signatures. + // If RemoveSignatures is also true, RemoveSignatures takes precedence. + RemoveListSignatures bool // Signers to use to add signatures during the copy. // Callers are still responsible for closing these Signer objects; they can be reused for multiple copy.Image operations in a row. Signers []*signer.Signer @@ -102,6 +121,9 @@ type Options struct { ImageListSelection ImageListSelection // set to either CopySystemImage (the default), CopyAllImages, or CopySpecificImages to control which instances we copy when the source reference is a list; ignored if the source reference is not a list Instances []digest.Digest // if ImageListSelection is CopySpecificImages, copy only these instances, instances matching the InstancePlatforms list, and the list itself InstancePlatforms []InstancePlatformFilter // if ImageListSelection is CopySpecificImages, copy instances with matching OS/Architecture (all variants and compressions), it also copies the index/manifest_list instance. + // When only a subset of images of a list is copied, this action indicates if the manifest should be kept or stripped. + // See CopySpecificImages. + SparseManifestListAction SparseManifestListAction // Give priority to pulling gzip images if multiple images are present when configured to OptionalBoolTrue, // prefers the best compression if this is configured as OptionalBoolFalse. Choose automatically (and the choice may change over time) // if this is set to OptionalBoolUndefined (which is the default behavior, and recommended for most callers). @@ -318,30 +340,15 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, return nil, fmt.Errorf("determining manifest MIME type for %s: %w", transports.ImageName(srcRef), err) } + var singleInstance *image.UnparsedImage + var singleInstanceErrorWrapping string if !multiImage { - if len(options.EnsureCompressionVariantsExist) > 0 { - return nil, fmt.Errorf("EnsureCompressionVariantsExist is not implemented when not creating a multi-architecture image") - } - requireCompressionFormatMatch, err := shouldRequireCompressionFormatMatch(options) - if err != nil { - return nil, err - } // The simple case: just copy a single image. - single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, copySingleImageOptions{requireCompressionFormatMatch: requireCompressionFormatMatch}) - if err != nil { - return nil, err - } - copiedManifest = single.manifest + singleInstance = c.unparsedToplevel + singleInstanceErrorWrapping = "" } 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") - } - requireCompressionFormatMatch, err := shouldRequireCompressionFormatMatch(options) - if err != nil { - return nil, err - } - // 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. + // 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. mfest, manifestType, err := c.unparsedToplevel.Manifest(ctx) if err != nil { return nil, fmt.Errorf("reading manifest for %s: %w", transports.ImageName(srcRef), err) @@ -355,13 +362,30 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, return nil, fmt.Errorf("choosing an image from manifest list %s: %w", transports.ImageName(srcRef), err) } 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, copySingleImageOptions{requireCompressionFormatMatch: requireCompressionFormatMatch}) + singleInstance = image.UnparsedInstance(rawSource, &instanceDigest) + singleInstanceErrorWrapping = "copying system image from manifest list" + } // else: a multi-instance copy + + if singleInstance != nil { + if len(options.EnsureCompressionVariantsExist) > 0 { + return nil, fmt.Errorf("EnsureCompressionVariantsExist is not implemented when not creating a multi-architecture image") + } + if c.options.RemoveListSignatures { + return nil, fmt.Errorf("RemoveListSignatures is not applicable when copying a single instance") + } + requireCompressionFormatMatch, err := shouldRequireCompressionFormatMatch(options) + if err != nil { + return nil, err + } + single, err := c.copySingleImage(ctx, singleInstance, nil, copySingleImageOptions{requireCompressionFormatMatch: requireCompressionFormatMatch}) if err != nil { - return nil, fmt.Errorf("copying system image from manifest list: %w", err) + if singleInstanceErrorWrapping != "" { + return nil, fmt.Errorf("%s: %w", singleInstanceErrorWrapping, err) + } + return nil, err } copiedManifest = single.manifest - } else { /* c.options.ImageListSelection == CopyAllImages or c.options.ImageListSelection == CopySpecificImages, */ + } else { // 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()) diff --git a/image/copy/multiple.go b/image/copy/multiple.go index 4ee57e5f58..e49cfd8c2a 100644 --- a/image/copy/multiple.go +++ b/image/copy/multiple.go @@ -21,27 +21,32 @@ import ( "go.podman.io/image/v5/pkg/compression" ) -type instanceCopyKind int +type instanceOpKind int const ( - instanceCopyCopy instanceCopyKind = iota - instanceCopyClone + instanceOpCopy instanceOpKind = iota + instanceOpClone + instanceOpDelete ) -type instanceCopy struct { - op instanceCopyKind +type instanceOp struct { + op instanceOpKind sourceDigest digest.Digest // Fields which can be used by callers when operation - // is `instanceCopyCopy` + // is `instanceOpCopy` copyForceCompressionFormat bool // Fields which can be used by callers when operation - // is `instanceCopyClone` + // is `instanceOpClone` cloneArtifactType string cloneCompressionVariant OptionCompressionVariant clonePlatform *imgspecv1.Platform cloneAnnotations map[string]string + + // Fields which can be used by callers when operation + // is `instanceOpDelete` + deleteIndex int } // internal type only to make imgspecv1.Platform comparable @@ -99,24 +104,26 @@ func validateCompressionVariantExists(input []OptionCompressionVariant) error { 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{} +// prepareInstanceOps prepares a list of operations to perform on instances (copy, clone, or delete). +// It returns a unified list of all operations and the count of copy/clone operations (excluding deletes). +func prepareInstanceOps(list internalManifest.List, instanceDigests []digest.Digest, options *Options, cannotModifyManifestListReason string) ([]instanceOp, int, error) { + res := []instanceOp{} + deleteOps := []instanceOp{} 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") + return res, -1, fmt.Errorf("EnsureCompressionVariantsExist is not implemented for CopySpecificImages") } err := validateCompressionVariantExists(options.EnsureCompressionVariantsExist) if err != nil { - return res, err + return res, -1, err } compressionsByPlatform, err := platformCompressionMap(list, instanceDigests) if err != nil { - return nil, err + return nil, -1, err } // Determine which specific images to copy (combining digest-based and platform-based selection) @@ -124,26 +131,37 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest. if options.ImageListSelection == CopySpecificImages { specificImages, err = determineSpecificImages(options, list) if err != nil { - return nil, err + return nil, -1, err } } for i, instanceDigest := range instanceDigests { if options.ImageListSelection == CopySpecificImages && !specificImages.Contains(instanceDigest) { - logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests)) + if options.SparseManifestListAction == StripSparseManifestList { + if cannotModifyManifestListReason != "" { + return nil, -1, fmt.Errorf("we should delete instance %s from manifest list, but we cannot: %s", instanceDigest, cannotModifyManifestListReason) + } + logrus.Debugf("deleting instance %s from destination’s manifest (%d/%d)", instanceDigest, i+1, len(instanceDigests)) + deleteOps = append(deleteOps, instanceOp{ + op: instanceOpDelete, + deleteIndex: i, + }) + } else { + 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) + return res, -1, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err) } forceCompressionFormat, err := shouldRequireCompressionFormatMatch(options) if err != nil { - return nil, err + return nil, -1, err } - res = append(res, instanceCopy{ - op: instanceCopyCopy, + res = append(res, instanceOp{ + op: instanceOpCopy, sourceDigest: instanceDigest, copyForceCompressionFormat: forceCompressionFormat, }) @@ -151,20 +169,30 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest. compressionList := compressionsByPlatform[platform] for _, compressionVariant := range options.EnsureCompressionVariantsExist { if !compressionList.Contains(compressionVariant.Algorithm.Name()) { - res = append(res, instanceCopy{ - op: instanceCopyClone, + res = append(res, instanceOp{ + op: instanceOpClone, sourceDigest: instanceDigest, cloneArtifactType: instanceDetails.ReadOnly.ArtifactType, cloneCompressionVariant: compressionVariant, clonePlatform: instanceDetails.ReadOnly.Platform, cloneAnnotations: maps.Clone(instanceDetails.ReadOnly.Annotations), }) - // add current compression to the list so that we don’t create duplicate clones compressionList.Add(compressionVariant.Algorithm.Name()) } } } - return res, nil + + // Add delete operations in reverse order (highest to lowest index) to avoid shifting + slices.Reverse(deleteOps) + copyLen := len(res) // Count copy/clone operations before appending deletes + + if copyLen == 0 && len(deleteOps) == len(instanceDigests) { + return nil, -1, fmt.Errorf("requested operation filtered out all platforms and would create an empty image") + } + + res = append(res, deleteOps...) + + return res, copyLen, nil } // determineSpecificImages returns a set of images to copy based on the @@ -222,7 +250,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, sigs, err := c.sourceSignatures(ctx, c.unparsedToplevel, "Getting image list signatures", - "Checking if image list destination supports signatures") + "Checking if image list destination supports signatures", true) if err != nil { return nil, err } @@ -248,7 +276,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, // Compare, and perhaps keep in sync with, the version in copySingleImage. cannotModifyManifestListReason := "" if len(sigs) > 0 { - cannotModifyManifestListReason = "Would invalidate signatures" + cannotModifyManifestListReason = "Would invalidate signatures; consider removing them from the multi-platform list" } if destIsDigestedReference { cannotModifyManifestListReason = "Destination specifies a digest" @@ -272,29 +300,31 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, } if selectedListType != originalList.MIMEType() { if cannotModifyManifestListReason != "" { - return nil, fmt.Errorf("Manifest list must be converted to type %q to be written to destination, but we cannot modify it: %q", selectedListType, cannotModifyManifestListReason) + return nil, fmt.Errorf("Manifest list must be converted to type %q to be written to destination, but we cannot modify it: %s", selectedListType, cannotModifyManifestListReason) } } // Copy each image, or just the ones we want to copy, in turn. instanceDigests := updatedList.Instances() instanceEdits := []internalManifest.ListEdit{} - instanceCopyList, err := prepareInstanceCopies(updatedList, instanceDigests, c.options) + instanceOpList, copyLen, err := prepareInstanceOps(updatedList, instanceDigests, c.options, cannotModifyManifestListReason) if err != nil { return nil, fmt.Errorf("preparing instances for copy: %w", err) } - c.Printf("Copying %d images generated from %d images in list\n", len(instanceCopyList), len(instanceDigests)) - for i, instance := range instanceCopyList { + c.Printf("Copying %d images generated from %d images in list\n", copyLen, len(instanceDigests)) + copyCount := 0 // Track copy/clone operations separately from delete operations + for i, instance := range instanceOpList { // Update instances to be edited by their `ListOperation` and // populate necessary fields. switch instance.op { - case instanceCopyCopy: - 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, copySingleImageOptions{requireCompressionFormatMatch: instance.copyForceCompressionFormat}) + case instanceOpCopy: + copyCount++ + logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, copyCount, copyLen) + c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, copyCount, copyLen) + unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceOpList[i].sourceDigest) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceOpList[i].sourceDigest, copySingleImageOptions{requireCompressionFormatMatch: instance.copyForceCompressionFormat}) if err != nil { - return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err) + return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", copyCount, copyLen, err) } // Record the result of a possible conversion here. instanceEdits = append(instanceEdits, internalManifest.ListEdit{ @@ -305,17 +335,18 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, 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{ + case instanceOpClone: + copyCount++ + logrus.Debugf("Replicating instance %s (%d/%d)", instance.sourceDigest, copyCount, copyLen) + c.Printf("Replicating image %s (%d/%d)\n", instance.sourceDigest, copyCount, copyLen) + unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceOpList[i].sourceDigest) + updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceOpList[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) + return nil, fmt.Errorf("replicating image %d/%d from manifest list: %w", copyCount, copyLen, err) } // Record the result of a possible conversion here. instanceEdits = append(instanceEdits, internalManifest.ListEdit{ @@ -328,12 +359,17 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, AddAnnotations: instance.cloneAnnotations, AddCompressionAlgorithms: updated.compressionAlgorithms, }) + case instanceOpDelete: + instanceEdits = append(instanceEdits, internalManifest.ListEdit{ + ListOperation: internalManifest.ListOpDelete, + DeleteIndex: instance.deleteIndex, + }) default: return nil, fmt.Errorf("copying image: invalid copy operation %d", instance.op) } } - // Now reset the digest/size/types of the manifests in the list to account for any conversions that we made. + // Now reset the digest/size/types of the manifests in the list and remove deleted instances. if err = updatedList.EditInstances(instanceEdits, cannotModifyManifestListReason != ""); err != nil { return nil, fmt.Errorf("updating manifest list: %w", err) } diff --git a/image/copy/multiple_test.go b/image/copy/multiple_test.go index 7486757ade..3856d2c939 100644 --- a/image/copy/multiple_test.go +++ b/image/copy/multiple_test.go @@ -1,6 +1,7 @@ package copy import ( + "context" "os" "path/filepath" "slices" @@ -9,13 +10,18 @@ import ( digest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.podman.io/image/v5/directory" internalManifest "go.podman.io/image/v5/internal/manifest" "go.podman.io/image/v5/pkg/compression" ) -// Test `instanceCopyCopy` cases. -func TestPrepareCopyInstancesforInstanceCopyCopy(t *testing.T) { - validManifest, err := os.ReadFile(filepath.Join("..", "internal", "manifest", "testdata", "oci1.index.zstd-selection.json")) +const ( + ociIndexZstdFile = "../internal/manifest/testdata/oci1.index.zstd-selection.json" +) + +// Test `instanceOpCopy` cases. +func TestPrepareInstanceOpsForInstanceCopy(t *testing.T) { + validManifest, err := os.ReadFile(ociIndexZstdFile) require.NoError(t, err) list, err := internalManifest.ListFromBlob(validManifest, internalManifest.GuessMIMEType(validManifest)) require.NoError(t, err) @@ -27,39 +33,78 @@ func TestPrepareCopyInstancesforInstanceCopyCopy(t *testing.T) { digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), } - instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{}) + instancesToCopy, copyCount, err := prepareInstanceOps(list, sourceInstances, &Options{}, "") require.NoError(t, err) - compare := []instanceCopy{} + assert.Equal(t, len(sourceInstances), copyCount) + compare := []instanceOp{} for _, instance := range sourceInstances { - compare = append(compare, instanceCopy{ - op: instanceCopyCopy, + compare = append(compare, instanceOp{ + op: instanceOpCopy, sourceDigest: instance, copyForceCompressionFormat: false, }) } assert.Equal(t, instancesToCopy, compare) // Test CopySpecificImages where selected instance is sourceInstances[1] - instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages}) + instancesToCopy, copyCount, err = prepareInstanceOps(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages}, "") require.NoError(t, err) - compare = []instanceCopy{{ - op: instanceCopyCopy, + compare = []instanceOp{{ + op: instanceOpCopy, sourceDigest: sourceInstances[1], }} + assert.Equal(t, len(compare), copyCount) assert.Equal(t, instancesToCopy, compare) - _, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages, ForceCompressionFormat: true}) + // Test CopySpecificImages with StripSparseManifestList where selected instance is sourceInstances[1] + instancesToCopy, copyCount, err = prepareInstanceOps(list, sourceInstances, &Options{ + Instances: []digest.Digest{sourceInstances[1]}, + ImageListSelection: CopySpecificImages, + SparseManifestListAction: StripSparseManifestList, + }, "") + require.NoError(t, err) + // Should have 1 copy operation followed by 2 delete operations (for indices 0 and 2) + expected := []instanceOp{ + { + op: instanceOpCopy, + sourceDigest: sourceInstances[1], + }, + { + op: instanceOpDelete, + deleteIndex: 2, // Delete from highest to lowest + }, + { + op: instanceOpDelete, + deleteIndex: 0, + }, + } + // Count copy/clone operations and delete operations separately + expectedCopyCount := 0 + expectedDeleteCount := 0 + for _, op := range expected { + switch op.op { + case instanceOpCopy, instanceOpClone: + expectedCopyCount++ + case instanceOpDelete: + expectedDeleteCount++ + } + } + assert.Equal(t, expectedCopyCount, copyCount) + assert.Equal(t, expectedDeleteCount, len(instancesToCopy)-copyCount) + assert.Equal(t, expected, instancesToCopy) + + _, _, err = prepareInstanceOps(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages, ForceCompressionFormat: true}, "") require.EqualError(t, err, "cannot use ForceCompressionFormat with undefined default compression format") } -// Test `instanceCopyClone` cases. -func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { - validManifest, err := os.ReadFile(filepath.Join("..", "internal", "manifest", "testdata", "oci1.index.zstd-selection.json")) +// Test `instanceOpClone` cases. +func TestPrepareInstanceOpsForInstanceClone(t *testing.T) { + validManifest, err := os.ReadFile(ociIndexZstdFile) require.NoError(t, err) list, err := internalManifest.ListFromBlob(validManifest, internalManifest.GuessMIMEType(validManifest)) require.NoError(t, err) - // Prepare option for `instanceCopyClone` case. + // Prepare option for `instanceOpClone` case. ensureCompressionVariantsExist := []OptionCompressionVariant{{Algorithm: compression.Zstd}} sourceInstances := []digest.Digest{ @@ -69,15 +114,15 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { } // CopySpecificImage must fail with error - _, err = prepareInstanceCopies(list, sourceInstances, &Options{ + _, _, err = prepareInstanceOps(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}) + instancesToCopy, copyCount, err := prepareInstanceOps(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}, "") require.NoError(t, err) // Following test ensures @@ -91,34 +136,36 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { expectedResponse := []simplerInstanceCopy{} for _, instance := range sourceInstances { expectedResponse = append(expectedResponse, simplerInstanceCopy{ - op: instanceCopyCopy, + op: instanceOpCopy, 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: instanceOpClone, sourceDigest: instance, cloneCompressionVariant: "zstd", clonePlatform: "arm64-linux-"}) } } actualResponse := convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) + assert.Equal(t, len(expectedResponse), copyCount) assert.Equal(t, expectedResponse, actualResponse) // Test option with multiple copy request for same compression format. // The above expectation should stay the same, if ensureCompressionVariantsExist requests zstd twice. ensureCompressionVariantsExist = []OptionCompressionVariant{{Algorithm: compression.Zstd}, {Algorithm: compression.Zstd}} - instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}) + instancesToCopy, copyCount, err = prepareInstanceOps(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}, "") require.NoError(t, err) expectedResponse = []simplerInstanceCopy{} for _, instance := range sourceInstances { expectedResponse = append(expectedResponse, simplerInstanceCopy{ - op: instanceCopyCopy, + op: instanceOpCopy, 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: instanceOpClone, sourceDigest: instance, cloneCompressionVariant: "zstd", clonePlatform: "arm64-linux-"}) } } actualResponse = convertInstanceCopyToSimplerInstanceCopy(instancesToCopy) + assert.Equal(t, len(expectedResponse), copyCount) assert.Equal(t, expectedResponse, actualResponse) // Add same instance twice but clone must appear only once. @@ -127,32 +174,33 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) { digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), } - instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist}) + instancesToCopy, copyCount, err = prepareInstanceOps(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 { + if instance.op == instanceOpClone { numberOfCopyClone++ } } assert.Equal(t, 1, numberOfCopyClone) + assert.Equal(t, len(sourceInstances)+numberOfCopyClone, copyCount) } -// simpler version of `instanceCopy` for testing where fields are string +// simpler version of `instanceOp` for testing where fields are string // instead of pointer type simplerInstanceCopy struct { - op instanceCopyKind + op instanceOpKind sourceDigest digest.Digest // Fields which can be used by callers when operation - // is `instanceCopyClone` + // is `instanceOpClone` cloneCompressionVariant string clonePlatform string cloneAnnotations map[string]string } -func convertInstanceCopyToSimplerInstanceCopy(copies []instanceCopy) []simplerInstanceCopy { +func convertInstanceCopyToSimplerInstanceCopy(copies []instanceOp) []simplerInstanceCopy { res := []simplerInstanceCopy{} for _, instance := range copies { platform := "" @@ -322,3 +370,106 @@ func TestDetermineSpecificImages(t *testing.T) { }) } } + +// TestStripSparseManifestListRequiresSignatureHandling tests that when using +// StripSparseManifestList with a signed manifest list, the user must explicitly +// choose how to handle signatures via RemoveSignatures or RemoveListSignatures. +func TestStripSparseManifestListRequiresSignatureHandling(t *testing.T) { + // Load a manifest list + manifest, err := os.ReadFile(ociIndexZstdFile) + require.NoError(t, err) + + tests := []struct { + name string + options *Options + addSignature bool + expectedError string + }{ + { + name: "Valid: StripSparseManifestList with signed manifest + RemoveSignatures", + options: &Options{ + ImageListSelection: CopySpecificImages, + Instances: []digest.Digest{digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}, + SparseManifestListAction: StripSparseManifestList, + RemoveSignatures: true, + }, + addSignature: true, + expectedError: "", + }, + { + name: "Valid: StripSparseManifestList with signed manifest + RemoveListSignatures", + options: &Options{ + ImageListSelection: CopySpecificImages, + Instances: []digest.Digest{digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}, + SparseManifestListAction: StripSparseManifestList, + RemoveListSignatures: true, + }, + addSignature: true, + expectedError: "", + }, + { + name: "Invalid: StripSparseManifestList with signed manifest without signature handling", + options: &Options{ + ImageListSelection: CopySpecificImages, + Instances: []digest.Digest{digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}, + SparseManifestListAction: StripSparseManifestList, + }, + addSignature: true, + expectedError: "we should delete instance", + }, + { + name: "Valid: StripSparseManifestList with unsigned manifest (no signature handling needed)", + options: &Options{ + ImageListSelection: CopySpecificImages, + Instances: []digest.Digest{digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}, + SparseManifestListAction: StripSparseManifestList, + }, + addSignature: false, + expectedError: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set up source directory with the manifest + srcDir := t.TempDir() + srcManifestPath := filepath.Join(srcDir, "manifest.json") + require.NoError(t, os.WriteFile(srcManifestPath, manifest, 0o644)) + + // Add a signature file if requested + if tt.addSignature { + // For directory transport, signatures are stored as "signature-1", "signature-2", etc. + // Copy an existing signature file from testdata + existingSignature, err := os.ReadFile(filepath.Join("..", "internal", "signature", "testdata", "simple.signature")) + require.NoError(t, err) + signaturePath := filepath.Join(srcDir, "signature-1") + require.NoError(t, os.WriteFile(signaturePath, existingSignature, 0o644)) + } + + // Set up destination directory + destDir := t.TempDir() + + // Create source and destination references + srcRef, err := directory.NewReference(srcDir) + require.NoError(t, err) + destRef, err := directory.NewReference(destDir) + require.NoError(t, err) + + // Call the real copy.Image() function + // Note: nil PolicyContext is invalid, but we expect validation to fail before PolicyContext is used + _, err = Image(context.Background(), nil, destRef, srcRef, tt.options) + + // Verify the error matches expectations + if tt.expectedError != "" { + require.Error(t, err, "Expected validation error from copy.Image()") + assert.ErrorContains(t, err, tt.expectedError) + } else { + // Note: The copy may fail for other reasons (missing blobs, etc.) + // but should not fail with the signature handling error + if err != nil { + assert.NotContains(t, err.Error(), "invalidate signatures") + } + } + }) + } +} diff --git a/image/copy/sign.go b/image/copy/sign.go index 70295538d0..fab53252f7 100644 --- a/image/copy/sign.go +++ b/image/copy/sign.go @@ -54,10 +54,10 @@ func (c *copier) setupSigners() error { // 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, - gettingSignaturesMessage, checkingDestMessage string, + gettingSignaturesMessage, checkingDestMessage string, isManifestList bool, ) ([]internalsig.Signature, error) { var sigs []internalsig.Signature - if c.options.RemoveSignatures { + if c.options.RemoveSignatures || (c.options.RemoveListSignatures && isManifestList) { sigs = []internalsig.Signature{} } else { c.Printf("%s\n", gettingSignaturesMessage) diff --git a/image/copy/single.go b/image/copy/single.go index 26a4d57e44..9f15c77e05 100644 --- a/image/copy/single.go +++ b/image/copy/single.go @@ -116,7 +116,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar sigs, err := c.sourceSignatures(ctx, src, "Getting image source signatures", - "Checking if image destination supports signatures") + "Checking if image destination supports signatures", false) if err != nil { return copySingleImageResult{}, err } diff --git a/image/internal/manifest/docker_schema2_list.go b/image/internal/manifest/docker_schema2_list.go index af4cda5bdb..5854a3d121 100644 --- a/image/internal/manifest/docker_schema2_list.go +++ b/image/internal/manifest/docker_schema2_list.go @@ -132,6 +132,11 @@ func (list *Schema2ListPublic) editInstances(editInstances []ListEdit, cannotMod }, schema2PlatformSpecFromOCIPlatform(*editInstance.AddPlatform), }) + case ListOpDelete: + if editInstance.DeleteIndex < 0 || editInstance.DeleteIndex >= len(list.Manifests) { + return fmt.Errorf("Schema2List.EditInstances: invalid delete index %d (list has %d instances)", editInstance.DeleteIndex, len(list.Manifests)) + } + list.Manifests = slices.Delete(list.Manifests, editInstance.DeleteIndex, editInstance.DeleteIndex+1) default: return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation) } diff --git a/image/internal/manifest/docker_schema2_list_test.go b/image/internal/manifest/docker_schema2_list_test.go index e347456658..564da23f42 100644 --- a/image/internal/manifest/docker_schema2_list_test.go +++ b/image/internal/manifest/docker_schema2_list_test.go @@ -90,6 +90,28 @@ func TestSchema2ListEditInstances(t *testing.T) { digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"), ), list.Instances()) + + // Create a fresh list for delete tests + list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest)) + require.NoError(t, err) + originalInstances := list.Instances() + require.GreaterOrEqual(t, len(originalInstances), 1) + + // Test successful deletion of first instance + err = list.EditInstances([]ListEdit{{ + ListOperation: ListOpDelete, + DeleteIndex: 0, + }}, false) + require.NoError(t, err) + assert.Equal(t, originalInstances[1:], list.Instances()) + + // Test error on invalid index + err = list.EditInstances([]ListEdit{{ + ListOperation: ListOpDelete, + DeleteIndex: 999, + }}, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid delete index") } func TestSchema2ListFromManifest(t *testing.T) { diff --git a/image/internal/manifest/list.go b/image/internal/manifest/list.go index a0d535891e..aa328ee1af 100644 --- a/image/internal/manifest/list.go +++ b/image/internal/manifest/list.go @@ -82,6 +82,7 @@ const ( listOpInvalid ListOp = iota ListOpAdd ListOpUpdate + ListOpDelete ) // ListEdit includes the fields which a List's EditInstances() method will modify. @@ -105,6 +106,9 @@ type ListEdit struct { AddPlatform *imgspecv1.Platform AddAnnotations map[string]string AddCompressionAlgorithms []compression.Algorithm + + // If Op = ListOpDelete. Callers should submit operations from highest index to lowest to avoid index shifting. + DeleteIndex int } // ListPublicFromBlob parses a list of manifests. diff --git a/image/internal/manifest/oci_index.go b/image/internal/manifest/oci_index.go index 4e4060255a..2e97906e76 100644 --- a/image/internal/manifest/oci_index.go +++ b/image/internal/manifest/oci_index.go @@ -176,6 +176,11 @@ func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit, cannotModi Platform: editInstance.AddPlatform, Annotations: annotations, }) + case ListOpDelete: + if editInstance.DeleteIndex < 0 || editInstance.DeleteIndex >= len(index.Manifests) { + return fmt.Errorf("OCI1Index.EditInstances: invalid delete index %d (list has %d instances)", editInstance.DeleteIndex, len(index.Manifests)) + } + index.Manifests = slices.Delete(index.Manifests, editInstance.DeleteIndex, editInstance.DeleteIndex+1) default: return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation) } diff --git a/image/internal/manifest/oci_index_test.go b/image/internal/manifest/oci_index_test.go index 9045f1ed4c..359339cd87 100644 --- a/image/internal/manifest/oci_index_test.go +++ b/image/internal/manifest/oci_index_test.go @@ -228,6 +228,28 @@ func TestOCI1EditInstances(t *testing.T) { assert.Equal(t, originalInstance, instance) } } + + // Create a fresh list for delete tests + list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest)) + require.NoError(t, err) + originalInstances := list.Instances() + require.GreaterOrEqual(t, len(originalInstances), 1) + + // Test successful deletion of first instance + err = list.EditInstances([]ListEdit{{ + ListOperation: ListOpDelete, + DeleteIndex: 0, + }}, false) + require.NoError(t, err) + assert.Equal(t, originalInstances[1:], list.Instances()) + + // Test error on invalid index + err = list.EditInstances([]ListEdit{{ + ListOperation: ListOpDelete, + DeleteIndex: 999, + }}, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid delete index") } func TestOCI1IndexChooseInstanceByCompression(t *testing.T) {