From 64293f9034f04c41b20e58a8a6bcbe03cc2d97d7 Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Tue, 3 Dec 2024 13:09:49 -0600 Subject: [PATCH 1/2] llbsolver: tie op metadata to the op before recomputing digests When recomputing digests happens, the metadata is lost. This previously happened only with sources that were affected by source policies so it wasn't noticed, but now any LLB Op that is rewritten is affected since the gogo protobuf switch will cause all digests to be rewritten if the frontend and buildkit are using different versions. Signed-off-by: Jonathan A. Sternberg (cherry picked from commit c644fb81a8eee05af35e14e2d5263f466fa5f512) --- solver/llbsolver/vertex.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go index c2f659c5788e..156d62b4235f 100644 --- a/solver/llbsolver/vertex.go +++ b/solver/llbsolver/vertex.go @@ -155,9 +155,8 @@ func (dpc *detectPrunedCacheID) Load(op *pb.Op, md *pb.OpMetadata, opt *solver.V } func Load(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEvaluator, opts ...LoadOpt) (solver.Edge, error) { - return loadLLB(ctx, def, polEngine, func(dgst digest.Digest, pbOp *pb.Op, load func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error) { - opMetadata := def.Metadata[string(dgst)] - vtx, err := newVertex(dgst, pbOp, opMetadata, load, opts...) + return loadLLB(ctx, def, polEngine, func(dgst digest.Digest, op *op, load func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error) { + vtx, err := newVertex(dgst, &op.Op, op.Metadata, load, opts...) if err != nil { return nil, err } @@ -198,7 +197,7 @@ func newVertex(dgst digest.Digest, op *pb.Op, opMeta *pb.OpMetadata, load func(d return vtx, nil } -func recomputeDigests(ctx context.Context, all map[digest.Digest]*pb.Op, visited map[digest.Digest]digest.Digest, dgst digest.Digest) (digest.Digest, error) { +func recomputeDigests(ctx context.Context, all map[digest.Digest]*op, visited map[digest.Digest]digest.Digest, dgst digest.Digest) (digest.Digest, error) { if dgst, ok := visited[dgst]; ok { return dgst, nil } @@ -235,20 +234,29 @@ func recomputeDigests(ctx context.Context, all map[digest.Digest]*pb.Op, visited return newDgst, nil } +type op struct { + pb.Op + Metadata *pb.OpMetadata +} + +func (o *op) Unmarshal(data []byte) error { + return o.Op.UnmarshalVT(data) +} + // loadLLB loads LLB. // fn is executed sequentially. -func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEvaluator, fn func(digest.Digest, *pb.Op, func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error)) (solver.Edge, error) { +func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEvaluator, fn func(digest.Digest, *op, func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error)) (solver.Edge, error) { if len(def.Def) == 0 { return solver.Edge{}, errors.New("invalid empty definition") } - allOps := make(map[digest.Digest]*pb.Op) + allOps := make(map[digest.Digest]*op) var lastDgst digest.Digest for _, dt := range def.Def { - var op pb.Op - if err := op.UnmarshalVT(dt); err != nil { + var op op + if err := op.Unmarshal(dt); err != nil { return solver.Edge{}, errors.Wrap(err, "failed to parse llb proto op") } dgst := digest.FromBytes(dt) @@ -257,6 +265,7 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval return solver.Edge{}, errors.Wrap(err, "error evaluating the source policy") } } + op.Metadata = def.Metadata[string(dgst)] allOps[dgst] = &op lastDgst = dgst @@ -300,7 +309,7 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval return nil, errors.Errorf("invalid missing input digest %s", dgst) } - if err := opsutils.Validate(op); err != nil { + if err := opsutils.Validate(&op.Op); err != nil { return nil, err } From ec39add68103d0762f051c18d50b95aa26e0dc38 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 3 Dec 2024 11:28:43 -0800 Subject: [PATCH 2/2] llbsolver: fix recompute test and avoid struct copy Signed-off-by: Tonis Tiigi (cherry picked from commit d1a3df310d617d6f3f25e7316fddc440764fd1b8) --- solver/llbsolver/vertex.go | 24 +++++++++++------------- solver/llbsolver/vertex_test.go | 20 ++++++++++---------- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go index 156d62b4235f..c5e3870a9d43 100644 --- a/solver/llbsolver/vertex.go +++ b/solver/llbsolver/vertex.go @@ -156,7 +156,7 @@ func (dpc *detectPrunedCacheID) Load(op *pb.Op, md *pb.OpMetadata, opt *solver.V func Load(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEvaluator, opts ...LoadOpt) (solver.Edge, error) { return loadLLB(ctx, def, polEngine, func(dgst digest.Digest, op *op, load func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error) { - vtx, err := newVertex(dgst, &op.Op, op.Metadata, load, opts...) + vtx, err := newVertex(dgst, op.Op, op.Metadata, load, opts...) if err != nil { return nil, err } @@ -234,15 +234,12 @@ func recomputeDigests(ctx context.Context, all map[digest.Digest]*op, visited ma return newDgst, nil } +// op is a private wrapper around pb.Op that includes its metadata. type op struct { - pb.Op + *pb.Op Metadata *pb.OpMetadata } -func (o *op) Unmarshal(data []byte) error { - return o.Op.UnmarshalVT(data) -} - // loadLLB loads LLB. // fn is executed sequentially. func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEvaluator, fn func(digest.Digest, *op, func(digest.Digest) (solver.Vertex, error)) (solver.Vertex, error)) (solver.Edge, error) { @@ -255,19 +252,20 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval var lastDgst digest.Digest for _, dt := range def.Def { - var op op - if err := op.Unmarshal(dt); err != nil { + var pbop pb.Op + if err := pbop.Unmarshal(dt); err != nil { return solver.Edge{}, errors.Wrap(err, "failed to parse llb proto op") } dgst := digest.FromBytes(dt) if polEngine != nil { - if _, err := polEngine.Evaluate(ctx, op.GetSource()); err != nil { + if _, err := polEngine.Evaluate(ctx, pbop.GetSource()); err != nil { return solver.Edge{}, errors.Wrap(err, "error evaluating the source policy") } } - op.Metadata = def.Metadata[string(dgst)] - - allOps[dgst] = &op + allOps[dgst] = &op{ + Op: &pbop, + Metadata: def.Metadata[string(dgst)], + } lastDgst = dgst } @@ -309,7 +307,7 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval return nil, errors.Errorf("invalid missing input digest %s", dgst) } - if err := opsutils.Validate(&op.Op); err != nil { + if err := opsutils.Validate(op.Op); err != nil { return nil, err } diff --git a/solver/llbsolver/vertex_test.go b/solver/llbsolver/vertex_test.go index 3123207221f2..7aa68c990f44 100644 --- a/solver/llbsolver/vertex_test.go +++ b/solver/llbsolver/vertex_test.go @@ -38,9 +38,9 @@ func TestRecomputeDigests(t *testing.T) { require.NoError(t, err) op2Digest := digest.FromBytes(op2Data) - all := map[digest.Digest]*pb.Op{ - newDigest: op1, - op2Digest: op2, + all := map[digest.Digest]*op{ + newDigest: {Op: op1}, + op2Digest: {Op: op2}, } visited := map[digest.Digest]digest.Digest{oldDigest: newDigest} @@ -48,10 +48,10 @@ func TestRecomputeDigests(t *testing.T) { require.NoError(t, err) require.Len(t, visited, 2) require.Len(t, all, 2) - assert.Equal(t, op1, all[newDigest]) + assert.Equal(t, op1, all[newDigest].Op) require.Equal(t, newDigest, visited[oldDigest]) - require.Equal(t, op1, all[newDigest]) - assert.Equal(t, op2, all[updated]) + require.Equal(t, op1, all[newDigest].Op) + assert.Equal(t, op2, all[updated].Op) require.Equal(t, newDigest, digest.Digest(op2.Inputs[0].Digest)) assert.NotEqual(t, op2Digest, updated) } @@ -88,14 +88,14 @@ func TestIngestDigest(t *testing.T) { // Read the definition from the test data and ensure it uses the // canonical digests after recompute. var lastDgst digest.Digest - all := map[digest.Digest]*pb.Op{} + all := map[digest.Digest]*op{} for _, in := range def.Def { - op := new(pb.Op) - err := op.Unmarshal(in) + opNew := new(pb.Op) + err := opNew.Unmarshal(in) require.NoError(t, err) lastDgst = digest.FromBytes(in) - all[lastDgst] = op + all[lastDgst] = &op{Op: opNew} } fmt.Println(all, lastDgst)