🐛 fix(webhook): enforce cluster-scope when the profile is missing - #304
Merged
Merged
Conversation
konih
added a commit
that referenced
this pull request
Aug 17, 2026
The watch in the previous commit changes the failure mode of a missing `kollectclusterscopes` grant. Today the grant is missing everywhere and surfaces lazily: `LoadCluster` errors per reconcile. Once the controller registers a watch on the type, the informer starts with the manager, and a cache that cannot sync a watched type fails the controller's Start — `cmd/main.go` exits non-zero on `mgr.Start`, so the pod crash-loops. That is safe on this branch, where the grant and the watch land together, and safe for a normal `helm upgrade`. It is not safe for the skew this project's install model invites: RBAC is Helm-managed while operators pin `image.tag`, so a new image against an un-bumped chart now crash-loops rather than degrading one controller. Placed under "Upgrade the operator" next to the existing image-pinning guidance, away from the behaviour-changes section that the docs branch for #304 edits, so the two do not collide on rebase. Gates: lint:markdown 0 issues, scrub ok.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Keep namespace and profileRef checks on ClusterTarget admission even when the profile does not exist yet. Re-check allowedGVKs at reconcile once the profile loads. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
konih
force-pushed
the
fix/clustertarget-scope-gvk
branch
from
August 18, 2026 09:10
ac589ef to
8b0dbf0
Compare
konih
added a commit
that referenced
this pull request
Aug 18, 2026
…mpact PR #304 makes the KollectClusterTarget reconciler re-check KollectClusterScope `allowedGVKs` after the profile loads, and degrades with `ScopeGVKDenied`. That is a new user-visible condition reason on a kind whose reference page did not list it, and it is a behaviour change an operator can be surprised by: An existing, collecting ClusterTarget whose profile `targetGVK` sits outside a non-empty `allowedGVKs` now unregisters its informers and goes Degraded on the first reconcile after upgrade. Reaching that state needs no bug — admission only runs when the object is written, so every target admitted before the ceiling was created or tightened is in the affected set. - kollectclustertarget.md: `ScopeGVKDenied` and `ScopeNamespaceDenied` rows in the Degraded table (the latter was already reachable and undocumented), the two admission denials the fix adds, and the post-upgrade symptom. - kollectclusterscope.md: an Enforcement section stating which stage checks what, so the reconcile backstop ADR-0207 always specified is visible on the ceiling's own page. Includes the ClusterInventory sink reasons for completeness. - upgrading.md: a pre-upgrade audit and post-upgrade triage for the affected set, under the existing behaviour-changes section. - troubleshooting.md: the two scope reasons applied to Target *and* ClusterTarget; they named only the namespaced kind and KollectScope. Docs-only; no version invented for the note, which says "after v0.18.0". Gates: lint:markdown 0 issues (148 files), scrub ok. Requires #304 to merge first, or the reconcile column of the enforcement table describes code that is not on main yet.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
…mpact PR #304 makes the KollectClusterTarget reconciler re-check KollectClusterScope `allowedGVKs` after the profile loads, and degrades with `ScopeGVKDenied`. That is a new user-visible condition reason on a kind whose reference page did not list it, and it is a behaviour change an operator can be surprised by: An existing, collecting ClusterTarget whose profile `targetGVK` sits outside a non-empty `allowedGVKs` now unregisters its informers and goes Degraded on the first reconcile after upgrade. Reaching that state needs no bug — admission only runs when the object is written, so every target admitted before the ceiling was created or tightened is in the affected set. - kollectclustertarget.md: `ScopeGVKDenied` and `ScopeNamespaceDenied` rows in the Degraded table (the latter was already reachable and undocumented), the two admission denials the fix adds, and the post-upgrade symptom. - kollectclusterscope.md: an Enforcement section stating which stage checks what, so the reconcile backstop ADR-0207 always specified is visible on the ceiling's own page. Includes the ClusterInventory sink reasons for completeness. - upgrading.md: a pre-upgrade audit and post-upgrade triage for the affected set, under the existing behaviour-changes section. - troubleshooting.md: the two scope reasons applied to Target *and* ClusterTarget; they named only the namespaced kind and KollectScope. Docs-only; no version invented for the note, which says "after v0.18.0". Gates: lint:markdown 0 issues (148 files), scrub ok. Requires #304 to merge first, or the reconcile column of the enforcement table describes code that is not on main yet.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
…e writes The KollectClusterTarget controller watched KollectProfile and Namespace but not the ceiling itself, and the manager sets no SyncPeriod, so editing a KollectClusterScope changed nothing until an unrelated event or the 10h resync. Tightening `allowedGVKs` left offending targets collecting; widening it left correctly degraded targets stuck Degraded. With PR #304 adding the reconcile-time GVK check, that lag is now the difference between a ceiling that is enforced and one that is merely declared. The map function deliberately does not filter to the currently enforced scope: `scope.LoadCluster` resolves the ceiling as the lowest-named KollectClusterScope of all of them, so creating, renaming, or deleting any of them can change which object is enforced. Every cluster target is enqueued on any scope write. The fan-out is bounded by the number of cluster targets — a platform-level, cluster-scoped kind — and reconcile is idempotent. Scope of this commit is KollectClusterTarget only. KollectClusterInventory shares `enforceClusterScopePolicy` and has the identical lag on `sinkRefs`, and the namespaced KollectTarget/KollectScope pair has it too; both are left alone here rather than fixed silently, and neither regresses. Test first: mapClusterScopeToClusterTargets returns one namespace-free request per cluster target for a scope object that is not the enforced one, and nil for a non-scope object. Red as undefined before the change. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, controller unit tests green. envtest suites remain CI-only on this host.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
The watch in the previous commit changes the failure mode of a missing `kollectclusterscopes` grant. Today the grant is missing everywhere and surfaces lazily: `LoadCluster` errors per reconcile. Once the controller registers a watch on the type, the informer starts with the manager, and a cache that cannot sync a watched type fails the controller's Start — `cmd/main.go` exits non-zero on `mgr.Start`, so the pod crash-loops. That is safe on this branch, where the grant and the watch land together, and safe for a normal `helm upgrade`. It is not safe for the skew this project's install model invites: RBAC is Helm-managed while operators pin `image.tag`, so a new image against an un-bumped chart now crash-loops rather than degrading one controller. Placed under "Upgrade the operator" next to the existing image-pinning guidance, away from the behaviour-changes section that the docs branch for #304 edits, so the two do not collide on rebase. Gates: lint:markdown 0 issues, scrub ok.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
…e writes The KollectClusterTarget controller watched KollectProfile and Namespace but not the ceiling itself, and the manager sets no SyncPeriod, so editing a KollectClusterScope changed nothing until an unrelated event or the 10h resync. Tightening `allowedGVKs` left offending targets collecting; widening it left correctly degraded targets stuck Degraded. With PR #304 adding the reconcile-time GVK check, that lag is now the difference between a ceiling that is enforced and one that is merely declared. The map function deliberately does not filter to the currently enforced scope: `scope.LoadCluster` resolves the ceiling as the lowest-named KollectClusterScope of all of them, so creating, renaming, or deleting any of them can change which object is enforced. Every cluster target is enqueued on any scope write. The fan-out is bounded by the number of cluster targets — a platform-level, cluster-scoped kind — and reconcile is idempotent. Scope of this commit is KollectClusterTarget only. KollectClusterInventory shares `enforceClusterScopePolicy` and has the identical lag on `sinkRefs`, and the namespaced KollectTarget/KollectScope pair has it too; both are left alone here rather than fixed silently, and neither regresses. Test first: mapClusterScopeToClusterTargets returns one namespace-free request per cluster target for a scope object that is not the enforced one, and nil for a non-scope object. Red as undefined before the change. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, controller unit tests green. envtest suites remain CI-only on this host.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
The watch in the previous commit changes the failure mode of a missing `kollectclusterscopes` grant. Today the grant is missing everywhere and surfaces lazily: `LoadCluster` errors per reconcile. Once the controller registers a watch on the type, the informer starts with the manager, and a cache that cannot sync a watched type fails the controller's Start — `cmd/main.go` exits non-zero on `mgr.Start`, so the pod crash-loops. That is safe on this branch, where the grant and the watch land together, and safe for a normal `helm upgrade`. It is not safe for the skew this project's install model invites: RBAC is Helm-managed while operators pin `image.tag`, so a new image against an un-bumped chart now crash-loops rather than degrading one controller. Placed under "Upgrade the operator" next to the existing image-pinning guidance, away from the behaviour-changes section that the docs branch for #304 edits, so the two do not collide on rebase. Gates: lint:markdown 0 issues, scrub ok.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
`validateScope` returned nil as soon as the referenced KollectProfile was absent, so a KollectTarget applied before its profile skipped the namespace allow/deny checks and the resourceRules GVK check entirely. PR #304 fixes the same skip on the cluster-scoped kind; this is the namespaced half. Severity is lower here than on the cluster kind: `scopeCheck.enforceTarget` already re-checks GVKs and namespaces at reconcile, so a target admitted this way degrades instead of collecting. What was wrong is that admission accepted a spec it can fully evaluate — `deniedNamespaces`, `allowedNamespaces` and `spec.resourceRules[].gvk` do not depend on the profile at all. Only the profile `targetGVK` does, and `CollectRuleGVKs` ignores it whenever resourceRules is non-empty. The resolve now runs as a switch: found resolves the GVK, NotFound continues with `profileResolved = false`, any other error still fails closed. GVK validation is skipped only in the one case where it would compare against a zero GVK — profile missing *and* no resourceRules — which would otherwise reject a legal create-before-profile against a non-empty allowedGVKs. Test first: `TestKollectTargetValidator_scopeAdmissionMissingProfile` covers denied namespace, out-of-allowlist namespace, and out-of-scope resourceRules GVK with no profile object present, plus the in-scope case that must stay admissible. Red before the change on the denied-namespace assertion. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook unit tests green. envtest suites (TestWebhookEnvtest, TestSetupWithManager_*) do not run on this host — bin/k8s ships linux-amd64 assets only and setup-envtest is rate-limited — so they are CI-only, and they fail identically on an unmodified main checkout here.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
Follow-up to #304, which is the parent commit of this branch. #304 defers the whole GVK check when profileRef does not resolve, but only the profile targetGVK actually depends on the profile. `CollectRuleGVKs` returns the `spec.resourceRules[].gvk` list and ignores the profile GVK entirely whenever resourceRules is non-empty, so those GVKs are knowable at admission and were being waved through. Not an exploit path — a target cannot collect while its profile is missing, and reconcile now catches it once the profile appears — but admission should reject a spec it can fully evaluate. The NotFound branch also duplicated the happy path's two namespace checks. That is the divergence shape that caused the bug #304 fixes: a third check added to the tail later would silently not run for missing profiles. Resolve is now a switch that yields `profileGVK` plus `profileResolved`, and the whole tail runs once for both paths. GVK validation is skipped only when the profile is missing *and* resourceRules is empty, where the alternative is comparing a zero GVK against allowedGVKs and rejecting a legal create-before-profile. Tests: - `missingProfileSpecDerivedChecks` — denied resourceRules GVK and denied profileRef namespace, both with no profile object; red on the first before this change. It also covers the `ValidateClusterScopeStaticRefNamespace` call #304 added, which its own fixture could not exercise: allowedStaticRefNamespaces was unset there, so the call returned nil unconditionally. - `deniedRuleGVKDegrades` — reconcile side, permitted profile targetGVK plus a denied resourceRules GVK. Green before this commit; it locks the rules path through CollectRuleGVKs that #304's test did not reach. - ScopeCeiling's doc comment claimed "namespace and GVK checks" while the struct only carries namespaces, which points readers at the wrong enforcement point. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook and controller unit tests green. envtest suites are CI-only on this host; a GVK-denied spec in cluster_scope_enforce_envtest_test.go is left as follow-up rather than added unverified.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
`validateScope` returned nil as soon as the referenced KollectProfile was absent, so a KollectTarget applied before its profile skipped the namespace allow/deny checks and the resourceRules GVK check entirely. PR #304 fixes the same skip on the cluster-scoped kind; this is the namespaced half. Severity is lower here than on the cluster kind: `scopeCheck.enforceTarget` already re-checks GVKs and namespaces at reconcile, so a target admitted this way degrades instead of collecting. What was wrong is that admission accepted a spec it can fully evaluate — `deniedNamespaces`, `allowedNamespaces` and `spec.resourceRules[].gvk` do not depend on the profile at all. Only the profile `targetGVK` does, and `CollectRuleGVKs` ignores it whenever resourceRules is non-empty. The resolve now runs as a switch: found resolves the GVK, NotFound continues with `profileResolved = false`, any other error still fails closed. GVK validation is skipped only in the one case where it would compare against a zero GVK — profile missing *and* no resourceRules — which would otherwise reject a legal create-before-profile against a non-empty allowedGVKs. Test first: `TestKollectTargetValidator_scopeAdmissionMissingProfile` covers denied namespace, out-of-allowlist namespace, and out-of-scope resourceRules GVK with no profile object present, plus the in-scope case that must stay admissible. Red before the change on the denied-namespace assertion. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook unit tests green. envtest suites (TestWebhookEnvtest, TestSetupWithManager_*) do not run on this host — bin/k8s ships linux-amd64 assets only and setup-envtest is rate-limited — so they are CI-only, and they fail identically on an unmodified main checkout here.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
Follow-up to #304, which is the parent commit of this branch. #304 defers the whole GVK check when profileRef does not resolve, but only the profile targetGVK actually depends on the profile. `CollectRuleGVKs` returns the `spec.resourceRules[].gvk` list and ignores the profile GVK entirely whenever resourceRules is non-empty, so those GVKs are knowable at admission and were being waved through. Not an exploit path — a target cannot collect while its profile is missing, and reconcile now catches it once the profile appears — but admission should reject a spec it can fully evaluate. The NotFound branch also duplicated the happy path's two namespace checks. That is the divergence shape that caused the bug #304 fixes: a third check added to the tail later would silently not run for missing profiles. Resolve is now a switch that yields `profileGVK` plus `profileResolved`, and the whole tail runs once for both paths. GVK validation is skipped only when the profile is missing *and* resourceRules is empty, where the alternative is comparing a zero GVK against allowedGVKs and rejecting a legal create-before-profile. Tests: - `missingProfileSpecDerivedChecks` — denied resourceRules GVK and denied profileRef namespace, both with no profile object; red on the first before this change. It also covers the `ValidateClusterScopeStaticRefNamespace` call #304 added, which its own fixture could not exercise: allowedStaticRefNamespaces was unset there, so the call returned nil unconditionally. - `deniedRuleGVKDegrades` — reconcile side, permitted profile targetGVK plus a denied resourceRules GVK. Green before this commit; it locks the rules path through CollectRuleGVKs that #304's test did not reach. - ScopeCeiling's doc comment claimed "namespace and GVK checks" while the struct only carries namespaces, which points readers at the wrong enforcement point. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook and controller unit tests green. envtest suites are CI-only on this host; a GVK-denied spec in cluster_scope_enforce_envtest_test.go is left as follow-up rather than added unverified.
konih
added a commit
that referenced
this pull request
Aug 18, 2026
Follow-up to #304, which is the parent commit of this branch. #304 defers the whole GVK check when profileRef does not resolve, but only the profile targetGVK actually depends on the profile. `CollectRuleGVKs` returns the `spec.resourceRules[].gvk` list and ignores the profile GVK entirely whenever resourceRules is non-empty, so those GVKs are knowable at admission and were being waved through. Not an exploit path — a target cannot collect while its profile is missing, and reconcile now catches it once the profile appears — but admission should reject a spec it can fully evaluate. The NotFound branch also duplicated the happy path's two namespace checks. That is the divergence shape that caused the bug #304 fixes: a third check added to the tail later would silently not run for missing profiles. Resolve is now a switch that yields `profileGVK` plus `profileResolved`, and the whole tail runs once for both paths. GVK validation is skipped only when the profile is missing *and* resourceRules is empty, where the alternative is comparing a zero GVK against allowedGVKs and rejecting a legal create-before-profile. Tests: - `missingProfileSpecDerivedChecks` — denied resourceRules GVK and denied profileRef namespace, both with no profile object; red on the first before this change. It also covers the `ValidateClusterScopeStaticRefNamespace` call #304 added, which its own fixture could not exercise: allowedStaticRefNamespaces was unset there, so the call returned nil unconditionally. - `deniedRuleGVKDegrades` — reconcile side, permitted profile targetGVK plus a denied resourceRules GVK. Green before this commit; it locks the rules path through CollectRuleGVKs that #304's test did not reach. - ScopeCeiling's doc comment claimed "namespace and GVK checks" while the struct only carries namespaces, which points readers at the wrong enforcement point. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook and controller unit tests green. envtest suites are CI-only on this host; a GVK-denied spec in cluster_scope_enforce_envtest_test.go is left as follow-up rather than added unverified.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Keep KollectClusterScope namespace checks when a ClusterTarget profile is missing, and re-check allowed GVKs at reconcile after the profile loads.
Problem
validateClusterScopereturned nil on profile NotFound. That skippedIncludedNamespacesandprofileRef.namespacechecks.ValidateClusterScopeGVKsran only at admission. A ClusterTarget could be created against a missing profile, then a later profile with a denied GVK would start collection because reconcile never re-checked GVK.Namespace ceiling still filters collect-time namespaces. GVK had no second line of defense.
Change
profileRef.namespace. Create-before-profile remains allowed when those are in scope.loadClusterScopeBindingcallsValidateClusterScopeGVKsand degrades withScopeGVKDenied.Validation
kube-systemwas admitted; reconcile of a denied-GVK profile leftDegradedunset.go test ./internal/webhook/v1alpha1/ -run ClusterTargetandgo test ./internal/controller/ -run ClusterTargetplus customgolangci-linton the changed packages.Origin
Admission NotFound skip landed in
8c63fcb(2026-06-05) with collection filtering.