Skip to content

Commit d59218e

Browse files
jsternbergjsternberg
committed
llb: deterministic marshaling for protobuf and store results from multiple constraints
This fixes a problem with the new protobuf marshaling with the standard library. LLB digests are now forced into deterministic marshaling to ensure they produce the same digest when marshaled multiple times. In addition, the marshal cache has also been fixed to work in multi-threaded frontends with multiple different constraints. Previously, if an LLB vertex was used in multiple goroutines and marshaled concurrently, the cache would be broken. This could cause certain problems when a specific node was used multiple times in the same LLB tree. Signed-off-by: Jonathan A. Sternberg <[email protected]>
1 parent 185751b commit d59218e

File tree

8 files changed

+63
-49
lines changed

8 files changed

+63
-49
lines changed

‎client/llb/diff.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55

66
"github.com/moby/buildkit/solver/pb"
77
digest "github.com/opencontainers/go-digest"
8-
protobuf "google.golang.org/protobuf/proto"
98
)
109

1110
type DiffOp struct {
@@ -32,8 +31,8 @@ func (m *DiffOp) Validate(ctx context.Context, constraints *Constraints) error {
3231
}
3332

3433
func (m *DiffOp) Marshal(ctx context.Context, constraints *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
35-
if m.Cached(constraints) {
36-
return m.Load()
34+
if dgst, dt, md, srcs, err := m.Load(constraints); err == nil {
35+
return dgst, dt, md, srcs, nil
3736
}
3837
if err := m.Validate(ctx, constraints); err != nil {
3938
return "", nil, nil, nil, err
@@ -68,13 +67,12 @@ func (m *DiffOp) Marshal(ctx context.Context, constraints *Constraints) (digest.
6867

6968
proto.Op = &pb.Op_Diff{Diff: op}
7069

71-
dt, err := protobuf.Marshal(proto)
70+
dt, err := deterministicMarshal(proto)
7271
if err != nil {
7372
return "", nil, nil, nil, err
7473
}
7574

76-
m.Store(dt, md, m.constraints.SourceLocations, constraints)
77-
return m.Load()
75+
return m.Store(dt, md, m.constraints.SourceLocations, constraints)
7876
}
7977

8078
func (m *DiffOp) Output() Output {

‎client/llb/exec.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,10 @@ func (e *ExecOp) Validate(ctx context.Context, c *Constraints) error {
129129
}
130130

131131
func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
132-
if e.Cached(c) {
133-
return e.Load()
132+
if dgst, dt, md, srcs, err := e.Load(c); err == nil {
133+
return dgst, dt, md, srcs, nil
134134
}
135+
135136
if err := e.Validate(ctx, c); err != nil {
136137
return "", nil, nil, nil, err
137138
}
@@ -442,12 +443,11 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []
442443
peo.Mounts = append(peo.Mounts, pm)
443444
}
444445

445-
dt, err := proto.Marshal(pop)
446+
dt, err := deterministicMarshal(pop)
446447
if err != nil {
447448
return "", nil, nil, nil, err
448449
}
449-
e.Store(dt, md, e.constraints.SourceLocations, c)
450-
return e.Load()
450+
return e.Store(dt, md, e.constraints.SourceLocations, c)
451451
}
452452

453453
func (e *ExecOp) Output() Output {

‎client/llb/fileop.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -730,9 +730,10 @@ func (ms *marshalState) add(fa *FileAction, c *Constraints) (*fileActionState, e
730730
}
731731

732732
func (f *FileOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
733-
if f.Cached(c) {
734-
return f.Load()
733+
if dgst, dt, md, srcs, err := f.Load(c); err == nil {
734+
return dgst, dt, md, srcs, nil
735735
}
736+
736737
if err := f.Validate(ctx, c); err != nil {
737738
return "", nil, nil, nil, err
738739
}
@@ -795,12 +796,11 @@ func (f *FileOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []
795796
})
796797
}
797798

798-
dt, err := proto.Marshal(pop)
799+
dt, err := deterministicMarshal(pop)
799800
if err != nil {
800801
return "", nil, nil, nil, err
801802
}
802-
f.Store(dt, md, f.constraints.SourceLocations, c)
803-
return f.Load()
803+
return f.Store(dt, md, f.constraints.SourceLocations, c)
804804
}
805805

806806
func normalizePath(parent, p string, keepSlash bool) string {

‎client/llb/llbbuild/llbbuild.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,10 @@ func (b *build) Validate(context.Context, *llb.Constraints) error {
4747
}
4848

4949
func (b *build) Marshal(ctx context.Context, c *llb.Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*llb.SourceLocation, error) {
50-
if b.Cached(c) {
51-
return b.Load()
50+
if dgst, dt, md, srcs, err := b.Load(c); err == nil {
51+
return dgst, dt, md, srcs, nil
5252
}
53+
5354
pbo := &pb.BuildOp{
5455
Builder: int64(pb.LLBBuilder),
5556
Inputs: map[string]*pb.BuildInput{
@@ -84,8 +85,7 @@ func (b *build) Marshal(ctx context.Context, c *llb.Constraints) (digest.Digest,
8485
if err != nil {
8586
return "", nil, nil, nil, err
8687
}
87-
b.Store(dt, md, b.constraints.SourceLocations, c)
88-
return b.Load()
88+
return b.Store(dt, md, b.constraints.SourceLocations, c)
8989
}
9090

9191
func (b *build) Output() llb.Output {

‎client/llb/marshal.go

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ package llb
22

33
import (
44
"io"
5+
"sync"
56

7+
cerrdefs "github.com/containerd/errdefs"
68
"github.com/containerd/platforms"
79
"github.com/moby/buildkit/solver/pb"
810
digest "github.com/opencontainers/go-digest"
@@ -115,25 +117,37 @@ func MarshalConstraints(base, override *Constraints) (*pb.Op, *pb.OpMetadata) {
115117
}
116118

117119
type MarshalCache struct {
118-
digest digest.Digest
119-
dt []byte
120-
md *pb.OpMetadata
121-
srcs []*SourceLocation
122-
constraints *Constraints
120+
cache sync.Map
123121
}
124122

125-
func (mc *MarshalCache) Cached(c *Constraints) bool {
126-
return mc.dt != nil && mc.constraints == c
123+
func (mc *MarshalCache) Load(c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
124+
v, ok := mc.cache.Load(c)
125+
if !ok {
126+
return "", nil, nil, nil, cerrdefs.ErrNotFound
127+
}
128+
129+
res := v.(*marshalCacheResult)
130+
return res.digest, res.dt, res.md, res.srcs, nil
131+
}
132+
133+
func (mc *MarshalCache) Store(dt []byte, md *pb.OpMetadata, srcs []*SourceLocation, c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
134+
res := &marshalCacheResult{
135+
digest: digest.FromBytes(dt),
136+
dt: dt,
137+
md: md,
138+
srcs: srcs,
139+
}
140+
mc.cache.Store(c, res)
141+
return res.digest, res.dt, res.md, res.srcs, nil
127142
}
128143

129-
func (mc *MarshalCache) Load() (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
130-
return mc.digest, mc.dt, mc.md, mc.srcs, nil
144+
type marshalCacheResult struct {
145+
digest digest.Digest
146+
dt []byte
147+
md *pb.OpMetadata
148+
srcs []*SourceLocation
131149
}
132150

133-
func (mc *MarshalCache) Store(dt []byte, md *pb.OpMetadata, srcs []*SourceLocation, c *Constraints) {
134-
mc.digest = digest.FromBytes(dt)
135-
mc.dt = dt
136-
mc.md = md
137-
mc.constraints = c
138-
mc.srcs = srcs
151+
func deterministicMarshal[Message proto.Message](m Message) ([]byte, error) {
152+
return proto.MarshalOptions{Deterministic: true}.Marshal(m)
139153
}

‎client/llb/merge.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"github.com/moby/buildkit/solver/pb"
77
digest "github.com/opencontainers/go-digest"
88
"github.com/pkg/errors"
9-
"google.golang.org/protobuf/proto"
109
)
1110

1211
type MergeOp struct {
@@ -33,9 +32,10 @@ func (m *MergeOp) Validate(ctx context.Context, constraints *Constraints) error
3332
}
3433

3534
func (m *MergeOp) Marshal(ctx context.Context, constraints *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
36-
if m.Cached(constraints) {
37-
return m.Load()
35+
if dgst, dt, md, srcs, err := m.Load(constraints); err == nil {
36+
return dgst, dt, md, srcs, nil
3837
}
38+
3939
if err := m.Validate(ctx, constraints); err != nil {
4040
return "", nil, nil, nil, err
4141
}
@@ -54,13 +54,12 @@ func (m *MergeOp) Marshal(ctx context.Context, constraints *Constraints) (digest
5454
}
5555
pop.Op = &pb.Op_Merge{Merge: op}
5656

57-
dt, err := proto.Marshal(pop)
57+
dt, err := deterministicMarshal(pop)
5858
if err != nil {
5959
return "", nil, nil, nil, err
6060
}
6161

62-
m.Store(dt, md, m.constraints.SourceLocations, constraints)
63-
return m.Load()
62+
return m.Store(dt, md, m.constraints.SourceLocations, constraints)
6463
}
6564

6665
func (m *MergeOp) Output() Output {

‎client/llb/source.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"github.com/moby/buildkit/util/sshutil"
1818
digest "github.com/opencontainers/go-digest"
1919
"github.com/pkg/errors"
20-
protobuf "google.golang.org/protobuf/proto"
2120
)
2221

2322
type SourceOp struct {
@@ -50,9 +49,10 @@ func (s *SourceOp) Validate(ctx context.Context, c *Constraints) error {
5049
}
5150

5251
func (s *SourceOp) Marshal(ctx context.Context, constraints *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) {
53-
if s.Cached(constraints) {
54-
return s.Load()
52+
if dgst, dt, md, srcs, err := s.Load(constraints); err == nil {
53+
return dgst, dt, md, srcs, nil
5554
}
55+
5656
if err := s.Validate(ctx, constraints); err != nil {
5757
return "", nil, nil, nil, err
5858
}
@@ -77,13 +77,12 @@ func (s *SourceOp) Marshal(ctx context.Context, constraints *Constraints) (diges
7777
proto.Platform = nil
7878
}
7979

80-
dt, err := protobuf.Marshal(proto)
80+
dt, err := deterministicMarshal(proto)
8181
if err != nil {
8282
return "", nil, nil, nil, err
8383
}
8484

85-
s.Store(dt, md, s.constraints.SourceLocations, constraints)
86-
return s.Load()
85+
return s.Store(dt, md, s.constraints.SourceLocations, constraints)
8786
}
8887

8988
func (s *SourceOp) Output() Output {

‎solver/llbsolver/vertex.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func recomputeDigests(ctx context.Context, all map[digest.Digest]*pb.Op, visited
228228
return dgst, nil
229229
}
230230

231-
dt, err := proto.Marshal(op)
231+
dt, err := deterministicMarshal(op)
232232
if err != nil {
233233
return "", err
234234
}
@@ -263,7 +263,7 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval
263263
return solver.Edge{}, errors.Wrap(err, "error evaluating the source policy")
264264
}
265265
if mutated {
266-
dtMutated, err := proto.Marshal(&op)
266+
dtMutated, err := deterministicMarshal(&op)
267267
if err != nil {
268268
return solver.Edge{}, err
269269
}
@@ -397,3 +397,7 @@ func fileOpName(actions []*pb.FileAction) string {
397397

398398
return strings.Join(names, ", ")
399399
}
400+
401+
func deterministicMarshal[Message proto.Message](m Message) ([]byte, error) {
402+
return proto.MarshalOptions{Deterministic: true}.Marshal(m)
403+
}

0 commit comments

Comments
 (0)