llbsolver: fix policy rule ordering #4014
Conversation
|
@cpuguy83 Was there a reason to deduplicate in this function? Other variants could be:
|
|
Nice find! 🎉 ❤️ Since they're order-dependent, can rewrite mid-chain, and have fixed limits on the number of, for example, converts that can happen during a single evaluation, deduplicating feels premature to me, but I definitely might be missing something important. 😅 |
|
I don't remember writing this at all. Wow. |
| err := b.EachValue(context.TODO(), keySourcePolicy, func(v interface{}) error { | ||
| x, ok := v.(spb.Policy) | ||
| if !ok { | ||
| return errors.Errorf("invalid source policy %T", v) | ||
| } | ||
| for _, f := range x.Rules { | ||
| set[*f] = struct{}{} | ||
| r := *f | ||
| srcPol.Rules = append(srcPol.Rules, &r) |
There was a problem hiding this comment.
I guess we should copy Version over as well?
There was a problem hiding this comment.
sigh and rules have pointers in them I guess we need to dereference.
There was a problem hiding this comment.
sigh and rules have pointers in them I guess we need to dereference.
Do we need to make sure these are copied? I just left it like this because that is what old code did?
There was a problem hiding this comment.
This is probably fine since nothing is modifying the policies.
There was a problem hiding this comment.
I added Version just for completeness. Especially for the common case where EachVertex only returns one value. I think as a follow-up, returning the policy array would make more sense here.
The older of rules in policy matters. Eg. in [DENY *, ALLOW ref] mixing the order would deny all sources so map can't be used to deduplicate the rules. Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
fix-policy-rule-order
branch
from
bf292c7
to
22d8446
Compare
|
It seems there might still be a race here somewhere -- I can reproduce it a lot less frequently with this change, but it does still come periodically: $ printf 'FROM bash:latest' | EXPERIMENTAL_BUILDKIT_SOURCE_POLICY=<(jq -nc '{ rules: [ { action: "DENY", selector: { identifier: "*" } }, { action: "ALLOW", selector: { identifier: "local://dockerfile" } }, { action: "CONVERT", selector: { identifier: "docker-image://docker.io/library/bash:latest@*" }, updates: { identifier: "docker-image://docker.io/library/debian:bullseye-slim" } }, { action: "ALLOW", selector: { identifier: "docker-image://docker.io/library/debian:bullseye-slim" } } ] }') ./buildx --builder foo build --progress=plain -
#0 building with "foo" instance using docker-container driver
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 53B done
#1 DONE 0.0s
#2 [internal] load metadata for docker.io/library/bash:latest
#2 DONE 0.2s
#3 docker-image://docker.io/library/debian:bullseye-slim
#3 resolve docker.io/library/debian:bullseye-slim
#3 resolve docker.io/library/debian:bullseye-slim 0.2s done
#3 DONE 0.2s
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
ERROR: failed to solve: missing provenance for h9oahwy6afza4swiriqsd7aorbut every so often (I had to run it 16 times in quick sequence to get this, where before this PR it was way more frequent): $ printf 'FROM bash:latest' | EXPERIMENTAL_BUILDKIT_SOURCE_POLICY=<(jq -nc '{ rules: [ { action: "DENY", selector: { identifier: "*" } }, { action: "ALLOW", selector: { identifier: "local://dockerfile" } }, { action: "CONVERT", selector: { identifier: "docker-image://docker.io/library/bash:latest@*" }, updates: { identifier: "docker-image://docker.io/library/debian:bullseye-slim" } }, { action: "ALLOW", selector: { identifier: "docker-image://docker.io/library/debian:bullseye-slim" } } ] }') ./buildx --builder foo build --progress=plain -
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
ERROR: failed to solve: failed to read dockerfile: failed to load LLB: error evaluating the source policy: source "local://dockerfile" denied by policy: source denied by policy
` |
|
Just to note: The selector With changing that ref to just |
The order of rules in policy matters. Eg. in [DENY *, ALLOW ref] mixing the order would deny all sources so map can't be used to deduplicate the rules.
@cpuguy83 @tianon