From 4eb39f1c7b57e9fb299611dc4312b55f46dbfb92 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Mon, 17 Aug 2026 17:04:31 +0200 Subject: [PATCH 1/4] :bug: fix(controller): grant the manager RBAC to read KollectClusterScope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .github/workflows/ci.yaml | 2 + charts/kollect/templates/clusterrole.yaml | 2 +- config/rbac/role.yaml | 1 + hack/test/cluster_scope_rbac_test.sh | 69 +++++++++++++++++++ .../kollectclusterinventory_controller.go | 1 + .../kollectclustertarget_controller.go | 1 + 6 files changed, 75 insertions(+), 1 deletion(-) create mode 100755 hack/test/cluster_scope_rbac_test.sh diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a1e0ea42..7b3bea2c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -140,6 +140,8 @@ jobs: run: bash hack/test/hyg_07_nightly_race_test.sh - name: Verify manager RBAC grants core Events (not events.k8s.io) run: bash hack/test/core_events_rbac_test.sh + - name: Verify manager RBAC can read KollectClusterScope + run: bash hack/test/cluster_scope_rbac_test.sh - name: Verify changelog-sync release guard (fbb5196a3 regression lock) run: bash hack/test/changelog_sync_release_guard_test.sh # LAB-DEKIND: this suite is the enforcement mechanism for the lab substrate allowlist diff --git a/charts/kollect/templates/clusterrole.yaml b/charts/kollect/templates/clusterrole.yaml index d565ae3f..0e47ea52 100644 --- a/charts/kollect/templates/clusterrole.yaml +++ b/charts/kollect/templates/clusterrole.yaml @@ -54,7 +54,7 @@ rules: - kollecttargets/status verbs: [get, patch, update] - apiGroups: [kollect.dev] - resources: [kollectprofiles, kollectscopes] + resources: [kollectclusterscopes, kollectprofiles, kollectscopes] verbs: [get, list, watch] - apiGroups: [cert-manager.io] resources: [certificates] diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index e50d570d..266c81a2 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -96,6 +96,7 @@ rules: - apiGroups: - kollect.dev resources: + - kollectclusterscopes - kollectprofiles - kollectscopes verbs: diff --git a/hack/test/cluster_scope_rbac_test.sh b/hack/test/cluster_scope_rbac_test.sh new file mode 100755 index 00000000..fa94fd74 --- /dev/null +++ b/hack/test/cluster_scope_rbac_test.sh @@ -0,0 +1,69 @@ +#!/usr/bin/env bash +# Regression lock: scope.LoadCluster lists KollectClusterScope on every +# KollectClusterTarget/KollectClusterInventory reconcile, and both cluster-kind +# webhooks call it with failurePolicy=fail. Without get/list/watch on +# kollectclusterscopes the manager cache cannot sync that type, so reconcile +# errors and admission rejects every cluster-kind write. The grant was missing +# from the generated role and the chart until this lock landed. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +ROLE="${ROOT}/config/rbac/role.yaml" +CLUSTERROLE_TMPL="${ROOT}/charts/kollect/templates/clusterrole.yaml" +CONTROLLERS=( + "${ROOT}/internal/controller/kollectclustertarget_controller.go" + "${ROOT}/internal/controller/kollectclusterinventory_controller.go" +) + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +pass() { + echo "ok - $*" +} + +[[ -f "${ROLE}" ]] || fail "missing ${ROLE}" +[[ -f "${CLUSTERROLE_TMPL}" ]] || fail "missing ${CLUSTERROLE_TMPL}" + +# Controllers that reach scope.LoadCluster must carry the marker; controller-gen +# aggregates them into the single manager role. +for f in "${CONTROLLERS[@]}"; do + [[ -f "${f}" ]] || fail "missing controller ${f}" + if ! grep -Eq 'kubebuilder:rbac:groups=kollect\.dev,resources=kollectclusterscopes,verbs=get;list;watch' "${f}"; then + fail "$(basename "${f}"): missing kollectclusterscopes get;list;watch marker" + fi + pass "$(basename "${f}") kollectclusterscopes marker" +done + +python3 - "${ROLE}" <<'PY' || fail "config/rbac/role.yaml missing kollectclusterscopes get/list/watch" +import sys, yaml +doc = yaml.safe_load(open(sys.argv[1])) +for rule in doc.get("rules") or []: + groups = set(rule.get("apiGroups") or []) + resources = set(rule.get("resources") or []) + verbs = set(rule.get("verbs") or []) + if "kollect.dev" in groups and "kollectclusterscopes" in resources and {"get", "list", "watch"} <= verbs: + print("ok - config/rbac/role.yaml kollectclusterscopes get/list/watch") + break +else: + sys.exit(1) +PY + +# The chart ClusterRole is hand-maintained and only rendered outside tenantMode, +# which is exactly where the cluster kinds are served. +if ! grep -Eq '^\s*resources: \[[^]]*kollectclusterscopes[^]]*\]' "${CLUSTERROLE_TMPL}"; then + fail "$(basename "${CLUSTERROLE_TMPL}"): no kollectclusterscopes in any resources list" +fi +if ! awk ' + /resources: \[[^]]*kollectclusterscopes[^]]*\]/ { found=1; next } + found && /verbs: \[get, list, watch\]/ { ok=1 } + found && /verbs:/ && !ok { exit 1 } + END { exit ok ? 0 : 1 } +' "${CLUSTERROLE_TMPL}"; then + fail "$(basename "${CLUSTERROLE_TMPL}"): kollectclusterscopes rule lacks verbs [get, list, watch]" +fi +pass "$(basename "${CLUSTERROLE_TMPL}") kollectclusterscopes get/list/watch" + +echo "All cluster_scope_rbac tests passed." diff --git a/internal/controller/kollectclusterinventory_controller.go b/internal/controller/kollectclusterinventory_controller.go index 024edfb5..77bdc205 100644 --- a/internal/controller/kollectclusterinventory_controller.go +++ b/internal/controller/kollectclusterinventory_controller.go @@ -59,6 +59,7 @@ type KollectClusterInventoryReconciler struct { // +kubebuilder:rbac:groups=kollect.dev,resources=kollectclusterinventories/finalizers,verbs=update // +kubebuilder:rbac:groups=kollect.dev,resources=kollectclustertargets,verbs=get;list;watch // +kubebuilder:rbac:groups=kollect.dev,resources=kollectsnapshotsinks;kollectdatabasesinks;kollecteventsinks,verbs=get;list;watch +// +kubebuilder:rbac:groups=kollect.dev,resources=kollectclusterscopes,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch diff --git a/internal/controller/kollectclustertarget_controller.go b/internal/controller/kollectclustertarget_controller.go index 0cce13d2..9daa96e6 100644 --- a/internal/controller/kollectclustertarget_controller.go +++ b/internal/controller/kollectclustertarget_controller.go @@ -38,6 +38,7 @@ type KollectClusterTargetReconciler struct { // +kubebuilder:rbac:groups=kollect.dev,resources=kollectclustertargets/status,verbs=get;update;patch // +kubebuilder:rbac:groups=kollect.dev,resources=kollectclustertargets/finalizers,verbs=update // +kubebuilder:rbac:groups=kollect.dev,resources=kollectprofiles,verbs=get;list;watch +// +kubebuilder:rbac:groups=kollect.dev,resources=kollectclusterscopes,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch From 2c665262a18383e608db36429d94c05f579e10c3 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Mon, 17 Aug 2026 17:07:34 +0200 Subject: [PATCH 2/4] :bug: fix(controller): re-reconcile cluster targets on KollectClusterScope writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../kollectclustertarget_controller.go | 31 +++++++++++++++ .../kollectclustertarget_map_test.go | 38 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/internal/controller/kollectclustertarget_controller.go b/internal/controller/kollectclustertarget_controller.go index 9daa96e6..5fee9481 100644 --- a/internal/controller/kollectclustertarget_controller.go +++ b/internal/controller/kollectclustertarget_controller.go @@ -354,6 +354,10 @@ func (r *KollectClusterTargetReconciler) SetupWithManager(mgr ctrl.Manager) erro &kollectdevv1alpha1.KollectProfile{}, handler.EnqueueRequestsFromMapFunc(r.mapProfileToClusterTargets), ). + Watches( + &kollectdevv1alpha1.KollectClusterScope{}, + handler.EnqueueRequestsFromMapFunc(r.mapClusterScopeToClusterTargets), + ). Named("kollectclustertarget"). Complete(r) } @@ -377,6 +381,33 @@ func (r *KollectClusterTargetReconciler) mapNamespaceToClusterTargets( return reqs } +// mapClusterScopeToClusterTargets re-reconciles every KollectClusterTarget on any +// KollectClusterScope write. It does not filter to the enforced scope on purpose: +// scope.LoadCluster resolves the ceiling as the lowest-named object of all of them, +// so creating or renaming any KollectClusterScope can change which one is enforced. +func (r *KollectClusterTargetReconciler) mapClusterScopeToClusterTargets( + ctx context.Context, + obj client.Object, +) []reconcile.Request { + if _, ok := obj.(*kollectdevv1alpha1.KollectClusterScope); !ok { + return nil + } + + var list kollectdevv1alpha1.KollectClusterTargetList + if err := r.List(ctx, &list); err != nil { + return nil + } + + reqs := make([]reconcile.Request, 0, len(list.Items)) + for i := range list.Items { + reqs = append(reqs, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: list.Items[i].Name}, + }) + } + + return reqs +} + func (r *KollectClusterTargetReconciler) mapProfileToClusterTargets( ctx context.Context, obj client.Object, diff --git a/internal/controller/kollectclustertarget_map_test.go b/internal/controller/kollectclustertarget_map_test.go index dff9ccfd..f3fd7e6c 100644 --- a/internal/controller/kollectclustertarget_map_test.go +++ b/internal/controller/kollectclustertarget_map_test.go @@ -55,3 +55,41 @@ func TestKollectClusterTargetReconciler_mapFunctions(t *testing.T) { t.Fatalf("non-profile object should return nil, got %#v", got) } } + +func TestKollectClusterTargetReconciler_mapClusterScopeToClusterTargets(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := kollectdevv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + first := &kollectdevv1alpha1.KollectClusterTarget{ObjectMeta: metav1.ObjectMeta{Name: "ct-a"}} + second := &kollectdevv1alpha1.KollectClusterTarget{ObjectMeta: metav1.ObjectMeta{Name: "ct-b"}} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(first, second).Build() + r := &KollectClusterTargetReconciler{Client: cl} + + // Any KollectClusterScope write can change the enforced ceiling — LoadCluster + // picks the lowest-named object of all of them — so every target re-reconciles + // regardless of which scope object was written. + scopeObj := &kollectdevv1alpha1.KollectClusterScope{ObjectMeta: metav1.ObjectMeta{Name: "zz-not-enforced"}} + reqs := r.mapClusterScopeToClusterTargets(context.Background(), scopeObj) + if len(reqs) != 2 { + t.Fatalf("cluster scope map reqs = %#v, want one per cluster target", reqs) + } + + names := map[string]bool{} + for _, req := range reqs { + if req.Namespace != "" { + t.Fatalf("cluster-scoped request must not carry a namespace: %#v", req) + } + names[req.Name] = true + } + if !names["ct-a"] || !names["ct-b"] { + t.Fatalf("cluster scope map reqs = %#v, want ct-a and ct-b", reqs) + } + + if got := r.mapClusterScopeToClusterTargets(context.Background(), first); got != nil { + t.Fatalf("non-scope object should return nil, got %#v", got) + } +} From 6bfb8113d9db74ed1142682f1157105125a886d2 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Mon, 17 Aug 2026 17:19:04 +0200 Subject: [PATCH 3/4] :memo: docs(operator): warn that chart and image must upgrade together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/operator-manual/upgrading.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/operator-manual/upgrading.md b/docs/operator-manual/upgrading.md index 4ece9cd0..b5e66b45 100644 --- a/docs/operator-manual/upgrading.md +++ b/docs/operator-manual/upgrading.md @@ -87,6 +87,15 @@ Pin `image.tag` to a specific release (or use the release-pinned `install.yaml`) chart default resolves to `v` — the image shipped with that chart version — rather than a floating `latest` tag. +!!! warning "Upgrade chart and image together" + RBAC is Helm-managed, so a pinned `image.tag` newer than the installed chart runs against the + older ClusterRole. Releases after **v0.18.0** add `kollectclusterscopes` `get`/`list`/`watch` to + the manager ClusterRole and make the controller *watch* that type. A manager that cannot watch a + type it registered fails its cache sync and exits, so pairing the new image with the old + ClusterRole crash-loops the operator instead of degrading one controller. Bump the chart in the + same change as the image — or, on the raw-manifest path, re-apply `install.yaml` from the same + release. + ### 4. Wait for rollout ```sh From f60f7facec8879479c7f2a5f544fdf10173a19c7 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Tue, 18 Aug 2026 11:33:37 +0200 Subject: [PATCH 4/4] :bug: fix(olm): sync CSV clusterPermissions with the kollectclusterscopes 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. --- .../olm/template/manifests/kollect.clusterserviceversion.yaml | 1 + hack/test/cluster_scope_rbac_test.sh | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/config/olm/template/manifests/kollect.clusterserviceversion.yaml b/config/olm/template/manifests/kollect.clusterserviceversion.yaml index e2ad26d0..5b0f287e 100644 --- a/config/olm/template/manifests/kollect.clusterserviceversion.yaml +++ b/config/olm/template/manifests/kollect.clusterserviceversion.yaml @@ -229,6 +229,7 @@ spec: - apiGroups: - kollect.dev resources: + - kollectclusterscopes - kollectprofiles - kollectscopes verbs: diff --git a/hack/test/cluster_scope_rbac_test.sh b/hack/test/cluster_scope_rbac_test.sh index fa94fd74..c70ce43d 100755 --- a/hack/test/cluster_scope_rbac_test.sh +++ b/hack/test/cluster_scope_rbac_test.sh @@ -5,6 +5,10 @@ # kollectclusterscopes the manager cache cannot sync that type, so reconcile # errors and admission rejects every cluster-kind write. The grant was missing # from the generated role and the chart until this lock landed. +# +# Third surface: the OLM CSV template carries its own copy of these rules. +# It is not re-asserted here — hack/test/dist_olm_bundle_test.sh already fails +# on any drift between config/rbac/role.yaml and the CSV clusterPermissions. set -euo pipefail ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"