From 0a045f6b83e2bcc22a1663af97343f7514d4c807 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 5 Jan 2024 14:08:33 +0000 Subject: [PATCH 1/3] gateway: avoid calling Definition on nil ref It's possible for ref to be nil (e.g. from llb.Scratch stored in a multi-platform result). In this case, we should handle a nil value correctly, and *not* call Definition on it. Also, we update the corresponding check for a single Ref to be the same for consistency. Signed-off-by: Justin Chadwell --- frontend/gateway/gateway.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/frontend/gateway/gateway.go b/frontend/gateway/gateway.go index 24b645f13bc3..d052e514ccca 100644 --- a/frontend/gateway/gateway.go +++ b/frontend/gateway/gateway.go @@ -682,14 +682,15 @@ func (lbf *llbBridgeForwarder) Solve(ctx context.Context, req *pb.SolveRequest) ids := make(map[string]string, len(res.Refs)) defs := make(map[string]*opspb.Definition, len(res.Refs)) for k, ref := range res.Refs { - id := identity.NewID() - if ref == nil { - id = "" - } else { + var id string + var def *opspb.Definition + if ref != nil { + id = identity.NewID() + def = ref.Definition() lbf.refs[id] = ref } ids[k] = id - defs[k] = ref.Definition() + defs[k] = def } if req.AllowResultArrayRef { @@ -703,12 +704,10 @@ func (lbf *llbBridgeForwarder) Solve(ctx context.Context, req *pb.SolveRequest) } } else { ref := res.Ref - id := identity.NewID() - + var id string var def *opspb.Definition - if ref == nil { - id = "" - } else { + if ref != nil { + id = identity.NewID() def = ref.Definition() lbf.refs[id] = ref } From 7c0f37b09b76593da7fe3394f249036af2551f9a Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 5 Jan 2024 14:12:16 +0000 Subject: [PATCH 2/3] solver: avoid discarding nil refs entry This is possible with llb.Scratch in a multi-platform build. We were accidentally discarding the nil entries in Refs. Instead, we just skip over nil refs. This isn't ideal - we *should* be able generate provenance for a nil ref. However, 1. this isn't handled for llb.Scratch as a single-platform result, and 2. this is tricky, since the ResultProxy is nil at this point (so there's no ID to match on). This should be ok for a quick fix, but we can come back and fix this later. Signed-off-by: Justin Chadwell --- exporter/containerimage/export.go | 6 ++++++ solver/llbsolver/provenance.go | 3 +++ solver/llbsolver/solver.go | 6 ++++++ solver/result/result.go | 6 ++++++ 4 files changed, 21 insertions(+) diff --git a/exporter/containerimage/export.go b/exporter/containerimage/export.go index 3f8865f8e830..107b387d31f4 100644 --- a/exporter/containerimage/export.go +++ b/exporter/containerimage/export.go @@ -292,6 +292,9 @@ func (e *imageExporterInstance) Export(ctx context.Context, src *exporter.Source refs = append(refs, src.Ref) } for _, ref := range src.Refs { + if ref == nil { + continue + } refs = append(refs, ref) } eg, ctx := errgroup.WithContext(ctx) @@ -358,6 +361,9 @@ func (e *imageExporterInstance) pushImage(ctx context.Context, src *exporter.Sou refs = append(refs, src.Ref) } for _, ref := range src.Refs { + if ref == nil { + continue + } refs = append(refs, ref) } diff --git a/solver/llbsolver/provenance.go b/solver/llbsolver/provenance.go index 5c2c13025d34..0ad255fd807b 100644 --- a/solver/llbsolver/provenance.go +++ b/solver/llbsolver/provenance.go @@ -80,6 +80,9 @@ func (b *provenanceBridge) requests(r *frontend.Result) (*resultRequests, error) } for k, ref := range r.Refs { + if ref == nil { + continue + } r, ok := b.findByResult(ref) if !ok { return nil, errors.Errorf("could not find request for ref %s", ref.ID()) diff --git a/solver/llbsolver/solver.go b/solver/llbsolver/solver.go index 4f6ab93c981f..7ddf44a37298 100644 --- a/solver/llbsolver/solver.go +++ b/solver/llbsolver/solver.go @@ -269,6 +269,9 @@ func (s *Solver) recordBuildHistory(ctx context.Context, id string, req frontend } for k, r := range res.Refs { + if r == nil { + continue + } k, r := k, r cp := res.Provenance.Refs[k] eg.Go(func() error { @@ -719,6 +722,9 @@ func addProvenanceToResult(res *frontend.Result, br *provenanceBridge) (*Result, out.Provenance.Refs = make(map[string]*provenance.Capture, len(res.Refs)) } for k, ref := range res.Refs { + if ref == nil { + continue + } cp, err := getProvenance(ref, reqs.refs[k].bridge, k, reqs) if err != nil { return nil, err diff --git a/solver/result/result.go b/solver/result/result.go index cfcfe9dcbd3e..ed04e970aa3c 100644 --- a/solver/result/result.go +++ b/solver/result/result.go @@ -142,6 +142,10 @@ func EachRef[U comparable, V comparable](a *Result[U], b *Result[V], fn func(U, return err } +// ConvertResult transforms a Result[U] into a Result[V], using a transfomer +// function that converts a U to a V. Zero values of type U are converted to +// zero values of type V directly, without passing through the transformer +// function. func ConvertResult[U comparable, V comparable](r *Result[U], fn func(U) (V, error)) (*Result[V], error) { var zero U @@ -160,6 +164,8 @@ func ConvertResult[U comparable, V comparable](r *Result[U], fn func(U) (V, erro } for k, r := range r.Refs { if r == zero { + var zero V + r2.Refs[k] = zero continue } r2.Refs[k], err = fn(r) From 206fa177ab73da28130de148c1e1938b27a9ddcb Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 5 Jan 2024 14:18:32 +0000 Subject: [PATCH 3/3] test: add test case for multi-platform scratch Signed-off-by: Justin Chadwell --- client/client_test.go | 132 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 107 insertions(+), 25 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 7951bd709765..3927bf849984 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2266,49 +2266,131 @@ func testBuildExportScratch(t *testing.T, sb integration.Sandbox) { require.NoError(t, err) defer c.Close() - st := llb.Scratch() - def, err := st.Marshal(sb.Context()) - require.NoError(t, err) - registry, err := sb.NewRegistry() if errors.Is(err, integration.ErrRequirements) { t.Skip(err.Error()) } require.NoError(t, err) + makeFrontend := func(ps []string) func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + return func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + st := llb.Scratch() + def, err := st.Marshal(sb.Context()) + require.NoError(t, err) + + r, err := c.Solve(ctx, gateway.SolveRequest{ + Definition: def.ToPB(), + }) + if err != nil { + return nil, err + } + + ref, err := r.SingleRef() + if err != nil { + return nil, err + } + + res := gateway.NewResult() + if ps == nil { + res.SetRef(ref) + } else { + for _, p := range ps { + res.AddRef(p, ref) + } + + expPlatforms := &exptypes.Platforms{ + Platforms: make([]exptypes.Platform, len(ps)), + } + for i, pk := range ps { + p := platforms.MustParse(pk) + + img := ocispecs.Image{ + Platform: p, + Config: ocispecs.ImageConfig{ + Labels: map[string]string{ + "foo": "i am platform " + platforms.Format(p), + }, + }, + } + config, err := json.Marshal(img) + if err != nil { + return nil, errors.Wrapf(err, "failed to marshal image config") + } + res.AddMeta(fmt.Sprintf("%s/%s", exptypes.ExporterImageConfigKey, pk), config) + + expPlatforms.Platforms[i] = exptypes.Platform{ + ID: pk, + Platform: p, + } + } + dt, err := json.Marshal(expPlatforms) + if err != nil { + return nil, err + } + res.AddMeta(exptypes.ExporterPlatformsKey, dt) + } + + return res, nil + } + } + target := registry + "/buildkit/build/exporter:withnocompressed" - _, err = c.Solve(sb.Context(), def, SolveOpt{ + _, err = c.Build(sb.Context(), SolveOpt{ Exports: []ExportEntry{ { Type: ExporterImage, Attrs: map[string]string{ - "name": target, - "push": "true", - "unpack": "true", - "compression": "uncompressed", + "name": target, + "push": "true", + "unpack": "true", + "compression": "uncompressed", + "attest:provenance": "mode=max", }, }, }, - }, nil) + }, "", makeFrontend(nil), nil) require.NoError(t, err) - ctx := namespaces.WithNamespace(sb.Context(), "buildkit") - cdAddress := sb.ContainerdAddress() - var client *containerd.Client - if cdAddress != "" { - client, err = newContainerd(cdAddress) - require.NoError(t, err) - defer client.Close() + targetMulti := registry + "/buildkit/build/exporter-multi:withnocompressed" + _, err = c.Build(sb.Context(), SolveOpt{ + Exports: []ExportEntry{ + { + Type: ExporterImage, + Attrs: map[string]string{ + "name": targetMulti, + "push": "true", + "unpack": "true", + "compression": "uncompressed", + "attest:provenance": "mode=max", + }, + }, + }, + }, "", makeFrontend([]string{"linux/amd64", "linux/arm64"}), nil) + require.NoError(t, err) - img, err := client.GetImage(ctx, target) - require.NoError(t, err) - mfst, err := images.Manifest(ctx, client.ContentStore(), img.Target(), nil) - require.NoError(t, err) - require.Equal(t, 0, len(mfst.Layers)) - err = client.ImageService().Delete(ctx, target, images.SynchronousDelete()) - require.NoError(t, err) - } + desc, provider, err := contentutil.ProviderFromRef(target) + require.NoError(t, err) + imgs, err := testutil.ReadImages(sb.Context(), provider, desc) + require.NoError(t, err) + require.Len(t, imgs.Images, 1) + img := imgs.Find(platforms.DefaultString()) + require.Empty(t, img.Layers) + require.Equal(t, platforms.DefaultSpec(), img.Img.Platform) + + desc, provider, err = contentutil.ProviderFromRef(targetMulti) + require.NoError(t, err) + imgs, err = testutil.ReadImages(sb.Context(), provider, desc) + require.NoError(t, err) + require.Len(t, imgs.Images, 2) + img = imgs.Find("linux/amd64") + require.Empty(t, img.Layers) + require.Equal(t, "linux/amd64", platforms.Format(img.Img.Platform)) + require.Equal(t, "i am platform linux/amd64", img.Img.Config.Labels["foo"]) + img = imgs.Find("linux/arm64") + require.Empty(t, img.Layers) + require.Equal(t, "linux/arm64", platforms.Format(img.Img.Platform)) + require.Equal(t, "i am platform linux/arm64", img.Img.Config.Labels["foo"]) } func testBuildHTTPSource(t *testing.T, sb integration.Sandbox) {