gateway: ensure llb digests are deterministic when sent by frontends #5517
Conversation
jsternberg
force-pushed
the
deterministic-llb-for-gateway
branch
from
f7041e1 to
8c41ea7
Compare
|
We did some more digging and the byte difference between the two is related to field order. Google's protobuf library will marshal the fields based on this code snippet: https://github.com/protocolbuffers/protobuf-go/blob/b98563540c0a4edb38526bcd6e6c97f9fac1f453/internal/order/order.go#L21-L41 It automatically puts any |
frontend/gateway/gateway.go
Outdated
| @@ -760,6 +760,17 @@ func (lbf *llbBridgeForwarder) Solve(ctx context.Context, req *pb.SolveRequest) | |||
| } | |||
| } | |||
|
|
|||
| if req.Definition != nil { | |||
| // Rewrite digests in the definition. This ensures the digests are validated | |||
There was a problem hiding this comment.
Why is this in gateway and not in solver loader where we already have ops remarshal logic for source policies?
There was a problem hiding this comment.
I'll update the logic there. I think the logic also needs to be slightly different. The current one has a kind of conditional rewrite and we'll just need to always remarshal it.
jsternberg
force-pushed
the
deterministic-llb-for-gateway
branch
from
8c41ea7 to
ea52c76
Compare
client/llb/diff.go
Outdated
| @@ -67,7 +67,7 @@ func (m *DiffOp) Marshal(ctx context.Context, constraints *Constraints) (digest. | |||
|
|
|||
| proto.Op = &pb.Op_Diff{Diff: op} | |||
|
|
|||
| dt, err := deterministicMarshal(proto) | |||
| dt, err := proto.Marshal() | |||
There was a problem hiding this comment.
Why these changes in marshal? Does it still call the deterministic internally?
There was a problem hiding this comment.
It's still deterministic but I was defining and calling it in multiple packages. I also figured that having any marshal function that calls MarshalVT when we know that's an improper way to marshal the data would not be a good idea.
To be fair, I made this change when I was adding a third location where we were doing this so I had a bigger interest in removing the duplication. It's now back to two locations.
I also debated renaming the function to MarshalDeterministic() but then I had to change a few call sites and I didn't think it was worth it.
solver/llbsolver/vertex.go
Outdated
|
|
||
| dm.mapping[dgst] = newDgst | ||
| if dgst != newDgst { | ||
| // Ensure the indices also map to the new digest. |
There was a problem hiding this comment.
This looks bit weird. Why do these containers mix old and new digests?
It should be that there is a container to check if digest has been converted already and then lookup by the old digest to op if it has not. I think this is also needed to avoid loops as otherwise loops should be possible by mixing old and new digests in the definition.
solver/llbsolver/vertex.go
Outdated
| } | ||
|
|
||
| index := dm.indexByDigest[dgst] | ||
| dm.out.Def[index] = data |
There was a problem hiding this comment.
Do we need this? We have already parsed the data once. If we write back the encoded data that means we need to parse it again. We should only need to marshal to get the new digest, don't actually need the new bytes.
There was a problem hiding this comment.
Hm I'll go back and see if this is an easier way of doing it. I think you're likely right. I'll give it a try.
jsternberg
force-pushed
the
deterministic-llb-for-gateway
branch
from
ea52c76 to
6081ae3
Compare
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>
jsternberg
force-pushed
the
deterministic-llb-for-gateway
branch
from
6081ae3 to
9f65f8c
Compare
|
Modified this PR to be a bit more faithful to the original code. The code to detect if a mutation happened has now been removed in favor of unconditional mutation, but the rest of it remains faithful to the original so it won't repeat deserialization. I've also added a testdata file with the gogo protobuf serialization to test that things get recomputed correctly. |
|
@cpuguy83 PTAL |
|
I see we always need to recompute all ops now, that's unfortunate but understandable. |
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.
This is done by rewriting the incoming definition sent by the llb bridge forwarder when a gateway calls solve with a protobuf definition.