fix: inject namespace into review data when auditing from cache #2335
Conversation
Signed-off-by: davis-haba <davishaba@google.com>
davis-haba
force-pushed
the
issue-2307
branch
from
ee9af4b
to
9cd45dd
Compare
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
| } | ||
|
|
||
| var ns corev1.Namespace | ||
| err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &ns) |
There was a problem hiding this comment.
Thanks for the PR!
One question: since the original issue mentioned they are syncing namespaces, shouldn’t that data already exist in the OPA cache such that we don’t need to ask the api server for them? We also mention the need to sync namespace here: https://open-policy-agent.github.io/gatekeeper/website/docs//gatekeeper/website/docs/sync/
There was a problem hiding this comment.
I might be confused about what cache you are referring to by "OPA Cache", but the namespaces being processed here are from the audit cache. The input to this func is generated from auditCache.ListObjects.
It should not necessary to reach out the the API server for these namespaces. If they are not caching namespaces, then no namespace metadata will be attached to the review.
| au := &target.AugmentedUnstructured{ | ||
| Object: obj, | ||
| Namespace: ns, | ||
| } | ||
| resp, err := am.opa.Review(ctx, au) |
There was a problem hiding this comment.
Feel free to ignore, but I am curious how/ if we'd wanna test this.
Putting on a TDD hat 🧢 , it'd be 💯 to add some form of test to A) catch any regressions on this issue and B) programmatically prove the issue exists and this patch solves it.
I was poking around to find a good place to unit test this but I am still getting familiar w the code here 🤷🏼 ; so I can't offer more specific guidance 😞
There was a problem hiding this comment.
Easiest would be to add an e2e test for this use case, but that wouldn't be trivial, as it would involve installing G8r with a second set of flags (doable, but more work than just adding an e2e test case).
Another option would be to mock the various k8s clients and use dependency injection to create unit tests. This is preferable from a test runtime perspective, but would involve significant mocking.
A third option would be to use a local instance of an API server, similar to how controller tests work
TBH the paucity of audit tests is an ongoing issue (currently e2e tested), and I'd prefer to not block a bugfix on what would probably be significant dev. time.
Codecov ReportBase: 53.44% // Head: 53.42% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2335 +/- ##
==========================================
- Coverage 53.44% 53.42% -0.02%
==========================================
Files 116 116
Lines 10174 10198 +24
==========================================
+ Hits 5437 5448 +11
- Misses 4318 4333 +15
+ Partials 419 417 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…-policy-agent#2335) * inject ns into review when auditing from cache Signed-off-by: davis-haba <davishaba@google.com> * Make linter happy Signed-off-by: davis-haba <davishaba@google.com> * more linter Signed-off-by: davis-haba <davishaba@google.com> Signed-off-by: davis-haba <davishaba@google.com> Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com> Co-authored-by: Max Smythe <smythe@google.com> Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
…ing from cache (#2367) Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com> Co-authored-by: Max Smythe <smythe@google.com> Co-authored-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Currently, when we audit from cache, we only pass the review object into OPA, without any metadata. This makes it impossible to use namespace matchers when using audit from cache.
What this PR does / why we need it:
When auditing from cache, inject the namespace. Without this, namespace matchers would not work when auditing from cache.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #2307
Special notes for your reviewer: