[gjkim42]

What type of PR is this?

/kind api

What this PR does / why we need it:

Sessions reject changes to spec.worker.podOverrides.serviceAccountName because replacing the runtime RoleBinding's subject can remove the running Pod's permission to acknowledge draining.

Allow service-account changes through the existing drain-and-replace flow. Authorize both the desired account and the observed Pod's account during the transition, then remove the previous account when the replacement Pod is observed. If no owned Pod is visible, retain the current StatefulSet template's account when creating or repairing the RoleBinding and preserve its existing Session-scoped grants. Unrelated Pods do not affect runtime permissions or preempt the controller's existing Pod-conflict and suspension handling. Clearing the override selects the managed service account.

Remove the immutability validation, regenerate CRDs, and clarify that inherited Kubernetes descriptions such as "Cannot be updated" describe existing Pods rather than Session overrides. Cover account transitions, repeated edits, terminating and absent Pods, RoleBinding recreation during a Pod gap, unrelated Pods, and suspended updates; extend the live drain end-to-end test to change service accounts.

Document the authorization contract explicitly: the runtime and agent share the Pod's token and can patch the Session spec, including its service account. Agents must therefore be trusted to select service accounts in their namespace. This PR does not add operator-only admission enforcement; deployments requiring that restriction must provide an admission policy.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

• make verify passed.
• make test-integration TEST_FLAGS=-p=2: all 12 install/upgrade specs passed. One controller/API run hit a Task test cache-ordering failure (github-token Secret was not yet visible to the Task controller); all Session specs passed.
• make test-integration TEST_FLAGS='-p=1 -run=^TestIntegration$' passed on retry: all 161 controller/API specs, without product or test changes for the unrelated timing failure.
• make test TEST_FLAGS="-p=2 -run='^(TestSession|TestEnsureSession)'" passed.
• env -u CODEX_HOME -u CODEX_AUTH_JSON make test TEST_FLAGS=-p=2 passed. Unsetting the ambient agent home and authentication variables isolates the entrypoint tests from this Session's environment.
• Cluster end-to-end execution runs in PR CI.

Does this PR introduce a user-facing change?

Session service accounts can be changed through spec.worker.podOverrides.serviceAccountName. Kelos preserves the running Pod's Session permissions while it drains and applies the change through Pod replacement. Clearing the override restores the managed service account.

[cubic-dev-ai]

1 issue found across 10 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/controller/session_runtime_access.go">

<violation number="1" location="internal/controller/session_runtime_access.go:76">
P2: The new no-Pod preservation path does not run when the runtime RoleBinding is recreated, because the create path returns before `if !podFound`. A service-account transition during a Pod/cache gap can therefore drop the old Pod’s permissions and leave the drain waiting indefinitely; retain the previous account when recreating the binding, using the current StatefulSet template or another persisted source.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[gjkim42]

/kelos review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-163dd4cc0a48 has succeeded. ✅

[kelos-bot]

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: COMMENT
Overall correctness: patch is correct
Scope: Makes Session.spec.worker.podOverrides.serviceAccountName mutable. The change goes through the drain-and-replace flow, and the runtime RoleBinding keeps the draining Pod's account until the replacement Pod is observed.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	1	api/v1alpha2/session_types.go:226	The Session runtime can now move its own Pod to any ServiceAccount in the namespace
P3	1	internal/controller/session_runtime_access.go:51	New Pod ownership check preempts the existing PodConflict status

Findings

Security

• [P2] api/v1alpha2/session_types.go:226, internal/controller/session_runtime_access.go:121-125: Removing the CEL immutability rule lets the Session's own runtime switch its Pod to any ServiceAccount in the namespace.
  • How: The runtime Role grants patch on sessions for this Session. RBAC cannot limit that verb to metadata. AutomountServiceAccountToken is forced to true (session_controller.go:1781), and the agent CLI runs in the same container as kelos-session-runtime. So the agent, including a prompt-injected one, can patch spec.worker.podOverrides.serviceAccountName. The controller then binds the new account and replaces the Pod under it once it drains, or immediately with the force annotation, which the runtime can also set.
  • Why this is new: The runtime could already change other podOverrides, for example to mount namespace Secrets. Those changes could not give it another account's RBAC permissions or its workload-identity cloud role (IRSA, GKE/Azure Workload Identity). That is the documented purpose of this field. Until this PR, the CEL rule was the only thing preventing that.
  • Options: Reject changes to this field when the requester is the Session Pod's own ServiceAccount, for example with a ValidatingAdmissionPolicy that checks request.userInfo.username. Or, if this is an accepted trust boundary, say explicitly in docs/reference.md that a Session runtime can change its own service account.

Correctness / Diagnostics

• [P3] internal/controller/session_runtime_access.go:51-53: The new ownership check runs before the existing one at session_controller.go:395-397.
  • Effect: A Pod at <name>-0 that the StatefulSet does not control now puts the Session in Failed with reason RuntimeAccessFailed instead of PodConflict. That makes the PodConflict branch effectively unreachable in the main path. The early return also skips reconcileSessionStatefulSet, and suspended Sessions, which never checked the Pod before, now fail too.
  • When it happens: For example, while a Pod from a deleted and recreated StatefulSet is still being garbage-collected.
  • Fix: Treat a Pod that the StatefulSet does not control as absent here, which keeps the existing subjects. The existing PodConflict branch would then report the conflict as before.

Suggestions (optional)

• [P3] internal/controller/session_runtime_access.go:76-84: While no Pod is visible, every edit adds another retained subject.
  • Changing A → B → C on a suspended Session leaves [C, B, A] bound until a Pod appears, although no Pod ever ran as B. Given the P2 above, each retained account can also patch the Session.
  • When the StatefulSet has spec.replicas == 0 and reports status.replicas == 0, no unobserved Pod can exist. Pruning to the desired account in that case would limit retention without reopening the cache-gap case.
• [P3] test/e2e/session_test.go:539, :586: The RoleBinding is fetched by pod.Spec.ServiceAccountName. This works only because the managed ServiceAccount and the RoleBinding share the name session-<name>-runtime. Naming the RoleBinding explicitly avoids a confusing failure if either name changes.

Key takeaways

• The RoleBinding transition logic looks right: it binds the desired account plus the observed Pod's account and prunes the old one once the replacement Pod is seen. Unit tests cover repeated and intermediate edits, a terminating Pod, a missing Pod, a recreated RoleBinding, and a managed ↔ configured round trip. The integration and e2e tests exercise the full flow.
• Relaxing the validation is backward-compatible (it only loosens it), the regenerated CRDs are included, and docs/reference.md matches the behavior.
• Open question for maintainers: should the Session runtime be able to change its own service account (P2)?