diff --git a/copy/copy.go b/copy/copy.go index aa5ffb9f3..8c7941b58 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -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 @@ -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") } @@ -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") } @@ -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) @@ -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. @@ -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) @@ -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 } @@ -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) diff --git a/copy/manifest.go b/copy/manifest.go index 86ec8863a..b65459f8c 100644 --- a/copy/manifest.go +++ b/copy/manifest.go @@ -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{}{} } } @@ -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. @@ -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 diff --git a/copy/manifest_test.go b/copy/manifest_test.go index d64a61300..36a9a8a24 100644 --- a/copy/manifest_test.go +++ b/copy/manifest_test.go @@ -6,10 +6,8 @@ import ( "fmt" "testing" - "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/internal/testing/mocks" "github.com/containers/image/v5/manifest" - "github.com/containers/image/v5/types" v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -29,56 +27,6 @@ func TestOrderedSet(t *testing.T) { } } -// fakeImageSource is an implementation of types.Image which only returns itself as a MIME type in Manifest -// except that "" means “reading the manifest should fail” -type fakeImageSource string - -func (f fakeImageSource) Reference() types.ImageReference { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) Manifest(ctx context.Context) ([]byte, string, error) { - if string(f) == "" { - return nil, "", errors.New("Manifest() directed to fail") - } - return nil, string(f), nil -} -func (f fakeImageSource) Signatures(context.Context) ([][]byte, error) { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) ConfigInfo() types.BlobInfo { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) ConfigBlob(context.Context) ([]byte, error) { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) OCIConfig(context.Context) (*v1.Image, error) { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) LayerInfos() []types.BlobInfo { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) LayerInfosForCopy(ctx context.Context) ([]types.BlobInfo, error) { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) EmbeddedDockerReferenceConflicts(ref reference.Named) bool { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) Inspect(context.Context) (*types.ImageInspectInfo, error) { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) UpdatedImageNeedsLayerDiffIDs(options types.ManifestUpdateOptions) bool { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) UpdatedImage(ctx context.Context, options types.ManifestUpdateOptions) (types.Image, error) { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) SupportsEncryption(ctx context.Context) bool { - panic("Unexpected call to a mock function") -} -func (f fakeImageSource) Size() (int64, error) { - panic("Unexpected call to a mock function") -} - func TestDetermineManifestConversion(t *testing.T) { supportS1S2OCI := []string{ v1.MediaTypeImageManifest, @@ -102,101 +50,171 @@ func TestDetermineManifestConversion(t *testing.T) { } cases := []struct { - description string - sourceType string - destTypes []string - expectedUpdate string - expectedOtherCandidates []string + description string + sourceType string + destTypes []string + expected manifestConversionPlan }{ // Destination accepts anything — no conversion necessary - {"s1→anything", manifest.DockerV2Schema1SignedMediaType, nil, "", []string{}}, - {"s2→anything", manifest.DockerV2Schema2MediaType, nil, "", []string{}}, + { + "s1→anything", manifest.DockerV2Schema1SignedMediaType, nil, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{}, + }, + }, + { + "s2→anything", manifest.DockerV2Schema2MediaType, nil, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema2MediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{}, + }, + }, // Destination accepts the unmodified original - {"s1→s1s2", manifest.DockerV2Schema1SignedMediaType, supportS1S2, "", []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1MediaType}}, - {"s2→s1s2", manifest.DockerV2Schema2MediaType, supportS1S2, "", supportOnlyS1}, - {"s1→s1", manifest.DockerV2Schema1SignedMediaType, supportOnlyS1, "", []string{manifest.DockerV2Schema1MediaType}}, + { + "s1→s1s2", manifest.DockerV2Schema1SignedMediaType, supportS1S2, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1MediaType}, + }, + }, + { + "s2→s1s2", manifest.DockerV2Schema2MediaType, supportS1S2, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema2MediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: supportOnlyS1, + }, + }, + { + "s1→s1", manifest.DockerV2Schema1SignedMediaType, supportOnlyS1, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{manifest.DockerV2Schema1MediaType}, + }, + }, // text/plain is normalized to s1, and if the destination accepts s1, no conversion happens. - {"text→s1s2", "text/plain", supportS1S2, "", []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1MediaType}}, - {"text→s1", "text/plain", supportOnlyS1, "", []string{manifest.DockerV2Schema1MediaType}}, + { + "text→s1s2", "text/plain", supportS1S2, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1MediaType}, + }, + }, + { + "text→s1", "text/plain", supportOnlyS1, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{manifest.DockerV2Schema1MediaType}, + }, + }, // Conversion necessary, a preferred format is acceptable - {"s2→s1", manifest.DockerV2Schema2MediaType, supportOnlyS1, manifest.DockerV2Schema1SignedMediaType, []string{manifest.DockerV2Schema1MediaType}}, + { + "s2→s1", manifest.DockerV2Schema2MediaType, supportOnlyS1, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{manifest.DockerV2Schema1MediaType}, + }, + }, // Conversion necessary, a preferred format is not acceptable - {"s2→OCI", manifest.DockerV2Schema2MediaType, []string{v1.MediaTypeImageManifest}, v1.MediaTypeImageManifest, []string{}}, + { + "s2→OCI", manifest.DockerV2Schema2MediaType, []string{v1.MediaTypeImageManifest}, + manifestConversionPlan{ + preferredMIMEType: v1.MediaTypeImageManifest, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{}, + }, + }, // text/plain is converted if the destination does not accept s1 - {"text→s2", "text/plain", []string{manifest.DockerV2Schema2MediaType}, manifest.DockerV2Schema2MediaType, []string{}}, + { + "text→s2", "text/plain", []string{manifest.DockerV2Schema2MediaType}, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema2MediaType, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{}, + }, + }, // Conversion necessary, try the preferred formats in order. // We abuse manifest.DockerV2ListMediaType here as a MIME type which is not in supportS1S2OCI, // but is still recognized by manifest.NormalizedMIMEType and not normalized to s1 { - "special→s2", manifest.DockerV2ListMediaType, supportS1S2OCI, manifest.DockerV2Schema2MediaType, - []string{manifest.DockerV2Schema1SignedMediaType, v1.MediaTypeImageManifest, manifest.DockerV2Schema1MediaType}, + "special→s2", manifest.DockerV2ListMediaType, supportS1S2OCI, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema2MediaType, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{manifest.DockerV2Schema1SignedMediaType, v1.MediaTypeImageManifest, manifest.DockerV2Schema1MediaType}, + }, }, { - "special→s1", manifest.DockerV2ListMediaType, supportS1OCI, manifest.DockerV2Schema1SignedMediaType, - []string{v1.MediaTypeImageManifest, manifest.DockerV2Schema1MediaType}, + "special→s1", manifest.DockerV2ListMediaType, supportS1OCI, + manifestConversionPlan{ + preferredMIMEType: manifest.DockerV2Schema1SignedMediaType, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{v1.MediaTypeImageManifest, manifest.DockerV2Schema1MediaType}, + }, }, { - "special→OCI", manifest.DockerV2ListMediaType, []string{v1.MediaTypeImageManifest, "other options", "with lower priority"}, v1.MediaTypeImageManifest, - []string{"other options", "with lower priority"}, + "special→OCI", manifest.DockerV2ListMediaType, []string{v1.MediaTypeImageManifest, "other options", "with lower priority"}, + manifestConversionPlan{ + preferredMIMEType: v1.MediaTypeImageManifest, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{"other options", "with lower priority"}, + }, }, } for _, c := range cases { - src := fakeImageSource(c.sourceType) - ic := &imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, - src: src, - cannotModifyManifestReason: "", - } - preferredMIMEType, otherCandidates, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) + res, err := determineManifestConversion(determineManifestConversionInputs{ + srcMIMEType: c.sourceType, + destSupportedManifestMIMETypes: c.destTypes, + forceManifestMIMEType: "", + requiresOCIEncryption: false, + cannotModifyManifestReason: "", + }) require.NoError(t, err, c.description) - assert.Equal(t, c.expectedUpdate, ic.manifestUpdates.ManifestMIMEType, c.description) - if c.expectedUpdate == "" { - assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), preferredMIMEType, c.description) - } else { - assert.Equal(t, c.expectedUpdate, preferredMIMEType, c.description) - } - assert.Equal(t, c.expectedOtherCandidates, otherCandidates, c.description) + assert.Equal(t, c.expected, res, c.description) } - // Whatever the input is, with !canModifyManifest we return "keep the original as is" + // Whatever the input is, with cannotModifyManifestReason we return "keep the original as is" for _, c := range cases { - src := fakeImageSource(c.sourceType) - ic := &imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, - src: src, - cannotModifyManifestReason: "Preserving digests", - } - preferredMIMEType, otherCandidates, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) + res, err := determineManifestConversion(determineManifestConversionInputs{ + srcMIMEType: c.sourceType, + destSupportedManifestMIMETypes: c.destTypes, + forceManifestMIMEType: "", + requiresOCIEncryption: false, + cannotModifyManifestReason: "Preserving digests", + }) require.NoError(t, err, c.description) - assert.Equal(t, "", ic.manifestUpdates.ManifestMIMEType, c.description) - assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), preferredMIMEType, c.description) - assert.Equal(t, []string{}, otherCandidates, c.description) + assert.Equal(t, manifestConversionPlan{ + preferredMIMEType: manifest.NormalizedMIMEType(c.sourceType), + preferredMIMETypeNeedsConversion: false, + otherMIMETypeCandidates: []string{}, + }, res, c.description) } // With forceManifestMIMEType, the output is always the forced manifest type (in this case oci manifest) for _, c := range cases { - src := fakeImageSource(c.sourceType) - ic := &imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, - src: src, - cannotModifyManifestReason: "", - } - preferredMIMEType, otherCandidates, err := ic.determineManifestConversion(context.Background(), c.destTypes, v1.MediaTypeImageManifest, false) + res, err := determineManifestConversion(determineManifestConversionInputs{ + srcMIMEType: c.sourceType, + destSupportedManifestMIMETypes: c.destTypes, + forceManifestMIMEType: v1.MediaTypeImageManifest, + requiresOCIEncryption: false, + cannotModifyManifestReason: "", + }) require.NoError(t, err, c.description) - assert.Equal(t, v1.MediaTypeImageManifest, ic.manifestUpdates.ManifestMIMEType, c.description) - assert.Equal(t, v1.MediaTypeImageManifest, preferredMIMEType, c.description) - assert.Equal(t, []string{}, otherCandidates, c.description) + assert.Equal(t, manifestConversionPlan{ + preferredMIMEType: v1.MediaTypeImageManifest, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{}, + }, res, c.description) } - - // Error reading the manifest — smoke test only. - ic := imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, - src: fakeImageSource(""), - cannotModifyManifestReason: "", - } - _, _, err := ic.determineManifestConversion(context.Background(), supportS1S2, "", false) - assert.Error(t, err) } // fakeUnparsedImage is an implementation of types.UnparsedImage which only returns itself as a MIME type in Manifest, diff --git a/internal/image/sourced.go b/internal/image/sourced.go index 7dfabfa38..dc09a9e04 100644 --- a/internal/image/sourced.go +++ b/internal/image/sourced.go @@ -70,15 +70,18 @@ func (ic *imageCloser) Close() error { return ic.src.Close() } -// sourcedImage is a general set of utilities for working with container images, -// whatever is their underlying location (i.e. dockerImageSource-independent). -// Note the existence of skopeo/docker.Image: some instances of a `types.Image` -// may not be a `sourcedImage` directly. However, most users of `types.Image` -// do not care, and those who care about `skopeo/docker.Image` know they do. -type sourcedImage struct { +// SourcedImage is a general set of utilities for working with container images, +// whatever is their underlying transport (i.e. ImageSource-independent). +// Note the existence of docker.Image and image.memoryImage: various instances +// of a types.Image may not be a SourcedImage directly. +// +// Most external users of `types.Image` do not care, and those who care about `docker.Image` know they do. +// +// Internal users may depend on methods available in SourcedImage but not (yet?) in types.Image. +type SourcedImage struct { *UnparsedImage - manifestBlob []byte - manifestMIMEType string + ManifestBlob []byte // The manifest of the relevant instance + ManifestMIMEType string // MIME type of ManifestBlob // genericManifest contains data corresponding to manifestBlob. // NOTE: The manifest may have been modified in the process; DO NOT reserialize and store genericManifest // if you want to preserve the original manifest; use manifestBlob directly. @@ -92,7 +95,7 @@ type sourcedImage struct { // The Image must not be used after the underlying ImageSource is Close()d. // // This is publicly visible as c/image/image.FromUnparsedImage. -func FromUnparsedImage(ctx context.Context, sys *types.SystemContext, unparsed *UnparsedImage) (types.Image, error) { +func FromUnparsedImage(ctx context.Context, sys *types.SystemContext, unparsed *UnparsedImage) (*SourcedImage, error) { // Note that the input parameter above is specifically *image.UnparsedImage, not types.UnparsedImage: // we want to be able to use unparsed.src. We could make that an explicit interface, but, well, // this is the only UnparsedImage implementation around, anyway. @@ -108,24 +111,24 @@ func FromUnparsedImage(ctx context.Context, sys *types.SystemContext, unparsed * return nil, err } - return &sourcedImage{ + return &SourcedImage{ UnparsedImage: unparsed, - manifestBlob: manifestBlob, - manifestMIMEType: manifestMIMEType, + ManifestBlob: manifestBlob, + ManifestMIMEType: manifestMIMEType, genericManifest: parsedManifest, }, nil } // Size returns the size of the image as stored, if it's known, or -1 if it isn't. -func (i *sourcedImage) Size() (int64, error) { +func (i *SourcedImage) Size() (int64, error) { return -1, nil } // Manifest overrides the UnparsedImage.Manifest to always use the fields which we have already fetched. -func (i *sourcedImage) Manifest(ctx context.Context) ([]byte, string, error) { - return i.manifestBlob, i.manifestMIMEType, nil +func (i *SourcedImage) Manifest(ctx context.Context) ([]byte, string, error) { + return i.ManifestBlob, i.ManifestMIMEType, nil } -func (i *sourcedImage) LayerInfosForCopy(ctx context.Context) ([]types.BlobInfo, error) { +func (i *SourcedImage) LayerInfosForCopy(ctx context.Context) ([]types.BlobInfo, error) { return i.UnparsedImage.src.LayerInfosForCopy(ctx, i.UnparsedImage.instanceDigest) }