Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add copy option to strip sparse manifests #1707

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
57 changes: 57 additions & 0 deletions copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,22 @@ const (
// specific images from the source reference.
type ImageListSelection int

const (
// SparseManifestListAction 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
)

// SparseImageListAction 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. SignBy will still add a new signature.
Expand Down Expand Up @@ -169,6 +185,10 @@ type Options struct {
// Download layer contents with "nondistributable" media types ("foreign" layers) and translate the layer media type
// 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.
SparseImageListAction SparseManifestListAction
}

// validateImageListSelection returns an error if the passed-in value is not one that we recognize as a valid ImageListSelection value
Expand Down Expand Up @@ -390,6 +410,33 @@ func compareImageDestinationManifestEqual(ctx context.Context, options *Options,
return true, destManifest, destManifestType, destManifestDigest, nil
}

// removeInstanceFromList removes the given image from the list
// this should probably move to an internal API
func removeInstancesFromList(manifestList manifest.List, imageIndices map[int]bool) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No; let’s build the infrastructure the right way rather than piling on tech debt.

It will never be any easier to get that done, and each such “small temporary thing” makes it harder.

Copy link
Author

Choose a reason for hiding this comment

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

I appreciate your way of thinking, but its just that I'm a bit lost at how to do it. I can't move the public interface of course to not break it. I can extend it (embed it in an internal one). But I also can't move the implementation structs I guess because those also seem to be public. I can extend those with the internal interface, but then the methods would still be public on the struct and I'm not sure if that's ok. And is it possible in go to do something like return type overloading to get methods like Clone() right?
With some pointers I might be able to perform the refactoring but at this point I'm a bit stuck.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this is the hard part of all this work.

Unlike interfaces, adding new methods to structs is ABI-safe. So my first guess is that the manifest.List interface, and the methods that create that (ListFromBlob) should be ~duplicated in internal packages. The implementations would probably also be moved to internal-only, with public aliases (type OCI1Index = internal.OCI1Index), but I’m much less certain about this part.


Alternatively… I think we’ll need to make these refactors for other purposes (adding/removing zstd-compressed variants) fairly soon anyway (but I’m not exactly sure when). So it might be an option to wait with this PR until that infrastructure work is done by other people.

Cc: @vrothberg .

Copy link
Author

Choose a reason for hiding this comment

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

That's helpful, I'll give it a try.

Copy link
Author

Choose a reason for hiding this comment

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

I tried but didn't succeed. Apparently go requires return types of a method to be equal in order to implement an interface, a subtype is not supported (I think its the lack of covariance support as described here: golang/go#30602). So the Clone method for example has to return a public List and not an internal one. But it also doesn't allow a more specific ListUpdate to be returned for example.
One option I considered was to add methods like CloneInternal() to the private interface. But that seems to be impossible without introducing package cycles because the public interface and public creator method are in the same package. The only workaround/hack I can think of is to do some dynamic registration to redirect the public creator to the private creator.
On the other hand, while I learned more about go with this exercise there's still a lot more for me to learn so I might have overlooked some other options. So unless someone can give me a clear pointer to something I missed I think I'll give up for now.

switch list := manifestList.(type) {
case *manifest.Schema2List:
var result = make([]manifest.Schema2ManifestDescriptor, 0, len(list.Manifests))
for i, manifest := range list.Manifests {
if !imageIndices[i] {
result = append(result, manifest)
}
}
list.Manifests = result
return nil
case *manifest.OCI1Index:
var result = make([]imgspecv1.Descriptor, 0, len(list.Manifests))
for i, manifest := range list.Manifests {
if !imageIndices[i] {
result = append(result, manifest)
}
}
list.Manifests = result
return nil
default:
return fmt.Errorf("stripping images from manifest list type %s is not supported", manifestList.MIMEType())
}
}

// 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) {
Expand Down Expand Up @@ -467,6 +514,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
}
c.Printf("Copying %d of %d images in list\n", imagesToCopy, len(instanceDigests))
updates := make([]manifest.ListUpdate, len(instanceDigests))
skipped := make(map[int]bool)
instancesCopied := 0
for i, instanceDigest := range instanceDigests {
if options.ImageListSelection == CopySpecificImages {
Expand All @@ -485,6 +533,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests))
// Record the digest/size/type of the manifest that we didn't copy.
updates[i] = update
skipped[i] = true
continue
}
}
Expand All @@ -510,6 +559,14 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
return nil, fmt.Errorf("updating manifest list: %w", err)
}

// Remove skipped images from the manifest if StripManifestList == true
if options.SparseImageListAction == StripSparseManifestList {
logrus.Debugf("Removing instances %v from manifest list", skipped)
if err := removeInstancesFromList(updatedList, skipped); err != nil {
return nil, fmt.Errorf("striping 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