Skip to content

copy: try list conversions on list push failures #820

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.

Sign up for GitHub

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

Merged
rhatdan merged 1 commit into from
Mar 18, 2020
Merged

copy: try list conversions on list push failures #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
82 changes: 49 additions & 33 deletions copy/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,11 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
if err != nil {
return nil, "", errors.Wrapf(err, "Error reading manifest list")
}
list, err := manifest.ListFromBlob(manifestList, manifestType)
originalList, err := manifest.ListFromBlob(manifestList, manifestType)
if err != nil {
return nil, "", errors.Wrapf(err, "Error parsing manifest list %q", string(manifestList))
}
originalList := list.Clone()
updatedList := originalList.Clone()

// Read and/or clear the set of signatures for this list.
var sigs [][]byte
Expand Down Expand Up @@ -390,18 +390,18 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
case imgspecv1.MediaTypeImageManifest:
forceListMIMEType = imgspecv1.MediaTypeImageIndex
}
selectedListType, err := c.determineListConversion(manifestType, c.dest.SupportedManifestMIMETypes(), forceListMIMEType)
selectedListType, otherManifestMIMETypeCandidates, err := c.determineListConversion(manifestType, c.dest.SupportedManifestMIMETypes(), forceListMIMEType)
if err != nil {
return nil, "", errors.Wrapf(err, "Error determining manifest list type to write to destination")
}
if selectedListType != list.MIMEType() {
if selectedListType != originalList.MIMEType() {
if !canModifyManifestList {
return nil, "", errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", selectedListType)
}
}

// Copy each image, or just the ones we want to copy, in turn.
instanceDigests := list.Instances()
instanceDigests := updatedList.Instances()
imagesToCopy := len(instanceDigests)
if options.ImageListSelection == CopySpecificImages {
imagesToCopy = len(options.Instances)
Expand All @@ -419,7 +419,7 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
}
}
if skip {
update, err := list.Instance(instanceDigest)
update, err := updatedList.Instance(instanceDigest)
if err != nil {
return nil, "", err
}
Expand Down Expand Up @@ -447,42 +447,58 @@ func (c *copier) copyMultipleImages(ctx context.Context, policyContext *signatur
}

// Now reset the digest/size/types of the manifests in the list to account for any conversions that we made.
if err = list.UpdateInstances(updates); err != nil {
if err = updatedList.UpdateInstances(updates); err != nil {
return nil, "", errors.Wrapf(err, "Error updating manifest list")
}

// Perform the list conversion.
if selectedListType != list.MIMEType() {
list, err = list.ConvertToMIMEType(selectedListType)
// Iterate through supported list types, preferred format first.
c.Printf("Writing manifest list to image destination\n")
var errs []string
for _, thisListType := range append([]string{selectedListType}, otherManifestMIMETypeCandidates...) {
Copy link
Collaborator

@mtrmac mtrmac Mar 16, 2020

Choose a reason for hiding this comment

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

Non-blocking: Can/should this use ManifestTypeRejectedError to avoid unnecessary attempts (and duplicated error messages) for other kinds of failures?

OTOH right now we only have two types, so it’s not at all urgent.

attemptedList := updatedList

logrus.Debugf("Trying to use manifest list type %s…", thisListType)

// Perform the list conversion, if we need one.
if thisListType != updatedList.MIMEType() {
attemptedList, err = updatedList.ConvertToMIMEType(thisListType)
if err != nil {
return nil, "", errors.Wrapf(err, "Error converting manifest list to list with MIME type %q", thisListType)
}
}

// Check if the updates or a type conversion meaningfully changed the list of images
// by serializing them both so that we can compare them.
attemptedManifestList, err := attemptedList.Serialize()
if err != nil {
return nil, "", errors.Wrapf(err, "Error converting manifest list to list with MIME type %q", selectedListType)
return nil, "", errors.Wrapf(err, "Error encoding updated manifest list (%q: %#v)", updatedList.MIMEType(), updatedList.Instances())
}
originalManifestList, err := originalList.Serialize()
if err != nil {
return nil, "", errors.Wrapf(err, "Error encoding original manifest list for comparison (%q: %#v)", originalList.MIMEType(), originalList.Instances())
}
}

// Check if the updates or a type conversion meaningfully changed the list of images
// by serializing them both so that we can compare them.
updatedManifestList, err := list.Serialize()
if err != nil {
return nil, "", errors.Wrapf(err, "Error encoding updated manifest list (%q: %#v)", list.MIMEType(), list.Instances())
}
originalManifestList, err := originalList.Serialize()
if err != nil {
return nil, "", errors.Wrapf(err, "Error encoding original manifest list for comparison (%q: %#v)", originalList.MIMEType(), originalList.Instances())
}
// If we can't just use the original value, but we have to change it, flag an error.
if !bytes.Equal(attemptedManifestList, originalManifestList) {
if !canModifyManifestList {
return nil, "", errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", thisListType)
}
logrus.Debugf("Manifest list has been updated")
}

// If we can't just use the original value, but we have to change it, flag an error.
if !bytes.Equal(updatedManifestList, originalManifestList) {
if !canModifyManifestList {
return nil, "", errors.Errorf("Error: manifest list must be converted to type %q to be written to destination, but that would invalidate signatures", selectedListType)
// Save the manifest list.
err = c.dest.PutManifest(ctx, attemptedManifestList, nil)
if err != nil {
logrus.Debugf("Upload of manifest list type %s failed: %v", thisListType, err)
errs = append(errs, fmt.Sprintf("%s(%v)", thisListType, err))
continue
}
manifestList = updatedManifestList
logrus.Debugf("Manifest list has been updated")
errs = nil
manifestList = attemptedManifestList
break
}

// Save the manifest list.
c.Printf("Writing manifest list to image destination\n")
if err = c.dest.PutManifest(ctx, manifestList, nil); err != nil {
return nil, "", errors.Wrapf(err, "Error writing manifest list %q", string(manifestList))
if errs != nil {
return nil, "", fmt.Errorf("Uploading manifest list failed, attempted the following formats: %s", strings.Join(errs, ", "))
}

// Sign the manifest list.
Expand Down
21 changes: 15 additions & 6 deletions copy/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,10 @@ func isMultiImage(ctx context.Context, img types.UnparsedImage) (bool, error) {
// determineListConversion takes the current MIME type of a list of manifests,
// the list of MIME types supported for a given destination, and a possible
// forced value, and returns the MIME type to which we should convert the list
// of manifests, whether we are converting to it or using it unmodified.
func (c *copier) determineListConversion(currentListMIMEType string, destSupportedMIMETypes []string, forcedListMIMEType string) (string, error) {
// of manifests (regardless of whether we are converting to it or using it
// unmodified) and a slice of other list types which might be supported by the
// destination.
func (c *copier) determineListConversion(currentListMIMEType string, destSupportedMIMETypes []string, forcedListMIMEType string) (string, []string, error) {
// If there's no list of supported types, then anything we support is expected to be supported.
if len(destSupportedMIMETypes) == 0 {
destSupportedMIMETypes = manifest.SupportedListMIMETypes
Expand All @@ -136,6 +138,7 @@ func (c *copier) determineListConversion(currentListMIMEType string, destSupport
destSupportedMIMETypes = []string{forcedListMIMEType}
}
var selectedType string
var otherSupportedTypes []string
for i := range destSupportedMIMETypes {
// The second priority is the first member of the list of acceptable types that is a list,
// but keep going in case current type occurs later in the list.
Expand All @@ -148,15 +151,21 @@ func (c *copier) determineListConversion(currentListMIMEType string, destSupport
selectedType = destSupportedMIMETypes[i]
}
}
// Pick out the other list types that we support.
for i := range destSupportedMIMETypes {
if selectedType != destSupportedMIMETypes[i] && manifest.MIMETypeIsMultiImage(destSupportedMIMETypes[i]) {
otherSupportedTypes = append(otherSupportedTypes, destSupportedMIMETypes[i])
}
}
logrus.Debugf("Manifest list has MIME type %s, ordered candidate list [%s]", currentListMIMEType, strings.Join(destSupportedMIMETypes, ", "))
if selectedType == "" {
return "", errors.Errorf("destination does not support any supported manifest list types (%v)", manifest.SupportedListMIMETypes)
return "", nil, errors.Errorf("destination does not support any supported manifest list types (%v)", manifest.SupportedListMIMETypes)
}
if selectedType != currentListMIMEType {
logrus.Debugf("... will convert to %s", selectedType)
logrus.Debugf("... will convert to %s first, and then try %v", selectedType, otherSupportedTypes)
} else {
logrus.Debugf("... will use the original manifest list type")
logrus.Debugf("... will use the original manifest list type, and then try %v", otherSupportedTypes)
}
// Done.
return selectedType, nil
return selectedType, otherSupportedTypes, nil
}