policy-test: init test runner - #2582
Conversation
db9ddf2 to
86f772f
Compare
86f772f to
3e71596
Compare
charludo
left a comment
charludo
left a comment
There was a problem hiding this comment.
Thanks, this is great! 🎉
Apart from my comments below, a couple general thoughts:
- not sure if we want to export
ManipulateInitdatapublicly... Maybe it belongs in an internal package instead? - I don't think there's a ticket for running this in CI yet, right? Which we can probably do after the local registry PR
- why use
log? I think most everywhere usesslog? (But on the other hand... w/e) - please add some devdocs info about how to obtain the testcases. (Can also be a separate PR, esp. if that's not settled yet.)
- My biggest questionmark is currently how negative testcases will be obtained and implemented here
3e71596 to
667f516
Compare
Hm, I don't like the idea of having this in an internal package either (like
There is a ticket for automating this process, i.e., periodically generating testcases, so I don't think it's necessary to document at this point. |
667f516 to
ecd4a47
Compare
charludo
left a comment
charludo
left a comment
There was a problem hiding this comment.
Thanks, great work! Just some leftover re: errors in parsing files etc cancelling all test cases.
| log.Printf("===== Running test case: %s", file.Name()) | ||
| fileData, err := os.ReadFile(filepath.Join(dataDir, file.Name())) | ||
| if err != nil { | ||
| return fmt.Errorf("read test case file %s: %w", file.Name(), err) |
There was a problem hiding this comment.
same thing with the early return on errors here. Should be collected and skipped.
| } | ||
| var tc []TestCase | ||
| if err := json.Unmarshal(fileData, &tc); err != nil { | ||
| return fmt.Errorf("unmarshal test case %s: %w", file.Name(), err) |
|
|
||
| p, err := NewOPAPolicy(policy) | ||
| if err != nil { | ||
| return fmt.Errorf("create OPA policy: %w", err) |
| } | ||
|
|
||
| var errs []error | ||
| for _, file := range dirs { |
There was a problem hiding this comment.
This may be paranoid, but: wdyt about counting testcases and erroring also when no those are 0? right now, if someone e.g. moves the files to a subdir, the test succeeds, right?
| } | ||
|
|
||
| if len(res) == 0 { | ||
| return false, "", fmt.Errorf("no rule %s found in policy", query) |
There was a problem hiding this comment.
maybe we should also return prints from this function? SO in the error case, we can debug the error directly?
There was a problem hiding this comment.
In this particular case (i.e., len(res) = 0), there are no prints afaik. But in general I agree to add the prints to this block when checking the result length, and only if it is to catch regressions.
ecd4a47 to
7c9e278
Compare
2 participants
This creates the new subpackage
policy-testwhich contains the genpolicy test suite. Currently, all it does is:pod.ymland runcontrast generateon it.Policyinterface (currently rego, i.e.,*OPAPolicy)tesdata/and compare them against the policy (CreateSandboxRequest,CreateContainerRequest, ...)The current test case is taken from the
/tmp/policy.jsonlfrom inside a running container, similar to the one in thepod.yml. For now these are just a few simple cases to test, before we can automate getting these and properly normalize them (e.g., replacing image references, patching namespaces, etc.).The rego policy checker is based on the upstream test runner at https://github.com/kata-containers/kata-containers/tree/main/src/tools/genpolicy/tests/policy. Note that the rego rules don't always return true/false, but can return a metadata response, for example:
In this case, if the request is allowed, we have to apply the json patches contained in
opsto the saved state of the policy engine. This is used to store information like sandbox names or ids over multiple requests.To run the policy test suite, run the
just policytarget. This currently does not use a local registry, so generating the policy one time at the beginning will take a few seconds. The next step is to integrate a local registry into this process, to speed up the policy generation.Fixes CON-250