From 1050795af1a767f57118de2b550cc200e6fc529f Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Tue, 6 Dec 2022 11:24:01 +0000 Subject: [PATCH 1/5] client: solve method should not modify opt argument Signed-off-by: Justin Chadwell --- client/solve.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/client/solve.go b/client/solve.go index 8a8ac5b10c0c..b1d78b5368d5 100644 --- a/client/solve.go +++ b/client/solve.go @@ -210,11 +210,12 @@ func (c *Client) solve(ctx context.Context, def *llb.Definition, runGateway runG }) } + frontendAttrs := map[string]string{} + for k, v := range opt.FrontendAttrs { + frontendAttrs[k] = v + } for k, v := range cacheOpt.frontendAttrs { - if opt.FrontendAttrs == nil { - opt.FrontendAttrs = map[string]string{} - } - opt.FrontendAttrs[k] = v + frontendAttrs[k] = v } solveCtx, cancelSolve := context.WithCancel(ctx) @@ -254,7 +255,7 @@ func (c *Client) solve(ctx context.Context, def *llb.Definition, runGateway runG ExporterAttrs: ex.Attrs, Session: s.ID(), Frontend: opt.Frontend, - FrontendAttrs: opt.FrontendAttrs, + FrontendAttrs: frontendAttrs, FrontendInputs: frontendInputs, Cache: cacheOpt.options, Entitlements: opt.AllowedEntitlements, @@ -270,7 +271,7 @@ func (c *Client) solve(ctx context.Context, def *llb.Definition, runGateway runG if runGateway != nil { eg.Go(func() error { - err := runGateway(ref, s, opt.FrontendAttrs) + err := runGateway(ref, s, frontendAttrs) if err == nil { return nil } From 8edf57d0a12fece977ea133d4e73d468cecb5d5e Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 25 Nov 2022 17:21:41 +0000 Subject: [PATCH 2/5] client: don't clear frontend attributes to send to server Originally, for a Build, we avoided sending any of the FrontendAttrs to the main Solve call, and sent them all to the sub-Solve calls. However, when we added attestations, we had to add an additional Filter, to ensure those args were sent to both locations. While unintuitive, there wasn't logical location to put them, which would have applied to the entire build. This pattern has become fairly common, for instances where the buildkit backend can provide a fallback when the frontend does not support a specified option, for example, with SOURCE_DATE_EPOCH. This means that this filtering must be updated for each instance - instead of this, we can remove the filtering entirely, which should provide for easier upgrade paths going forwards. Signed-off-by: Justin Chadwell --- client/build.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/build.go b/client/build.go index f40d83cc27bf..2a4bc9e105d1 100644 --- a/client/build.go +++ b/client/build.go @@ -4,7 +4,6 @@ import ( "context" "github.com/moby/buildkit/client/buildid" - "github.com/moby/buildkit/frontend/attestations" gateway "github.com/moby/buildkit/frontend/gateway/client" "github.com/moby/buildkit/frontend/gateway/grpcclient" gatewayapi "github.com/moby/buildkit/frontend/gateway/pb" @@ -24,7 +23,6 @@ func (c *Client) Build(ctx context.Context, opt SolveOpt, product string, buildF feOpts := opt.FrontendAttrs opt.Frontend = "" - opt.FrontendAttrs = attestations.Filter(opt.FrontendAttrs) if product == "" { product = apicaps.ExportedProduct From 2fe41bcec05906f995f14c5f5f686ba5d97d852f Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 25 Nov 2022 17:35:00 +0000 Subject: [PATCH 3/5] dockerfile: correct typo multiple platforms in error Signed-off-by: Justin Chadwell --- frontend/dockerfile/builder/build.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index 5763ae77a1e0..52d05dd6279f 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -414,7 +414,7 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { return nil, errors.Errorf("invalid boolean value %s", v) } if !b && exportMap { - return nil, errors.Errorf("returning multiple target plaforms is not allowed") + return nil, errors.Errorf("returning multiple target platforms is not allowed") } exportMap = b } From 50c2083304ecd46569b6f6ec0e7ac3d428cf8111 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 25 Nov 2022 17:36:18 +0000 Subject: [PATCH 4/5] epoch: refactor frontend attribute parsing Signed-off-by: Justin Chadwell --- control/control.go | 4 ++-- exporter/containerimage/opts.go | 2 +- exporter/local/export.go | 2 +- exporter/tar/export.go | 2 +- exporter/util/epoch/parse.go | 9 ++++++++- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/control/control.go b/control/control.go index 9e9a1c5f3f10..0057a4a52253 100644 --- a/control/control.go +++ b/control/control.go @@ -295,12 +295,12 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* } // if SOURCE_DATE_EPOCH is set, enable it for the exporter - if epochVal, ok := req.FrontendAttrs["build-arg:SOURCE_DATE_EPOCH"]; ok { + if v, ok := epoch.ParseBuildArgs(req.FrontendAttrs); ok { if _, ok := req.ExporterAttrs[epoch.KeySourceDateEpoch]; !ok { if req.ExporterAttrs == nil { req.ExporterAttrs = make(map[string]string) } - req.ExporterAttrs[epoch.KeySourceDateEpoch] = epochVal + req.ExporterAttrs[epoch.KeySourceDateEpoch] = v } } diff --git a/exporter/containerimage/opts.go b/exporter/containerimage/opts.go index b44c855e9632..057bd299e4f2 100644 --- a/exporter/containerimage/opts.go +++ b/exporter/containerimage/opts.go @@ -45,7 +45,7 @@ func (c *ImageCommitOpts) Load(opt map[string]string) (map[string]string, error) } opt = toStringMap(optb) - c.Epoch, opt, err = epoch.ParseAttr(opt) + c.Epoch, opt, err = epoch.ParseExporterAttrs(opt) if err != nil { return nil, err } diff --git a/exporter/local/export.go b/exporter/local/export.go index 381e68f4b87e..6538bfef1d6e 100644 --- a/exporter/local/export.go +++ b/exporter/local/export.go @@ -41,7 +41,7 @@ func New(opt Opt) (exporter.Exporter, error) { } func (e *localExporter) Resolve(ctx context.Context, opt map[string]string) (exporter.ExporterInstance, error) { - tm, _, err := epoch.ParseAttr(opt) + tm, _, err := epoch.ParseExporterAttrs(opt) if err != nil { return nil, err } diff --git a/exporter/tar/export.go b/exporter/tar/export.go index 42c6313fddb6..c8a7c8ac4a3e 100644 --- a/exporter/tar/export.go +++ b/exporter/tar/export.go @@ -48,7 +48,7 @@ func New(opt Opt) (exporter.Exporter, error) { func (e *localExporter) Resolve(ctx context.Context, opt map[string]string) (exporter.ExporterInstance, error) { li := &localExporterInstance{localExporter: e} - tm, _, err := epoch.ParseAttr(opt) + tm, _, err := epoch.ParseExporterAttrs(opt) if err != nil { return nil, err } diff --git a/exporter/util/epoch/parse.go b/exporter/util/epoch/parse.go index e7c88864f4bb..9d581ed9136c 100644 --- a/exporter/util/epoch/parse.go +++ b/exporter/util/epoch/parse.go @@ -10,10 +10,17 @@ import ( ) const ( + frontendSourceDateEpochArg = "build-arg:SOURCE_DATE_EPOCH" + KeySourceDateEpoch = "source-date-epoch" ) -func ParseAttr(opt map[string]string) (*time.Time, map[string]string, error) { +func ParseBuildArgs(opt map[string]string) (string, bool) { + v, ok := opt[frontendSourceDateEpochArg] + return v, ok +} + +func ParseExporterAttrs(opt map[string]string) (*time.Time, map[string]string, error) { rest := make(map[string]string, len(opt)) var tm *time.Time From 6f21d6b4030b15c75a54101cfc7adf2ffa6fa97d Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 25 Nov 2022 17:42:04 +0000 Subject: [PATCH 5/5] exporter: detect if multi-platform is set This is required now that the correspondence between platforms and refs has been broken. Without it, there's no way to detect between the following cases from the output that they emit: buildctl build --opt platform=linux/amd64 --opt attest:sbom buildctl build --opt platform=linux/amd64 --opt attest:sbom --opt build-arg:BUILDKIT_MULTI_PLATFORM=true We'd like the former to produce a flat file structure through the local exporter, and the latter to produce an explicitly nested file structure through the same exporter. However, both of these produce Refs instead of a singular Ref, so we can't just look at the Result to know which one. Similar to how we handle the SOURCE_DATE_EPOCH build-arg, we can handle the multi-platform args at the control API boundary, setting the explicit option multi-platform in the exporter if it is set. In the future, we can guide users away from the BUILDKIT_MULTI_PLATFORM args entirely, and towards the multi-platform exporter option if we want. Signed-off-by: Justin Chadwell --- control/control.go | 11 +++++++ exporter/containerimage/opts.go | 7 +++++ exporter/containerimage/writer.go | 33 ++++++++++++++++++-- exporter/local/export.go | 22 ++++++++++++-- exporter/local/fs.go | 1 + exporter/tar/export.go | 22 ++++++++++++-- exporter/util/multiplatform/parse.go | 45 ++++++++++++++++++++++++++++ 7 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 exporter/util/multiplatform/parse.go diff --git a/control/control.go b/control/control.go index 0057a4a52253..c250cd9cc252 100644 --- a/control/control.go +++ b/control/control.go @@ -17,6 +17,7 @@ import ( controlgateway "github.com/moby/buildkit/control/gateway" "github.com/moby/buildkit/exporter" "github.com/moby/buildkit/exporter/util/epoch" + "github.com/moby/buildkit/exporter/util/multiplatform" "github.com/moby/buildkit/frontend" "github.com/moby/buildkit/frontend/attestations" "github.com/moby/buildkit/session" @@ -304,6 +305,16 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* } } + // if multi-platform is set, enable it for the exporter + if v, ok := multiplatform.ParseBuildArgs(req.FrontendAttrs); ok { + if _, ok := req.ExporterAttrs[multiplatform.KeyMultiPlatform]; !ok { + if req.ExporterAttrs == nil { + req.ExporterAttrs = make(map[string]string) + } + req.ExporterAttrs[multiplatform.KeyMultiPlatform] = v + } + } + if req.Exporter != "" { exp, err := w.Exporter(req.Exporter, c.opt.SessionManager) if err != nil { diff --git a/exporter/containerimage/opts.go b/exporter/containerimage/opts.go index 057bd299e4f2..50e57e7360c3 100644 --- a/exporter/containerimage/opts.go +++ b/exporter/containerimage/opts.go @@ -6,6 +6,7 @@ import ( cacheconfig "github.com/moby/buildkit/cache/config" "github.com/moby/buildkit/exporter/util/epoch" + "github.com/moby/buildkit/exporter/util/multiplatform" "github.com/moby/buildkit/util/compression" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -34,6 +35,7 @@ type ImageCommitOpts struct { BuildInfoAttrs bool Annotations AnnotationsGroup Epoch *time.Time + MultiPlatform *bool } func (c *ImageCommitOpts) Load(opt map[string]string) (map[string]string, error) { @@ -50,6 +52,11 @@ func (c *ImageCommitOpts) Load(opt map[string]string) (map[string]string, error) return nil, err } + c.MultiPlatform, opt, err = multiplatform.ParseExporterAttrs(opt) + if err != nil { + return nil, err + } + for k, v := range opt { var err error switch k { diff --git a/exporter/containerimage/writer.go b/exporter/containerimage/writer.go index c4155f251f53..fa266e3d36bf 100644 --- a/exporter/containerimage/writer.go +++ b/exporter/containerimage/writer.go @@ -63,11 +63,20 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session return nil, errors.Errorf("unable to export multiple refs, missing platforms mapping") } + multiPlatform := len(inp.Refs) > 0 + var p exptypes.Platforms if ok && len(platformsBytes) > 0 { if err := json.Unmarshal(platformsBytes, &p); err != nil { return nil, errors.Wrapf(err, "failed to parse platforms passed to exporter") } + if len(p.Platforms) > 1 { + multiPlatform = true + } + } + + if opts.MultiPlatform != nil { + multiPlatform = *opts.MultiPlatform } if opts.Epoch == nil { @@ -92,8 +101,26 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session } } - if len(inp.Refs) == 0 { - remotes, err := ic.exportLayers(ctx, opts.RefCfg, session.NewGroup(sessionID), inp.Ref) + if !multiPlatform { + if len(p.Platforms) > 1 { + return nil, errors.Errorf("cannot export multiple platforms without multi-platform enabled") + } + + var ref cache.ImmutableRef + if inp.Ref != nil { + ref = inp.Ref + } else if len(p.Platforms) > 0 { + p := p.Platforms[0] + if _, ok := inp.Attestations[p.ID]; ok { + return nil, errors.Errorf("cannot export attestations without multi-platform enabled") + } + ref = inp.Refs[p.ID] + } else if len(inp.Refs) == 1 { + for _, ref = range inp.Refs { + } + } + + remotes, err := ic.exportLayers(ctx, opts.RefCfg, session.NewGroup(sessionID), ref) if err != nil { return nil, err } @@ -112,7 +139,7 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session return nil, errors.Errorf("index annotations not supported for single platform export") } - mfstDesc, configDesc, err := ic.commitDistributionManifest(ctx, opts, inp.Ref, inp.Metadata[exptypes.ExporterImageConfigKey], &remotes[0], annotations, inp.Metadata[exptypes.ExporterInlineCache], dtbi, opts.Epoch, session.NewGroup(sessionID)) + mfstDesc, configDesc, err := ic.commitDistributionManifest(ctx, opts, ref, inp.Metadata[exptypes.ExporterImageConfigKey], &remotes[0], annotations, inp.Metadata[exptypes.ExporterInlineCache], dtbi, opts.Epoch, session.NewGroup(sessionID)) if err != nil { return nil, err } diff --git a/exporter/local/export.go b/exporter/local/export.go index 6538bfef1d6e..ab0b206977fd 100644 --- a/exporter/local/export.go +++ b/exporter/local/export.go @@ -11,6 +11,7 @@ import ( "github.com/moby/buildkit/exporter" "github.com/moby/buildkit/exporter/containerimage/exptypes" "github.com/moby/buildkit/exporter/util/epoch" + "github.com/moby/buildkit/exporter/util/multiplatform" "github.com/moby/buildkit/session" "github.com/moby/buildkit/session/filesync" "github.com/moby/buildkit/solver/result" @@ -46,10 +47,16 @@ func (e *localExporter) Resolve(ctx context.Context, opt map[string]string) (exp return nil, err } + multiPlatform, _, err := multiplatform.ParseExporterAttrs(opt) + if err != nil { + return nil, err + } + i := &localExporterInstance{ localExporter: e, opts: CreateFSOpts{ - Epoch: tm, + Epoch: tm, + MultiPlatform: multiPlatform, }, } @@ -93,6 +100,8 @@ func (e *localExporterInstance) Export(ctx context.Context, inp *exporter.Source return nil, err } + isMap := len(inp.Refs) > 0 + platformsBytes, ok := inp.Metadata[exptypes.ExporterPlatformsKey] if len(inp.Refs) > 0 && !ok { return nil, errors.Errorf("unable to export multiple refs, missing platforms mapping") @@ -103,8 +112,17 @@ func (e *localExporterInstance) Export(ctx context.Context, inp *exporter.Source if err := json.Unmarshal(platformsBytes, &p); err != nil { return nil, errors.Wrapf(err, "failed to parse platforms passed to exporter") } + if len(p.Platforms) > 1 { + isMap = true + } + } + + if e.opts.MultiPlatform != nil { + isMap = *e.opts.MultiPlatform + } + if !isMap && len(p.Platforms) > 1 { + return nil, errors.Errorf("unable to export multiple platforms without map") } - isMap := len(p.Platforms) > 1 now := time.Now().Truncate(time.Second) diff --git a/exporter/local/fs.go b/exporter/local/fs.go index a2026b55ea91..07d524f73b6a 100644 --- a/exporter/local/fs.go +++ b/exporter/local/fs.go @@ -27,6 +27,7 @@ import ( type CreateFSOpts struct { Epoch *time.Time AttestationPrefix string + MultiPlatform *bool } func CreateFS(ctx context.Context, sessionID string, k string, ref cache.ImmutableRef, refs map[string]cache.ImmutableRef, attestations []result.Attestation, defaultTime time.Time, opt CreateFSOpts) (fsutil.FS, func() error, error) { diff --git a/exporter/tar/export.go b/exporter/tar/export.go index c8a7c8ac4a3e..186a4b3ea164 100644 --- a/exporter/tar/export.go +++ b/exporter/tar/export.go @@ -13,6 +13,7 @@ import ( "github.com/moby/buildkit/exporter/containerimage/exptypes" "github.com/moby/buildkit/exporter/local" "github.com/moby/buildkit/exporter/util/epoch" + "github.com/moby/buildkit/exporter/util/multiplatform" "github.com/moby/buildkit/session" "github.com/moby/buildkit/session/filesync" "github.com/moby/buildkit/solver/result" @@ -48,12 +49,18 @@ func New(opt Opt) (exporter.Exporter, error) { func (e *localExporter) Resolve(ctx context.Context, opt map[string]string) (exporter.ExporterInstance, error) { li := &localExporterInstance{localExporter: e} - tm, _, err := epoch.ParseExporterAttrs(opt) + tm, opt, err := epoch.ParseExporterAttrs(opt) if err != nil { return nil, err } li.opts.Epoch = tm + multiPlatform, opt, err := multiplatform.ParseExporterAttrs(opt) + if err != nil { + return nil, err + } + li.opts.MultiPlatform = multiPlatform + for k, v := range opt { switch k { case preferNondistLayersKey: @@ -126,6 +133,8 @@ func (e *localExporterInstance) Export(ctx context.Context, inp *exporter.Source }, nil } + isMap := len(inp.Refs) > 0 + platformsBytes, ok := inp.Metadata[exptypes.ExporterPlatformsKey] if len(inp.Refs) > 0 && !ok { return nil, errors.Errorf("unable to export multiple refs, missing platforms mapping") @@ -136,8 +145,17 @@ func (e *localExporterInstance) Export(ctx context.Context, inp *exporter.Source if err := json.Unmarshal(platformsBytes, &p); err != nil { return nil, errors.Wrapf(err, "failed to parse platforms passed to exporter") } + if len(p.Platforms) > 1 { + isMap = true + } + } + + if e.opts.MultiPlatform != nil { + isMap = *e.opts.MultiPlatform + } + if !isMap && len(p.Platforms) > 1 { + return nil, errors.Errorf("unable to export multiple platforms without map") } - isMap := len(p.Platforms) > 1 var fs fsutil.FS diff --git a/exporter/util/multiplatform/parse.go b/exporter/util/multiplatform/parse.go new file mode 100644 index 000000000000..a9d6ef631851 --- /dev/null +++ b/exporter/util/multiplatform/parse.go @@ -0,0 +1,45 @@ +package multiplatform + +import ( + "strconv" + + "github.com/pkg/errors" +) + +const ( + frontendMultiPlatform = "multi-platform" + frontendMultiPlatformArg = "build-arg:BUILDKIT_MULTI_PLATFORM" + + KeyMultiPlatform = "multi-platform" +) + +func ParseBuildArgs(opt map[string]string) (string, bool) { + if v, ok := opt[frontendMultiPlatform]; ok { + return v, true + } + if v, ok := opt[frontendMultiPlatformArg]; ok { + return v, true + } + return "", false +} + +func ParseExporterAttrs(opt map[string]string) (*bool, map[string]string, error) { + rest := make(map[string]string, len(opt)) + + var multiPlatform *bool + + for k, v := range opt { + switch k { + case KeyMultiPlatform: + b, err := strconv.ParseBool(v) + if err != nil { + return nil, nil, errors.Errorf("invalid boolean value %s", v) + } + multiPlatform = &b + default: + rest[k] = v + } + } + + return multiPlatform, rest, nil +}