Skip to content

Commit

Permalink
gateway: ensure llb digests are deterministic when sent by frontends
Browse files Browse the repository at this point in the history
This ensures different valid protobuf serializations that are sent by
frontends will be rewritten into digests that are normalized for the
buildkit solver.

The most recent example of this is that older frontends would generate
protobuf with gogo and the newer buildkit is using the google protobuf
library. These produce different serializations and cause the solver to
think that identical operations are actually different.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
  • Loading branch information
jsternberg committed Nov 14, 2024
1 parent 11f4528 commit 9f65f8c
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 33 deletions.
Binary file added solver/llbsolver/testdata/gogoproto.data
Binary file not shown.
43 changes: 11 additions & 32 deletions solver/llbsolver/vertex.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
digest "github.com/opencontainers/go-digest"
ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
"google.golang.org/protobuf/proto"
)

type vertex struct {
Expand Down Expand Up @@ -208,7 +207,6 @@ func recomputeDigests(ctx context.Context, all map[digest.Digest]*pb.Op, visited
return "", errors.Errorf("invalid missing input digest %s", dgst)
}

var mutated bool
for _, input := range op.Inputs {
select {
case <-ctx.Done():
Expand All @@ -220,25 +218,20 @@ func recomputeDigests(ctx context.Context, all map[digest.Digest]*pb.Op, visited
if err != nil {
return "", err
}
if digest.Digest(input.Digest) != iDgst {
mutated = true
input.Digest = string(iDgst)
}
}

if !mutated {
visited[dgst] = dgst
return dgst, nil
input.Digest = string(iDgst)
}

dt, err := deterministicMarshal(op)
dt, err := op.Marshal()
if err != nil {
return "", err
}

newDgst := digest.FromBytes(dt)
if newDgst != dgst {
all[newDgst] = op
delete(all, dgst)
}
visited[dgst] = newDgst
all[newDgst] = op
delete(all, dgst)
return newDgst, nil
}

Expand All @@ -250,7 +243,6 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval
}

allOps := make(map[digest.Digest]*pb.Op)
mutatedDigests := make(map[digest.Digest]digest.Digest) // key: old, val: new

var lastDgst digest.Digest

Expand All @@ -261,27 +253,18 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval
}
dgst := digest.FromBytes(dt)
if polEngine != nil {
mutated, err := polEngine.Evaluate(ctx, op.GetSource())
if err != nil {
if _, err := polEngine.Evaluate(ctx, op.GetSource()); err != nil {
return solver.Edge{}, errors.Wrap(err, "error evaluating the source policy")
}
if mutated {
dtMutated, err := deterministicMarshal(&op)
if err != nil {
return solver.Edge{}, err
}
dgstMutated := digest.FromBytes(dtMutated)
mutatedDigests[dgst] = dgstMutated
dgst = dgstMutated
}
}

allOps[dgst] = &op
lastDgst = dgst
}

mutatedDigests := make(map[digest.Digest]digest.Digest) // key: old, val: new
for dgst := range allOps {
_, err := recomputeDigests(ctx, allOps, mutatedDigests, dgst)
if err != nil {
if _, err := recomputeDigests(ctx, allOps, mutatedDigests, dgst); err != nil {
return solver.Edge{}, err
}
}
Expand Down Expand Up @@ -400,7 +383,3 @@ func fileOpName(actions []*pb.FileAction) string {

return strings.Join(names, ", ")
}

func deterministicMarshal[Message proto.Message](m Message) ([]byte, error) {
return proto.MarshalOptions{Deterministic: true}.Marshal(m)
}
61 changes: 61 additions & 0 deletions solver/llbsolver/vertex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package llbsolver

import (
"context"
_ "embed"
"fmt"
"testing"

"github.com/moby/buildkit/solver/pb"
Expand Down Expand Up @@ -53,3 +55,62 @@ func TestRecomputeDigests(t *testing.T) {
require.Equal(t, newDigest, digest.Digest(op2.Inputs[0].Digest))
assert.NotEqual(t, op2Digest, updated)
}

//go:embed testdata/gogoproto.data
var gogoprotoData []byte

func TestIngestDigest(t *testing.T) {
op1 := &pb.Op{
Op: &pb.Op_Source{
Source: &pb.SourceOp{
Identifier: "docker-image://docker.io/library/busybox:latest",
},
},
}
op1Data, err := op1.Marshal()
require.NoError(t, err)
op1Digest := digest.FromBytes(op1Data)

op2 := &pb.Op{
Inputs: []*pb.Input{
{Digest: string(op1Digest)}, // Input is the old digest, this should be updated after recomputeDigests
},
}
op2Data, err := op2.Marshal()
require.NoError(t, err)
op2Digest := digest.FromBytes(op2Data)

var def pb.Definition
err = def.Unmarshal(gogoprotoData)
require.NoError(t, err)
require.Len(t, def.Def, 2)

// 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{}
for _, in := range def.Def {
op := new(pb.Op)
err := op.Unmarshal(in)
require.NoError(t, err)

lastDgst = digest.FromBytes(in)
all[lastDgst] = op
}
fmt.Println(all, lastDgst)

visited := map[digest.Digest]digest.Digest{}
newDgst, err := recomputeDigests(context.Background(), all, visited, lastDgst)
require.NoError(t, err)
require.Len(t, visited, 2)
require.Equal(t, op2Digest, newDgst)
require.Equal(t, op2Digest, visited[newDgst])
delete(visited, newDgst)

// Last element should correspond to op1.
// The old digest doesn't really matter.
require.Len(t, visited, 1)
for _, newDgst := range visited {
require.Equal(t, op1Digest, newDgst)
}
}
4 changes: 3 additions & 1 deletion solver/pb/ops.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package pb

import proto "google.golang.org/protobuf/proto"

func (m *Definition) IsNil() bool {
return m == nil || m.Metadata == nil
}
Expand All @@ -13,7 +15,7 @@ func (m *Definition) Unmarshal(dAtA []byte) error {
}

func (m *Op) Marshal() ([]byte, error) {
return m.MarshalVT()
return proto.MarshalOptions{Deterministic: true}.Marshal(m)
}

func (m *Op) Unmarshal(dAtA []byte) error {
Expand Down

0 comments on commit 9f65f8c

Please sign in to comment.