Merge pull request moby#5517 from jsternberg/deterministic-llb-for-ga…

…teway

gateway: ensure llb digests are deterministic when sent by frontends

solver/llbsolver/vertex.go

@@ -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 {
@@ -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():
@@ -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
 }
 
@@ -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
 
@@ -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
 		}
 	}
@@ -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)
-}

solver/llbsolver/vertex_test.go

@@ -2,6 +2,8 @@ package llbsolver
 
 import (
 	"context"
+	_ "embed"
+	"fmt"
 	"testing"
 
 	"github.com/moby/buildkit/solver/pb"
@@ -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)
+	}
+}

solver/pb/ops.go

@@ -1,5 +1,7 @@
 package pb
 
+import proto "google.golang.org/protobuf/proto"
+
 func (m *Definition) IsNil() bool {
 	return m == nil || m.Metadata == nil
 }
@@ -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 {