Repository navigation
fix(chart): drop kpr's cilium.io RBAC wildcard - #52
Merged
Merged
Conversation
The rule granted a per-node DaemonSet ServiceAccount get through delete on every cilium.io kind cluster-wide, CiliumNetworkPolicy included, on the assumption that Cilium's imported cells reconcile their own CRDs. Under kpr's wiring they do not: populating the exact hive main() assembles shows the only informers registered are Services and EndpointSlices (the LB reflector) plus node-local Pods and Namespaces (the daemon tables), and the lone cilium.io consumer among the wired cells — the CiliumLocalRedirectPolicy controller — registers neither reflector nor controller, because the stubbed DaemonConfig leaves EnableLocalRedirectPolicy false. The CiliumNode/CiliumEndpoint/policy resource constructors from daemonk8s.ResourcesCell sit in the DI graph unconstructed; nothing depends on them. The kind e2e had been proving this all along — its cluster serves no cilium.io group at all — and it passes 4/4 with the DaemonSet bound to exactly the narrowed role in place of the prototype's cluster-admin. The deploy manifest's comment promising "a scoped role (... + cilium.io)" as the follow-up is corrected to match. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
Review follow-up, two corrections to the comment that replaced the rule. It claimed main.go stubs DaemonConfig with EnableLocalRedirectPolicy=false. It does not -- the field appears nowhere in kpr/. The stub sets the two family gates and nothing else, so the flag keeps its zero value. Same outcome, but a comment justifying a security decision should say what the code actually does, not imply an explicit setting that would survive someone editing the stub. And it was a `#` comment, so four lines documenting a rule that is ABSENT shipped inside every applied manifest. It is a template comment now, which is the distinction the chart-wide comment trim is about. Rendered output is identical modulo comment lines. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
lllamnyp
added a commit
to cozystack/cozystack
that referenced
this pull request
Oct 7, 2026
Brings in the upstream review fixes (lllamnyp/cozyplane#52-#54): kpr's cilium.io RBAC wildcard is dropped outright — the wired cells watch only Services/EndpointSlices and node-local Pods/Namespaces, proven on a cluster serving no cilium.io group at all; controller leader election is on with its leases Role, so raising replicas can no longer run uncoordinated writers; the agent's per-VPC metrics are authenticated (TokenReview/SubjectAccessReview, secure by default, fail-closed) and bound to the node's primary address — the values file documents that an existing anonymous scrape gets 401 until the shipped cozyplane-metrics-reader ClusterRole is bound to the monitoring ServiceAccount; and the chart comments that shipped inside rendered manifests are trimmed to mechanism statements with doc pointers. The cozyplane image pin moves to the build carrying the agent changes; the kpr binary is unchanged, so its pin stays. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
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
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.
The cozyplane-kpr ClusterRole granted
apiGroups: [cilium.io], resources: ["*"]with get/list/watch/create/update/patch/delete — a per-node DaemonSet ServiceAccount able to mutate every cilium.io kind cluster-wide, CiliumNetworkPolicy included. The rule is dropped entirely: nothing kpr wires ever reads or writes that group.What the binary actually watches, verified by populating the exact hive
kpr/main.goassembles and inspecting the registered lifecycle hooks: Services and EndpointSlices (the LB reflector), node-local Pods and Namespaces (the daemon k8s tables), plus cozyplane's own net-0 reconciler (core Services/EndpointSlices informers and a Node get). The only cilium.io consumer in the imported cells is the CiliumLocalRedirectPolicy controller, and it registers neither its reflector nor its controller because kpr's stubbedDaemonConfigleavesEnableLocalRedirectPolicyfalse; thedaemonk8s.ResourcesCellCiliumNode/CiliumEndpoint/policy resource constructors are registered in the DI graph but never constructed, as nothing in this hive depends on them.test/kpr-e2e.shpasses (4/4) with the DaemonSet bound to exactly the narrowed role instead of the manifest's cluster-admin — on a kind cluster that serves no cilium.io group at all, which the existing e2e had in fact been proving all along.The stale comment in
deploy/kpr-daemonset.yamlthat promised "a scoped role (… + cilium.io)" as the follow-up is corrected to point at the chart role and state that no cilium.io access is needed.