From 4c5851ba1805af1b5fb3a30bd1517b2ff7c05583 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@tycho.ws>
Date: Mon, 8 Jun 2020 14:49:53 -0600
Subject: [PATCH] allow ImageDestination to decide compression based on blob
 info

The various ImageDestinations know what types of layers are supported
natively and how they should be compressed, but some (e.g. the OCI
distribution repo or the OCI image spec) allow arbitrary blobs to be
uploaded. We should not compress these blobs by default, since we don't
know what they are and may already be compressed (e.g. squashfs).

So, let's introduce a new DesiredBlobCompression() and deprecate
DesiredLayerCompression() to allow the various backends to tell the core
code about what kind of compression they want.

We implement mime-type-based compression detection for the docker and OCI
backends. As above, OCI supports arbitrary blobs so we shouldn't interfere
with ones we don't understand. The docker backend is intended to be the way
we interact with OCI dist spec repos, so we should also not compress
unknown media types when talking to the docker backend.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 copy/copy.go                     |  6 +++---
 directory/directory_dest.go      |  7 +++++++
 directory/directory_test.go      |  5 +++--
 docker/archive/dest.go           |  6 +++++-
 docker/daemon/daemon_dest.go     |  6 +++++-
 docker/docker_image_dest.go      | 20 ++++++++++++++++++++
 docker/docker_image_dest_test.go | 14 ++++++++++++++
 image/docker_schema2_test.go     |  3 +++
 oci/archive/oci_dest.go          |  6 +++++-
 oci/layout/oci_dest.go           | 19 +++++++++++++++++++
 oci/layout/oci_dest_test.go      | 13 +++++++++++++
 openshift/openshift.go           |  4 ++++
 ostree/ostree_dest.go            |  2 +-
 storage/storage_image.go         |  4 ++++
 storage/storage_test.go          | 12 ++++--------
 types/types.go                   |  6 ++++++
 16 files changed, 116 insertions(+), 17 deletions(-)
 create mode 100644 docker/docker_image_dest_test.go

diff --git a/copy/copy.go b/copy/copy.go
index 9fc0e5123b..e3c610974c 100644
--- a/copy/copy.go
+++ b/copy/copy.go
@@ -1232,7 +1232,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr
 		logrus.Debugf("Using original blob without modification for encrypted blob")
 		compressionOperation = types.PreserveOriginal
 		inputInfo = srcInfo
-	} else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !isCompressed {
+	} else if canModifyBlob && c.dest.DesiredBlobCompression(srcInfo) == types.Compress && !isCompressed {
 		logrus.Debugf("Compressing blob on the fly")
 		compressionOperation = types.Compress
 		pipeReader, pipeWriter := io.Pipe()
@@ -1245,7 +1245,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr
 		destStream = pipeReader
 		inputInfo.Digest = ""
 		inputInfo.Size = -1
-	} else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && isCompressed && desiredCompressionFormat.Name() != compressionFormat.Name() {
+	} else if canModifyBlob && c.dest.DesiredBlobCompression(srcInfo) == types.Compress && isCompressed && desiredCompressionFormat.Name() != compressionFormat.Name() {
 		// When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally
 		// re-compressed using the desired format.
 		logrus.Debugf("Blob will be converted")
@@ -1265,7 +1265,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr
 		destStream = pipeReader
 		inputInfo.Digest = ""
 		inputInfo.Size = -1
-	} else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && isCompressed {
+	} else if canModifyBlob && c.dest.DesiredBlobCompression(srcInfo) == types.Decompress && isCompressed {
 		logrus.Debugf("Blob will be decompressed")
 		compressionOperation = types.Decompress
 		s, err := decompressor(destStream)
diff --git a/directory/directory_dest.go b/directory/directory_dest.go
index d70b6c07fb..f6b7dc8f98 100644
--- a/directory/directory_dest.go
+++ b/directory/directory_dest.go
@@ -107,6 +107,13 @@ func (d *dirImageDestination) DesiredLayerCompression() types.LayerCompression {
 	return types.PreserveOriginal
 }
 
+func (d *dirImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	if d.compress {
+		return types.Compress
+	}
+	return types.PreserveOriginal
+}
+
 // AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
 // uploaded to the image destination, true otherwise.
 func (d *dirImageDestination) AcceptsForeignLayerURLs() bool {
diff --git a/directory/directory_test.go b/directory/directory_test.go
index fdb0ce68c1..4a4d04bb68 100644
--- a/directory/directory_test.go
+++ b/directory/directory_test.go
@@ -68,8 +68,9 @@ func TestGetPutBlob(t *testing.T) {
 	dest, err := ref.NewImageDestination(context.Background(), nil)
 	require.NoError(t, err)
 	defer dest.Close()
-	assert.Equal(t, types.PreserveOriginal, dest.DesiredLayerCompression())
-	info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}, cache, false)
+	blobInfo := types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}
+	assert.Equal(t, types.PreserveOriginal, dest.DesiredBlobCompression(blobInfo))
+	info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), blobInfo, cache, false)
 	assert.NoError(t, err)
 	err = dest.Commit(context.Background(), nil)
 	assert.NoError(t, err)
diff --git a/docker/archive/dest.go b/docker/archive/dest.go
index 1cf197429b..3ac5eac2ab 100644
--- a/docker/archive/dest.go
+++ b/docker/archive/dest.go
@@ -47,11 +47,15 @@ func newImageDestination(sys *types.SystemContext, ref archiveReference) (types.
 	}, nil
 }
 
-// DesiredLayerCompression indicates if layers must be compressed, decompressed or preserved
 func (d *archiveImageDestination) DesiredLayerCompression() types.LayerCompression {
 	return types.Decompress
 }
 
+// DesiredBlobCompression indicates if layers must be compressed, decompressed or preserved
+func (d *archiveImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	return types.Decompress
+}
+
 // Reference returns the reference used to set up this destination.  Note that this should directly correspond to user's intent,
 // e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects.
 func (d *archiveImageDestination) Reference() types.ImageReference {
diff --git a/docker/daemon/daemon_dest.go b/docker/daemon/daemon_dest.go
index c6afd4bde0..97ba8b76f1 100644
--- a/docker/daemon/daemon_dest.go
+++ b/docker/daemon/daemon_dest.go
@@ -87,11 +87,15 @@ func imageLoadGoroutine(ctx context.Context, c *client.Client, reader *io.PipeRe
 	defer resp.Body.Close()
 }
 
-// DesiredLayerCompression indicates if layers must be compressed, decompressed or preserved
 func (d *daemonImageDestination) DesiredLayerCompression() types.LayerCompression {
 	return types.PreserveOriginal
 }
 
+// DesiredBlobCompression indicates if layers must be compressed, decompressed or preserved
+func (d *daemonImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	return types.PreserveOriginal
+}
+
 // MustMatchRuntimeOS returns true iff the destination can store only images targeted for the current runtime architecture and OS. False otherwise.
 func (d *daemonImageDestination) MustMatchRuntimeOS() bool {
 	return d.mustMatchRuntimeOS
diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go
index 979100ee38..960e81c758 100644
--- a/docker/docker_image_dest.go
+++ b/docker/docker_image_dest.go
@@ -20,6 +20,7 @@ import (
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/pkg/blobinfocache/none"
 	"github.com/containers/image/v5/types"
+	ociencspec "github.com/containers/ocicrypt/spec"
 	"github.com/docker/distribution/registry/api/errcode"
 	v2 "github.com/docker/distribution/registry/api/v2"
 	"github.com/docker/distribution/registry/client"
@@ -92,6 +93,25 @@ func (d *dockerImageDestination) DesiredLayerCompression() types.LayerCompressio
 	return types.Compress
 }
 
+func (d *dockerImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	// it's ok to compress known media types
+	if manifest.SupportedSchema2MediaType(info.MediaType) == nil {
+		return types.Compress
+	}
+	switch info.MediaType {
+	case imgspecv1.MediaTypeImageLayer, imgspecv1.MediaTypeImageLayerGzip, imgspecv1.MediaTypeImageLayerNonDistributable, imgspecv1.MediaTypeImageLayerNonDistributableGzip, imgspecv1.MediaTypeImageLayerNonDistributableZstd, imgspecv1.MediaTypeImageLayerZstd, ociencspec.MediaTypeLayerEnc, ociencspec.MediaTypeLayerGzipEnc:
+		return types.Compress
+	}
+
+	// v1 manifests didn't have MediaTypes, so "" is still "known"
+	if info.MediaType == "" {
+		return types.Compress
+	}
+
+	// don't compress anything else
+	return types.PreserveOriginal
+}
+
 // AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
 // uploaded to the image destination, true otherwise.
 func (d *dockerImageDestination) AcceptsForeignLayerURLs() bool {
diff --git a/docker/docker_image_dest_test.go b/docker/docker_image_dest_test.go
new file mode 100644
index 0000000000..3641a4ff3c
--- /dev/null
+++ b/docker/docker_image_dest_test.go
@@ -0,0 +1,14 @@
+package docker
+
+import (
+	"testing"
+
+	"github.com/containers/image/v5/types"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDockerKnownLayerCompression(t *testing.T) {
+	dest := dockerImageDestination{}
+	info := types.BlobInfo{MediaType: "this is not a known media type"}
+	assert.Equal(t, types.PreserveOriginal, dest.DesiredBlobCompression(info))
+}
diff --git a/image/docker_schema2_test.go b/image/docker_schema2_test.go
index 5b2a2aa4e0..6deb25ed3f 100644
--- a/image/docker_schema2_test.go
+++ b/image/docker_schema2_test.go
@@ -403,6 +403,9 @@ func (d *memoryImageDest) SupportsSignatures(ctx context.Context) error {
 func (d *memoryImageDest) DesiredLayerCompression() types.LayerCompression {
 	panic("Unexpected call to a mock function")
 }
+func (d *memoryImageDest) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	panic("Unexpected call to a mock function")
+}
 func (d *memoryImageDest) AcceptsForeignLayerURLs() bool {
 	panic("Unexpected call to a mock function")
 }
diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go
index 0509eaa83b..aa6cac783f 100644
--- a/oci/archive/oci_dest.go
+++ b/oci/archive/oci_dest.go
@@ -61,7 +61,11 @@ func (d *ociArchiveImageDestination) SupportsSignatures(ctx context.Context) err
 }
 
 func (d *ociArchiveImageDestination) DesiredLayerCompression() types.LayerCompression {
-	return d.unpackedDest.DesiredLayerCompression()
+	return d.unpackedDest.DesiredBlobCompression(types.BlobInfo{})
+}
+
+func (d *ociArchiveImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	return d.unpackedDest.DesiredBlobCompression(info)
 }
 
 // AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go
index fb0449ca52..8a201fd951 100644
--- a/oci/layout/oci_dest.go
+++ b/oci/layout/oci_dest.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/types"
+	ociencspec "github.com/containers/ocicrypt/spec"
 	digest "github.com/opencontainers/go-digest"
 	imgspec "github.com/opencontainers/image-spec/specs-go"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
@@ -88,9 +89,27 @@ func (d *ociImageDestination) DesiredLayerCompression() types.LayerCompression {
 	if d.acceptUncompressedLayers {
 		return types.PreserveOriginal
 	}
+
 	return types.Compress
 }
 
+func (d *ociImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	if d.acceptUncompressedLayers {
+		return types.PreserveOriginal
+	}
+
+	if manifest.SupportedSchema2MediaType(info.MediaType) == nil {
+		return types.Compress
+	}
+
+	switch info.MediaType {
+	case imgspecv1.MediaTypeImageLayer, imgspecv1.MediaTypeImageLayerGzip, imgspecv1.MediaTypeImageLayerNonDistributable, imgspecv1.MediaTypeImageLayerNonDistributableGzip, imgspecv1.MediaTypeImageLayerNonDistributableZstd, imgspecv1.MediaTypeImageLayerZstd, ociencspec.MediaTypeLayerEnc, ociencspec.MediaTypeLayerGzipEnc:
+		return types.Compress
+	}
+
+	return types.PreserveOriginal
+}
+
 // AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
 // uploaded to the image destination, true otherwise.
 func (d *ociImageDestination) AcceptsForeignLayerURLs() bool {
diff --git a/oci/layout/oci_dest_test.go b/oci/layout/oci_dest_test.go
index f6feefc6ab..64cedca140 100644
--- a/oci/layout/oci_dest_test.go
+++ b/oci/layout/oci_dest_test.go
@@ -155,3 +155,16 @@ func putTestManifest(t *testing.T, ociRef ociReference, tmpDir string) {
 	digest := digest.FromBytes(data).Encoded()
 	assert.Contains(t, paths, filepath.Join(tmpDir, "blobs", "sha256", digest), "The OCI directory does not contain the new manifest data")
 }
+
+func TestOCIDesiredBlobCompression(t *testing.T) {
+	ref, tmpDir := refToTempOCI(t)
+	defer os.RemoveAll(tmpDir)
+	ociRef, ok := ref.(ociReference)
+	require.True(t, ok)
+
+	imageDest, err := newImageDestination(nil, ociRef)
+	assert.NoError(t, err)
+
+	info := types.BlobInfo{MediaType: "this is not a known media type"}
+	assert.Equal(t, types.PreserveOriginal, imageDest.DesiredBlobCompression(info))
+}
diff --git a/openshift/openshift.go b/openshift/openshift.go
index 28bfc456d5..37f621069d 100644
--- a/openshift/openshift.go
+++ b/openshift/openshift.go
@@ -372,6 +372,10 @@ func (d *openshiftImageDestination) DesiredLayerCompression() types.LayerCompres
 	return types.Compress
 }
 
+func (d *openshiftImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
+	return types.Compress
+}
+
 // AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
 // uploaded to the image destination, true otherwise.
 func (d *openshiftImageDestination) AcceptsForeignLayerURLs() bool {
diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go
index 1150970559..3b2bda8c24 100644
--- a/ostree/ostree_dest.go
+++ b/ostree/ostree_dest.go
@@ -110,7 +110,7 @@ func (d *ostreeImageDestination) SupportsSignatures(ctx context.Context) error {
 }
 
 // ShouldCompressLayers returns true iff it is desirable to compress layer blobs written to this destination.
-func (d *ostreeImageDestination) DesiredLayerCompression() types.LayerCompression {
+func (d *ostreeImageDestination) DesiredBlobCompression() types.LayerCompression {
 	return types.PreserveOriginal
 }
 
diff --git a/storage/storage_image.go b/storage/storage_image.go
index df4b67c7a7..89c7b5eaf6 100644
--- a/storage/storage_image.go
+++ b/storage/storage_image.go
@@ -372,6 +372,10 @@ func (s *storageImageDestination) Close() error {
 }
 
 func (s *storageImageDestination) DesiredLayerCompression() types.LayerCompression {
+	return types.PreserveOriginal
+}
+
+func (s *storageImageDestination) DesiredBlobCompression(info types.BlobInfo) types.LayerCompression {
 	// We ultimately have to decompress layers to populate trees on disk
 	// and need to explicitly ask for it here, so that the layers' MIME
 	// types can be set accordingly.
diff --git a/storage/storage_test.go b/storage/storage_test.go
index c4709db6e3..de1e61247c 100644
--- a/storage/storage_test.go
+++ b/storage/storage_test.go
@@ -392,16 +392,12 @@ func TestWriteRead(t *testing.T) {
 		if err := dest.SupportsSignatures(context.Background()); err != nil {
 			t.Fatalf("Destination image doesn't support signatures: %v", err)
 		}
-		t.Logf("compress layers: %v", dest.DesiredLayerCompression())
-		compression := archive.Uncompressed
-		if dest.DesiredLayerCompression() == types.Compress {
-			compression = archive.Gzip
-		}
-		digest, decompressedSize, size, blob := makeLayer(t, compression)
-		if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{
+		digest, decompressedSize, size, blob := makeLayer(t, archive.Uncompressed)
+		info := types.BlobInfo{
 			Size:   size,
 			Digest: digest,
-		}, cache, false); err != nil {
+		}
+		if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), info, cache, false); err != nil {
 			t.Fatalf("Error saving randomly-generated layer to destination: %v", err)
 		}
 		t.Logf("Wrote randomly-generated layer %q (%d/%d bytes) to destination", digest, size, decompressedSize)
diff --git a/types/types.go b/types/types.go
index d469e03b53..9ffe03e844 100644
--- a/types/types.go
+++ b/types/types.go
@@ -280,7 +280,13 @@ type ImageDestination interface {
 	// Note: It is still possible for PutSignatures to fail if SupportsSignatures returns nil.
 	SupportsSignatures(ctx context.Context) error
 	// DesiredLayerCompression indicates the kind of compression to apply on layers
+	//
+	// Deprecated: DesiredLayerCompression can vary by layer media type (in
+	// particular, destinations probably shouldn't mess with media types
+	// they don't understand). Use DesiredBlobCompression() instead.
 	DesiredLayerCompression() LayerCompression
+	// DesiredBlobCompression indicates the kind of compression to apply on layers
+	DesiredBlobCompression(info BlobInfo) LayerCompression
 	// AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
 	// uploaded to the image destination, true otherwise.
 	AcceptsForeignLayerURLs() bool