topdown/eval: Fix key construction for NDBCache. #5097
Merged
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
This commit fixes the construction process for keys used to look up entries in the non-deterministic builtins cache. The original cache lookup process could use refs that were not fully-grounded, and this meant that calling a builtin twice with differing parameters could appear identical to the cache if they involved refs as parameters. We now use fully-grounded terms for the cache keys during insertion/lookup, meaning that non-deterministic builtin calls with different parameters are now guaranteed to result in unique cache entries, even if hidden behind a ref. Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
The NDBCache is intended as an opt-in feature, and the initializations present in the `rego` module meant that it was forced on virtually every eval. This commit removes those initializations, so that the only way an NDBCache will ever make it to the evaluator is if it's provided from outside the evaluator. Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
This commit adds a test to ensure that the NDBCache correctly handles iterative calls of a builtin with different input parameters. Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
tsandall
approved these changes
Sep 6, 2022
tsandall
reviewed
Sep 6, 2022
Comment on lines
+2124
to
+2136
| urls := [ | ||
| "https://httpbin.org/headers", | ||
| "https://httpbin.org/ip", | ||
| "https://httpbin.org/user-agent" | ||
| ] | ||
|
|
||
| results[response] { | ||
| some url in urls | ||
| response := http.send({ | ||
| "method": "GET", | ||
| "url": url | ||
| }) | ||
| }`), |
There was a problem hiding this comment.
We're likely to run into test flakes w/ this because it has to reach out to the internet. We would be better off registering a mock built-in function that sets the Nondeterministic bit.
There was a problem hiding this comment.
...or use rand.int_n with a key from a variable...? But agreed let's find a way to do this without the internet.
There was a problem hiding this comment.
Let’s keep using http.send here, although with a mock server. The impact of breaking http.send vs. rand.int_n makes it valuable, even if the bug affects both in this case.
srenatus
approved these changes
Sep 7, 2022
Comment on lines
+2124
to
+2136
| urls := [ | ||
| "https://httpbin.org/headers", | ||
| "https://httpbin.org/ip", | ||
| "https://httpbin.org/user-agent" | ||
| ] | ||
|
|
||
| results[response] { | ||
| some url in urls | ||
| response := http.send({ | ||
| "method": "GET", | ||
| "url": url | ||
| }) | ||
| }`), |
There was a problem hiding this comment.
...or use rand.int_n with a key from a variable...? But agreed let's find a way to do this without the internet.
|
I'll update the test in a follow-up. Merging. |
srenatus
added a commit
that referenced
this pull request
Sep 7, 2022
Follow-up to #5097 (parent commit). Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
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.
This commit fixes the construction process for keys used to look up
entries in the non-deterministic builtins cache. The original cache
lookup process could use refs that were not fully-grounded, and this
meant that calling a builtin twice with differing parameters could
appear identical to the cache if they involved refs as parameters.
We now use fully-grounded terms for the cache keys during insertion/lookup,
meaning that non-deterministic builtin calls with different parameters
are now guaranteed to result in unique cache entries, even if hidden
behind a ref.
This commit also fixes a bug that prevented the NDBCache from being opt-in.