From 0cf416bd100788978d294d742084bb4a3a932ce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 10 Jun 2022 22:17:44 +0200 Subject: [PATCH 01/10] Have image.FromUnparsedImage return a SourcedImage instead of types.Image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will allow us to add more methods to SourcedImage without breaking external implementations of types.Image, if any. Should not change behavior. Signed-off-by: Miloslav Trmač --- internal/image/sourced.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/internal/image/sourced.go b/internal/image/sourced.go index 7dfabfa38..b99da7b46 100644 --- a/internal/image/sourced.go +++ b/internal/image/sourced.go @@ -70,12 +70,15 @@ 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 @@ -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,7 +111,7 @@ func FromUnparsedImage(ctx context.Context, sys *types.SystemContext, unparsed * return nil, err } - return &sourcedImage{ + return &SourcedImage{ UnparsedImage: unparsed, manifestBlob: manifestBlob, manifestMIMEType: manifestMIMEType, @@ -117,15 +120,15 @@ func FromUnparsedImage(ctx context.Context, sys *types.SystemContext, unparsed * } // 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) { +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) } From 2874e99f7a7d616c35146a95b0265525fb63a97b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 14 Jun 2022 00:17:17 +0200 Subject: [PATCH 02/10] Make SourcedImage.{ManifestBlob,ManifestMIMEType} public MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We know these values are available without having to worry about errors, which allows simplifying users in c/image/copy. We don't really need accessor functions, all users are internal (and a public field is actually better protected than a public method, because a public method can be used by third parties through a caller-defined interface). It's also usefo to make them available as individual values; various users only need one or the other. Signed-off-by: Miloslav Trmač --- internal/image/sourced.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/image/sourced.go b/internal/image/sourced.go index b99da7b46..dc09a9e04 100644 --- a/internal/image/sourced.go +++ b/internal/image/sourced.go @@ -80,8 +80,8 @@ func (ic *imageCloser) Close() error { // 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. @@ -113,8 +113,8 @@ func FromUnparsedImage(ctx context.Context, sys *types.SystemContext, unparsed * return &SourcedImage{ UnparsedImage: unparsed, - manifestBlob: manifestBlob, - manifestMIMEType: manifestMIMEType, + ManifestBlob: manifestBlob, + ManifestMIMEType: manifestMIMEType, genericManifest: parsedManifest, }, nil } @@ -126,7 +126,7 @@ func (i *SourcedImage) Size() (int64, error) { // 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 + return i.ManifestBlob, i.ManifestMIMEType, nil } func (i *SourcedImage) LayerInfosForCopy(ctx context.Context) ([]types.BlobInfo, error) { From cba0127e57dea14610bef8c17fa39a67d47e0abb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Jun 2022 21:44:41 +0200 Subject: [PATCH 03/10] Introduce manifestConversionPlan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To make determineManifestConversion easier to test, we'll want to separate it from imageCopier and make it a pure function. To do that, start with introducing a struct for its return value; that way we won't need three unnamed return values, and three variables with long names at the call site. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 16 +++++++--------- copy/manifest.go | 39 ++++++++++++++++++++++++++++----------- copy/manifest_test.go | 22 +++++++++++----------- 3 files changed, 46 insertions(+), 31 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index aa5ffb9f3..ff04b8555 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -702,9 +702,7 @@ 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 := ic.determineManifestConversion(ctx, c.dest.SupportedManifestMIMETypes(), options.ForceManifestMIMEType, destRequiresOciEncryption) if err != nil { return nil, "", "", err } @@ -742,22 +740,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 +764,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) diff --git a/copy/manifest.go b/copy/manifest.go index 86ec8863a..d5f48250e 100644 --- a/copy/manifest.go +++ b/copy/manifest.go @@ -38,14 +38,22 @@ func (os *orderedSet) append(s string) { } } +// 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 + otherMIMETypeCandidates []string // Other possible alternatives, in order +} + // 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) { +// Returns a plan for what formats, and possibly conversions, to use for the manifest in ic. +func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupportedManifestMIMETypes []string, forceManifestMIMEType string, requiresOciEncryption bool) (manifestConversionPlan, error) { _, srcType, err := ic.src.Manifest(ctx) if err != nil { // This should have been cached?! - return "", nil, errors.Wrap(err, "reading manifest") + return manifestConversionPlan{}, errors.Wrap(err, "reading manifest") } normalizedSrcType := manifest.NormalizedMIMEType(srcType) if srcType != normalizedSrcType { @@ -58,7 +66,10 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp } 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. + 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 { @@ -85,7 +96,10 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp // 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 +116,18 @@ 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") + } + res := manifestConversionPlan{ + preferredMIMEType: prioritizedTypes.list[0], + otherMIMETypeCandidates: prioritizedTypes.list[1:], } - preferredType := prioritizedTypes.list[0] - if preferredType != srcType { - ic.manifestUpdates.ManifestMIMEType = preferredType + if res.preferredMIMEType != srcType { + ic.manifestUpdates.ManifestMIMEType = res.preferredMIMEType } else { 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..6c7e7f97f 100644 --- a/copy/manifest_test.go +++ b/copy/manifest_test.go @@ -148,15 +148,15 @@ func TestDetermineManifestConversion(t *testing.T) { src: src, cannotModifyManifestReason: "", } - preferredMIMEType, otherCandidates, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) + res, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) 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) + assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), res.preferredMIMEType, c.description) } else { - assert.Equal(t, c.expectedUpdate, preferredMIMEType, c.description) + assert.Equal(t, c.expectedUpdate, res.preferredMIMEType, c.description) } - assert.Equal(t, c.expectedOtherCandidates, otherCandidates, c.description) + assert.Equal(t, c.expectedOtherCandidates, res.otherMIMETypeCandidates, c.description) } // Whatever the input is, with !canModifyManifest we return "keep the original as is" @@ -167,11 +167,11 @@ func TestDetermineManifestConversion(t *testing.T) { src: src, cannotModifyManifestReason: "Preserving digests", } - preferredMIMEType, otherCandidates, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) + res, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) 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, manifest.NormalizedMIMEType(c.sourceType), res.preferredMIMEType, c.description) + assert.Equal(t, []string{}, res.otherMIMETypeCandidates, c.description) } // With forceManifestMIMEType, the output is always the forced manifest type (in this case oci manifest) @@ -182,11 +182,11 @@ func TestDetermineManifestConversion(t *testing.T) { src: src, cannotModifyManifestReason: "", } - preferredMIMEType, otherCandidates, err := ic.determineManifestConversion(context.Background(), c.destTypes, v1.MediaTypeImageManifest, false) + res, err := ic.determineManifestConversion(context.Background(), c.destTypes, v1.MediaTypeImageManifest, false) 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, v1.MediaTypeImageManifest, res.preferredMIMEType, c.description) + assert.Equal(t, []string{}, res.otherMIMETypeCandidates, c.description) } // Error reading the manifest — smoke test only. @@ -195,7 +195,7 @@ func TestDetermineManifestConversion(t *testing.T) { src: fakeImageSource(""), cannotModifyManifestReason: "", } - _, _, err := ic.determineManifestConversion(context.Background(), supportS1S2, "", false) + _, err := ic.determineManifestConversion(context.Background(), supportS1S2, "", false) assert.Error(t, err) } From 225269f7b7cf293de19375756d65fe7462d0c5bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Jun 2022 21:59:03 +0200 Subject: [PATCH 04/10] Move the manifest conversion setup out of determineManifestConversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Have determineManifestConversion just set a flag in the returned plan; then copier.copyOneImage will actually act on that flag. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 7 +++++++ copy/manifest.go | 14 ++++++-------- copy/manifest_test.go | 10 +++------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index ff04b8555..e7e159461 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -706,6 +706,13 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli 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) diff --git a/copy/manifest.go b/copy/manifest.go index d5f48250e..5d782a266 100644 --- a/copy/manifest.go +++ b/copy/manifest.go @@ -43,13 +43,12 @@ 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 - otherMIMETypeCandidates []string // Other possible alternatives, in order + preferredMIMEType string + preferredMIMETypeNeedsConversion bool // True if using preferredMIMEType requires a conversion step. + otherMIMETypeCandidates []string // Other possible alternatives, in order } -// 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 a plan for what formats, and possibly conversions, to use for the manifest in ic. +// determineManifestConversion returns a plan for what formats, and possibly conversions, to use for the manifest in ic. func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupportedManifestMIMETypes []string, forceManifestMIMEType string, requiresOciEncryption bool) (manifestConversionPlan, error) { _, srcType, err := ic.src.Manifest(ctx) if err != nil { // This should have been cached?! @@ -122,9 +121,8 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp preferredMIMEType: prioritizedTypes.list[0], otherMIMETypeCandidates: prioritizedTypes.list[1:], } - if res.preferredMIMEType != srcType { - ic.manifestUpdates.ManifestMIMEType = res.preferredMIMEType - } else { + res.preferredMIMETypeNeedsConversion = res.preferredMIMEType != srcType + if !res.preferredMIMETypeNeedsConversion { logrus.Debugf("... will first try using the original manifest unmodified") } return res, nil diff --git a/copy/manifest_test.go b/copy/manifest_test.go index 6c7e7f97f..4e23967d6 100644 --- a/copy/manifest_test.go +++ b/copy/manifest_test.go @@ -144,13 +144,12 @@ func TestDetermineManifestConversion(t *testing.T) { for _, c := range cases { src := fakeImageSource(c.sourceType) ic := &imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, src: src, cannotModifyManifestReason: "", } res, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) require.NoError(t, err, c.description) - assert.Equal(t, c.expectedUpdate, ic.manifestUpdates.ManifestMIMEType, c.description) + assert.Equal(t, c.expectedUpdate != "", res.preferredMIMETypeNeedsConversion, c.description) if c.expectedUpdate == "" { assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), res.preferredMIMEType, c.description) } else { @@ -163,13 +162,12 @@ func TestDetermineManifestConversion(t *testing.T) { for _, c := range cases { src := fakeImageSource(c.sourceType) ic := &imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, src: src, cannotModifyManifestReason: "Preserving digests", } res, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) require.NoError(t, err, c.description) - assert.Equal(t, "", ic.manifestUpdates.ManifestMIMEType, c.description) + assert.False(t, res.preferredMIMETypeNeedsConversion, c.description) assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), res.preferredMIMEType, c.description) assert.Equal(t, []string{}, res.otherMIMETypeCandidates, c.description) } @@ -178,20 +176,18 @@ func TestDetermineManifestConversion(t *testing.T) { for _, c := range cases { src := fakeImageSource(c.sourceType) ic := &imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, src: src, cannotModifyManifestReason: "", } res, err := ic.determineManifestConversion(context.Background(), c.destTypes, v1.MediaTypeImageManifest, false) require.NoError(t, err, c.description) - assert.Equal(t, v1.MediaTypeImageManifest, ic.manifestUpdates.ManifestMIMEType, c.description) + assert.True(t, res.preferredMIMETypeNeedsConversion, c.description) assert.Equal(t, v1.MediaTypeImageManifest, res.preferredMIMEType, c.description) assert.Equal(t, []string{}, res.otherMIMETypeCandidates, c.description) } // Error reading the manifest — smoke test only. ic := imageCopier{ - manifestUpdates: &types.ManifestUpdateOptions{}, src: fakeImageSource(""), cannotModifyManifestReason: "", } From e117048c30b14c3a3d2d384ae134ad5170f9e351 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Jun 2022 22:39:36 +0200 Subject: [PATCH 05/10] Use manifestConversionPlan for expected results in TestDetermineManifestConversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is not smaller, but a bit more readable and consistent with the real call site. Also, on test failures we can see the full returned plan as a single unit, making it possibly easier to see what went wrong. Should not change (test) behavior. Signed-off-by: Miloslav Trmač --- copy/manifest_test.go | 147 ++++++++++++++++++++++++++++++++---------- 1 file changed, 113 insertions(+), 34 deletions(-) diff --git a/copy/manifest_test.go b/copy/manifest_test.go index 4e23967d6..c173250a9 100644 --- a/copy/manifest_test.go +++ b/copy/manifest_test.go @@ -102,42 +102,123 @@ 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"}, + }, }, } @@ -149,13 +230,7 @@ func TestDetermineManifestConversion(t *testing.T) { } res, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) require.NoError(t, err, c.description) - assert.Equal(t, c.expectedUpdate != "", res.preferredMIMETypeNeedsConversion, c.description) - if c.expectedUpdate == "" { - assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), res.preferredMIMEType, c.description) - } else { - assert.Equal(t, c.expectedUpdate, res.preferredMIMEType, c.description) - } - assert.Equal(t, c.expectedOtherCandidates, res.otherMIMETypeCandidates, c.description) + assert.Equal(t, c.expected, res, c.description) } // Whatever the input is, with !canModifyManifest we return "keep the original as is" @@ -167,9 +242,11 @@ func TestDetermineManifestConversion(t *testing.T) { } res, err := ic.determineManifestConversion(context.Background(), c.destTypes, "", false) require.NoError(t, err, c.description) - assert.False(t, res.preferredMIMETypeNeedsConversion, c.description) - assert.Equal(t, manifest.NormalizedMIMEType(c.sourceType), res.preferredMIMEType, c.description) - assert.Equal(t, []string{}, res.otherMIMETypeCandidates, 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) @@ -181,9 +258,11 @@ func TestDetermineManifestConversion(t *testing.T) { } res, err := ic.determineManifestConversion(context.Background(), c.destTypes, v1.MediaTypeImageManifest, false) require.NoError(t, err, c.description) - assert.True(t, res.preferredMIMETypeNeedsConversion, c.description) - assert.Equal(t, v1.MediaTypeImageManifest, res.preferredMIMEType, c.description) - assert.Equal(t, []string{}, res.otherMIMETypeCandidates, c.description) + assert.Equal(t, manifestConversionPlan{ + preferredMIMEType: v1.MediaTypeImageManifest, + preferredMIMETypeNeedsConversion: true, + otherMIMETypeCandidates: []string{}, + }, res, c.description) } // Error reading the manifest — smoke test only. From f395eeb9453dcec803dc6d800636f67038b9e134 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Jun 2022 22:51:56 +0200 Subject: [PATCH 06/10] Introduce determineManifestConversionInputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... and split the actual logic (determineManifestConversion) from imageCopier (imageCopier.determineManifestConversion). determineManifestConversion is now a pure function (well, apart from logging). Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/manifest.go | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/copy/manifest.go b/copy/manifest.go index 5d782a266..f73a151fc 100644 --- a/copy/manifest.go +++ b/copy/manifest.go @@ -38,6 +38,17 @@ func (os *orderedSet) append(s string) { } } +// 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). @@ -54,17 +65,30 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp if err != nil { // This should have been cached?! return manifestConversionPlan{}, errors.Wrap(err, "reading manifest") } + return determineManifestConversion(determineManifestConversionInputs{ + srcMIMEType: srcType, + destSupportedManifestMIMETypes: destSupportedManifestMIMETypes, + forceManifestMIMEType: forceManifestMIMEType, + requiresOCIEncryption: requiresOciEncryption, + cannotModifyManifestReason: ic.cannotModifyManifestReason, + }) +} + +// 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)) { + 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{}, @@ -72,7 +96,7 @@ func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupp } supportedByDest := map[string]struct{}{} for _, t := range destSupportedManifestMIMETypes { - if !requiresOciEncryption || manifest.MIMETypeSupportsEncryption(t) { + if !in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(t) { supportedByDest[t] = struct{}{} } } @@ -89,7 +113,7 @@ 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” From 7602cbd8afa186f113c84993a247c2a14c917be1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Jun 2022 22:59:43 +0200 Subject: [PATCH 07/10] Mostly test the pure function in TestDetermineManifestConversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep the "reading manifest failed" case, for now. Should not change (test) behavior, except that the test coverage is now a tiny bit smaller - but then the test not depending on imageCopier is the primary reason for this refactoring. Signed-off-by: Miloslav Trmač --- copy/manifest_test.go | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/copy/manifest_test.go b/copy/manifest_test.go index c173250a9..da2cfa11a 100644 --- a/copy/manifest_test.go +++ b/copy/manifest_test.go @@ -223,24 +223,26 @@ func TestDetermineManifestConversion(t *testing.T) { } for _, c := range cases { - src := fakeImageSource(c.sourceType) - ic := &imageCopier{ - src: src, - cannotModifyManifestReason: "", - } - res, 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.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{ - src: src, - cannotModifyManifestReason: "Preserving digests", - } - res, 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, manifestConversionPlan{ preferredMIMEType: manifest.NormalizedMIMEType(c.sourceType), @@ -251,12 +253,13 @@ func TestDetermineManifestConversion(t *testing.T) { // 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{ - src: src, - cannotModifyManifestReason: "", - } - res, 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, manifestConversionPlan{ preferredMIMEType: v1.MediaTypeImageManifest, From cce475b0ac1cac8aee4f6341a0e5874f17ea52dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 14 Jun 2022 00:11:53 +0200 Subject: [PATCH 08/10] Record an *image.SourcedImage in imageCopier.src MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is surprisingly somewhat convoluted. We've had to do a bunch of work to make determineManifestConversion not depend on a complete (and mostly valid!) image, to be able to make this change. Then, so that we don't lose test coverage (and to actually benefit from the new private type), use SourcedImage.ManifestMIMEType in imageCopier.determineManifestConversion. That allows us to get rid of the last imageCopier.determineManifestConversion call in tests. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 6 ++--- copy/manifest.go | 8 ++---- copy/manifest_test.go | 60 ------------------------------------------- 3 files changed, 5 insertions(+), 69 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index e7e159461..59dc7c2c6 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 @@ -702,7 +702,7 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli destRequiresOciEncryption := (isEncrypted(src) && ic.c.ociDecryptConfig != nil) || options.OciEncryptLayers != nil - manifestConversionPlan, err := ic.determineManifestConversion(ctx, c.dest.SupportedManifestMIMETypes(), options.ForceManifestMIMEType, destRequiresOciEncryption) + manifestConversionPlan, err := ic.determineManifestConversion(c.dest.SupportedManifestMIMETypes(), options.ForceManifestMIMEType, destRequiresOciEncryption) if err != nil { return nil, "", "", err } @@ -1027,7 +1027,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 f73a151fc..eec3ec920 100644 --- a/copy/manifest.go +++ b/copy/manifest.go @@ -60,13 +60,9 @@ type manifestConversionPlan struct { } // determineManifestConversion returns a plan for what formats, and possibly conversions, to use for the manifest in ic. -func (ic *imageCopier) determineManifestConversion(ctx context.Context, destSupportedManifestMIMETypes []string, forceManifestMIMEType string, requiresOciEncryption bool) (manifestConversionPlan, error) { - _, srcType, err := ic.src.Manifest(ctx) - if err != nil { // This should have been cached?! - return manifestConversionPlan{}, errors.Wrap(err, "reading manifest") - } +func (ic *imageCopier) determineManifestConversion(destSupportedManifestMIMETypes []string, forceManifestMIMEType string, requiresOciEncryption bool) (manifestConversionPlan, error) { return determineManifestConversion(determineManifestConversionInputs{ - srcMIMEType: srcType, + srcMIMEType: ic.src.ManifestMIMEType, destSupportedManifestMIMETypes: destSupportedManifestMIMETypes, forceManifestMIMEType: forceManifestMIMEType, requiresOCIEncryption: requiresOciEncryption, diff --git a/copy/manifest_test.go b/copy/manifest_test.go index da2cfa11a..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, @@ -267,14 +215,6 @@ func TestDetermineManifestConversion(t *testing.T) { otherMIMETypeCandidates: []string{}, }, res, c.description) } - - // Error reading the manifest — smoke test only. - ic := imageCopier{ - 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, From f07460093ce339eade1989bdfd04b9c44514281d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 14 Jun 2022 00:13:02 +0200 Subject: [PATCH 09/10] Use StoredImage.StoredManifest() where appropriate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This eliminates some unreachable error paths. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 59dc7c2c6..fe97ef090 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -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") } @@ -913,11 +904,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 } From acbde71ee67d6e6a81c0ec8aed87bafc2e353f72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Jun 2022 23:11:38 +0200 Subject: [PATCH 10/10] Inline imageCopier.determineManifestConversion into the caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's now basically a trivial call forwarder we don't need for anything, so just have the caller set determineManifestConversionInputs directly (and let it benefit from labeling the inputs). Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 8 +++++++- copy/manifest.go | 11 ----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index fe97ef090..8c7941b58 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -693,7 +693,13 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli destRequiresOciEncryption := (isEncrypted(src) && ic.c.ociDecryptConfig != nil) || options.OciEncryptLayers != nil - manifestConversionPlan, err := ic.determineManifestConversion(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 } diff --git a/copy/manifest.go b/copy/manifest.go index eec3ec920..b65459f8c 100644 --- a/copy/manifest.go +++ b/copy/manifest.go @@ -59,17 +59,6 @@ type manifestConversionPlan struct { otherMIMETypeCandidates []string // Other possible alternatives, in order } -// determineManifestConversion returns a plan for what formats, and possibly conversions, to use for the manifest in ic. -func (ic *imageCopier) determineManifestConversion(destSupportedManifestMIMETypes []string, forceManifestMIMEType string, requiresOciEncryption bool) (manifestConversionPlan, error) { - return determineManifestConversion(determineManifestConversionInputs{ - srcMIMEType: ic.src.ManifestMIMEType, - destSupportedManifestMIMETypes: destSupportedManifestMIMETypes, - forceManifestMIMEType: forceManifestMIMEType, - requiresOCIEncryption: requiresOciEncryption, - cannotModifyManifestReason: ic.cannotModifyManifestReason, - }) -} - // determineManifestConversion returns a plan for what formats, and possibly conversions, to use based on in. func determineManifestConversion(in determineManifestConversionInputs) (manifestConversionPlan, error) { srcType := in.srcMIMEType