Skip to content

ROX-33548: Avoid unnecessary image scan requests in k8s event webhook…#19480

Open
clickboo wants to merge 3 commits intorelease-4.10from
backport-19377-to-release-4.10
Open

ROX-33548: Avoid unnecessary image scan requests in k8s event webhook…#19480
clickboo wants to merge 3 commits intorelease-4.10from
backport-19377-to-release-4.10

Conversation

@clickboo
Copy link
Contributor

… (#19377)

(cherry picked from commit d37b611)

Description

change me!

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

change me!

@clickboo clickboo requested a review from a team as a code owner March 18, 2026 08:56
@github-actions github-actions bot added area/sensor area/admission-controller backport PR to backport changes from master to release branch ai-review labels Mar 18, 2026
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:

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

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.

@github-actions
Copy link
Contributor

/konflux-retest main-on-push

@github-actions
Copy link
Contributor

/konflux-retest operator-bundle-on-push

@github-actions
Copy link
Contributor

/konflux-retest scanner-v4-db-on-push

@github-actions
Copy link
Contributor

/konflux-retest main-on-push

1 similar comment
@github-actions
Copy link
Contributor

/konflux-retest main-on-push

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Mar 18, 2026

Images are ready for the commit at 1aa95cb.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.10.0-3-g1aa95cb0f2.

@codecov
Copy link

codecov bot commented Mar 18, 2026

Codecov Report

❌ Patch coverage is 0% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.37%. Comparing base (7b817fa) to head (1aa95cb).

Files with missing lines Patch % Lines
...nsor/admission-control/manager/evaluate_runtime.go 0.00% 21 Missing ⚠️
sensor/admission-control/manager/manager_impl.go 0.00% 15 Missing ⚠️
sensor/admission-control/manager/responses.go 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
go-unit-tests 49.37% <0.00%> (-0.02%) ⬇️

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.

@github-actions
Copy link
Contributor

/konflux-retest operator-bundle-on-push

1 similar comment
@github-actions
Copy link
Contributor

/konflux-retest operator-bundle-on-push

@openshift-ci
Copy link

openshift-ci bot commented Mar 18, 2026

@clickboo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/gke-nongroovy-compatibility-tests 1aa95cb link false /test gke-nongroovy-compatibility-tests

Full PR test history. Your PR dashboard.

Details

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

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

Labels

ai-review area/admission-controller area/sensor backport PR to backport changes from master to release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants