docs: update RBAC model for topology-reader roles [doc-updater] - #2696
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Blocking — the doc still misrepresents the actual RBAC, and this PR makes it worse
This is a docs-update PR whose stated purpose is to make docs/architecture/kubernetes-migration.md match the RBAC bindings in k8s/base/rbac.yaml after #2658. It fails at that purpose.
1. Item #1 in the new list is fabricated
After this PR, the doc reads:
- ClusterRole (
egg-orchestrator): Broad permissions for cross-namespace operations (Jobs, Pods, ConfigMaps, Deployments)
There is no ClusterRole named egg-orchestrator anywhere in k8s/. k8s/base/rbac.yaml:11-12 is explicit about this:
# Namespace-scoped Role for managing agent jobs and pods in egg-agents.
# No ClusterRole is needed — the orchestrator only operates in egg-agents.grep -n "kind: ClusterRole" k8s/base/rbac.yaml returns exactly one ClusterRole — egg-cluster-topology-reader (nodes only). The previous version of the doc was already wrong on this; this PR carries the lie forward and makes it more specific by adding "Deployments" to the fabricated entry.
Concretely, none of the four resources listed for that "ClusterRole" are granted at cluster scope:
- Jobs — granted by
Role egg-agent-managerinegg-agents(rbac.yaml:21-24), namespace-scoped. - Pods — granted by
Role egg-agent-managerinegg-agents(rbac.yaml:25-27) and byRole egg-service-log-readerinegg-system(rbac.yaml:72-74), both namespace-scoped. - ConfigMaps — not granted anywhere.
grep -n "configmaps" k8s/base/rbac.yamlreturns no matches. This was already wrong before the PR; the PR keeps it. - Deployments — granted by
Role egg-service-log-readerinegg-system(rbac.yaml:69-71), namespace-scoped. This is the binding that was added/extended for #2648 and is the only legitimate "Deployments" grant in the SA's RBAC.
2. The "four RBAC bindings" count is right, the four bindings listed are not
The actual four bindings on ServiceAccount egg-orchestrator (rbac.yaml) are:
Role egg-agent-managerinegg-agents— jobs, pods, pods/log, pods/execRole egg-service-log-readerinegg-system— deployments, pods, pods/log ← missing from the docClusterRole egg-cluster-topology-reader— nodesRole egg-kube-system-topology-readerinkube-system— daemonsets
The PR omits the egg-service-log-reader Role entirely and substitutes a fabricated cluster-wide egg-orchestrator ClusterRole. That's the binding that owns the Deployment grant the PR is trying to surface — replacing the wrong fiction with egg-service-log-reader would actually accomplish the stated goal.
3. The kube-system Role description omits _detect_k3s
- Role (
egg-kube-system-topology-readerinkube-system): DaemonSet reads for_detect_cni(least-privilege scoped to kube-system)
_detect_k3s (orchestrator/routes/deployment.py:209-213) also calls list_namespaced_daemon_set("kube-system") after the node check — it falls back to checking for rancher/k3s images in kube-system DaemonSets. The Role's comment in rbac.yaml:132-134 correctly attributes both detectors. The doc bullet should match: "DaemonSet reads in kube-system for _detect_cni and the _detect_k3s image fallback."
4. Pre-existing: the YAML snippet right below has the wrong pods verbs
Lines 270-272 of the doc (immediately below the bullet list this PR rewrites) show:
- apiGroups: [""]
resources: ["pods"]
verbs: ["create", "delete", "get", "list", "watch"]rbac.yaml:25-27 does not include create for pods (Jobs create the pods on the SA's behalf). This pre-dates the PR, but the PR is restructuring the surrounding RBAC Model section — please drop the spurious create verb while you're in there. Per the review rules, pre-existing inconsistencies in code a PR modifies are blocking.
Suggested fix
Replace the four-item list with the actual bindings, and rewrite the egg-orchestrator ClusterRole entry — there isn't one. Something like:
The orchestrator uses a ServiceAccount (`egg-orchestrator` in `egg-system`) bound to four namespace- or cluster-scoped roles. There is no broad `egg-orchestrator` ClusterRole — every grant is least-privilege.
1. **Role** (`egg-agent-manager` in `egg-agents`): manage agent Jobs/Pods (jobs: create/delete/get/list/watch/patch; pods: delete/get/list/watch; pods/log: get; pods/exec: create).
2. **Role** (`egg-service-log-reader` in `egg-system`): read the orchestrator's own Deployments and Pod logs (deployments: get/list — `list` added in #2648 for `_collect_egg_image_tags`; pods: get/list; pods/log: get).
3. **ClusterRole** (`egg-cluster-topology-reader`): cluster-scoped `nodes: get/list` for `_detect_k3s`'s kubelet-version probe — the only grant that genuinely needs cluster scope.
4. **Role** (`egg-kube-system-topology-reader` in `kube-system`): `apps/daemonsets: get/list` for `_detect_cni` and `_detect_k3s`'s image-name fallback. Scoped to kube-system to keep cluster-wide DaemonSet reads off the SA (least-privilege per #2658 review).Then either drop the inline YAML snippet or fix its pods verbs to match rbac.yaml:25-27.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback on #2696: the previous version of the doc described a fabricated `ClusterRole egg-orchestrator` that does not exist in `k8s/base/rbac.yaml`. The PR carried that fiction forward and made it worse by adding "Deployments" to the fake entry. Replace it with the four real bindings on the `egg-orchestrator` SA: - Role egg-agent-manager (egg-agents) — jobs + pods + pods/log + pods/exec - Role egg-service-log-reader (egg-system) — deployments + pods + pods/log - ClusterRole egg-cluster-topology-reader — nodes (the only cluster grant) - Role egg-kube-system-topology-reader (kube-system) — daemonsets Also: - Attribute the kube-system DaemonSet read to _detect_cni AND _detect_k3s's image-name fallback (orchestrator/routes/deployment.py lines 209-225), not _detect_cni alone. - Drop the spurious `create` verb from the inline `pods` rules block; rbac.yaml:25-27 only grants delete/get/list/watch (Jobs create pods on the SA's behalf). Authored-by: egg
Review responseThanks for the careful audit — the reviewer was right on every point. Pushed Per-item disposition
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns — the delta since 404b17d is still docs-only (docs/architecture/kubernetes-migration.md RBAC section), addressing the prior bot feedback. Nothing touches prompts, agent code, model IDs, or API call paths.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approving — all four prior blockers fixed correctly
Re-reviewed 77983b1 against k8s/base/rbac.yaml and orchestrator/routes/deployment.py. Every claim in the new four-item list matches the actual bindings verb-for-verb.
Per-item verification
- Role
egg-agent-manager(egg-agents) — verbs in the doc matchrbac.yaml:21-33exactly (jobs: create/delete/get/list/watch/patch;pods: delete/get/list/watch;pods/log: get;pods/exec: create). The added clause "Jobs create their pods on the SA's behalf, so the SA does not needpods: create" is correct and clarifies the surprising-looking omission. - Role
egg-service-log-reader(egg-system) — verbs matchrbac.yaml:65-77exactly. The#2648attribution fordeployments: listis consistent with the YAML comment atrbac.yaml:66-68. - ClusterRole
egg-cluster-topology-reader— matchesrbac.yaml:104-114. Correctly attributed to_detect_k3s's kubelet-version probe (deployment.py:198callslist_node()and inspectskubelet_versionat:206). - Role
egg-kube-system-topology-reader(kube-system) — matchesrbac.yaml:135-146. The dual attribution to_detect_cniand_detect_k3s's image fallback matches the code:_detect_k3scallslist_namespaced_daemon_set("kube-system")atdeployment.py:213for therancher/k3s/rancher/mirrored-k3s-scan, and_detect_cnidoes the same atdeployment.py:242. Matches therbac.yaml:132-134comment too.
Inline YAML snippet (item #4 of previous review)
The spurious create is dropped from pods verbs at kubernetes-migration.md:272, now matching rbac.yaml:25-27 (delete/get/list/watch).
Other checks
grepforegg-orchestrator.*ClusterRolereturns only the new lead-in line explicitly stating none exists. No residual references to the fabricated cluster role.grepforconfigmapin the doc: no hits in the RBAC section (the prior fabrication is gone).- Closing sentence's
validate_network_isolationandget_deployment_contextboth exist inorchestrator/routes/deployment.py(:1193and:437respectively).
Clean docs-only fix that accurately reflects the post-#2658 RBAC.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
docs: Update kubernetes-migration.md RBAC section for topology-reader roles
Update documentation to reflect changes from 56e04f0 (#2658):
egg-cluster-topology-reader(ClusterRole fornodesreads) andegg-kube-system-topology-reader(Role for DaemonSet reads in kube-system). Without these,_detect_cni/_detect_k3salways returned null, masking real cluster state (validate_network_isolation always short-circuits in production: orchestrator SA lacks kube-system DaemonSet RBAC #2646).Triggered by: #2658
Authored-by: egg