fix: look up ContainerProfile by with-container slug, not workload slug

[matthyx]

Overview

This PR fixes #395

Signed Commits

• Yes, I signed my commits.

ContainerProfile objects are named using the with-container slug (InstanceID.GetSlug(false)), but HandleImageScanningScopedRequest and handlePodWatcher looked them up using the no-container slug (GetSlug(true), the ApplicationProfile naming convention). The lookup therefore always missed, and every scheduled scan (the kubevuln-scheduler CronJob) silently fell back to a plain image scan instead of a relevancy (container-profile) scan.

Three fixes, all in this PR:

1. mainhandler/handlerequests.go — HandleImageScanningScopedRequest now passes the with-container slug (already computed as s) into GetContainerProfileForRelevancyScan, and dedups on slugs[s] instead of a separate no-container-slug map.
2. watcher/podwatcher.go — handlePodWatcher had the identical bug pattern at two call sites; same fix, using the outer loop's slug (already with-container).
3. utils/containerprofile.go — a second, previously-unreported bug found while verifying this fix: GetContainerProfileScanCommand built its command with CommandName: apis.TypeScanApplicationProfile, which has no case in runCommand's dispatch switch (it only recognizes utils.CommandScanContainerProfile) — so even a correctly-found profile's scan command would have been silently dropped. Fixed to use the correct constant.

Both call sites also previously deduped container-profile scans per workload instead of per container (via a no-container-slug map), which — once the slug itself was fixed naively — would have dropped every container after the first in multi-container workloads. That dedup logic has been removed; the existing per-container slug dedup (slugs[s] / the outer per-container loop key) already provides correct dedup.

How to Test

New regression tests were added and verified by mutation (fail against the pre-fix code, pass against the fix — confirmed by temporarily reverting each production change and rerunning):

• watcher/podwatcher_test.go: Test_handlePodWatcher_ContainerProfile — single-container hit case, and a multi-container case asserting one scan command per container (regression test for the removed per-workload dedup).
• mainhandler/handlerequests_test.go: TestHandleImageScanningScopedRequest_LooksUpWithContainerSlug and TestHandleImageScanningScopedRequest_MultiContainerDoesNotDedupeAcrossContainers. These enable Kubevuln against an httptest.Server stub so a correctly-dispatched scan completes end-to-end through actionHandler.scanContainerProfile, and assert on the stub's received request count — this is what actually distinguishes a correct fix from one that finds the profile but silently drops the resulting command (see fix Trigger ks #3) or dedups across containers (see the removed per-workload dedup).

go build ./..., go vet ./..., and go test -count=1 ./mainhandler/... ./watcher/... ./utils/... (and the rest of the repo's packages) pass. Note: watcher/registryhandler_test.go's TestRegistryCommandWatch is a testcontainers-based integration test that is independently flaky and not exercised by this PR's branch (it's untracked locally); it is unrelated to this change and fails the same way on main.

Additional Information

Out of scope for this PR: the issue also floats a larger redesign (replace per-container Get() calls with a single List matched on the kubescape.io/instance-id annotation, closing a remaining slug-truncation edge case) as a "worth considering instead" follow-up, not a requirement for this fix.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

AI-skills: oh-my-claudecode:team | cmds: /oh-my-claudecode:ralplan

Summary by CodeRabbit

• Bug Fixes
  • Improved image scanning to correctly track and process individual containers within the same pod.
  • Fixed container profile lookups so scans use the appropriate per-container profile.
  • Prevented valid scans from being skipped when multiple containers share a workload.
  • Ensured container-profile scan commands are correctly identified and processed.
• Tests
  • Added coverage for single-container and multi-container scanning scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

Container-profile lookups now use container-specific slugs in scheduled and pod-watcher scans. Deduplication remains per container, generated commands identify container-profile scans, and regression tests cover single- and multi-container workloads.

Changes

Container profile scanning

Layer / File(s)	Summary
Scheduled scan lookup and command identity mainhandler/handlerequests.go, mainhandler/handlerequests_test.go, utils/containerprofile.go	Scheduled scans use the container slug for profile lookup, mark both profile and image paths as scanned, and generate container-profile commands. Tests cover per-container lookup and multi-container deduplication.
Pod watcher profile selection watcher/podwatcher.go, watcher/podwatcher_test.go	Pod watcher relevancy checks use the container slug and emit scans for each matching container profile without the removed no-container deduplication. Regression tests cover single- and multi-container profiles.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• kubescape/operator#362: Modifies the same per-request container-versus-no-container slug deduplication behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The #395 requirements are met: both scan paths use the with-container slug, deduplication is removed, and regressions cover per-container scans.
Out of Scope Changes check	✅ Passed	All file changes support the #395 fix, and no unrelated behavior or features appear to have been added.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly states the main fix: ContainerProfile lookups now use the with-container slug instead of the workload slug.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/395-containerprofile-slug-lookup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

mainhandler/handlerequests_test.go (1)

182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace deprecated k8sfake.NewSimpleClientset in new test code.

Static analysis flags both new call sites as using a deprecated constructor (NewClientset is the replacement). These are newly-added lines, so unlike other pre-existing usages elsewhere in the repo, this could newly trip a lint gate for this PR.

♻️ Proposed fix

-	k8sClient := k8sfake.NewSimpleClientset(pod)
+	k8sClient := k8sfake.NewClientset(pod)

Apply the same substitution at line 210 (k8sfake.NewSimpleClientset(pod)).

Also applies to: 210-210

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mainhandler/handlerequests_test.go` at line 182, Replace both newly added
k8sfake.NewSimpleClientset calls in the test with the non-deprecated
k8sfake.NewClientset constructor, preserving the existing pod argument and test
behavior.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@mainhandler/handlerequests_test.go`:
- Line 182: Replace both newly added k8sfake.NewSimpleClientset calls in the
test with the non-deprecated k8sfake.NewClientset constructor, preserving the
existing pod argument and test behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3035a1c5-f33c-490e-b292-0022d2dd3cf8

📥 Commits

Reviewing files that changed from the base of the PR and between 54067a8 and 329bc40.

📒 Files selected for processing (5)

• mainhandler/handlerequests.go
• mainhandler/handlerequests_test.go
• utils/containerprofile.go
• watcher/podwatcher.go
• watcher/podwatcher_test.go

[github-actions]

Summary:

• License scan: success
• Credentials scan: failure
• Vulnerabilities scan: failure
• Unit test: success
• Go linting: failure

[matthyx]

The production fix is correct

All three changes verified:

1. Slug fix — mapSlugToInstanceID (podwatcher.go:230) and the slugsToImages map both key on GetSlug(false), so passing slug / s is right, and it matches the actual ContainerProfile naming.
2. CommandName fix — real bug, and the more severe of the two. Confirmed by printing the constants: apis.TypeScanApplicationProfile == "scanApplicationProfile" while utils.CommandScanContainerProfile == "scanContainerProfile", and runCommand's switch only cases the latter — so the command fell through to default: and was dropped. Note that scanContainerProfile still hardcodes apis.TypeScanApplicationProfile at vulnscan.go:303 for scanner-URL selection; that is correct (it is the kubevuln endpoint path) and rightly left untouched.
3. Dedup removal — safe. The SlugToImageID.Set / WlidAndImageID.Add calls that lived inside the deleted skip-branches are duplicated in the fall-through tail, so no cache state is lost.

I checked the fix direction by mutation rather than by reading: reverting the slug fix makes both new mainhandler tests fail, and reverting the CommandName fix makes the new podwatcher test fail.

Two test-quality problems, both proven by mutation

1. TestHandleImageScanningScopedRequest_MultiContainerDoesNotDedupeAcrossContainers does not test what it claims

I restored the per-workload dedup exactly as it was before this PR (re-adding the noContainerSlug lookup/continue and slugs[noContainerSlug] = true) while keeping the corrected slug in the profile lookup — and the test still passed.

Cause: newMainHandlerForTest disables Kubevuln, so HandleSingleRequest returns "kubevuln is not enabled" and the continue at handlerequests.go:444 fires before slugs[...] = true at :448. The dedup marker is therefore never set, and the dedup can never trigger inside this test. The test log confirms all three containers take the error path:

[info]  triggering container profile scan. name: pod-my-pod-nginx-d00d-dd12
[error] failed to complete action. error: kubevuln is not enabled; ...
[info]  triggering container profile scan. name: pod-my-pod-sidecar-7ab1-e32f
[error] failed to complete action. error: kubevuln is not enabled; ...
[info]  triggering container profile scan. name: pod-my-pod-init-66cb-5b76
[error] failed to complete action. error: kubevuln is not enabled; ...

As written it is effectively a duplicate of TestHandleImageScanningScopedRequest_LooksUpWithContainerSlug — it covers the slug, not the dedup.

2. Neither mainhandler test covers fix #3

Reverting only the utils/containerprofile.go change (CommandScanContainerProfile → apis.TypeScanApplicationProfile), leaving both slug fixes in place, leaves go test ./mainhandler/... green. Only the podwatcher test catches it — which it does, correctly.

So the scoped-request path, which is the one #395 is actually about, has no assertion that it emits a CommandScanContainerProfile.

Suggested fix for both

Both findings stem from the same choice: the mainhandler tests assert on recorded fake-client get containerprofiles actions rather than on the emitted command. Enabling Kubevuln in the test config and pointing KubevulnURL at an httptest.NewServer addresses both at once — HandleSingleRequest then succeeds, so the dedup marker actually gets set (making finding 1 detectable) and dispatch has to survive runCommand's switch (making finding 2 detectable). The new podwatcher tests already assert on emitted commands and are the better model here.

Minor

The description states that go test -count=1 ./... passes across the full repo. TestRegistryCommandWatch fails under ./watcher/.... I checked out main and it fails there too, so it is pre-existing and not caused by this PR — but the claim should be corrected so the failure is not mistaken for a regression from these changes.

[matthyx]

Thanks both for the review — pushed 70f45ca addressing this.

CodeRabbit nitpick: replaced the deprecated k8sfake.NewSimpleClientset with k8sfake.NewClientset in the two new call sites in mainhandler/handlerequests_test.go.

@matthyx — both test-quality findings were real, fixed by the same change: newMainHandlerForTest now enables Kubevuln and points KubevulnURL at an httptest.Server stub, so a correctly-dispatched scan completes end-to-end through actionHandler.scanContainerProfile instead of short-circuiting on "kubevuln is not enabled" before slugs[s] = true is ever reached. Both tests now additionally assert on the stub's received request count.

Verified by mutation, the same way you found it:

• Reverting the CommandName fix in utils/containerprofile.go → both tests now fail (0 requests reach the stub instead of 1/3).
• Reintroducing the old per-workload dedup (keyed on the no-container slug) in handlerequests.go → the multi-container test now fails (1 request instead of 3).

Also: you're right that the PR description's "go test -count=1 ./... passes across the full repo" claim was made without watcher/registryhandler_test.go in the picture — that file isn't part of this PR (it's untracked locally), and TestRegistryCommandWatch is a testcontainers-based integration test that's independently flaky/pre-existing on main, unrelated to this change. Updating the PR description to not overstate that.

[matthyx]

Re-reviewed at 70f45ca. Production code is unchanged from the previous round; this commit is test-only. Both earlier findings are addressed, and I verified the mutation claims independently rather than taking the commit message's word for them:

• Revert the CommandName fix → TestHandleImageScanningScopedRequest_LooksUpWithContainerSlug fails (expected: 1, actual: 0) and ..._MultiContainerDoesNotDedupeAcrossContainers fails (expected: 3, actual: 0). Previously both stayed green.
• Reintroduce the per-workload dedup → the multi-container test fails (expected: 3, actual: 1). Previously it stayed green.

Also confirmed: go vet clean on the touched packages, ./mainhandler/... and ./utils/... pass, and the two new tests pass under -race (the stub's counter is mutex-guarded and the handler goroutines are the only writers). TestRegistryCommandWatch still fails under ./watcher/..., unchanged and pre-existing on main.

Two minor points, neither blocking:

1. VulnScanHttpClient is mutated globally without restore

newMainHandlerForTest assigns the package-level VulnScanHttpClient (handlerequests.go:62) and never restores it, while the httptest.Server it points at is closed by t.Cleanup. Nothing else in the package currently reaches that global, so this is harmless today — but it is a latent trap. A future mainhandler test that reaches sendWorkloadWithCredentials after these two run would POST to a closed port and hit the connection-refused retry loop at vulnscan.go:448: 5 retries × a 5s sleep, i.e. a ~25s hang that would present as a mysteriously slow test rather than a clear failure.

Cheap guard:

prev := VulnScanHttpClient
t.Cleanup(func() { VulnScanHttpClient = prev })
VulnScanHttpClient = utils.InitHttpClient(clusterConfig.KubevulnURL)

(which requires threading t into the helper).

2. The scanner stub counts requests but does not discriminate by path

The stub accepts every path, so the request count proves a scan dispatched, not that it was a container-profile scan. Verified by mutation: reverting only the slug fix leaves both count assertions satisfied (1 and 3), because the fallback image-scan path POSTs to the same stub — only the getContainerProfileActionNames assertion catches that regression.

The two assertions together are adequate, so coverage is not actually missing. But asserting the path in the stub handler (/v1/scanApplicationProfile for the container-profile route, per getAPScanURL, vs. the image-scan route from getVulnScanURL) would make the count assertion mean what its comment claims — "the container-profile scan actually dispatched" — instead of "some scan dispatched".

LGTM otherwise.

[github-actions]

Summary:

• License scan: success
• Credentials scan: failure
• Vulnerabilities scan: failure
• Unit test: success
• Go linting: failure