-
Notifications
You must be signed in to change notification settings - Fork 1.4k
llbsolver: support pinning sources (same as the "Dockerfile.pin" proposal but agnostic to frontends) #2943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
llbsolver: support pinning sources (same as the "Dockerfile.pin" proposal but agnostic to frontends) #2943
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ( | |
| "github.com/moby/buildkit/solver" | ||
| "github.com/moby/buildkit/solver/llbsolver" | ||
| "github.com/moby/buildkit/solver/pb" | ||
| "github.com/moby/buildkit/sourcepolicy" | ||
| "github.com/moby/buildkit/util/bklog" | ||
| "github.com/moby/buildkit/util/imageutil" | ||
| "github.com/moby/buildkit/util/throttle" | ||
|
|
@@ -306,6 +307,18 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* | |
| Attrs: im.Attrs, | ||
| }) | ||
| } | ||
| var srcPol *sourcepolicy.SourcePolicy | ||
| if req.SourcePolicy != nil { | ||
| srcPol = &sourcepolicy.SourcePolicy{} | ||
| for _, f := range req.SourcePolicy.Sources { | ||
| s := sourcepolicy.Source{ | ||
| Type: sourcepolicy.SourceType(f.Type), | ||
| Ref: f.Ref, | ||
| Pin: f.Pin, | ||
| } | ||
| srcPol.Sources = append(srcPol.Sources, s) | ||
| } | ||
| } | ||
|
|
||
| resp, err := c.solver.Solve(ctx, req.Ref, req.Session, frontend.SolveRequest{ | ||
| Frontend: req.Frontend, | ||
|
|
@@ -317,7 +330,7 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* | |
| Exporter: expi, | ||
| CacheExporter: cacheExporter, | ||
| CacheExportMode: cacheExportMode, | ||
| }, req.Entitlements) | ||
| }, req.Entitlements, srcPol) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the policy is part of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decoupled from |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be quite future compatible.
SourceIdentifieris a wildcard comparison against https://github.com/moby/buildkit/blob/d1b0d8a/solver/pb/ops.proto#L179-L181 . Maybe without the protocol.Typeis separately so that identifier value can be normalized based on it. Attrs can be used to match a specific attribute value.Policy can allow, deny or convert a source. In case on conversion identifier is replaced and new attribute values can be defined. For example in images this would mean setting identifier to
ref@sha256and for HTTP it would mean setting thehttp.checksumattribute.Every source is checked against all policy conditions. Earlier rule might deny the source, and next may allow it. If source is converted, then the new identifier needs to match the policy again (there needs to be some infinite loop protection).
Some examples:
No images are allowed except if they are busybox latest or any alpine:
Any Busybox points to Alpine (like a mirror), Alpine:latest is pinned (meaning busybox:latest is pinned as well)?
(might need a special key that sets the value a regexp?)
If build uses Alpine then use the local OCI-layout version instead
@AkihiroSuda WDYT?
fyi @slimslenderslacks
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tonistiigi
One concern is that this policy structure looks an imperative program (like iptables rules) rather than a declarative structure (like
go.modandgo.sum).It might be hard to implement an automated locking tool like
go mod tidy.What is this map for?
Do we need version? #2943 (comment)
What is
$1?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is for user to actually define policies, eg. images that are allowed/denied, require certain signatures etc then it can't be done with something like
go mod tidy. If you are only interested of pinning, then you would only have convert rules in your policy and then you can do automatic update based on some conditions etc. Pinning is a subset, but you can use only that if you want to.The way I would imagine this defined is that you have one policy for your project and one for your whole host. Then they would be applied on top of each other. In the host file you, for example, would likely not put any pins. In the project one, you would likely not deny a specific image but either deny all (undefined) images or define specific signatures that your images require(also something that tidy possibly can't do). Then there are the use-cases where you use conversion rules the same as
--build-contexttoday during dev without a specific policy file.New
Attrsfor the conversion result, fixed.No, it was just carried over.
First match for the wildcard component. Same as regexp. I'm flexible on how this would be actually defined. Eg. in that example
busybox:testwould be converted toalpine:test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpuguy83 SGTY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of using a
oneofinstead of special keys likeoci-layout://? I guess the API has to be appended to for any new format to be supported (maybe not bad for policy like this).I'm a bit worried about bugs in the policy engine leading to infinite loops or possibly suprising behaviors with the way things are suggested here.
Taking the example:
[ {"docker-image", "DENY", "*"}, {"docker-image", "ALLOW", "docker.io/library/alpine:*"}, {"docker-image", "ALLOW", "docker.io/library/busybox:latest"}, ]I would almost say if someone were to append a
CONVERTrule where the original ref is not already allowed then the image should be denied.Just trying to think of all the cases here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have consensus on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpuguy83 no objections in over a week, so tempted to say that we do have consensus 😄
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consensus on avoiding OPA/Rego, and considering wasm later.
But still not sure how the buildkit-specific config would look like (to machine and humans).
Is anyone working on design and POC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything missing from the proposal on top of this comment?
I think this shouldn't be a requirement for this PR. I imagine the human format would also for example have policy for pushes that is not part of buildkit solver at all, just validation for the build parameters.