topdown+rego+server: allow opt-in for evaluating non-det builtins in PE #7313
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
srenatus
changed the title
srenatus
commented
Jan 27, 2025
v1/topdown/save.go
Outdated
Comment on lines
368
to
374
| found = len(node.With) > 0 | ||
| if !ic.nondeterministicBuiltins { // skip evaluating non-det builtins for PE | ||
| found = ignoreExprDuringPartial(node) | ||
| } |
There was a problem hiding this comment.
☝️ This is what it's about, really. The test is pushing the configurable through all the layers of HTTP, CLI and the Rego package.
Some use cases of PE, notably generating queries that are to be translated into filters of some sort (think SQL), require the evaluation of non-deterministic builtins. This is because the result of the builtin informs what queries are returned. Imagine that the user associated with a request is known at PE-time, but we need extra information from an HTTP API to determine the filters that should be applied. Previously, that was just impossible to do. Now, we can opt-in to evaluate non-det builtins during PE from the Rego API. Note that it would probably make sense to include this in the inlining controls, as sent to the Compile API. (Considered out of scope for this PR.) Also note that this will take highest precedence over the `ast.IgnoreDuringPartialEval` map and the "Nondeterministic" value of the registered builtin. If the new option is provided, both of these are ignored. Signed-off-by: Stephan Renatus <[email protected]>
With `foo.rego` as ```rego package ex include if input.fruits.name == object.get(http.send(input.req).body, input.path, "unknown") ``` the following queries show the difference: ```interactive $ curl -v http://127.0.0.1:8181/v1/compile \ -d '{"input": {"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}, "query": "data.ex.include", "unknowns": ["input.fruits"]}' { "result": { "queries": [ [ { "index": 0, "terms": [ { "type": "ref", "value": [ { "type": "var", "value": "http" }, { "type": "string", "value": "send" } ] }, { "type": "object", "value": [ [ { "type": "string", "value": "method" }, { "type": "string", "value": "GET" } ], [ { "type": "string", "value": "url" }, { "type": "string", "value": "https://httpbin.org/json" } ] ] }, { "type": "var", "value": "__local0__1" } ] }, { "index": 1, "terms": [ { "type": "ref", "value": [ { "type": "var", "value": "eq" } ] }, { "type": "ref", "value": [ { "type": "var", "value": "input" }, { "type": "string", "value": "fruits" }, { "type": "string", "value": "name" } ] }, { "type": "call", "value": [ { "type": "ref", "value": [ { "type": "var", "value": "object" }, { "type": "string", "value": "get" } ] }, { "type": "ref", "value": [ { "type": "var", "value": "__local0__1" }, { "type": "string", "value": "body" } ] }, { "type": "array", "value": [ { "type": "string", "value": "slideshow" }, { "type": "string", "value": "title" } ] }, { "type": "string", "value": "unknown" } ] } ] } ] ] } } ``` Here, the builtin call to http.send is preserved. If we also pass `nondeterminsticBuiltins: true` to the options, we get this: ```interactive $ curl http://127.0.0.1:8181/v1/compile \ -d '{"input": {"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}, "query": "data.ex.include", "unknowns": ["input.fruits"], "options": {"nondeterministicBuiltins": true}}' { "result": { "queries": [ [ { "index": 0, "terms": [ { "type": "ref", "value": [ { "type": "var", "value": "eq" } ] }, { "type": "ref", "value": [ { "type": "var", "value": "input" }, { "type": "string", "value": "fruits" }, { "type": "string", "value": "name" } ] }, { "type": "string", "value": "Sample Slide Show" } ] } ] ] } } ``` Here, all args to http.send have been known at PE time and the call was fully evaluated. Signed-off-by: Stephan Renatus <[email protected]>
```interactive
$ echo '{"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}'| ./opa_darwin_amd64 eval -fpretty -p -I -d foo.rego -u input.fruits data.ex.include
+---------+-------------------------------------------------------------------------------------+
| Query 1 | http.send({"method": "GET", "url": "https://httpbin.org/json"}, __local0__1) |
| | input.fruits.name = object.get(__local0__1.body, ["slideshow", "title"], "unknown") |
+---------+-------------------------------------------------------------------------------------+
$ echo '{"req": {"url": "https://httpbin.org/json", "method":"GET"}, "path": ["slideshow", "title"]}'| ./opa_darwin_amd64 eval -fpretty -p -I -d foo.rego -u input.fruits data.ex.include --nondeterminstic-builtins
+---------+-----------------------------------------+
| Query 1 | input.fruits.name = "Sample Slide Show" |
+---------+-----------------------------------------+
```
Signed-off-by: Stephan Renatus <[email protected]>
srenatus
force-pushed
the
sr/pe-with-non-det-builtins
branch
from
20a502d to
44d5d11
Compare
johanfylling
approved these changes
Jan 27, 2025
| @@ -328,6 +329,7 @@ access. | |||
| evalCommand.Flags().BoolVarP(¶ms.coverage, "coverage", "", false, "report coverage") | |||
| evalCommand.Flags().StringArrayVarP(¶ms.disableInlining, "disable-inlining", "", []string{}, "set paths of documents to exclude from inlining") | |||
| evalCommand.Flags().BoolVarP(¶ms.shallowInlining, "shallow-inlining", "", false, "disable inlining of rules that depend on unknowns") | |||
| evalCommand.Flags().BoolVarP(¶ms.nondeterministicBuiltions, "nondeterminstic-builtins", "", false, "evaluate nondeterministic builtins (if all arguments are known) during partial eval") | |||
There was a problem hiding this comment.
if there's a future need for a more fine-grained control, we can invest more time
Agreed, no need to over-engineer this from the get-go 👍 .
If we want to give the user precise control of what built-ins to evaluate, we can add something like a --nondeterminstic-builtin (singular) that takes the name and can be repeated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some use cases of PE, notably generating queries that are to be translated into filters of some sort (think SQL), require the evaluation of non-deterministic builtins. This is because the result of the builtin informs what queries are returned.
Imagine that the user associated with a request is known at PE-time, but we need extra information from an HTTP API to determine the filters that should be applied.
Previously, that was just impossible to do. Now, we can opt-in to evaluate non-det builtins during PE from the Rego API.
Note that it would probably make sense to include this in the inlining controls, as sent to the Compile API. (Considered out of scope for this PR.)
Also note that this will take highest precedence over the
ast.IgnoreDuringPartialEvalmap and the "Nondeterministic" value of the registered builtin. If the new option is provided, both of these are ignored.It's nicest to compare with and without in
opa eval's PE CLI:Fixes #6496. More or less. Previously, all non-det builtin calls were saved; with this, we allow the user to declare that none of them should be saved (unless their args are known). For builtins like
time.now_ns(), which has no args, this doesn't really make a difference. Let's see where this takes us, if there's a future need for a more fine-grained control, we can invest more time.