Repository navigation
fix(admission): take the cluster-object opt-out from the binding, not the object - #494
Conversation
… the object ListRulesForObject decided whether admission rules apply to a cluster-scoped object from the includeClusterObjects label on the object being admitted, so whoever created or updated a ClusterRoleBinding, ClusterRole, webhook configuration or Namespace could switch off its evaluation by adding that label. Read it from the RuntimeRuleAlertBinding instead (a binding labelled includeClusterObjects: "false" applies to namespaced objects only), and, when rule bindings are ignored, from a new operator setting rulesUpdateConfig.excludeClusterObjects (default false, today's behaviour). The admitted object's label no longer has any effect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: itsvishalyadav <vishalyadav87695@gmail.com>
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
matthyx
left a comment
There was a problem hiding this comment.
Reviewed a4a26e37635535eb8437d50834b73b1a995e9be8 against main at a9fc09a55f434dbdf2f6b56e86a60952705a1a00.
The issue in #493 is real: the old code let the object under admission suppress its own cluster-scoped evaluation. This change moves the decision to the binding label or operator configuration, keeps the default behavior, and adds regression tests for both paths. I found no code-level blocker. Related PRs #346 (ignore-bindings mode) and #402 (namespace selector matching) were merged; neither rejected this approach. Broader closed-PR/issue searches were limited by GitHub search API rate limiting; a title scan found no superseding fix.
CI passed for Basic-Test (including admission/rulebinding/cache), go test -tags=integration ./..., cross-platform build, CodeQL, DCO, GitGuardian, and Dependency Quality. License Compliance is failing with FOSSA's “1 issues found.” The same failure appears on merged PRs #492 and #346, but the FOSSA report does not expose the finding details here, so I cannot verify it is an unrelated infrastructure failure. I did not run tests locally because this shared workspace is not an isolated environment for executing untrusted PR code.
Request changes for the failing CI check only; no source-code blocker found. Please get License Compliance green or have the failure verified as unrelated before approval. A Helm follow-up was opened: kubescape/helm-charts#1023 (the chart does not yet expose the new setting).
|
Thanks for the review. On License Compliance: this PR doesn't touch |
No operator release has kubescape/operator#494 yet: v0.2.220 and earlier, including the chart's default image tag, ignore the setting. Say so in values.yaml and the README, and point to operator.image.tag until the default moves to a release that includes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: itsvishalyadav <vishalyadav87695@gmail.com>
Overview
This PR fixes #493
Signed Commits
ListRulesForObjectno longer readsincludeClusterObjectsfrom the object being admitted. The opt-out now comes from configuration:includeClusterObjects: "false"applies to namespaced objects only; without the label it applies to cluster-scoped objects too, as today. The helper isbindingIncludesClusterObjects(rb)inhelpers.go. The binding is cluster-scoped, so only whoever can manage bindings can set it.rulesUpdateConfig.enabled): a new operator setting decides,rulesUpdateConfig.excludeClusterObjects(RulesUpdaterConfig.ExcludeClusterObjects). It defaults tofalse, today's default behaviour.NewCachetakes it as a fourth argument, wired frommain.go.docs/features/cel-admission-rules.mdgets a "Cluster-Scoped Objects" section.Design notes:
exclude…defaulting to false: the zero value keeps today's behaviour, so a struct literal can't silently switch evaluation off.kubescape/helm-chartswould needexcludeClusterObjectsintemplates/operator/configmap.yaml. I can send that once you're happy with the shape here.Questions where I'd value your preference:
specfield instead;rulesUpdateConfigor should be admission-wide;How to Test
TestListRulesForObjectIgnoreBindings(rewritten) covers:NewCache, so the wiring is covered too).New
TestListRulesForClusterObjectcovers:truekeeps cluster objects;I ran the package tests for Linux on WSL: with the fix all pass; with
main'scache.gothe new tests fail, including both "object label doesn't opt out" cases.GOOS=linux go vetis clean for the touched packages, andgofmtis clean on the touched files.Checklist before requesting a review
🤖 Generated with Claude Code
Summary by CodeRabbit