Merge pull request #2204 from mtrmac/zstd-manifest-choice

Trigger a conversion to OCI when compressing to Zstd

copy/manifest.go

@@ -6,8 +6,10 @@ import (
 	"fmt"
 	"strings"
 
+	internalManifest "github.com/containers/image/v5/internal/manifest"
 	"github.com/containers/image/v5/internal/set"
 	"github.com/containers/image/v5/manifest"
+	compressiontypes "github.com/containers/image/v5/pkg/compression/types"
 	"github.com/containers/image/v5/types"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/sirupsen/logrus"
@@ -19,8 +21,8 @@ import (
 // Include v2s1 signed but not v2s1 unsigned, because docker/distribution requires a signature even if the unsigned MIME type is used.
 var preferredManifestMIMETypes = []string{manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1SignedMediaType}
 
-// ociEncryptionMIMETypes lists manifest MIME types that are known to support OCI encryption.
-var ociEncryptionMIMETypes = []string{v1.MediaTypeImageManifest}
+// allManifestMIMETypes lists all possible manifest MIME types.
+var allManifestMIMETypes = []string{v1.MediaTypeImageManifest, manifest.DockerV2Schema2MediaType, manifest.DockerV2Schema1SignedMediaType, manifest.DockerV2Schema1MediaType}
 
 // orderedSet is a list of strings (MIME types or platform descriptors in our case), with each string appearing at most once.
 type orderedSet struct {
@@ -51,9 +53,10 @@ type determineManifestConversionInputs struct {
 
 	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
+	forceManifestMIMEType      string                      // User’s choice of forced manifest MIME type
+	requestedCompressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user _explictily_ requested one.
+	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.
@@ -80,41 +83,74 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
 		destSupportedManifestMIMETypes = []string{in.forceManifestMIMEType}
 	}
 
+	restrictiveCompressionRequired := in.requestedCompressionFormat != nil && !internalManifest.CompressionAlgorithmIsUniversallySupported(*in.requestedCompressionFormat)
 	if len(destSupportedManifestMIMETypes) == 0 {
-		if !in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(srcType) {
+		if (!in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(srcType)) &&
+			(!restrictiveCompressionRequired || internalManifest.MIMETypeSupportsCompressionAlgorithm(srcType, *in.requestedCompressionFormat)) {
 			return manifestConversionPlan{ // Anything goes; just use the original as is, do not try any conversions.
 				preferredMIMEType:       srcType,
 				otherMIMETypeCandidates: []string{},
 			}, nil
 		}
-		destSupportedManifestMIMETypes = ociEncryptionMIMETypes
+		destSupportedManifestMIMETypes = allManifestMIMETypes
 	}
 	supportedByDest := set.New[string]()
 	for _, t := range destSupportedManifestMIMETypes {
-		if !in.requiresOCIEncryption || manifest.MIMETypeSupportsEncryption(t) {
-			supportedByDest.Add(t)
+		if in.requiresOCIEncryption && !manifest.MIMETypeSupportsEncryption(t) {
+			continue
 		}
+		if restrictiveCompressionRequired && !internalManifest.MIMETypeSupportsCompressionAlgorithm(t, *in.requestedCompressionFormat) {
+			continue
+		}
+		supportedByDest.Add(t)
 	}
 	if supportedByDest.Empty() {
-		if len(destSupportedManifestMIMETypes) == 0 { // Coverage: This should never happen, empty values were replaced by ociEncryptionMIMETypes
+		if len(destSupportedManifestMIMETypes) == 0 { // Coverage: This should never happen, empty values were replaced by allManifestMIMETypes
 			return manifestConversionPlan{}, errors.New("internal error: destSupportedManifestMIMETypes is empty")
 		}
-		// We know, and have verified, that destSupportedManifestMIMETypes is not empty, so encryption must have been involved.
-		if !in.requiresOCIEncryption { // Coverage: This should never happen, destSupportedManifestMIMETypes was not empty, so we should have filtered for encryption.
-			return manifestConversionPlan{}, errors.New("internal error: supportedByDest is empty but destSupportedManifestMIMETypes is not, and not encrypting")
-		}
+		// We know, and have verified, that destSupportedManifestMIMETypes is not empty, so some filtering of supported MIME types must have been involved.
+
 		// destSupportedManifestMIMETypes has three possible origins:
 		if in.forceManifestMIMEType != "" { // 1. forceManifestType specified
-			return manifestConversionPlan{}, fmt.Errorf("encryption required together with format %s, which does not support encryption",
-				in.forceManifestMIMEType)
+			switch {
+			case in.requiresOCIEncryption && restrictiveCompressionRequired:
+				return manifestConversionPlan{}, fmt.Errorf("compression using %s, and encryption, required together with format %s, which does not support both",
+					in.requestedCompressionFormat.Name(), in.forceManifestMIMEType)
+			case in.requiresOCIEncryption:
+				return manifestConversionPlan{}, fmt.Errorf("encryption required together with format %s, which does not support encryption",
+					in.forceManifestMIMEType)
+			case restrictiveCompressionRequired:
+				return manifestConversionPlan{}, fmt.Errorf("compression using %s required together with format %s, which does not support it",
+					in.requestedCompressionFormat.Name(), in.forceManifestMIMEType)
+			default:
+				return manifestConversionPlan{}, errors.New("internal error: forceManifestMIMEType was rejected for an unknown reason")
+			}
+		}
+		if len(in.destSupportedManifestMIMETypes) == 0 { // 2. destination accepts anything and we have chosen allManifestTypes
+			if !restrictiveCompressionRequired {
+				// Coverage: This should never happen.
+				// If we have not rejected for encryption reasons, we must have rejected due to encryption, but
+				// allManifestTypes includes OCI, which supports encryption.
+				return manifestConversionPlan{}, errors.New("internal error: in.destSupportedManifestMIMETypes is empty but supportedByDest is empty as well")
+			}
+			// This can legitimately happen when the user asks for completely unsupported formats like Bzip2 or Xz.
+			return manifestConversionPlan{}, fmt.Errorf("compression using %s required, but none of the known manifest formats support it", in.requestedCompressionFormat.Name())
 		}
-		if len(in.destSupportedManifestMIMETypes) == 0 { // 2. destination accepts anything and we have chosen ociEncryptionMIMETypes
-			// Coverage: This should never happen, ociEncryptionMIMETypes all support encryption
-			return manifestConversionPlan{}, errors.New("internal error: in.destSupportedManifestMIMETypes is empty but supportedByDest is empty as well")
+		// 3. destination accepts a restricted list of mime types
+		destMIMEList := strings.Join(destSupportedManifestMIMETypes, ", ")
+		switch {
+		case in.requiresOCIEncryption && restrictiveCompressionRequired:
+			return manifestConversionPlan{}, fmt.Errorf("compression using %s, and encryption, required but the destination only supports MIME types [%s], none of which support both",
+				in.requestedCompressionFormat.Name(), destMIMEList)
+		case in.requiresOCIEncryption:
+			return manifestConversionPlan{}, fmt.Errorf("encryption required but the destination only supports MIME types [%s], none of which support encryption",
+				destMIMEList)
+		case restrictiveCompressionRequired:
+			return manifestConversionPlan{}, fmt.Errorf("compression using %s required but the destination only supports MIME types [%s], none of which support it",
+				in.requestedCompressionFormat.Name(), destMIMEList)
+		default: // Coverage: This should never happen, we only filter for in.requiresOCIEncryption || restrictiveCompressionRequired
+			return manifestConversionPlan{}, errors.New("internal error: supportedByDest is empty but destSupportedManifestMIMETypes is not, and we are neither encrypting nor requiring a restrictive compression algorithm")
 		}
-		// 3. destination does not support encryption.
-		return manifestConversionPlan{}, fmt.Errorf("encryption required but the destination only supports MIME types [%s], none of which support encryption",
-			strings.Join(destSupportedManifestMIMETypes, ", "))
 	}
 
 	// destSupportedManifestMIMETypes is a static guess; a particular registry may still only support a subset of the types.
@@ -156,7 +192,7 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
 	}
 
 	logrus.Debugf("Manifest has MIME type %s, ordered candidate list [%s]", srcType, strings.Join(prioritizedTypes.list, ", "))
-	if len(prioritizedTypes.list) == 0 { // Coverage: destSupportedManifestMIMETypes and supportedByDest, which is a subset, is not empty (or we would have exited  above), so this should never happen.
+	if len(prioritizedTypes.list) == 0 { // Coverage: destSupportedManifestMIMETypes and supportedByDest, which is a subset, is not empty (or we would have exited above), so this should never happen.
 		return manifestConversionPlan{}, errors.New("Internal error: no candidate MIME types")
 	}
 	res := manifestConversionPlan{

copy/manifest_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/containers/image/v5/internal/testing/mocks"
 	"github.com/containers/image/v5/manifest"
+	"github.com/containers/image/v5/pkg/compression"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -216,10 +217,11 @@ func TestDetermineManifestConversion(t *testing.T) {
 		}, res, c.description)
 	}
 
-	// When encryption is required:
+	// When encryption or zstd is required:
+	// In both of these cases, we we are restricted to OCI
 	for _, c := range []struct {
 		description string
-		in          determineManifestConversionInputs // with requiresOCIEncryption implied
+		in          determineManifestConversionInputs // with requiresOCIEncryption or requestedCompressionFormat: zstd implied
 		expected    manifestConversionPlan            // Or {} to expect a failure
 	}{
 		{ // Destination accepts anything - no conversion necessary
@@ -234,7 +236,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 				otherMIMETypeCandidates:          []string{},
 			},
 		},
-		{ // Destination accepts anything - need to convert for encryption
+		{ // Destination accepts anything - need to convert to OCI
 			"s2→anything",
 			determineManifestConversionInputs{
 				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
@@ -246,7 +248,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 				otherMIMETypeCandidates:          []string{},
 			},
 		},
-		// Destination accepts an encrypted format
+		// Destination accepts OCI
 		{
 			"OCI→OCI",
 			determineManifestConversionInputs{
@@ -271,7 +273,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 				otherMIMETypeCandidates:          []string{},
 			},
 		},
-		// Destination does not accept an encrypted format
+		// Destination does not accept OCI
 		{
 			"OCI→s2",
 			determineManifestConversionInputs{
@@ -289,9 +291,9 @@ func TestDetermineManifestConversion(t *testing.T) {
 			manifestConversionPlan{},
 		},
 		// Whatever the input is, with cannotModifyManifestReason we return "keep the original as is".
-		// Still, encryption is necessarily going to fail…
+		// Still, encryption/compression is necessarily going to fail…
 		{
-			"OCI→OCI cannotModifyManifestReason",
+			"OCI cannotModifyManifestReason",
 			determineManifestConversionInputs{
 				srcMIMEType:                    v1.MediaTypeImageManifest,
 				destSupportedManifestMIMETypes: supportS1S2OCI,
@@ -304,7 +306,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 			},
 		},
 		{
-			"s2→OCI cannotModifyManifestReason",
+			"s2 cannotModifyManifestReason",
 			determineManifestConversionInputs{
 				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
 				destSupportedManifestMIMETypes: supportS1S2OCI,
@@ -316,7 +318,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 				otherMIMETypeCandidates:          []string{},
 			},
 		},
-		// forceManifestMIMEType to a type that supports encryption
+		// forceManifestMIMEType to a type that supports OCI features
 		{
 			"OCI→OCI forced",
 			determineManifestConversionInputs{
@@ -343,7 +345,7 @@ func TestDetermineManifestConversion(t *testing.T) {
 				otherMIMETypeCandidates:          []string{},
 			},
 		},
-		// forceManifestMIMEType to a type that does not support encryption
+		// forceManifestMIMEType to a type that does not support OCI features
 		{
 			"OCI→s2 forced",
 			determineManifestConversionInputs{
@@ -363,16 +365,154 @@ func TestDetermineManifestConversion(t *testing.T) {
 			manifestConversionPlan{},
 		},
 	} {
-		in := c.in
-		in.requiresOCIEncryption = true
-		res, err := determineManifestConversion(in)
-		if c.expected.preferredMIMEType != "" {
-			require.NoError(t, err, c.description)
-			assert.Equal(t, c.expected, res, c.description)
-		} else {
-			assert.Error(t, err, c.description)
+		for _, restriction := range []struct {
+			description string
+			edit        func(in *determineManifestConversionInputs)
+		}{
+			{
+				description: "encrypted",
+				edit: func(in *determineManifestConversionInputs) {
+					in.requiresOCIEncryption = true
+				},
+			},
+			{
+				description: "zstd",
+				edit: func(in *determineManifestConversionInputs) {
+					in.requestedCompressionFormat = &compression.Zstd
+				},
+			},
+			{
+				description: "zstd:chunked",
+				edit: func(in *determineManifestConversionInputs) {
+					in.requestedCompressionFormat = &compression.ZstdChunked
+				},
+			},
+			{
+				description: "encrypted+zstd",
+				edit: func(in *determineManifestConversionInputs) {
+					in.requiresOCIEncryption = true
+					in.requestedCompressionFormat = &compression.Zstd
+				},
+			},
+		} {
+			desc := c.description + " / " + restriction.description
+
+			in := c.in
+			restriction.edit(&in)
+			res, err := determineManifestConversion(in)
+			if c.expected.preferredMIMEType != "" {
+				require.NoError(t, err, desc)
+				assert.Equal(t, c.expected, res, desc)
+			} else {
+				assert.Error(t, err, desc)
+			}
 		}
 	}
+
+	// When encryption using a completely unsupported algorithm is required:
+	for _, c := range []struct {
+		description string
+		in          determineManifestConversionInputs // with requiresOCIEncryption or requestedCompressionFormat: zstd implied
+	}{
+		{ // Destination accepts anything
+			"OCI→anything",
+			determineManifestConversionInputs{
+				srcMIMEType:                    v1.MediaTypeImageManifest,
+				destSupportedManifestMIMETypes: nil,
+			},
+		},
+		{ // Destination accepts anything - need to convert to OCI
+			"s2→anything",
+			determineManifestConversionInputs{
+				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
+				destSupportedManifestMIMETypes: nil,
+			},
+		},
+		// Destination only supports some formats
+		{
+			"OCI→OCI",
+			determineManifestConversionInputs{
+				srcMIMEType:                    v1.MediaTypeImageManifest,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+			},
+		},
+		{
+			"s2→OCI",
+			determineManifestConversionInputs{
+				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+			},
+		},
+		{
+			"OCI→s2",
+			determineManifestConversionInputs{
+				srcMIMEType:                    v1.MediaTypeImageManifest,
+				destSupportedManifestMIMETypes: supportS1S2,
+			},
+		},
+		{
+			"s2→s2",
+			determineManifestConversionInputs{
+				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
+				destSupportedManifestMIMETypes: supportS1S2,
+			},
+		},
+		// cannotModifyManifestReason
+		{
+			"OCI cannotModifyManifestReason",
+			determineManifestConversionInputs{
+				srcMIMEType:                    v1.MediaTypeImageManifest,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+				cannotModifyManifestReason:     "Preserving digests",
+			},
+		},
+		{
+			"s2 cannotModifyManifestReason",
+			determineManifestConversionInputs{
+				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+				cannotModifyManifestReason:     "Preserving digests",
+			},
+		},
+		// forceManifestMIMEType
+		{
+			"OCI→OCI forced",
+			determineManifestConversionInputs{
+				srcMIMEType:                    v1.MediaTypeImageManifest,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+				forceManifestMIMEType:          v1.MediaTypeImageManifest,
+			},
+		},
+		{
+			"s2→OCI forced",
+			determineManifestConversionInputs{
+				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+				forceManifestMIMEType:          v1.MediaTypeImageManifest,
+			},
+		},
+		{
+			"OCI→s2 forced",
+			determineManifestConversionInputs{
+				srcMIMEType:                    v1.MediaTypeImageManifest,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+				forceManifestMIMEType:          manifest.DockerV2Schema2MediaType,
+			},
+		},
+		{
+			"s2→s2 forced",
+			determineManifestConversionInputs{
+				srcMIMEType:                    manifest.DockerV2Schema2MediaType,
+				destSupportedManifestMIMETypes: supportS1S2OCI,
+				forceManifestMIMEType:          manifest.DockerV2Schema2MediaType,
+			},
+		},
+	} {
+		in := c.in
+		in.requestedCompressionFormat = &compression.Xz
+		_, err := determineManifestConversion(in)
+		assert.Error(t, err, c.description)
+	}
 }
 
 // fakeUnparsedImage is an implementation of types.UnparsedImage which only returns itself as a MIME type in Manifest,

copy/single.go

@@ -167,6 +167,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
 		srcMIMEType:                    ic.src.ManifestMIMEType,
 		destSupportedManifestMIMETypes: ic.c.dest.SupportedManifestMIMETypes(),
 		forceManifestMIMEType:          c.options.ForceManifestMIMEType,
+		requestedCompressionFormat:     ic.compressionFormat,
 		requiresOCIEncryption:          destRequiresOciEncryption,
 		cannotModifyManifestReason:     ic.cannotModifyManifestReason,
 	})