Skip to content

Commit

Permalink
Merge pull request #1578 from mtrmac/sourced-image-struct
Browse files Browse the repository at this point in the history 
Use a SourcedImage struct in c/image/copy
  • Loading branch information
@rhatdan
rhatdan committed Jun 15, 2022
2 parents af65496 + acbde71 commit a78a007
Show file tree
Hide file tree
Showing 4 changed files with 226 additions and 183 deletions.
54 changes: 26 additions & 28 deletions copy/copy.go
View file
Expand Up @@ -82,7 +82,7 @@ type copier struct {
type imageCopier struct {
c *copier
manifestUpdates *types.ManifestUpdateOptions
src types.Image
src *image.SourcedImage
diffIDsAreNeeded bool
cannotModifyManifestReason string // The reason the manifest cannot be modified, or an empty string if it can
canSubstituteBlobs bool
Expand Down Expand Up @@ -349,13 +349,8 @@ func supportsMultipleImages(dest types.ImageDestination) bool {

// compareImageDestinationManifestEqual compares the `src` and `dest` image manifests (reading the manifest from the
// (possibly remote) destination). Returning true and the destination's manifest, type and digest if they compare equal.
func compareImageDestinationManifestEqual(ctx context.Context, options *Options, src types.Image, targetInstance *digest.Digest, dest types.ImageDestination) (bool, []byte, string, digest.Digest, error) {
srcManifest, _, err := src.Manifest(ctx)
if err != nil {
return false, nil, "", "", errors.Wrapf(err, "reading manifest from image")
}

srcManifestDigest, err := manifest.Digest(srcManifest)
func compareImageDestinationManifestEqual(ctx context.Context, options *Options, src *image.SourcedImage, targetInstance *digest.Digest, dest types.ImageDestination) (bool, []byte, string, digest.Digest, error) {
srcManifestDigest, err := manifest.Digest(src.ManifestBlob)
if err != nil {
return false, nil, "", "", errors.Wrapf(err, "calculating manifest digest")
}
Expand Down Expand Up @@ -620,11 +615,7 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli
if named := c.dest.Reference().DockerReference(); named != nil {
if digested, ok := named.(reference.Digested); ok {
destIsDigestedReference = true
sourceManifest, _, err := src.Manifest(ctx)
if err != nil {
return nil, "", "", errors.Wrapf(err, "reading manifest from source image")
}
matches, err := manifest.MatchesDigest(sourceManifest, digested.Digest())
matches, err := manifest.MatchesDigest(src.ManifestBlob, digested.Digest())
if err != nil {
return nil, "", "", errors.Wrapf(err, "computing digest of source image's manifest")
}
Expand Down Expand Up @@ -702,12 +693,23 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli

destRequiresOciEncryption := (isEncrypted(src) && ic.c.ociDecryptConfig != nil) || options.OciEncryptLayers != nil

// We compute preferredManifestMIMEType only to show it in error messages.
// Without having to add this context in an error message, we would be happy enough to know only that no conversion is needed.
preferredManifestMIMEType, otherManifestMIMETypeCandidates, err := ic.determineManifestConversion(ctx, c.dest.SupportedManifestMIMETypes(), options.ForceManifestMIMEType, destRequiresOciEncryption)
manifestConversionPlan, err := determineManifestConversion(determineManifestConversionInputs{
srcMIMEType: ic.src.ManifestMIMEType,
destSupportedManifestMIMETypes: ic.c.dest.SupportedManifestMIMETypes(),
forceManifestMIMEType: options.ForceManifestMIMEType,
requiresOCIEncryption: destRequiresOciEncryption,
cannotModifyManifestReason: ic.cannotModifyManifestReason,
})
if err != nil {
return nil, "", "", err
}
// We set up this part of ic.manifestUpdates quite early, not just around the
// code that calls copyUpdatedConfigAndManifest, so that other parts of the copy code
// (e.g. the UpdatedImageNeedsLayerDiffIDs check just below) can make decisions based
// on the expected destination format.
if manifestConversionPlan.preferredMIMETypeNeedsConversion {
ic.manifestUpdates.ManifestMIMEType = manifestConversionPlan.preferredMIMEType
}

// If src.UpdatedImageNeedsLayerDiffIDs(ic.manifestUpdates) will be true, it needs to be true by the time we get here.
ic.diffIDsAreNeeded = src.UpdatedImageNeedsLayerDiffIDs(*ic.manifestUpdates)
Expand Down Expand Up @@ -742,22 +744,22 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli
// So, try the preferred manifest MIME type with possibly-updated blob digests, media types, and sizes if
// we're altering how they're compressed. If the process succeeds, fine…
manifestBytes, retManifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance)
retManifestType = preferredManifestMIMEType
retManifestType = manifestConversionPlan.preferredMIMEType
if err != nil {
logrus.Debugf("Writing manifest using preferred type %s failed: %v", preferredManifestMIMEType, err)
logrus.Debugf("Writing manifest using preferred type %s failed: %v", manifestConversionPlan.preferredMIMEType, err)
// … if it fails, and the failure is either because the manifest is rejected by the registry, or
// because we failed to create a manifest of the specified type because the specific manifest type
// doesn't support the type of compression we're trying to use (e.g. docker v2s2 and zstd), we may
// have other options available that could still succeed.
_, isManifestRejected := errors.Cause(err).(types.ManifestTypeRejectedError)
_, isCompressionIncompatible := errors.Cause(err).(manifest.ManifestLayerCompressionIncompatibilityError)
if (!isManifestRejected && !isCompressionIncompatible) || len(otherManifestMIMETypeCandidates) == 0 {
if (!isManifestRejected && !isCompressionIncompatible) || len(manifestConversionPlan.otherMIMETypeCandidates) == 0 {
// We don’t have other options.
// In principle the code below would handle this as well, but the resulting error message is fairly ugly.
// Don’t bother the user with MIME types if we have no choice.
return nil, "", "", err
}
// If the original MIME type is acceptable, determineManifestConversion always uses it as preferredManifestMIMEType.
// If the original MIME type is acceptable, determineManifestConversion always uses it as manifestConversionPlan.preferredMIMEType.
// So if we are here, we will definitely be trying to convert the manifest.
// With ic.cannotModifyManifestReason != "", that would just be a string of repeated failures for the same reason,
// so let’s bail out early and with a better error message.
Expand All @@ -766,8 +768,8 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli
}

// errs is a list of errors when trying various manifest types. Also serves as an "upload succeeded" flag when set to nil.
errs := []string{fmt.Sprintf("%s(%v)", preferredManifestMIMEType, err)}
for _, manifestMIMEType := range otherManifestMIMETypeCandidates {
errs := []string{fmt.Sprintf("%s(%v)", manifestConversionPlan.preferredMIMEType, err)}
for _, manifestMIMEType := range manifestConversionPlan.otherMIMETypeCandidates {
logrus.Debugf("Trying to use manifest type %s…", manifestMIMEType)
ic.manifestUpdates.ManifestMIMEType = manifestMIMEType
attemptedManifest, attemptedManifestDigest, err := ic.copyUpdatedConfigAndManifest(ctx, targetInstance)
Expand Down Expand Up @@ -908,11 +910,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error {

// The manifest is used to extract the information whether a given
// layer is empty.
manifestBlob, manifestType, err := ic.src.Manifest(ctx)
if err != nil {
return err
}
man, err := manifest.FromBlob(manifestBlob, manifestType)
man, err := manifest.FromBlob(ic.src.ManifestBlob, ic.src.ManifestMIMEType)
if err != nil {
return err
}
Expand Down Expand Up @@ -1022,7 +1020,7 @@ func layerDigestsDiffer(a, b []types.BlobInfo) bool {
// stores the resulting config and manifest to the destination, and returns the stored manifest
// and its digest.
func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanceDigest *digest.Digest) ([]byte, digest.Digest, error) {
pendingImage := ic.src
var pendingImage types.Image = ic.src
if !ic.noPendingManifestUpdates() {
if ic.cannotModifyManifestReason != "" {
return nil, "", errors.Errorf("Internal error: copy needs an updated manifest but that was known to be forbidden: %q", ic.cannotModifyManifestReason)
Expand Down
68 changes: 46 additions & 22 deletions copy/manifest.go
View file
Expand Up @@ -38,31 +38,50 @@ func (os *orderedSet) append(s string) {
}
}

// determineManifestConversion updates ic.manifestUpdates to convert manifest to a supported MIME type, if necessary and ic.canModifyManifest.
// Note that the conversion will only happen later, through ic.src.UpdatedImage
// Returns the preferred manifest MIME type (whether we are converting to it or using it unmodified),
// and a list of other possible alternatives, in order.
func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupportedManifestMIMETypes []string, forceManifestMIMEType string, requiresOciEncryption bool) (string, []string, error) {
_, srcType, err := ic.src.Manifest(ctx)
if err != nil { // This should have been cached?!
return "", nil, errors.Wrap(err, "reading manifest")
}
// determineManifestConversionInputs contains the inputs for determineManifestConversion.
type determineManifestConversionInputs struct {
srcMIMEType string // MIME type of the input manifest

destSupportedManifestMIMETypes []string // MIME types supported by the destination, per types.ImageDestination.SupportedManifestMIMETypes()

forceManifestMIMEType string // User’s choice of forced manifest MIME type
requiresOCIEncryption bool // Restrict to manifest formats that can support OCI encryption
cannotModifyManifestReason string // The reason the manifest cannot be modified, or an empty string if it can
}

// manifestConversionPlan contains the decisions made by determineManifestConversion.
type manifestConversionPlan struct {
// The preferred manifest MIME type (whether we are converting to it or using it unmodified).
// We compute this only to show it in error messages; without having to add this context
// in an error message, we would be happy enough to know only that no conversion is needed.
preferredMIMEType string
preferredMIMETypeNeedsConversion bool // True if using preferredMIMEType requires a conversion step.
otherMIMETypeCandidates []string // Other possible alternatives, in order
}

// determineManifestConversion returns a plan for what formats, and possibly conversions, to use based on in.
func determineManifestConversion(in determineManifestConversionInputs) (manifestConversionPlan, error) {
srcType := in.srcMIMEType
normalizedSrcType := manifest.NormalizedMIMEType(srcType)
if srcType != normalizedSrcType {
logrus.Debugf("Source manifest MIME type %s, treating it as %s", srcType, normalizedSrcType)
srcType = normalizedSrcType
}

if forceManifestMIMEType != "" {
destSupportedManifestMIMETypes = []string{forceManifestMIMEType}
destSupportedManifestMIMETypes := in.destSupportedManifestMIMETypes
if in.forceManifestMIMEType != "" {
destSupportedManifestMIMETypes = []string{in.forceManifestMIMEType}
}

if len(destSupportedManifestMIMETypes) == 0 && (!requiresOciEncryption || manifest.MIMETypeSupportsEncryption(srcType)) {
return srcType, []string{}, nil // Anything goes; just use the original as is, do not try any conversions.
if len(destSupportedManifestMIMETypes) == 0 && (!in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(srcType)) {
return manifestConversionPlan{ // Anything goes; just use the original as is, do not try any conversions.
preferredMIMEType: srcType,
otherMIMETypeCandidates: []string{},
}, nil
}
supportedByDest := map[string]struct{}{}
for _, t := range destSupportedManifestMIMETypes {
if !requiresOciEncryption || manifest.MIMETypeSupportsEncryption(t) {
if !in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(t) {
supportedByDest[t] = struct{}{}
}
}
Expand All @@ -79,13 +98,16 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp
if _, ok := supportedByDest[srcType]; ok {
prioritizedTypes.append(srcType)
}
if ic.cannotModifyManifestReason != "" {
if in.cannotModifyManifestReason != "" {
// We could also drop this check and have the caller
// make the choice; it is already doing that to an extent, to improve error
// messages. But it is nice to hide the “if we can't modify, do no conversion”
// special case in here; the caller can then worry (or not) only about a good UI.
logrus.Debugf("We can't modify the manifest, hoping for the best...")
return srcType, []string{}, nil // Take our chances - FIXME? Or should we fail without trying?
return manifestConversionPlan{ // Take our chances - FIXME? Or should we fail without trying?
preferredMIMEType: srcType,
otherMIMETypeCandidates: []string{},
}, nil
}

// Then use our list of preferred types.
Expand All @@ -102,15 +124,17 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp

logrus.Debugf("Manifest has MIME type %s, ordered candidate list [%s]", srcType, strings.Join(prioritizedTypes.list, ", "))
if len(prioritizedTypes.list) == 0 { // Coverage: destSupportedManifestMIMETypes is not empty (or we would have exited in the “Anything goes” case above), so this should never happen.
return "", nil, errors.New("Internal error: no candidate MIME types")
return manifestConversionPlan{}, errors.New("Internal error: no candidate MIME types")
}
preferredType := prioritizedTypes.list[0]
if preferredType != srcType {
ic.manifestUpdates.ManifestMIMEType = preferredType
} else {
res := manifestConversionPlan{
preferredMIMEType: prioritizedTypes.list[0],
otherMIMETypeCandidates: prioritizedTypes.list[1:],
}
res.preferredMIMETypeNeedsConversion = res.preferredMIMEType != srcType
if !res.preferredMIMETypeNeedsConversion {
logrus.Debugf("... will first try using the original manifest unmodified")
}
return preferredType, prioritizedTypes.list[1:], nil
return res, nil
}

// isMultiImage returns true if img is a list of images
Expand Down

0 comments on commit a78a007

Please sign in to comment.