fix(storagenodeset): give each StorageNodeSet its own ServiceAccount

[boddumanohar]

Summary

• If the shared simplyblock-storage-node-sa ServiceAccount is ever deleted and recreated, every already-running pod's mounted token stays bound to the old, now-nonexistent ServiceAccount UID. The API server correctly rejects those with 401 Unauthorized, and nothing self-heals until kubelet's next token refresh (default ~48min) — in practice a long, manual-intervention outage (had to force-delete pods to get fresh tokens).
• Root cause: reconcileRBAC gives the SA a controller: true ownerReference to a single StorageNodeSet. The SA is shared by every StorageNodeSet in the namespace, not owned 1:1 by any one of them, so whichever one reconciled last became the sole owner GC cares about. Deleting that StorageNodeSet — including as an ordinary part of cluster expansion, adding one and then removing it — let kube-controller-manager's garbage collector cascade-delete the SA out from under every other StorageNodeSet's already-running pods.
• Fix: give each StorageNodeSet its own ServiceAccount and ClusterRoleBinding instead of sharing one across the namespace, so the ownerReference is correct by construction.

This targets the next release, not the one currently being cut (release/26.3.0), so the fix has to be upgrade-safe: existing clusters already running the shared-ServiceAccount design must not have their already-deployed StorageNodeSet's ServiceAccount renamed, since that changes the DaemonSet pod template and forces an unwanted rolling restart of already-running storage nodes. See "Upgrade compatibility" below for how that's handled.

Root cause (confirmed, reproduced)

1. reconcileRBAC builds a fresh, ownerless ServiceAccount struct on every reconcile and unconditionally overwrites its ownerReferences to point at whichever StorageNodeSet is currently reconciling. With more than one StorageNodeSet sharing a namespace, ownership of the single shared SA just flip-flops to whichever reconciled most recently — there is no coordination between them.
2. Deterministic trigger, no race required: add a second StorageNodeSet (e.g. for cluster expansion) — it reconciles almost immediately and becomes the SA's current owner — then delete it. Once its finalizer is removed and the object is actually gone, the garbage collector sees the SA's sole ownerReferences entry pointing at a UID that no longer exists and deletes the SA. It has no way to know any other StorageNodeSet still depends on it, since only the deleted one was ever recorded as owner.
3. Separately confirmed the underlying GC mechanism is close to instantaneous, live on a real cluster, with a disposable ConfigMap/ServiceAccount pair carrying the same ownerReference shape: recreating the parent object under the same name still got the orphaned child deleted by GC in well under a second — even patching the child's ownerReferences to the new UID in the very next command was already too late. ownerReferences match by UID, not name, so GC doesn't care that an object with the same name exists again.
4. Blast radius: the control-plane→storage-node HTTP channel (webappapi/tasks-runner calling the storage-node's Flask API) is TLS-authenticated and stays reachable — this SA/token is unrelated to it. What breaks is the storage-node pod's own outbound calls to the Kubernetes API server (simplyblock_web/api/internal/storage_node/kubernetes.py in sbcli, using the pod's own SA token): creating/deleting the SPDK device-prep Jobs, creating/deleting the spdk-proxy pod, and polling Node cordon/MachineConfigPool status. Once the SA token is invalid those calls return 401, so every request that tries to actually start, stop, or health-check a node's SPDK backend fails at that point — effectively freezing node lifecycle operations across the whole cluster, since every storage-node pod shares this one SA.

Fix

simplyblock-storage-node-sa-<StorageNodeSet name> and simplyblock-storage-node-binding-<namespace>-<StorageNodeSet name> — one ServiceAccount and one ClusterRoleBinding per StorageNodeSet, each owned 1:1 by the StorageNodeSet that created it, so the ownerReference is correct by construction: deleting one StorageNodeSet can only ever garbage-collect its own SA, never another's. simplyblock-storage-node-role (the ClusterRole) stays shared and cluster-scoped with no owner, since its Rules are static regardless of who reconciles it.

The ClusterRoleBinding deliberately carries no ownerReference either, even though it's now 1:1 per StorageNodeSet: it's cluster-scoped, and Kubernetes documents that a cluster-scoped dependent naming a namespaced kind as owner is unresolvable by the garbage collector — it would just be dead weight, not real cleanup.

Upgrade compatibility

Since this ships in the next release rather than the one currently being cut, upgrading clusters will already have the pre-fix shared simplyblock-storage-node-sa in place, most commonly owned by exactly one StorageNodeSet (the common case: one StorageNodeSet per namespace). Renaming it unconditionally would change every existing StorageNodeSet's DaemonSet pod template, forcing a rolling restart of every already-running storage node on upgrade — for a fix about avoiding disruption, that's the wrong trade.

reconcileRBAC now resolves, per StorageNodeSet, whether to keep the legacy shared names or move to its own:

• If the legacy simplyblock-storage-node-sa already exists and its ownerReference already names this StorageNodeSet, it keeps the legacy names — its DaemonSet's pod template, and thus its already-running pods, are untouched by the upgrade.
• Otherwise (no legacy SA yet, or it's owned by a different StorageNodeSet) it gets its own per-StorageNodeSet ServiceAccount and ClusterRoleBinding.

Since the legacy design's ownerReference always names exactly one StorageNodeSet at any moment, this deterministically grandfathers exactly the one that already owned it — zero restarts for the common single-StorageNodeSet-per-namespace case. Any other pre-existing StorageNodeSet sharing a namespace (the multi-StorageNodeSet scenario the original bug required) gets split off onto its own correctly-owned ServiceAccount, which does mean one rolling restart to actually fix it — an acceptable, minimal cost for the rare case that was exposed to the bug in the first place. Any brand-new StorageNodeSet created after this ships (fresh namespace, no legacy SA) gets the clean per-instance naming immediately, no legacy baggage.

Test plan

• go build ./... passes
• make test passes (full suite, including envtest-backed controller tests)
• golangci-lint run ./internal/utils/... ./internal/controller/... --new-from-rev=origin/main — 0 new issues
• Unit test simulating the upgrade scenario: a legacy ServiceAccount pre-seeded with an ownerReference to one existing StorageNodeSet, reconciled alongside a second StorageNodeSet in the same namespace — confirms the owner keeps the legacy names (DaemonSet ServiceAccountName unchanged) and the other gets its own
• RBAC unit tests updated for per-StorageNodeSet ServiceAccount/ClusterRoleBinding naming and ownership
• Live-verified the core cascade-delete fix (two StorageNodeSets in the same namespace, delete one, confirm the other's ServiceAccount and running pods are unaffected) against a real cluster in an earlier iteration of this design (per-StorageNodeSet naming for all, no grandfathering) — the upgrade-compatibility layer added here doesn't change that underlying mechanism
• Live end-to-end verification of the upgrade/grandfathering path specifically (seed a cluster with the pre-fix shared SA, upgrade the operator, confirm the owning StorageNodeSet's pods are not restarted and a second StorageNodeSet in the same namespace gets split onto its own SA) — not yet verified live

🤖 Generated with Claude Code