🐛 fix(controller): make KollectClusterScope enforceable at runtime (RBAC + watch) - #306
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
`scope.LoadCluster` lists KollectClusterScope on every KollectClusterTarget reconcile, in `enforceClusterScopePolicy` for KollectClusterInventory, and in both cluster-kind validating webhooks — but no kubebuilder marker ever declared that access, so neither `config/rbac/role.yaml` nor the chart ClusterRole granted it. `kollectscopes` (the namespaced ceiling) was granted; its cluster-scoped sibling was not. In an RBAC-enforcing cluster the manager cache cannot start an informer for a type it may not list, so `LoadCluster` fails: cluster-target reconcile returns an error and requeues forever, and because both cluster webhooks are failurePolicy=fail, admission rejects every KollectClusterTarget and KollectClusterInventory write with "load KollectClusterScope". envtest does not enforce RBAC and the e2e suites do not exercise the cluster kinds, which is why no gate caught it. - Markers on both controllers that reach LoadCluster, mirroring how `kollectscopes` is declared on the two namespaced controllers. - `config/rbac/role.yaml` regenerated via `task manifests` (one line). - Chart ClusterRole extended to match. It is hand-maintained and rendered only when `not tenantMode`, which is exactly where the cluster kinds are served, so the namespaced role.yaml template deliberately stays untouched. - `hack/test/cluster_scope_rbac_test.sh`, a regression lock in the shape of core_events_rbac_test.sh: it asserts the markers, the generated role, and the chart template together. Verified red with the fix stashed. Gates: new lock red→green, lint:shell clean, helm-test 40/40, verify ok, scrub ok. The actionlint style finding at ci.yaml:207 is pre-existing on main and only shifted by the added step.
…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.
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
force-pushed
the
fix/clusterscope-watch
branch
from
August 18, 2026 09:24
84479ee to
6bfb811
Compare
… grant The OLM ClusterServiceVersion template carries a hand-maintained copy of the manager rules. Adding kollectclusterscopes to config/rbac/role.yaml without it left the bundle short of a permission the manager now needs to watch, so an OLM-installed operator would fail its cache sync and exit. hack/test/dist_olm_bundle_test.sh caught the drift; it is the existing lock for this surface, so cluster_scope_rbac_test.sh points at it rather than duplicating the assertion.
|
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
1 participant
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.
Independent of #304 — mergeable in any order, no textual overlap.
Two commits, both about the ceiling actually taking effect in a real cluster.
1. The manager has no RBAC to read KollectClusterScope
scope.LoadClusterlists KollectClusterScope on every KollectClusterTarget reconcile, inenforceClusterScopePolicyfor KollectClusterInventory, and in both cluster-kind validating webhooks — but no kubebuilder marker ever declared that access. Neitherconfig/rbac/role.yamlnor the chart ClusterRole grants it.kollectscopesis granted; its cluster-scoped sibling is not.In an RBAC-enforcing cluster the manager cache cannot start an informer for a type it may not list, so
LoadClusterfails: cluster-target reconcile errors and requeues forever, and since both cluster webhooks arefailurePolicy=fail, admission rejects every KollectClusterTarget and KollectClusterInventory write withload KollectClusterScope. envtest does not enforce RBAC and the e2e suites do not exercise the cluster kinds, which is why nothing caught it.Fixed with markers on both controllers,
task manifests, the chart ClusterRole (rendered only whennot tenantMode, which is where cluster kinds are served), andhack/test/cluster_scope_rbac_test.sh— a regression lock shaped like the existingcore_events_rbac_test.sh, wired into CI. Verified red with the fix stashed.2. Ceiling edits did not re-reconcile targets
The controller watched KollectProfile and Namespace but not the ceiling, and no
SyncPeriodis set — so editing a KollectClusterScope changed nothing until an unrelated event or the 10h resync. TighteningallowedGVKsleft offending targets collecting; widening it left correctly degraded targets stuck. With #304 adding the reconcile-time GVK check, that lag is the difference between a ceiling that is enforced and one that is merely declared.The map function does not filter to the enforced scope on purpose:
LoadClusterresolves the ceiling as the lowest-named object of all of them, so any scope write can change which one is enforced. Fan-out is bounded by the number of cluster targets.Left alone deliberately: KollectClusterInventory shares
enforceClusterScopePolicyand has the identical lag onsinkRefs; the namespaced KollectTarget/KollectScope pair has it too. Neither regresses here.Gates: new RBAC lock red-to-green,
task lint(golangci v2 + arch-lint) clean,task verifyok,task lint:shellclean,task helm-test40/40,task scrubok, controller unit tests green. envtest suites are CI-only on this host and fail identically on unmodified main here. The actionlint style finding atci.yaml:207is pre-existing and only shifted by the added step.