From a0579215967a44e084c4535732e1fb3a543a1fbf Mon Sep 17 00:00:00 2001 From: Matt Kang Date: Tue, 2 May 2023 18:01:28 -0700 Subject: [PATCH] feedback changes for moby/buildkit #2251 Signed-off-by: Matt Kang --- README.md | 4 +- cache/remotecache/export.go | 137 +++++++++++++++++++------ cache/remotecache/import.go | 74 ++----------- client/client_test.go | 31 ++++++ frontend/dockerfile/dockerfile_test.go | 96 +++++++++++++++++ 5 files changed, 245 insertions(+), 97 deletions(-) diff --git a/README.md b/README.md index 466ef8b27898a..f76ade72ef2ec 100644 --- a/README.md +++ b/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=`: specify repository reference to store cache, e.g. `docker.io/user/image:tag` -* `image-manifest=`: whether to export cache manifest as an OCI-compatible image manifest rather than a manifest list/index (default: `false`) +* `image-manifest=`: 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=`: whether to use OCI mediatypes in exported manifests (default: `true`, since BuildKit `v0.8`) * `compression=`: 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=`: 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=`: destination directory for cache exporter * `tag=`: specify custom tag of image to write to local index (default: `latest`) -* `image-manifest=`: whether to export cache manifest as an OCI-compatible image manifest rather than a manifest list/index (default: `false`) +* `image-manifest=`: 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=`: whether to use OCI mediatypes in exported manifests (default `true`, since BuildKit `v0.8`) * `compression=`: 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=`: compression level for gzip, estargz (0-9) and zstd (0-22) diff --git a/cache/remotecache/export.go b/cache/remotecache/export.go index 64726361e82d9..a7c3a309038a5 100644 --- a/cache/remotecache/export.go +++ b/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,103 @@ 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" + CacheManifestSchemaVersion = 2 +) + +const ( + NotSet CacheType = iota + ManifestList + ImageManifest ) +func (data CacheType) String() string { + switch data { + case NotSet: + return "Not Set" + 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 +} + +func NewExportableCache(cacheType CacheType, mediaType string, schemaVersion specs.Versioned) *ExportableCache { + if cacheType == ManifestList { + return &ExportableCache{ExportedIndex: ocispecs.Index{ + MediaType: mediaType, + Versioned: schemaVersion, + }, + CacheType: cacheType, + } + } + return &ExportableCache{ExportedManifest: ocispecs.Manifest{ + MediaType: mediaType, + Versioned: schemaVersion, + }, + CacheType: cacheType, + } +} + +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, ce *contentCacheExporter) { + // Nothing needed here for Manifest-type cache manifests + if ec.CacheType == ManifestList { + ec.ExportedIndex.Manifests = compression.ConvertAllLayerMediaTypes(ctx, ce.oci, ec.ExportedIndex.Manifests...) + } else { + ec.ExportedManifest.Layers = compression.ConvertAllLayerMediaTypes(ctx, ce.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,25 +161,24 @@ 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 + schemaVersion := specs.Versioned{SchemaVersion: CacheManifestSchemaVersion} - 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 + var mediaType string if ce.oci && !ce.imageManifest { - mfst.MediaType = ocispecs.MediaTypeImageIndex + mediaType = ocispecs.MediaTypeImageIndex } else if ce.imageManifest { - mfst.MediaType = ocispecs.MediaTypeImageManifest + if !ce.oci { + return nil, errors.Errorf("invalid configuration for remote cache") + } + mediaType = ocispecs.MediaTypeImageManifest + } else { + mediaType = images.MediaTypeDockerSchema2ManifestList + } + cacheType := ManifestList + if ce.imageManifest { + cacheType = ImageManifest } + cache := NewExportableCache(cacheType, mediaType, schemaVersion) for _, l := range config.Layers { dgstPair, ok := descs[l.Blob] @@ -105,16 +190,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, ce) dt, err := json.Marshal(config) if err != nil { @@ -132,13 +211,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 +222,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) diff --git a/cache/remotecache/import.go b/cache/remotecache/import.go index d294c08dbce9e..347d935e4a641 100644 --- a/cache/remotecache/import.go +++ b/cache/remotecache/import.go @@ -8,9 +8,6 @@ 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" @@ -18,6 +15,7 @@ import ( "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" @@ -25,27 +23,6 @@ import ( "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 { diff --git a/client/client_test.go b/client/client_test.go index b2b1c2e64ba23..d7b789dce07a8 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -195,6 +195,7 @@ func TestIntegration(t *testing.T) { testMountStubsDirectory, testMountStubsTimestamp, testSourcePolicy, + testImageManifestRegistryCacheImportExport, testLLBMountPerformance, testClientCustomGRPCOpts, testMultipleRecordsWithSameLayersCacheImportExport, @@ -4710,6 +4711,36 @@ func testZstdLocalCacheImportExport(t *testing.T, sb integration.Sandbox) { testBasicCacheImportExport(t, sb, []CacheOptionsEntry{im}, []CacheOptionsEntry{ex}) } +func testImageManifestRegistryCacheImportExport(t *testing.T, sb integration.Sandbox) { + integration.CheckFeatureCompat(t, sb, + integration.FeatureCacheExport, + integration.FeatureCacheImport, + integration.FeatureCacheBackendRegistry, + ) + registry, err := sb.NewRegistry() + if errors.Is(err, integration.ErrRequirements) { + t.Skip(err.Error()) + } + require.NoError(t, err) + target := registry + "/buildkit/testexport:latest" + im := CacheOptionsEntry{ + Type: "registry", + Attrs: map[string]string{ + "ref": target, + }, + } + ex := CacheOptionsEntry{ + Type: "registry", + Attrs: map[string]string{ + "ref": target, + "image-manifest": "true", + "oci-mediatypes": "true", + "mode": "max", + }, + } + testBasicCacheImportExport(t, sb, []CacheOptionsEntry{im}, []CacheOptionsEntry{ex}) +} + func testZstdRegistryCacheImportExport(t *testing.T, sb integration.Sandbox) { integration.CheckFeatureCompat(t, sb, integration.FeatureCacheExport, diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 7633de7851299..6d4e39f9548aa 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -80,6 +80,7 @@ var allTests = integration.TestFuncs( testMultiStageCaseInsensitive, testLabels, testCacheImportExport, + testImageManifestCacheImportExport, testReproducibleIDs, testImportExportReproducibleIDs, testNoCache, @@ -4079,6 +4080,101 @@ COPY --from=base arch / } } +func testImageManifestCacheImportExport(t *testing.T, sb integration.Sandbox) { + integration.CheckFeatureCompat(t, sb, integration.FeatureCacheExport, integration.FeatureCacheBackendLocal) + f := getFrontend(t, sb) + + registry, err := sb.NewRegistry() + if errors.Is(err, integration.ErrRequirements) { + t.Skip(err.Error()) + } + require.NoError(t, err) + + dockerfile := []byte(` +FROM busybox AS base +COPY foo const +#RUN echo -n foobar > const +RUN cat /dev/urandom | head -c 100 | sha256sum > unique +FROM scratch +COPY --from=base const / +COPY --from=base unique / +`) + + dir, err := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + fstest.CreateFile("foo", []byte("foobar"), 0600), + ) + require.NoError(t, err) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + destDir := t.TempDir() + + target := registry + "/buildkit/testexportdf:latest" + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + CacheExports: []client.CacheOptionsEntry{ + { + Type: "registry", + Attrs: map[string]string{ + "ref": target, + "oci-mediatypes": "true", + "image-manifest": "true", + }, + }, + }, + LocalDirs: map[string]string{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dt, err := os.ReadFile(filepath.Join(destDir, "const")) + require.NoError(t, err) + require.Equal(t, "foobar", string(dt)) + + dt, err = os.ReadFile(filepath.Join(destDir, "unique")) + require.NoError(t, err) + + ensurePruneAll(t, c, sb) + + destDir = t.TempDir() + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + FrontendAttrs: map[string]string{ + "cache-from": target, + }, + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + LocalDirs: map[string]string{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dt2, err := os.ReadFile(filepath.Join(destDir, "const")) + require.NoError(t, err) + require.Equal(t, "foobar", string(dt2)) + + dt2, err = os.ReadFile(filepath.Join(destDir, "unique")) + require.NoError(t, err) + require.Equal(t, string(dt), string(dt2)) +} func testCacheImportExport(t *testing.T, sb integration.Sandbox) { integration.CheckFeatureCompat(t, sb, integration.FeatureCacheExport, integration.FeatureCacheBackendLocal) f := getFrontend(t, sb)