From c600bda6c170380e0a1485764f5b7b82daa159be Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Mon, 30 Sep 2024 16:53:30 -0500 Subject: [PATCH] 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 --- client/llb/diff.go | 10 +++----- client/llb/exec.go | 10 ++++---- client/llb/fileop.go | 10 ++++---- client/llb/llbbuild/llbbuild.go | 8 +++--- client/llb/marshal.go | 44 ++++++++++++++++++++++----------- client/llb/merge.go | 11 ++++----- client/llb/source.go | 11 ++++----- solver/llbsolver/vertex.go | 8 ++++-- 8 files changed, 63 insertions(+), 49 deletions(-) diff --git a/client/llb/diff.go b/client/llb/diff.go index c216259385be..96c60dd62c7a 100644 --- a/client/llb/diff.go +++ b/client/llb/diff.go @@ -5,7 +5,6 @@ import ( "github.com/moby/buildkit/solver/pb" digest "github.com/opencontainers/go-digest" - protobuf "google.golang.org/protobuf/proto" ) type DiffOp struct { @@ -32,8 +31,8 @@ func (m *DiffOp) Validate(ctx context.Context, constraints *Constraints) error { } func (m *DiffOp) Marshal(ctx context.Context, constraints *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { - if m.Cached(constraints) { - return m.Load() + if dgst, dt, md, srcs, err := m.Load(constraints); err == nil { + return dgst, dt, md, srcs, nil } if err := m.Validate(ctx, constraints); err != nil { return "", nil, nil, nil, err @@ -68,13 +67,12 @@ func (m *DiffOp) Marshal(ctx context.Context, constraints *Constraints) (digest. proto.Op = &pb.Op_Diff{Diff: op} - dt, err := protobuf.Marshal(proto) + dt, err := deterministicMarshal(proto) if err != nil { return "", nil, nil, nil, err } - m.Store(dt, md, m.constraints.SourceLocations, constraints) - return m.Load() + return m.Store(dt, md, m.constraints.SourceLocations, constraints) } func (m *DiffOp) Output() Output { diff --git a/client/llb/exec.go b/client/llb/exec.go index 382499109bac..1cf6652332db 100644 --- a/client/llb/exec.go +++ b/client/llb/exec.go @@ -129,9 +129,10 @@ func (e *ExecOp) Validate(ctx context.Context, c *Constraints) error { } func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { - if e.Cached(c) { - return e.Load() + if dgst, dt, md, srcs, err := e.Load(c); err == nil { + return dgst, dt, md, srcs, nil } + if err := e.Validate(ctx, c); err != nil { return "", nil, nil, nil, err } @@ -442,12 +443,11 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, [] peo.Mounts = append(peo.Mounts, pm) } - dt, err := proto.Marshal(pop) + dt, err := deterministicMarshal(pop) if err != nil { return "", nil, nil, nil, err } - e.Store(dt, md, e.constraints.SourceLocations, c) - return e.Load() + return e.Store(dt, md, e.constraints.SourceLocations, c) } func (e *ExecOp) Output() Output { diff --git a/client/llb/fileop.go b/client/llb/fileop.go index 85af57e093c1..ae289199c159 100644 --- a/client/llb/fileop.go +++ b/client/llb/fileop.go @@ -730,9 +730,10 @@ func (ms *marshalState) add(fa *FileAction, c *Constraints) (*fileActionState, e } func (f *FileOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { - if f.Cached(c) { - return f.Load() + if dgst, dt, md, srcs, err := f.Load(c); err == nil { + return dgst, dt, md, srcs, nil } + if err := f.Validate(ctx, c); err != nil { return "", nil, nil, nil, err } @@ -795,12 +796,11 @@ func (f *FileOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, [] }) } - dt, err := proto.Marshal(pop) + dt, err := deterministicMarshal(pop) if err != nil { return "", nil, nil, nil, err } - f.Store(dt, md, f.constraints.SourceLocations, c) - return f.Load() + return f.Store(dt, md, f.constraints.SourceLocations, c) } func normalizePath(parent, p string, keepSlash bool) string { diff --git a/client/llb/llbbuild/llbbuild.go b/client/llb/llbbuild/llbbuild.go index c02557319b3f..6e9bd075de3d 100644 --- a/client/llb/llbbuild/llbbuild.go +++ b/client/llb/llbbuild/llbbuild.go @@ -47,9 +47,10 @@ func (b *build) Validate(context.Context, *llb.Constraints) error { } func (b *build) Marshal(ctx context.Context, c *llb.Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*llb.SourceLocation, error) { - if b.Cached(c) { - return b.Load() + if dgst, dt, md, srcs, err := b.Load(c); err == nil { + return dgst, dt, md, srcs, nil } + pbo := &pb.BuildOp{ Builder: int64(pb.LLBBuilder), Inputs: map[string]*pb.BuildInput{ @@ -84,8 +85,7 @@ func (b *build) Marshal(ctx context.Context, c *llb.Constraints) (digest.Digest, if err != nil { return "", nil, nil, nil, err } - b.Store(dt, md, b.constraints.SourceLocations, c) - return b.Load() + return b.Store(dt, md, b.constraints.SourceLocations, c) } func (b *build) Output() llb.Output { diff --git a/client/llb/marshal.go b/client/llb/marshal.go index 3da5e957fac5..869247a26f3b 100644 --- a/client/llb/marshal.go +++ b/client/llb/marshal.go @@ -2,7 +2,9 @@ package llb import ( "io" + "sync" + "github.com/containerd/errdefs" "github.com/containerd/platforms" "github.com/moby/buildkit/solver/pb" digest "github.com/opencontainers/go-digest" @@ -115,25 +117,37 @@ func MarshalConstraints(base, override *Constraints) (*pb.Op, *pb.OpMetadata) { } type MarshalCache struct { - digest digest.Digest - dt []byte - md *pb.OpMetadata - srcs []*SourceLocation - constraints *Constraints + cache sync.Map } -func (mc *MarshalCache) Cached(c *Constraints) bool { - return mc.dt != nil && mc.constraints == c +func (mc *MarshalCache) Load(c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { + v, ok := mc.cache.Load(c) + if !ok { + return "", nil, nil, nil, errdefs.ErrNotFound + } + + res := v.(*marshalCacheResult) + return res.digest, res.dt, res.md, res.srcs, nil +} + +func (mc *MarshalCache) Store(dt []byte, md *pb.OpMetadata, srcs []*SourceLocation, c *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { + res := &marshalCacheResult{ + digest: digest.FromBytes(dt), + dt: dt, + md: md, + srcs: srcs, + } + mc.cache.Store(c, res) + return res.digest, res.dt, res.md, res.srcs, nil } -func (mc *MarshalCache) Load() (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { - return mc.digest, mc.dt, mc.md, mc.srcs, nil +type marshalCacheResult struct { + digest digest.Digest + dt []byte + md *pb.OpMetadata + srcs []*SourceLocation } -func (mc *MarshalCache) Store(dt []byte, md *pb.OpMetadata, srcs []*SourceLocation, c *Constraints) { - mc.digest = digest.FromBytes(dt) - mc.dt = dt - mc.md = md - mc.constraints = c - mc.srcs = srcs +func deterministicMarshal[Message proto.Message](m Message) ([]byte, error) { + return proto.MarshalOptions{Deterministic: true}.Marshal(m) } diff --git a/client/llb/merge.go b/client/llb/merge.go index 369ea0f6d56e..289c84c4f2ae 100644 --- a/client/llb/merge.go +++ b/client/llb/merge.go @@ -6,7 +6,6 @@ import ( "github.com/moby/buildkit/solver/pb" digest "github.com/opencontainers/go-digest" "github.com/pkg/errors" - "google.golang.org/protobuf/proto" ) type MergeOp struct { @@ -33,9 +32,10 @@ func (m *MergeOp) Validate(ctx context.Context, constraints *Constraints) error } func (m *MergeOp) Marshal(ctx context.Context, constraints *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { - if m.Cached(constraints) { - return m.Load() + if dgst, dt, md, srcs, err := m.Load(constraints); err == nil { + return dgst, dt, md, srcs, nil } + if err := m.Validate(ctx, constraints); err != nil { return "", nil, nil, nil, err } @@ -54,13 +54,12 @@ func (m *MergeOp) Marshal(ctx context.Context, constraints *Constraints) (digest } pop.Op = &pb.Op_Merge{Merge: op} - dt, err := proto.Marshal(pop) + dt, err := deterministicMarshal(pop) if err != nil { return "", nil, nil, nil, err } - m.Store(dt, md, m.constraints.SourceLocations, constraints) - return m.Load() + return m.Store(dt, md, m.constraints.SourceLocations, constraints) } func (m *MergeOp) Output() Output { diff --git a/client/llb/source.go b/client/llb/source.go index 46669c0a7444..cae3b2fcc5db 100644 --- a/client/llb/source.go +++ b/client/llb/source.go @@ -17,7 +17,6 @@ import ( "github.com/moby/buildkit/util/sshutil" digest "github.com/opencontainers/go-digest" "github.com/pkg/errors" - protobuf "google.golang.org/protobuf/proto" ) type SourceOp struct { @@ -50,9 +49,10 @@ func (s *SourceOp) Validate(ctx context.Context, c *Constraints) error { } func (s *SourceOp) Marshal(ctx context.Context, constraints *Constraints) (digest.Digest, []byte, *pb.OpMetadata, []*SourceLocation, error) { - if s.Cached(constraints) { - return s.Load() + if dgst, dt, md, srcs, err := s.Load(constraints); err == nil { + return dgst, dt, md, srcs, nil } + if err := s.Validate(ctx, constraints); err != nil { return "", nil, nil, nil, err } @@ -77,13 +77,12 @@ func (s *SourceOp) Marshal(ctx context.Context, constraints *Constraints) (diges proto.Platform = nil } - dt, err := protobuf.Marshal(proto) + dt, err := deterministicMarshal(proto) if err != nil { return "", nil, nil, nil, err } - s.Store(dt, md, s.constraints.SourceLocations, constraints) - return s.Load() + return s.Store(dt, md, s.constraints.SourceLocations, constraints) } func (s *SourceOp) Output() Output { diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go index d4dae31467b7..d4ef8886dfaf 100644 --- a/solver/llbsolver/vertex.go +++ b/solver/llbsolver/vertex.go @@ -228,7 +228,7 @@ func recomputeDigests(ctx context.Context, all map[digest.Digest]*pb.Op, visited return dgst, nil } - dt, err := proto.Marshal(op) + dt, err := deterministicMarshal(op) if err != nil { return "", err } @@ -263,7 +263,7 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval return solver.Edge{}, errors.Wrap(err, "error evaluating the source policy") } if mutated { - dtMutated, err := proto.Marshal(&op) + dtMutated, err := deterministicMarshal(&op) if err != nil { return solver.Edge{}, err } @@ -397,3 +397,7 @@ 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) +}