Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions image/copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,22 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
StripSparseManifestList
StripSparseManifestList SparseManifestListAction

IIRC, otherwise it doesn’t have the specialized type.

)

// 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.
Expand Down Expand Up @@ -129,6 +145,10 @@ type Options struct {
// to not indicate "nondistributable".
DownloadForeignLayers bool

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better placed closer to the ImageListSelection field.


// 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
Expand Down
35 changes: 35 additions & 0 deletions image/copy/multiple.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,41 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
return nil, fmt.Errorf("updating manifest list: %w", err)
}

// Remove skipped instances from the manifest list if StripSparseManifestList is enabled
if c.options.ImageListSelection == CopySpecificImages && c.options.SparseManifestListAction == StripSparseManifestList {
// Build a set of digests that were copied
copiedDigests := set.New[digest.Digest]()
for _, instance := range instanceCopyList {
copiedDigests.Add(instance.sourceDigest)
}
Comment on lines +294 to +298
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the same digest is included multiple times in the index? That’s not very likely with per-platform images, but with CopySpecificImages, the caller can choose to copy one of the duplicates and not the other.

prepareInstanceCopies knows exactly which instances are copied and which are not, so I think it can also prepare some version of the deletion instructions.

Also, prepareInstanceCopies is intentionally a ~pure function, so that it is unit-testable. Unit tests for the logic here, ensuring we ask for deleting the correct instances, seem worthwhile.


// Find which indices were skipped
var indicesToDelete []int
Copy link
Contributor

Choose a reason for hiding this comment

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

(If the structure remains: I think this variable is unnecessary — we can do something like for range slices.Backwards(instanceDigests) { if !copiedDigests.Contains(…) { deleteEdits = append(…) } in one loop)

for i, instanceDigest := range instanceDigests {
if !copiedDigests.Contains(instanceDigest) {
indicesToDelete = append(indicesToDelete, i)
}
}

// Build delete operations for skipped instances
// Delete from highest to lowest index to avoid shifting
var deleteEdits []internalManifest.ListEdit
for i := len(indicesToDelete) - 1; i >= 0; i-- {
deleteEdits = append(deleteEdits, internalManifest.ListEdit{
ListOperation: internalManifest.ListOpDelete,
DeleteIndex: indicesToDelete[i],
})
}

// Remove skipped instances from the manifest list using EditInstances
if len(deleteEdits) > 0 {
logrus.Debugf("Removing %d instances from manifest list", len(deleteEdits))
if err := updatedList.EditInstances(deleteEdits, false); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can only call EditInstances once, appending to existing instanceEdits.

return nil, fmt.Errorf("stripping sparse manifest list: %w", err)
}
}
}

// Iterate through supported list types, preferred format first.
c.Printf("Writing manifest list to image destination\n")
var errs []string
Expand Down
6 changes: 6 additions & 0 deletions image/internal/manifest/docker_schema2_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,12 @@ 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))
}
// Remove the element by appending slices before and after the target index
list.Manifests = append(list.Manifests[:editInstance.DeleteIndex], list.Manifests[editInstance.DeleteIndex+1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

slices.Delete would be a bit better, (also in the other copy)

default:
return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation)
}
Expand Down
4 changes: 4 additions & 0 deletions image/internal/manifest/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ const (
listOpInvalid ListOp = iota
ListOpAdd
ListOpUpdate
ListOpDelete
)

// ListEdit includes the fields which a List's EditInstances() method will modify.
Expand All @@ -105,6 +106,9 @@ type ListEdit struct {
AddPlatform *imgspecv1.Platform
AddAnnotations map[string]string
AddCompressionAlgorithms []compression.Algorithm

// If Op = ListOpDelete. Must delete from highest index to lowest to avoid index shifting.
DeleteIndex int
}

// ListPublicFromBlob parses a list of manifests.
Expand Down
37 changes: 37 additions & 0 deletions image/internal/manifest/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,40 @@ func TestChooseInstance(t *testing.T) {
}
}
}

// TestListDeleteInstances tests the ListOpDelete functionality for both OCI and Docker manifest formats.
func TestListDeleteInstances(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d prefer this to be tested in the existing Test…EditInstances methods (but the two can share a helper if that is beneficial)

manifestFiles := []string{
"ociv1.image.index.json",
"v2list.manifest.json",
}

for _, manifestFile := range manifestFiles {
t.Run(manifestFile, func(t *testing.T) {
validManifest, err := os.ReadFile(filepath.Join("testdata", manifestFile))
require.NoError(t, err)
list, err := ListFromBlob(validManifest, GuessMIMEType(validManifest))
require.NoError(t, err)

originalInstances := list.Instances()
require.GreaterOrEqual(t, len(originalInstances), 1)

// Test successful deletion
err = list.EditInstances([]ListEdit{{
ListOperation: ListOpDelete,
DeleteIndex: 0,
}}, false)
require.NoError(t, err)
assert.Equal(t, len(originalInstances)-1, len(list.Instances()))
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")
})
}
}
6 changes: 6 additions & 0 deletions image/internal/manifest/oci_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,12 @@ 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))
}
// Remove the element by appending slices before and after the target index
index.Manifests = append(index.Manifests[:editInstance.DeleteIndex], index.Manifests[editInstance.DeleteIndex+1:]...)
default:
return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation)
}
Expand Down