topdown: fix re-wrapping of ndb_cache errors #5543
Conversation
Errors happening in the "cache hit" code path for NDBCache calls would
come from the iterators called with the function call results retrieved
from the cache. Now, these errors include the sentinel "early exit"
errors, which would ordinarily be suppressed further up the call stack.
However, since the iter()-returned errors had been wrapped into
topdown.Halt{}, they were not picked up by the suppression mechanisms.
Now, those iter-derived errors are no longer wrapped into Halt, and the
code section that suggests that they should be has gotten a new comment
explaining what's going on there.
To improve our blinds spots in testing, the Rego Yaml tests are now also
run with NDBCache enabled. No further assertions are added, those are
tested elsewhere, but it would have caught this problem. Hence it seems
worthwhile to spend the extra 3s in tests.
Signed-off-by: Stephan Renatus <[email protected]>
| @@ -1696,7 +1696,7 @@ func (e evalBuiltin) eval(iter unifyIterator) error { | |||
|
|
|||
| operands := make([]*ast.Term, len(e.terms)) | |||
|
|
|||
| for i := 0; i < len(e.terms); i++ { | |||
| for i := range e.terms { | |||
There was a problem hiding this comment.
style update, immaterial
| } | ||
|
|
||
| if err != nil { | ||
| return Halt{Err: err} |
There was a problem hiding this comment.
The Halt-wrapping is what we'd like to get rid of. The rest of this section is just a small restructuring, going with straightaway
return iter()calls instead of
err = iter()followed by a plain
return errbelow. Since the Halt-wrapping is gone, this became applicable.
There was a problem hiding this comment.
Looks like a solid refactoring to me!
| @@ -34,7 +35,19 @@ func TestOPARego(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func testRun(t *testing.T, tc cases.TestCase) { | |||
| func TestRegoWithNDBCache(t *testing.T) { | |||
There was a problem hiding this comment.
There was one (!!) test case in our ~1700 cases that would have broken if we did this before. But still worthwhile, we'll now run all yaml tests with NDBCache enabled. Since those tests are our safety net for anything more complex going on in topdown, it's worth it, I think.
This is the case -- but it depends on your machine's IP setup, I think:
opa/test/cases/testdata/netlookupipaddr/test-netlookupipaddr.yaml
Lines 28 to 45 in 43be06e
There was a problem hiding this comment.
I'm surprised that it's just one testcase that blows up. 🤔 It's better than none though!
There was a problem hiding this comment.
I guess that's because by their very nature, non-deterministic builtins are difficult to test? 😅
| want_result: | ||
| - x: | ||
| p: true | ||
| q: true |
There was a problem hiding this comment.
This is the test case that @istalker2 came up with to reproduce this with NDBCache enabled.
There was a problem hiding this comment.
Interesting that this one is what breaks everything. 🤔
| } | ||
|
|
||
| if err != nil { | ||
| return Halt{Err: err} |
There was a problem hiding this comment.
Looks like a solid refactoring to me!
| want_result: | ||
| - x: | ||
| p: true | ||
| q: true |
There was a problem hiding this comment.
Interesting that this one is what breaks everything. 🤔
| @@ -34,7 +35,19 @@ func TestOPARego(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func testRun(t *testing.T, tc cases.TestCase) { | |||
| func TestRegoWithNDBCache(t *testing.T) { | |||
There was a problem hiding this comment.
I'm surprised that it's just one testcase that blows up. 🤔 It's better than none though!
|
|
||
| type opt func(*Query) *Query | ||
|
|
||
| func testRun(t *testing.T, tc cases.TestCase, opts ...opt) { |
There was a problem hiding this comment.
Nice refactoring to allow parameterizing the test runner. 👍
Errors happening in the "cache hit" code path for NDBCache calls would come from the iterators called with the function call results retrieved from the cache. Now, these errors include the sentinel "early exit" errors, which would ordinarily be suppressed further up the call stack.
However, since the iter()-returned errors had been wrapped into topdown.Halt{}, they were not picked up by the suppression mechanisms.
Now, those iter-derived errors are no longer wrapped into Halt, and the code section that suggests that they should be has gotten a new comment explaining what's going on there.
To improve our blinds spots in testing, the Rego Yaml tests are now also run with NDBCache enabled. No further assertions are added, those are tested elsewhere, but it would have caught this problem. Hence it seems worthwhile to spend the extra 3s in tests.