Skip to content

sec(#1876): grant jobs-manager list nodes for GPU→CPU pending fallback - #701

Merged
saadqbal merged 3 commits into
developfrom
sec/1876-jobs-manager-list-nodes
Aug 13, 2026
Merged

sec(#1876): grant jobs-manager list nodes for GPU→CPU pending fallback#701
saadqbal merged 3 commits into
developfrom
sec/1876-jobs-manager-list-nodes

Conversation

@saadqbal

@saadqbal saadqbal commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Grants the jobs-manager ServiceAccount list on nodes so the GPU→CPU pending-pod fallback (client-runtime#217) can actually run.

Why

check_pending_jobs's GPU→CPU fallback calls list_node() to tell an autoscaling-in GPU node from a genuinely absent one. The runtime fails safe on any list-nodes error (assume a GPU node is present, never downgrade). nodes is cluster-scoped and was granted nowhere in the chart — so on every multi-node install list_node() 403s, the fail-safe kicks in, and the fallback is silently disabled: an overdue Pending GPU pod wedges forever while the backend shows RUNNING.

Diagnosed live on tb-client-dev-templates (EKS): two GPU pods Pending ~17h, auth can-i list nodesno, jobs-manager logging Could not list nodes for GPU-node check ((403) every 60s. Full write-up in backend#1876.

Change

  • client/templates/rbac.yaml: add nodes: ["list"] to the jobs-manager ClusterRole. Cluster-scoped, so it can only live in the clusterScope: true branch — same constraint the existing tokenreviews grant documents. The namespaced Role (clusterScope: false) intentionally omits it.
  • client/tests/rbac_test.yaml: assert nodes: ["list"] is present in the ClusterRole and absent from the Role.

Test

helm unittest ./client -f 'tests/rbac_test.yaml' → 14/14 pass.

Rollout note

RBAC-only; takes effect per namespace on the chart upgrade + jobs-manager rollout. No effect on clusterScope: false multi-node installs (nodes ungrantable there) — tracked in backend#1876 alongside the runtime hardening (surface a persistent 403 loudly instead of silently failing safe).

Refs backend#1876.


Note

Low Risk
RBAC-only change with read-only cluster node listing; no auth or data-path changes beyond enabling existing runtime behavior on cluster-scoped installs.

Overview
Grants jobs-manager list on cluster-scoped nodes in the Helm chart’s ClusterRole (clusterScope: true only), so check_pending_jobs can call list_node() for the GPU→CPU pending-pod fallback instead of getting 403s and silently skipping downgrade.

Chart version bumps 1.9.38 → 1.9.39. Helm unittest coverage asserts the rule is on the ClusterRole and not on the namespaced Role (nodes cannot be granted there).

Reviewed by Cursor Bugbot for commit 4bf3206. Bugbot is set up for automated code reviews on this repo. Configure here.

The check_pending_jobs GPU->CPU fallback (client-runtime#217) calls
list_node() to tell an autoscaling-in GPU node from a genuinely absent
one. nodes is cluster-scoped, so the grant can only live in the
clusterScope: true ClusterRole (like tokenreviews). Without it list_node()
403s and the runtime fail-safe assumes a GPU node is present, silently
disabling the fallback — a Pending GPU pod wedges forever while the
backend shows RUNNING.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal saadqbal self-assigned this Aug 13, 2026
saadqbal and others added 2 commits August 13, 2026 17:27
The rbac.yaml ClusterRole change is packaged chart content, so it only
reaches installs via a new chart version (chart-version-guard). Merge
current develop in and bump version + appVersion.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@shujaatTracebloc shujaatTracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Clean, well-scoped RBAC fix for the GPU→CPU pending fallback (backend#1876).

Verified:

  • nodes: ["list"] is correctly placed inside the clusterScope: true ClusterRole branch alongside the existing cluster-scoped tokenreviews grant; the namespaced Role rightly omits it (nodes are ungrantable there).
  • Least-privilege: read-only list verb only.
  • Tests assert both present-in-ClusterRole (documentIndex: 1, clusterScope true) and absent-in-Role (documentIndex 1, clusterScope false) — correct document indices.
  • Chart version bumped lockstep 1.9.38 → 1.9.39.
  • All CI green; Cursor Bugbot clean.

The rollout note correctly scopes this to clusterScope: true installs, with the clusterScope: false gap + the fail-safe-should-log-loudly hardening tracked in backend#1876.

@saqlainsyed007 saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness review — approving. Clean, tightly-scoped RBAC fix.

What it does: grants the jobs-manager ClusterRole list on cluster-scoped nodes so check_pending_jobs's GPU→CPU pending-pod fallback (client-runtime#217) can call list_node() instead of 403ing into the silent fail-safe.

Verified for correctness:

  • nodes: ["list"] is placed only in the clusterScope: true ClusterRole branch, next to the existing cluster-scoped tokenreviews grant; the namespaced Role rightly omits it (nodes are not grantable by a namespace Role). Correct scoping.
  • Least privilege: read-only list only — which is exactly what list_node() (GET /api/v1/nodes) needs; no get/watch.
  • Tests: documentIndex: 1 resolves to the ClusterRole under clusterScope: true and the Role under clusterScope: false (consistent with the existing document-index convention in this suite). The contains (ClusterRole) + notContains (Role) pair asserts both the presence and the intended absence.
  • Chart version bumped in lockstep 1.9.38 → 1.9.39, sequential from the current develop tip; required by the version-bump-gate check.

All CI green and Cursor Bugbot clean. The clusterScope: false gap and the runtime hardening (surface the persistent 403 loudly rather than silently failing safe) are correctly deferred to backend#1876. No correctness issues found.

@saadqbal
saadqbal merged commit eeefca3 into develop Aug 13, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants