fix(remediation): honour selector.namespace when resolving findings-driven targets - #407
Conversation
…riven targets resolveSelectorTargets always listed WorkloadConfigurationScanSummaries across every namespace and never read selector.Namespace, even though the field is part of the operatorAction contract and documented as scoping a selector to a single namespace. A caller asking to remediate one namespace therefore got every matching workload in the cluster, with nothing in the output to indicate the wider scope. Scope the listing server-side, and drop any summary whose label-derived target namespace falls outside the requested one, since that is the namespace the remediator actually acts on. An empty Namespace keeps the existing cluster-wide behaviour. Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughFindings-driven selector resolution now scopes summary queries and resolved targets to the selector namespace. An empty namespace remains cluster-wide. Tests cover control, severity, empty-result, cluster-wide, and mismatched-label cases. ChangesFindings namespace scoping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change narrows findings-driven remediation to the requested namespace while preserving cluster-wide behavior for empty selectors, with no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the diff, the resolver logic, and the new tests.
- Listing is now scoped server-side via
WorkloadConfigurationScanSummaries(selector.Namespace), and an empty namespace correctly falls back to cluster-wide, matching the pre-existing behavior for unscoped selectors. - The added defense-in-depth check (
target.Namespace != selector.Namespace) is sound: it guards against a summary's label-derived namespace disagreeing with the object's own namespace, which is the namespace the remediator actually acts on. - The five new tests in
mainhandler/findings_test.gocover the namespaced case, the cluster-wide/empty-namespace case, namespace scoping combined withMinSeverity, a namespace with no matches, and the mislabelled-summary defense-in-depth case. Good regression coverage, and the description notes the scoping tests were confirmed to fail onmainwithout the fix. - All CI checks (build, tests, CodeQL, GitGuardian, DCO, FOSSA) are green. CodeRabbit's automated review found no actionable comments — only a generic "docstring coverage" heuristic warning, which isn't a real blocker for this package's conventions.
No blockers. This is a correct, well-tested, appropriately-scoped fix. Approving.
The problem
OperatorActionSelector.Namespaceis part of theoperatorActioncontract and is documented as limiting a findings-driven selector to a single namespace:resolveSelectorTargetsnever read it. It always listed cluster-wide:So a request to quarantine workloads failing C-0016 in
paymentswould resolve to every workload failing C-0016 anywhere in the cluster, and nothing in the response or the events would indicate the scope was wider than asked for. For an action set whose whole point is to mutate running workloads, silently ignoring the field that bounds the blast radius is the wrong failure mode.The fix
Two parts, because there are two different namespaces in play.
The listing is now scoped server-side to
selector.Namespace, which is both correct and cheaper than filtering client-side. An emptyNamespacestill lists across all namespaces, so cluster-wide selectors behave exactly as before.The resolver also drops any summary whose label-derived target namespace falls outside the requested one. That namespace comes from the
kubescape.io/workload-namespacelabel and is what the remediator actually acts on, so if it ever disagreed with the namespace the summary object is stored in, the server-side scoping alone would not be enough. This is defence in depth rather than a bug I observed in practice, but the cost is one comparison and the downside of getting it wrong is mutating a workload in a namespace the caller did not name.Context
Part of the phased
operatorActionremediation framework:operatorActioncommand contract for CLI-driven cluster operations armosec/armoapi-go#655operatorAction(resolveselectorfrom stored scan results) #391Found while wiring the CLI side of findings-driven targeting in kubescape/kubescape#3474. That PR adds
--controland--min-severity, and deliberately rejects--target-namespacecombined with a selector precisely because of this bug, since offering the flag while the operator ignores it would be worse than not offering it at all.Once this merges and ships in a release, the CLI guard can be lifted and
--target-namespacepassed through. The guard needs to outlive this fix by a release, because a CLI that sendsselector.namespaceto an older operator would hit exactly the silent widening described above.Testing
Five new tests in
mainhandler/findings_test.go:I confirmed the three scoping tests fail against
mainwithout the change and pass with it, so they are genuine regression tests rather than tests written to match current behaviour. Fullmainhandlerandmainhandler/remediatorssuites pass.Summary by CodeRabbit