ROX-33548: Avoid unnecessary image scan requests in k8s event webhook…#19480
ROX-33548: Avoid unnecessary image scan requests in k8s event webhook…#19480clickboo wants to merge 3 commits intorelease-4.10from
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
DetectForDeploymentAndKubeEventcalls usecontext.Background(); consider threading through the existing admission request / timeout context so these detections are also bounded by the configured webhook timeout. - In
shouldBypassRuntimeDetection, you only check for a nil detector; if the policy set is empty you can short‑circuit early (similar to theslowPathDetectorempty check) to avoid unnecessary work and log noise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `DetectForDeploymentAndKubeEvent` calls use `context.Background()`; consider threading through the existing admission request / timeout context so these detections are also bounded by the configured webhook timeout.
- In `shouldBypassRuntimeDetection`, you only check for a nil detector; if the policy set is empty you can short‑circuit early (similar to the `slowPathDetector` empty check) to avoid unnecessary work and log noise.
## Individual Comments
### Comment 1
<location path="sensor/admission-control/manager/evaluate_runtime.go" line_range="107-111" />
<code_context>
log.Debugf("Found deployment %s (id=%s) for %s/%s", deployment.GetName(), deployment.GetId(),
event.GetObject().GetNamespace(), event.GetObject().GetName())
+ // Fast path: skip image fetches when no k8s event policies require deploy-time fields.
+ // Note: This webhook handles user-initiated commands (exec, port-forward) and
+ // lacks the requirement for burst resilience at scale, so this optimization is intentionally kept simple
+ if len(s.slowPathDetector.PolicySet().GetCompiledPolicies()) == 0 {
+ alerts, err := s.fastPathDetector.DetectForDeploymentAndKubeEvent(context.Background(),
+ booleanpolicy.EnhancedDeployment{
+ Deployment: deployment,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider threading a request-scoped context into runtime detection instead of using context.Background.
Using `context.Background()` here (and in the enriched path) means detection will ignore admission timeouts and caller cancellations. Please thread a request-scoped context into these calls (or reuse `fetchImgCtx` where available) so detection respects admission lifetimes and configured timeouts.
Suggested implementation:
```golang
log.Debugf("Found deployment %s (id=%s) for %s/%s", deployment.GetName(), deployment.GetId(),
event.GetObject().GetNamespace(), event.GetObject().GetName())
// Derive a request-scoped context for runtime detection so it respects admission timeouts.
// This is shared by both the fast and slow paths below.
var fetchImgCtx context.Context
if timeoutSecs := s.GetClusterConfig().GetAdmissionControllerConfig().GetTimeoutSeconds(); timeoutSecs > 1 {
// Bound detection to the remaining admission lifetime.
var cancel context.CancelFunc
fetchImgCtx, cancel = context.WithTimeout(context.Background(), time.Duration(timeoutSecs)*time.Second)
defer cancel()
} else {
// Fallback when no explicit timeout is configured.
fetchImgCtx = context.Background()
}
// Fast path: skip image fetches when no k8s event policies require deploy-time fields.
// Note: This webhook handles user-initiated commands (exec, port-forward) and
// lacks the requirement for burst resilience at scale, so this optimization is intentionally kept simple
if len(s.slowPathDetector.PolicySet().GetCompiledPolicies()) == 0 {
alerts, err := s.fastPathDetector.DetectForDeploymentAndKubeEvent(fetchImgCtx,
```
1. Ensure the file imports `time` if it is not already imported:
- Add `time` to the existing import block: `import ("context" ... "time")`.
2. If the existing (now-removed) `fetchImgCtx` initialization block contained additional logic (e.g., deriving from a parent/request context instead of `context.Background()`), that logic should be merged into the new initialization above to preserve behavior while still threading a request-scoped context into detection.
3. If a true request-scoped context is available higher in the call stack (e.g., the function signature includes `ctx context.Context` or there is a per-request context in the admission handler), prefer using that as the parent context instead of `context.Background()` when calling `context.WithTimeout`.
4. In the “slow path” later in this function/file, ensure any other calls to runtime detection or image fetching that previously used a different context are updated to use `fetchImgCtx` so they share the same admission-scoped lifetime.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
/konflux-retest main-on-push |
|
/konflux-retest operator-bundle-on-push |
|
/konflux-retest scanner-v4-db-on-push |
|
/konflux-retest main-on-push |
1 similar comment
|
/konflux-retest main-on-push |
|
Images are ready for the commit at 1aa95cb. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-4.10 #19480 +/- ##
================================================
- Coverage 49.38% 49.37% -0.02%
================================================
Files 2660 2660
Lines 200669 200684 +15
================================================
- Hits 99103 99088 -15
- Misses 94125 94153 +28
- Partials 7441 7443 +2
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:
|
|
/konflux-retest operator-bundle-on-push |
1 similar comment
|
/konflux-retest operator-bundle-on-push |
|
@clickboo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
… (#19377)
(cherry picked from commit d37b611)
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!