copy: add option to strip sparse manifest lists#655
copy: add option to strip sparse manifest lists#655TomSweeneyRedHat merged 1 commit intocontainers:mainfrom
Conversation
|
Packit jobs failed. @containers/packit-build please check. |
mtrmac
left a comment
There was a problem hiding this comment.
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.
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>
|
@mtrmac when using |
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
Just a very quick look for now.
Hum… it’s a thought. We currently ask the user to explicitly use 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 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 |
|
LGTM |
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>
I added a validation which requires the user to use
I added an option
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. |
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 |
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>
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>
9889bbf to
586fc62
Compare
|
|
||
| // TestStripOnlyListSignaturesValidation tests the validation logic for StripOnlyListSignatures | ||
| // by actually calling copy.Image() with various option combinations. | ||
| func TestStripOnlyListSignaturesValidation(t *testing.T) { |
There was a problem hiding this comment.
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.
586fc62 to
ab225b6
Compare
| } | ||
|
|
||
| instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{}) | ||
| instancesToCopy, _, err := prepareInstanceOps(list, sourceInstances, &Options{}, "") |
There was a problem hiding this comment.
Please add assertions for the copy count, throughout.
4747944 to
6d9fd31
Compare
mtrmac
left a comment
There was a problem hiding this comment.
Thanks! Looks good, please squash to avoid the PR-time fixups.
6d9fd31 to
7f9a11c
Compare
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>
81169ea to
38fa8f9
Compare
|
LGTM |
|
/lgtm |
| if copyLen == 0 && len(deleteOps) == len(instanceDigests) { | ||
| return nil, -1, fmt.Errorf("requested operation filtered out all platforms and would create an empty image") | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
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
CopySpecificImagesto 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:
This PR provides users explicit control over this behavior.
Changes
1. New
SparseManifestListActiontype and optionSparseManifestListActionenum with two values:KeepSparseManifestList(default): Preserves existing behavior - keeps manifest list as-is even when some instances are missingStripSparseManifestList: Removes non-copied instances from the destination manifest listSparseManifestListActionfield toOptionsstructCopySpecificImagesmode2. New
RemoveListSignaturesoption for granular signature controlRemoveListSignaturesfield toOptionsstructRemoveSignatures(which removes all signatures)RemoveSignaturesis true, it takes precedence3. Instance operation refactoring
instanceCopy→instanceOpandinstanceCopyKind→instanceOpKindfor clarityinstanceOpDeleteoperation alongsideinstanceOpCopyandinstanceOpCloneprepareInstanceOps()to return both the operation list and the count of copy/clone operations (excluding deletes)4. Manifest list editing support
ListOpDeleteoperation toListEditDeleteIndexfield toListEditfor specifying which instance to delete5. Validation and error handling
RemoveSignaturesorRemoveListSignatures)User Experience
Before (sparse lists always created):
Implementation Details
Code structure refactoring
Safe deletion processing
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.