Skip to content

ROX-33326: Split fast and slow path detection in policy eval webhook#19416

Draft
clickboo wants to merge 2 commits intoboo-image-enrichment-policy-fieldsfrom
boo-adm-ctrl-fast-detection
Draft

ROX-33326: Split fast and slow path detection in policy eval webhook#19416
clickboo wants to merge 2 commits intoboo-image-enrichment-policy-fieldsfrom
boo-adm-ctrl-fast-detection

Conversation

@clickboo
Copy link
Contributor

@clickboo clickboo commented Mar 13, 2026

No description provided.

@clickboo
Copy link
Contributor Author

@openshift-ci
Copy link

openshift-ci bot commented Mar 13, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@stackrox stackrox deleted a comment from sourcery-ai bot Mar 13, 2026
@rhacs-bot
Copy link
Contributor

Images are ready for the commit at 3ef0a5d.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-315-g3ef0a5d43b.

@codecov
Copy link

codecov bot commented Mar 13, 2026

Codecov Report

❌ Patch coverage is 0% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.66%. Comparing base (1b8beb5) to head (3ef0a5d).

Files with missing lines Patch % Lines
...r/admission-control/manager/evaluate_deploytime.go 0.00% 27 Missing ⚠️
sensor/admission-control/manager/manager_impl.go 0.00% 18 Missing ⚠️
...nsor/admission-control/manager/evaluate_runtime.go 0.00% 6 Missing ⚠️
sensor/admission-control/manager/responses.go 0.00% 3 Missing ⚠️
pkg/detection/policy_set_impl.go 0.00% 2 Missing ⚠️
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     
Flag Coverage Δ
go-unit-tests 49.66% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants