Skip to content

copy: add option to strip sparse manifest lists#655

Merged
TomSweeneyRedHat merged 1 commit intocontainers:mainfrom
aguidirh:sparse-manifest-strip-pr1707
Mar 20, 2026
Merged

copy: add option to strip sparse manifest lists#655
TomSweeneyRedHat merged 1 commit intocontainers:mainfrom
aguidirh:sparse-manifest-strip-pr1707

Conversation

@aguidirh
Copy link
Copy Markdown
Contributor

@aguidirh aguidirh commented Feb 11, 2026

Add option to strip sparse manifest lists

This PR adds the ability to strip missing instances from manifest lists when copying only a subset of images from a multi-architecture manifest, addressing compatibility issues with registries that don't support sparse manifest lists. The implementation of this PR was based on containers/image#1707

Motivation

Closes #227

When using CopySpecificImages to copy only certain platforms from a multi-architecture image, the resulting manifest list in the destination still references all original instances, even though only a subset was actually copied. This creates a "sparse" manifest list where some referenced instances don't exist.

Problem:

  1. Registry Compatibility: Some registries reject sparse manifest lists or fail to serve them correctly
  2. Digest Mismatches: The manifest list still has the original digest even though the content differs
  3. No User Control: Users had no way to control this behavior - they were forced to accept sparse lists

This PR provides users explicit control over this behavior.

Changes

1. New SparseManifestListAction type and option

  • Added SparseManifestListAction enum with two values:
    • KeepSparseManifestList (default): Preserves existing behavior - keeps manifest list as-is even when some instances are missing
    • StripSparseManifestList: Removes non-copied instances from the destination manifest list
  • Added SparseManifestListAction field to Options struct
  • Works with CopySpecificImages mode

2. New RemoveListSignatures option for granular signature control

  • Added RemoveListSignatures field to Options struct
  • Removes only the manifest list signature while preserving per-instance signatures
  • Provides more granular control than RemoveSignatures (which removes all signatures)
  • When RemoveSignatures is true, it takes precedence

3. Instance operation refactoring

  • Renamed instanceCopyinstanceOp and instanceCopyKindinstanceOpKind for clarity
  • Added instanceOpDelete operation alongside instanceOpCopy and instanceOpClone
  • Modified prepareInstanceOps() to return both the operation list and the count of copy/clone operations (excluding deletes)
  • Delete operations are processed in reverse order (highest to lowest index) to avoid index shifting issues

4. Manifest list editing support

  • Added ListOpDelete operation to ListEdit
  • Added DeleteIndex field to ListEdit for specifying which instance to delete
  • Implemented delete support in both OCI Index and Docker Schema2 List formats
  • Added bounds checking to prevent invalid delete operations

5. Validation and error handling

  • Prevents creating empty manifest lists (returns error if all instances would be deleted)
  • Validates that stripping can only happen when signatures are handled (via RemoveSignatures or RemoveListSignatures)
  • Clear error messages when stripping is requested but signatures prevent modification
  • Improved error context for system image selection from manifest lists

User Experience

Before (sparse lists always created):

options := &copy.Options{
    ImageListSelection: copy.CopySpecificImages,
    Instances:         []digest.Digest{amd64Digest},
}
// Result: Sparse manifest list with missing instances
// Some registries may reject or fail to serve this

opy.Options{
    ImageListSelection:       copy.CopySpecificImages,
    Instances:                []digest.Digest{amd64Digest},
    SparseManifestListAction: copy.StripSparseManifestList,
    RemoveSignatures:         true,
}

Implementation Details

Code structure refactoring

  • Unified single-instance and manifest-list system image code paths for better maintainability
  • Moved error wrapping logic to common code path
  • Added validation to prevent RemoveListSignatures with single-instance copies

Safe deletion processing

  • Delete operations appended in reverse index order to prevent shifting issues
  • Separate tracking of copy/clone operations vs delete operations
  • Progress reporting reflects actual copy count, not total operation count

Testing

Comprehensive test coverage includes:

✅ Basic copy operations with and without stripping
✅ Delete operations in OCI Index and Docker Schema2 formats
✅ Bounds checking for invalid delete indices
✅ Signature handling validation (requires explicit removal when stripping)
✅ Empty manifest list prevention
✅ Integration tests with actual copy operations
✅ Copy count assertions in prepareInstanceOps tests
✅ All existing tests continue to pass
Compatibility
✅ Fully backward compatible - new optional fields with safe defaults
✅ Default behavior (KeepSparseManifestList) matches existing behavior
✅ Existing code continues to work unchanged
✅ No breaking changes to public API

Credits

This implementation is based on the original work by @bertbaron and @mtrmac in containers/image#1707, adapted for the container-libs monorepo structure.

@github-actions github-actions bot added the image Related to "image" package label Feb 11, 2026
@packit-as-a-service
Copy link
Copy Markdown

Packit jobs failed. @containers/packit-build please check.

Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just a brief first look: generally the structure and choice of affected sub packages look correct.

This is impressively small, I was expecting a that a larger change to internal/manifest would be necessary.

Comment thread image/copy/multiple.go Outdated
Comment thread image/internal/manifest/docker_schema2_list.go Outdated
Comment thread image/internal/manifest/list_test.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/copy.go
Comment thread image/copy/copy.go Outdated
aguidirh added a commit to aguidirh/container-libs that referenced this pull request Feb 17, 2026
copy: implement code review feedback for sparse manifest stripping

Address feedback from mtrmac's review on PR containers#655:

1. Simplify deletion logic by combining EditInstances calls
   - Moved deletion tracking into prepareInstanceCopies function
   - Combined update and delete operations into single EditInstances call
   - Removed separate digest-based tracking and redundant loops

2. Use slices.Delete for manifest entry removal
   - Replaced manual slice manipulation with slices.Delete in both
     docker_schema2_list.go and oci_index.go for cleaner code

3. Integrate deletion tests into existing test methods
   - Removed standalone TestListDeleteInstances function
   - Added deletion test cases to TestSchema2ListEditInstances and
     TestOCI1EditInstances for better test organization

4. Move deletion logic into prepareInstanceCopies (most critical fix)
   - Modified prepareInstanceCopies to return ([]instanceCopy, []int, error)
   - Track indices to delete when SparseManifestListAction == StripSparseManifestList
   - Properly handles duplicate digests by using instance indices instead
     of digest-based lookup, fixing edge case where same digest appears
     multiple times in manifest list

5. Use slices.Backward for reverse iteration
   - Simplified deletion loop using slices.Backward iterator
   - Maintains high-to-low deletion order to avoid index shifting

6. Move SparseManifestListAction field for better organization
   - Relocated field from line 150 to line 116 in copy.go
   - Now positioned near related ImageListSelection and Instances fields
   - Improves logical grouping of manifest list options

All tests pass. The implementation now correctly handles:
- Duplicate digests in manifest lists
- Single EditInstances call for efficiency
- Cleaner separation of concerns
- Better code organization

Fixes feedback on containers#655

Signed-off-by: Alex Guidi <aguidi@redhat.com>
@aguidirh
Copy link
Copy Markdown
Contributor Author

@mtrmac when using StripSparseManifestList it will fail with Manifest list was edited, but we cannot modify it: "Would invalidate signatures" should we make mandatory the usage of --remove-signatures when using StripSparseManifestList ?

Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Just a very quick look for now.

Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
@mtrmac
Copy link
Copy Markdown
Contributor

mtrmac commented Feb 17, 2026

should we make mandatory the usage of --remove-signatures when using StripSparseManifestList ?

Hum… it’s a thought.

We currently ask the user to explicitly use --remove-signatures for all other edits (e.g. converting the manifest format or changing compression), so I think also requiring an explicit ACK that the signatures are going to be broken would be consistent here.


Actually, c/image only cares about the signatures of the per-platform instances, it never validates the signature on the list (but it does create it). So, ideally, we should allow stripping the list’s signature but keep the per-instance signatures, so that users can create sparse mirrors but still validate image integrity. I think that would be a new option in copy.Options — and maybe some mirroring tools (in some situations?) would enable it by default when the user explicitly asks for sparse copies.

E.g., purely hypothetically, it might make sense to hard-code this to on when mirroring the OpenShift product payload, because we know how signatures are validated there; and maybe not hard-code it for other images, because those might be validated using some other software like cosign.

@TomSweeneyRedHat
Copy link
Copy Markdown
Member

LGTM
and happy green test buttons.

aguidirh added a commit to aguidirh/container-libs that referenced this pull request Feb 26, 2026
Add StripOnlyListSignatures option to strip manifest list signatures
while preserving per-instance signatures when using CopySpecificImages
with StripSparseManifestList.

This implements "Option 2" from PR containers#655. Previously, users had to use
RemoveSignatures which removed all signatures, including per-instance
ones that would still be valid after stripping the manifest list.

The option requires explicit validation:
- Only valid with CopySpecificImages + StripSparseManifestList
- Validates at multiple points in copy.Image() and copyMultipleImages()
- When used with signed images, requires explicit signature handling

Per-instance signatures are preserved because copySingleImage handles
each instance independently.

Signed-off-by: Alex Guidi <aguidi@redhat.com>
@aguidirh
Copy link
Copy Markdown
Contributor Author

We currently ask the user to explicitly use --remove-signatures for all other edits (e.g. converting the manifest format or changing compression), so I think also requiring an explicit ACK that the signatures are going to be broken would be consistent here.

I added a validation which requires the user to use RemoveSignatures or the new option StripOnlyListSignatures when SparseManifestListAction == StripSparseManifestList

Actually, c/image only cares about the signatures of the per-platform instances, it never validates the signature on the list (but it does create it). So, ideally, we should allow striping the list’s signature but keep the per-instance signatures, so that users can create sparse mirrors but still validate image integrity. I think that would be a new option in copy.Options — and maybe some mirroring tools (in some situations?) would enable it by default when the user explicitly asks for sparse copies.

I added an option StripOnlyListSignatures where it removes the signature of the manifest list while it keeps the signature of the instances requested by the user.

E.g., purely hypothetically, it might make sense to hard-code this to on when mirroring the OpenShift product payload, because we know how signatures are validated there; and maybe not hard-code it for other images, because those might be validated using some other software like cosign.

I'm not sure if I understood this last comment, could you please explain it to me?

I just pushed a commit with the last changes that I mentioned above.

@mtrmac
Copy link
Copy Markdown
Contributor

mtrmac commented Feb 26, 2026

purely hypothetically, it might make sense to hard-code this to on when mirroring the OpenShift product payload, because we know how signatures are validated there; and maybe not hard-code it for other images, because those might be validated using some other software like cosign.

I'm not sure if I understood this last comment, could you please explain it to me?

Software that knows how the images is going to be used can hard-code option values so that users don’t have to care; generic software like skopeo should probably default to doing the safe thing, and not discard potentially-valuable data or break signatures without the user understanding the consequence.

Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread image/copy/copy.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go
Comment thread image/internal/manifest/docker_schema2_list_test.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple_test.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple_test.go Outdated
@aguidirh aguidirh changed the title copy: add option to strip sparse manifest lists WIP: copy: add option to strip sparse manifest lists Feb 27, 2026
aguidirh added a commit to aguidirh/container-libs that referenced this pull request Mar 3, 2026
copy: implement code review feedback for sparse manifest stripping

Address feedback from mtrmac's review on PR containers#655:

1. Simplify deletion logic by combining EditInstances calls
   - Moved deletion tracking into prepareInstanceCopies function
   - Combined update and delete operations into single EditInstances call
   - Removed separate digest-based tracking and redundant loops

2. Use slices.Delete for manifest entry removal
   - Replaced manual slice manipulation with slices.Delete in both
     docker_schema2_list.go and oci_index.go for cleaner code

3. Integrate deletion tests into existing test methods
   - Removed standalone TestListDeleteInstances function
   - Added deletion test cases to TestSchema2ListEditInstances and
     TestOCI1EditInstances for better test organization

4. Move deletion logic into prepareInstanceCopies (most critical fix)
   - Modified prepareInstanceCopies to return ([]instanceCopy, []int, error)
   - Track indices to delete when SparseManifestListAction == StripSparseManifestList
   - Properly handles duplicate digests by using instance indices instead
     of digest-based lookup, fixing edge case where same digest appears
     multiple times in manifest list

5. Use slices.Backward for reverse iteration
   - Simplified deletion loop using slices.Backward iterator
   - Maintains high-to-low deletion order to avoid index shifting

6. Move SparseManifestListAction field for better organization
   - Relocated field from line 150 to line 116 in copy.go
   - Now positioned near related ImageListSelection and Instances fields
   - Improves logical grouping of manifest list options

All tests pass. The implementation now correctly handles:
- Duplicate digests in manifest lists
- Single EditInstances call for efficiency
- Cleaner separation of concerns
- Better code organization

Fixes feedback on containers#655

Signed-off-by: Alex Guidi <aguidi@redhat.com>
aguidirh added a commit to aguidirh/container-libs that referenced this pull request Mar 3, 2026
Add StripOnlyListSignatures option to strip manifest list signatures
while preserving per-instance signatures when using CopySpecificImages
with StripSparseManifestList.

This implements "Option 2" from PR containers#655. Previously, users had to use
RemoveSignatures which removed all signatures, including per-instance
ones that would still be valid after stripping the manifest list.

The option requires explicit validation:
- Only valid with CopySpecificImages + StripSparseManifestList
- Validates at multiple points in copy.Image() and copyMultipleImages()
- When used with signed images, requires explicit signature handling

Per-instance signatures are preserved because copySingleImage handles
each instance independently.

Signed-off-by: Alex Guidi <aguidi@redhat.com>
@aguidirh aguidirh force-pushed the sparse-manifest-strip-pr1707 branch from 9889bbf to 586fc62 Compare March 3, 2026 13:52
@aguidirh aguidirh changed the title WIP: copy: add option to strip sparse manifest lists copy: add option to strip sparse manifest lists Mar 3, 2026
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple_test.go Outdated

// TestStripOnlyListSignaturesValidation tests the validation logic for StripOnlyListSignatures
// by actually calling copy.Image() with various option combinations.
func TestStripOnlyListSignaturesValidation(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really didn’t mean to ask for the in-memory test harness here.

It was more of… “if there is no plausible unit test, it is acceptable to leave the code untested here instead of writing thousands of lines”.


I would actually like to ask for an added test — but not here. At least one integration test in the Skopeo repo, perhaps exercising the platform filtering and sparse creation together. (Integration tests in Skopeo are where we run the top-level skopeo copy commands, and where we have local registries, other infrastructure, and license to take more than a few hundred milliseconds.) That would, though, mean also wiring up the new features as Skopeo options, which might not be in scope — so, non-blocking as well.

Comment thread image/copy/copy.go
Comment thread image/copy/multiple_test.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/multiple.go Outdated
Comment thread image/copy/copy.go Outdated
Comment thread image/copy/multiple_test.go Outdated
}

instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{})
instancesToCopy, _, err := prepareInstanceOps(list, sourceInstances, &Options{}, "")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add assertions for the copy count, throughout.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at commit c4ba928830.

@aguidirh aguidirh force-pushed the sparse-manifest-strip-pr1707 branch from 4747944 to 6d9fd31 Compare March 17, 2026 08:32
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good, please squash to avoid the PR-time fixups.

Comment thread image/copy/multiple_test.go Outdated
Comment thread image/copy/multiple_test.go Outdated
@aguidirh aguidirh force-pushed the sparse-manifest-strip-pr1707 branch from 6d9fd31 to 7f9a11c Compare March 20, 2026 11:43
Add the ability to strip non-copied instances from manifest lists when
using CopySpecificImages. This implements the functionality originally
proposed in containers/image#1707.

When copying a subset of images from a multi-architecture manifest list
using CopySpecificImages, the current behavior always produces a sparse
manifest list - a manifest list that references platforms that weren't
actually copied. While this maintains the original digest, many
registries reject sparse manifests with "blob unknown to registry"
errors.

This commit adds a new SparseManifestListAction option that gives users
control over this behavior:

- KeepSparseManifestList (default): Preserves the manifest list as-is,
  even when some instances aren't copied. Some registries may not
  support sparse manifest lists.

- StripSparseManifestList (new): Removes instances that are not being copied
  from the destination manifest list. This changes the digest but
  ensures compatibility with all registries.

Based on original work by @bertbaron and @mtrmac in
containers/image#1707, adapted for the container-libs monorepo
structure.

Relates to containers#227

Signed-off-by: Alex Guidi <aguidi@redhat.com>
@aguidirh aguidirh force-pushed the sparse-manifest-strip-pr1707 branch from 81169ea to 38fa8f9 Compare March 20, 2026 12:53
@TomSweeneyRedHat
Copy link
Copy Markdown
Member

LGTM

@TomSweeneyRedHat
Copy link
Copy Markdown
Member

/lgtm

@TomSweeneyRedHat TomSweeneyRedHat merged commit 172e052 into containers:main Mar 20, 2026
24 checks passed
Comment thread image/copy/multiple.go
Comment on lines +189 to +192
if copyLen == 0 && len(deleteOps) == len(instanceDigests) {
return nil, -1, fmt.Errorf("requested operation filtered out all platforms and would create an empty image")
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? Pushing an empty manifest used to work and is done by several podman tests as such this breaks podman CI now.

containers/podman#28339
https://api.cirrus-ci.com/v1/artifact/task/5657829631066112/html/int-podman-debian-14-root-host.log.html#t--Podman-push-podman-push-to-local-registry-with-authorization--1
https://api.cirrus-ci.com/v1/artifact/task/5857432867438592/html/sys-podman-rawhide-rootless-host.log.html

I have not looked at the larger picture here if this limitation should just be removed. If this needs more work then I would propose to revert this PR to unblock others people vendor work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added only to avoid pushing empty manifests.

Why empty manifests are a valid scenario? I can create another PR to remove those lines above if empty manifests are a valid scenario. What do you think @mtrmac ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think copying empty images should work, why not; but stripping a non-empty manifest to empty is almost certainly a mistake.

I’ll try to fix that today.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

image Related to "image" package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support copying images for specific platforms/architectures

4 participants