ROX-33326: Split fast and slow path detection in policy eval webhook#19416
ROX-33326: Split fast and slow path detection in policy eval webhook#19416clickboo wants to merge 2 commits intoboo-image-enrichment-policy-fieldsfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
evaluateAdmissionRequest, consider short-circuiting the new fast-path evaluation (and skippingtoPlaceholderImages) whenfastPathDeployDetector.PolicySet().GetCompiledPolicies()is empty to avoid unnecessary per-request work. - The new
UnevaluatedPolicyCountmessage is driven by a baremap[string]interface{}inmessage; consider introducing a small typed struct for the template data so future changes remain type-safe and easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `evaluateAdmissionRequest`, consider short-circuiting the new fast-path evaluation (and skipping `toPlaceholderImages`) when `fastPathDeployDetector.PolicySet().GetCompiledPolicies()` is empty to avoid unnecessary per-request work.
- The new `UnevaluatedPolicyCount` message is driven by a bare `map[string]interface{}` in `message`; consider introducing a small typed struct for the template data so future changes remain type-safe and easier to reason about.
## Individual Comments
### Comment 1
<location path="sensor/admission-control/manager/responses.go" line_range="33-35" />
<code_context>
+{{ end -}}
+{{- if gt .UnevaluatedPolicyCount 0}}
+{{.UnevaluatedPolicyCount}} additional {{if eq .UnevaluatedPolicyCount 1}}policy depends{{else}}policies depend{{end}} on image enrichment results and will be evaluated only after the above violations are addressed.
{{ end -}}
{{- if .BypassAnnotationKey}}
</code_context>
<issue_to_address>
**suggestion:** Unevaluated policy count may overstate how many policies actually apply to the request
`UnevaluatedPolicyCount` is derived from the total policies in `slowPathDeployDetector` (`len(s.slowPathDeployDetector.PolicySet().GetCompiledPolicies())`), so the message can suggest that N additional policies depend on enrichment even if many would never match this deployment. This may confuse users about what’s actually blocked. Consider either narrowing this count to policies that could plausibly match the current deployment (e.g., via a cheap prefilter on non-enriched fields) or tweaking the text to emphasize that it’s the number of *configured* enrichment-dependent policies, not necessarily those applicable here.
```suggestion
{{- if gt .UnevaluatedPolicyCount 0}}
{{.UnevaluatedPolicyCount}} additional configured {{if eq .UnevaluatedPolicyCount 1}}policy depends{{else}}policies depend{{end}} on image enrichment results and may apply to this deployment. These enrichment-dependent policies will be evaluated only after the above violations are addressed and image enrichment has completed.
{{ end -}}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 3ef0a5d. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## boo-image-enrichment-policy-fields #19416 +/- ##
======================================================================
- Coverage 49.70% 49.66% -0.04%
======================================================================
Files 2702 2701 -1
Lines 203518 203413 -105
======================================================================
- Hits 101160 101030 -130
- Misses 94833 94855 +22
- Partials 7525 7528 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.