feedback changes for moby/buildkit moby#2251

Signed-off-by: Matt Kang <[email protected]>

Author: kattmang

README.md

@@ -388,7 +388,7 @@ buildctl build ... \
   * `min`: only export layers for the resulting image
   * `max`: export all the layers of all intermediate steps
 * `ref=<ref>`: specify repository reference to store cache, e.g. `docker.io/user/image:tag`
-* `image-manifest=<true|false>`: whether to export cache manifest as an OCI-compatible image manifest rather than a manifest list/index (default: `false`)
+* `image-manifest=<true|false>`: whether to export cache manifest as an OCI-compatible image manifest rather than a manifest list/index (default: `false`, must be used with `oci-mediatypes=true`)
 * `oci-mediatypes=<true|false>`: whether to use OCI mediatypes in exported manifests (default: `true`, since BuildKit `v0.8`)
 * `compression=<uncompressed|gzip|estargz|zstd>`: choose compression type for layers newly created and cached, gzip is default value. estargz and zstd should be used with `oci-mediatypes=true`
 * `compression-level=<value>`: choose compression level for gzip, estargz (0-9) and zstd (0-22)
@@ -415,7 +415,7 @@ The directory layout conforms to OCI Image Spec v1.0.
   * `max`: export all the layers of all intermediate steps
 * `dest=<path>`: destination directory for cache exporter
 * `tag=<tag>`: specify custom tag of image to write to local index (default: `latest`)
-* `image-manifest=<true|false>`: whether to export cache manifest as an OCI-compatible image manifest rather than a manifest list/index (default: `false`)
+* `image-manifest=<true|false>`: whether to export cache manifest as an OCI-compatible image manifest rather than a manifest list/index (default: `false`, must be used with `oci-mediatypes=true`)
 * `oci-mediatypes=<true|false>`: whether to use OCI mediatypes in exported manifests (default `true`, since BuildKit `v0.8`)
 * `compression=<uncompressed|gzip|estargz|zstd>`: choose compression type for layers newly created and cached, gzip is default value. estargz and zstd should be used with `oci-mediatypes=true`.
 * `compression-level=<value>`: compression level for gzip, estargz (0-9) and zstd (0-22)

cache/remotecache/export.go

@@ -16,7 +16,7 @@ import (
 	"github.com/moby/buildkit/util/progress"
 	"github.com/moby/buildkit/util/progress/logs"
 	digest "github.com/opencontainers/go-digest"
-	specs "github.com/opencontainers/image-spec/specs-go"
+	"github.com/opencontainers/image-spec/specs-go"
 	ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 )
@@ -37,17 +37,127 @@ type Config struct {
 	Compression compression.Config
 }
 
+type CacheType int
+
 const (
 	// ExportResponseManifestDesc is a key for the map returned from Exporter.Finalize.
 	// The map value is a JSON string of an OCI desciptor of a manifest.
 	ExporterResponseManifestDesc = "cache.manifest"
 )
 
+const (
+	NotSet CacheType = iota
+	ManifestList
+	ImageManifest
+)
+
+func (data CacheType) String() string {
+	switch data {
+	case ManifestList:
+		return "Manifest List"
+	case ImageManifest:
+		return "Image Manifest"
+	default:
+		return "Not Set"
+	}
+}
+
 func NewExporter(ingester content.Ingester, ref string, oci bool, imageManifest bool, compressionConfig compression.Config) Exporter {
 	cc := v1.NewCacheChains()
 	return &contentCacheExporter{CacheExporterTarget: cc, chains: cc, ingester: ingester, oci: oci, imageManifest: imageManifest, ref: ref, comp: compressionConfig}
 }
 
+type ExportableCache struct {
+	// This cache describes two distinct styles of exportable cache, one is an Index (or Manifest List) of blobs,
+	// or as an artifact using the OCI image manifest format.
+	ExportedManifest ocispecs.Manifest
+	ExportedIndex    ocispecs.Index
+	CacheType        CacheType
+	OCI              bool
+}
+
+func NewExportableCache(oci bool, imageManifest bool) (*ExportableCache, error) {
+	var mediaType string
+
+	if imageManifest {
+		mediaType = ocispecs.MediaTypeImageManifest
+		if !oci {
+			return nil, errors.Errorf("invalid configuration for remote cache")
+		}
+	} else {
+		if oci {
+			mediaType = ocispecs.MediaTypeImageIndex
+		} else {
+			mediaType = images.MediaTypeDockerSchema2ManifestList
+		}
+	}
+
+	cacheType := ManifestList
+	if imageManifest {
+		cacheType = ImageManifest
+	}
+
+	schemaVersion := specs.Versioned{SchemaVersion: 2}
+	switch cacheType {
+	case ManifestList:
+		return &ExportableCache{ExportedIndex: ocispecs.Index{
+			MediaType: mediaType,
+			Versioned: schemaVersion,
+		},
+			CacheType: cacheType,
+			OCI:       oci,
+		}, nil
+	case ImageManifest:
+		return &ExportableCache{ExportedManifest: ocispecs.Manifest{
+			MediaType: mediaType,
+			Versioned: schemaVersion,
+		},
+			CacheType: cacheType,
+			OCI:       oci,
+		}, nil
+	default:
+		return nil, errors.Errorf("exportable cache type not set")
+	}
+}
+
+func (ec *ExportableCache) MediaType() string {
+	if ec.CacheType == ManifestList {
+		return ec.ExportedIndex.MediaType
+	}
+	return ec.ExportedManifest.MediaType
+}
+
+func (ec *ExportableCache) AddCacheBlob(blob ocispecs.Descriptor) {
+	if ec.CacheType == ManifestList {
+		ec.ExportedIndex.Manifests = append(ec.ExportedIndex.Manifests, blob)
+	} else {
+		ec.ExportedManifest.Layers = append(ec.ExportedManifest.Layers, blob)
+	}
+}
+
+func (ec *ExportableCache) FinalizeCache(ctx context.Context) {
+	if ec.CacheType == ManifestList {
+		ec.ExportedIndex.Manifests = compression.ConvertAllLayerMediaTypes(ctx, ec.OCI, ec.ExportedIndex.Manifests...)
+	} else {
+		ec.ExportedManifest.Layers = compression.ConvertAllLayerMediaTypes(ctx, ec.OCI, ec.ExportedManifest.Layers...)
+	}
+}
+
+func (ec *ExportableCache) SetConfig(config ocispecs.Descriptor) {
+	if ec.CacheType == ManifestList {
+		ec.ExportedIndex.Manifests = append(ec.ExportedIndex.Manifests, config)
+	} else {
+		ec.ExportedManifest.Config = config
+	}
+}
+
+func (ec *ExportableCache) MarshalJSON() ([]byte, error) {
+	if ec.CacheType == ManifestList {
+		return json.Marshal(ec.ExportedIndex)
+	}
+	return json.Marshal(ec.ExportedManifest)
+}
+
 type contentCacheExporter struct {
 	solver.CacheExporterTarget
 	chains        *v1.CacheChains
@@ -75,24 +185,9 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 		return nil, err
 	}
 
-	// own type because oci type can't be pushed and docker type doesn't have annotations
-	type abstractManifest struct {
-		specs.Versioned
-
-		MediaType string               `json:"mediaType,omitempty"`
-		Config    *ocispecs.Descriptor `json:"config,omitempty"`
-		// Manifests references platform specific manifests.
-		Manifests []ocispecs.Descriptor `json:"manifests,omitempty"`
-		Layers    []ocispecs.Descriptor `json:"layers,omitempty"`
-	}
-
-	var mfst abstractManifest
-	mfst.SchemaVersion = 2
-	mfst.MediaType = images.MediaTypeDockerSchema2ManifestList
-	if ce.oci && !ce.imageManifest {
-		mfst.MediaType = ocispecs.MediaTypeImageIndex
-	} else if ce.imageManifest {
-		mfst.MediaType = ocispecs.MediaTypeImageManifest
+	cache, err := NewExportableCache(ce.oci, ce.imageManifest)
+	if err != nil {
+		return nil, err
 	}
 
 	for _, l := range config.Layers {
@@ -105,16 +200,10 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 			return nil, layerDone(errors.Wrap(err, "error writing layer blob"))
 		}
 		layerDone(nil)
-		if ce.imageManifest {
-			mfst.Layers = append(mfst.Layers, dgstPair.Descriptor)
-		} else {
-			mfst.Manifests = append(mfst.Manifests, dgstPair.Descriptor)
-		}
+		cache.AddCacheBlob(dgstPair.Descriptor)
 	}
 
-	if !ce.imageManifest {
-		mfst.Manifests = compression.ConvertAllLayerMediaTypes(ctx, ce.oci, mfst.Manifests...)
-	}
+	cache.FinalizeCache(ctx)
 
 	dt, err := json.Marshal(config)
 	if err != nil {
@@ -132,13 +221,9 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 	}
 	configDone(nil)
 
-	if ce.imageManifest {
-		mfst.Config = &desc
-	} else {
-		mfst.Manifests = append(mfst.Manifests, desc)
-	}
+	cache.SetConfig(desc)
 
-	dt, err = json.Marshal(mfst)
+	dt, err = cache.MarshalJSON()
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to marshal manifest")
 	}
@@ -147,7 +232,7 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 	desc = ocispecs.Descriptor{
 		Digest:    dgst,
 		Size:      int64(len(dt)),
-		MediaType: mfst.MediaType,
+		MediaType: cache.MediaType(),
 	}
 
 	mfstLog := fmt.Sprintf("writing cache manifest %s", dgst)

cache/remotecache/import.go

@@ -8,44 +8,21 @@ import (
 	"sync"
 	"time"
 
-	"github.com/moby/buildkit/util/progress"
-	"github.com/opencontainers/image-spec/specs-go"
-
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/images"
 	v1 "github.com/moby/buildkit/cache/remotecache/v1"
 	"github.com/moby/buildkit/session"
 	"github.com/moby/buildkit/solver"
 	"github.com/moby/buildkit/util/bklog"
 	"github.com/moby/buildkit/util/imageutil"
+	"github.com/moby/buildkit/util/progress"
 	"github.com/moby/buildkit/worker"
 	digest "github.com/opencontainers/go-digest"
 	ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"golang.org/x/sync/errgroup"
 )
 
-type ManifestType int
-
-const (
-	NotInferred ManifestType = iota
-	ManifestList
-	ImageManifest
-)
-
-func (data ManifestType) String() string {
-	switch data {
-	case NotInferred:
-		return "Not Inferred"
-	case ManifestList:
-		return "Manifest List"
-	case ImageManifest:
-		return "Image Manifest"
-	default:
-		return "Not Inferred"
-	}
-}
-
 // ResolveCacheImporterFunc returns importer and descriptor.
 type ResolveCacheImporterFunc func(ctx context.Context, g session.Group, attrs map[string]string) (Importer, ocispecs.Descriptor, error)
 
@@ -72,7 +49,7 @@ func (ci *contentCacheImporter) Resolve(ctx context.Context, desc ocispecs.Descr
 		return nil, err
 	}
 
-	manifestType, err := inferManifestType(ctx, dt)
+	manifestType, err := imageutil.DetectManifestBlobMediaType(dt)
 	if err != nil {
 		return nil, err
 	}
@@ -83,7 +60,8 @@ func (ci *contentCacheImporter) Resolve(ctx context.Context, desc ocispecs.Descr
 	allLayers := v1.DescriptorProvider{}
 	var configDesc ocispecs.Descriptor
 
-	if manifestType == ManifestList {
+	switch manifestType {
+	case images.MediaTypeDockerSchema2ManifestList, ocispecs.MediaTypeImageIndex:
 		var mfst ocispecs.Index
 		if err := json.Unmarshal(dt, &mfst); err != nil {
 			return nil, err
@@ -99,24 +77,23 @@ func (ci *contentCacheImporter) Resolve(ctx context.Context, desc ocispecs.Descr
 				Provider:   ci.provider,
 			}
 		}
-	} else if manifestType == ImageManifest {
+	case images.MediaTypeDockerSchema2Manifest, ocispecs.MediaTypeImageManifest:
 		var mfst ocispecs.Manifest
 		if err := json.Unmarshal(dt, &mfst); err != nil {
 			return nil, err
 		}
 
+		if mfst.Config.MediaType == v1.CacheConfigMediaTypeV0 {
+			configDesc = mfst.Config
+		}
 		for _, m := range mfst.Layers {
-			if m.MediaType == v1.CacheConfigMediaTypeV0 {
-				configDesc = m
-				continue
-			}
 			allLayers[m.Digest] = v1.DescriptorProviderPair{
 				Descriptor: m,
 				Provider:   ci.provider,
 			}
 		}
-	} else {
-		err = errors.Wrapf(err, "Unsupported or uninferrable manifest type")
+	default:
+		err = errors.Wrapf(err, "unsupported or uninferrable manifest type")
 		return nil, err
 	}
 
@@ -150,37 +127,6 @@ func (ci *contentCacheImporter) Resolve(ctx context.Context, desc ocispecs.Descr
 	return solver.NewCacheManager(ctx, id, keysStorage, resultStorage), nil
 }
 
-// extends support for "new"-style image-manifest style remote cache manifests and determining downstream
-// handling based on inference of document structure (is this a new or old cache manifest type?)
-func inferManifestType(ctx context.Context, dt []byte) (ManifestType, error) {
-	// this is a loose schema superset of both OCI Index and Manifest in order to
-	// be able to poke at the structure of the imported cache manifest
-	type OpenManifest struct {
-		specs.Versioned
-
-		MediaType string                 `json:"mediaType,omitempty"`
-		Config    map[string]interface{} `json:"config,omitempty"`
-		// Manifests references platform specific manifests.
-		Manifests []map[string]interface{} `json:"manifests,omitempty"`
-		Layers    []map[string]interface{} `json:"layers,omitempty"`
-	}
-
-	var openManifest OpenManifest
-	if err := json.Unmarshal(dt, &openManifest); err != nil {
-		return NotInferred, err
-	}
-
-	if len(openManifest.Manifests) == 0 && len(openManifest.Layers) > 0 {
-		return ImageManifest, nil
-	}
-
-	if len(openManifest.Layers) == 0 && len(openManifest.Manifests) > 0 {
-		return ManifestList, nil
-	}
-
-	return NotInferred, nil
-}
-
 func readBlob(ctx context.Context, provider content.Provider, desc ocispecs.Descriptor) ([]byte, error) {
 	maxBlobSize := int64(1 << 20)
 	if desc.Size > maxBlobSize {