fix(storagenodeset): give each StorageNodeSet its own ServiceAccount

atlas-lib/kube/names.go

@@ -59,6 +59,36 @@ func StorageNodeSetDaemonSetName(storageNodeSetName string) string {
 	return "simplyblock-storage-node-ds-" + storageNodeSetName
 }
 
+// StorageNodeSetServiceAccountName is the name of the ServiceAccount owned by
+// the named StorageNodeSet and mounted by its storage-node DaemonSet pods.
+// Named per-set, not shared, so it can safely carry a controller
+// ownerReference to exactly the StorageNodeSet that owns it.
+func StorageNodeSetServiceAccountName(storageNodeSetName string) string {
+	return "simplyblock-storage-node-sa-" + storageNodeSetName
+}
+
+// StorageNodeSetClusterRoleBindingName is the name of the ClusterRoleBinding
+// that grants the named StorageNodeSet's own ServiceAccount the shared
+// simplyblock-storage-node-role ClusterRole. Includes both namespace and set
+// name since ClusterRoleBindings are cluster-scoped.
+func StorageNodeSetClusterRoleBindingName(namespace, storageNodeSetName string) string {
+	return "simplyblock-storage-node-binding-" + namespace + "-" + storageNodeSetName
+}
+
+// LegacyStorageNodeSetServiceAccountName is the pre-migration ServiceAccount
+// name, shared by every StorageNodeSet in a namespace before each set got its
+// own. Whichever StorageNodeSet already owns it when upgrading keeps this
+// name (and its DaemonSet's pod template, and thus its running pods,
+// untouched); every other StorageNodeSet uses StorageNodeSetServiceAccountName.
+const LegacyStorageNodeSetServiceAccountName = "simplyblock-storage-node-sa"
+
+// LegacyStorageNodeSetClusterRoleBindingName is the pre-migration, per-
+// namespace (not per-set) ClusterRoleBinding name, paired with
+// LegacyStorageNodeSetServiceAccountName.
+func LegacyStorageNodeSetClusterRoleBindingName(namespace string) string {
+	return "simplyblock-storage-node-binding-" + namespace
+}
+
 // StorageClass parameter keys. These are the operator/CSI-controller inputs
 // that describe how to provision a logical volume; the CSI controller reads
 // them at CreateVolume (see PropertiesFromStorageClass, which parses them into

operator/internal/controller/simplyblockstoragenodeset_controller.go

@@ -171,7 +171,8 @@ func (r *StorageNodeSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, err
 	}
 
-	if err := r.reconcileRBAC(ctx, snCR); err != nil {
+	serviceAccountName, err := r.reconcileRBAC(ctx, snCR)
+	if err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -196,7 +197,7 @@ func (r *StorageNodeSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		log.Error(err, "failed to reconcile per-node ConfigMap")
 	}
 
-	if err := r.reconcileDaemonSet(ctx, snCR); err != nil {
+	if err := r.reconcileDaemonSet(ctx, snCR, serviceAccountName); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -589,6 +590,7 @@ func labelWorkerNodes(
 func (r *StorageNodeSetReconciler) reconcileDaemonSet(
 	ctx context.Context,
 	snCR *simplyblockv1alpha1.StorageNodeSet,
+	serviceAccountName string,
 ) error {
 
 	if snCR.Spec.ClusterImage == "" {
@@ -608,7 +610,7 @@ func (r *StorageNodeSetReconciler) reconcileDaemonSet(
 		return err
 	}
 
-	ds := utils.BuildStorageNodeSetDaemonSet(snCR, r.TLSEnabled, r.TLSMutualEnabled, r.TLSProvider, tlsSecretRV)
+	ds := utils.BuildStorageNodeSetDaemonSet(snCR, serviceAccountName, r.TLSEnabled, r.TLSMutualEnabled, r.TLSProvider, tlsSecretRV)
 
 	if err := controllerutil.SetControllerReference(snCR, ds, r.Scheme); err != nil {
 		return err
@@ -1099,17 +1101,30 @@ func (r *StorageNodeSetReconciler) reconcileWorkerNodes(
 
 // reconcileRBAC ensures the ServiceAccount, ClusterRole, and ClusterRoleBinding
 // required by the storage-node DaemonSet are present and up to date.
-func (r *StorageNodeSetReconciler) reconcileRBAC(ctx context.Context, snCR *simplyblockv1alpha1.StorageNodeSet) error {
-	sa := utils.BuildStorageNodeSetServiceAccount(snCR.Namespace)
+//
+// The ServiceAccount is named and owned per-StorageNodeSet (rather than shared
+// across the namespace), so its ownerReference is always 1:1 and deleting one
+// StorageNodeSet can never cascade-delete another's ServiceAccount out from
+// under its running pods. The ClusterRoleBinding is per-StorageNodeSet too,
+// for the same reason; it stays unowned since it's cluster-scoped and a
+// namespaced owner reference wouldn't be garbage-collected. The ClusterRole
+// itself stays shared with no owner, since its Rules are static.
+func (r *StorageNodeSetReconciler) reconcileRBAC(ctx context.Context, snCR *simplyblockv1alpha1.StorageNodeSet) (string, error) {
+	saName, crbName, err := r.resolveStorageNodeSetRBACNames(ctx, snCR)
+	if err != nil {
+		return "", fmt.Errorf("failed to resolve RBAC object names: %w", err)
+	}
+
+	sa := utils.BuildStorageNodeSetServiceAccount(snCR, saName)
 	if err := controllerutil.SetControllerReference(snCR, sa, r.Scheme); err != nil {
-		return fmt.Errorf("failed to set ServiceAccount owner reference: %w", err)
+		return "", fmt.Errorf("failed to set ServiceAccount owner reference: %w", err)
 	}
 	desiredSAOwnerRefs := sa.OwnerReferences
 	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, sa, func() error {
 		sa.OwnerReferences = desiredSAOwnerRefs
 		return nil
 	}); err != nil {
-		return fmt.Errorf("failed to apply ServiceAccount: %w", err)
+		return "", fmt.Errorf("failed to apply ServiceAccount: %w", err)
 	}
 
 	cr := utils.BuildStorageNodeSetClusterRole(ptr.BoolFromOrFalse(snCR.Spec.OpenShiftCluster))
@@ -1118,20 +1133,62 @@ func (r *StorageNodeSetReconciler) reconcileRBAC(ctx context.Context, snCR *simp
 		cr.Rules = desiredCRRules
 		return nil
 	}); err != nil {
-		return fmt.Errorf("failed to apply ClusterRole: %w", err)
+		return "", fmt.Errorf("failed to apply ClusterRole: %w", err)
 	}
 
-	crb := utils.BuildStorageNodeSetClusterRoleBinding(snCR.Namespace)
+	crb := utils.BuildStorageNodeSetClusterRoleBinding(snCR, crbName, saName)
 	desiredCRBSubjects := crb.Subjects
 	desiredCRBRoleRef := crb.RoleRef
 	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, crb, func() error {
 		crb.Subjects = desiredCRBSubjects
 		crb.RoleRef = desiredCRBRoleRef
 		return nil
 	}); err != nil {
-		return fmt.Errorf("failed to apply ClusterRoleBinding: %w", err)
+		return "", fmt.Errorf("failed to apply ClusterRoleBinding: %w", err)
 	}
-	return nil
+	return saName, nil
+}
+
+// resolveStorageNodeSetRBACNames decides whether snCR keeps the legacy
+// ServiceAccount/ClusterRoleBinding names it shared with every other
+// StorageNodeSet in the namespace pre-migration, or gets its own
+// per-StorageNodeSet names.
+//
+// Before this fix, every StorageNodeSet in a namespace shared one
+// ServiceAccount, and reconcileRBAC unconditionally pointed its
+// ownerReference at whichever StorageNodeSet reconciled last — the bug this
+// fix addresses. On upgrade, that legacy ServiceAccount's ownerReference
+// still names exactly one already-existing StorageNodeSet: that one keeps
+// the legacy names, so its DaemonSet's pod template — and thus its already
+// running pods — is untouched by this upgrade. Every other StorageNodeSet
+// (including any created after this fix ships) gets its own names.
+func (r *StorageNodeSetReconciler) resolveStorageNodeSetRBACNames(
+	ctx context.Context,
+	snCR *simplyblockv1alpha1.StorageNodeSet,
+) (saName, crbName string, err error) {
+	perSetSAName := kube.StorageNodeSetServiceAccountName(snCR.Name)
+	perSetCRBName := kube.StorageNodeSetClusterRoleBindingName(snCR.Namespace, snCR.Name)
+
+	legacySA := &corev1.ServiceAccount{}
+	err = r.Get(ctx, client.ObjectKey{
+		Name:      kube.LegacyStorageNodeSetServiceAccountName,
+		Namespace: snCR.Namespace,
+	}, legacySA)
+	if apierrors.IsNotFound(err) {
+		return perSetSAName, perSetCRBName, nil
+	}
+	if err != nil {
+		return "", "", err
+	}
+
+	for _, ref := range legacySA.OwnerReferences {
+		if ref.Kind == "StorageNodeSet" && ref.UID == snCR.UID {
+			return kube.LegacyStorageNodeSetServiceAccountName,
+				kube.LegacyStorageNodeSetClusterRoleBindingName(snCR.Namespace), nil
+		}
+	}
+
+	return perSetSAName, perSetCRBName, nil
 }
 
 // fdbWorkerSet returns the set of worker node names (from snCR.Spec.WorkerNodes)

operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go

@@ -10,6 +10,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/simplyblock/atlas/kube"
+
 	simplyblockv1alpha1 "github.com/simplyblock/simplyblock-operator/api/v1alpha1"
 	"github.com/simplyblock/simplyblock-operator/internal/utils"
 	"github.com/simplyblock/simplyblock-operator/internal/webapi"
@@ -343,7 +345,7 @@ func TestStorageNodeSetDaemonSetReconcileCreatesWhenMissing(t *testing.T) {
 	}
 	r := newStorageNodeSetStateTestReconciler(t, sn)
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("reconcileDaemonSet returned error: %v", err)
 	}
 
@@ -373,7 +375,7 @@ func TestStorageNodeSetDaemonSetReconcileUpdatesExisting(t *testing.T) {
 	}
 	r := newStorageNodeSetStateTestReconciler(t, sn, existing)
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("reconcileDaemonSet returned error: %v", err)
 	}
 
@@ -394,7 +396,7 @@ func TestStorageNodeSetDaemonSetReconcileTLSDisabled(t *testing.T) {
 	r := newStorageNodeSetStateTestReconciler(t, sn)
 	r.TLSEnabled = false
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("reconcileDaemonSet returned error: %v", err)
 	}
 
@@ -470,7 +472,7 @@ func TestStorageNodeSetDaemonSetReconcileTLSEnabled(t *testing.T) {
 	r.TLSProvider = utils.TLSProviderOpenShift
 	r.TLSMutualEnabled = true
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("reconcileDaemonSet returned error: %v", err)
 	}
 
@@ -539,7 +541,7 @@ func TestStorageNodeSetDaemonSetReconcileTLSCertManagerProvider(t *testing.T) {
 	r.TLSProvider = utils.TLSProviderCertManager
 	r.TLSMutualEnabled = false
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("reconcileDaemonSet returned error: %v", err)
 	}
 
@@ -900,7 +902,7 @@ func TestStorageNodeSetReconcileServiceAccountHasOwnerReference(t *testing.T) {
 
 	sa := &corev1.ServiceAccount{}
 	if err := r.Get(context.Background(), client.ObjectKey{
-		Name:      "simplyblock-storage-node-sa",
+		Name:      "simplyblock-storage-node-sa-" + sn.Name,
 		Namespace: namespace,
 	}, sa); err != nil {
 		t.Fatalf("failed to fetch serviceaccount: %v", err)
@@ -911,7 +913,102 @@ func TestStorageNodeSetReconcileServiceAccountHasOwnerReference(t *testing.T) {
 	}
 }
 
-func TestStorageNodeSetReconcileCreatesNamespaceSpecificClusterRoleBindings(t *testing.T) {
+// TestStorageNodeSetReconcileGrandfathersLegacyServiceAccountName simulates
+// upgrading from the pre-fix shared-ServiceAccount design: a legacy
+// ServiceAccount already exists, owned by one already-existing
+// StorageNodeSet (as reconcileRBAC always set it, pre-fix). That
+// StorageNodeSet must keep the legacy names — so its DaemonSet's
+// ServiceAccountName, and thus its already-running pods, are untouched by
+// the upgrade — while any other StorageNodeSet gets its own per-instance
+// names.
+func TestStorageNodeSetReconcileGrandfathersLegacyServiceAccountName(t *testing.T) {
+	const namespace = "default"
+	const clusterName = "cluster-legacy-sa"
+	const clusterUUID = "cluster-uuid-legacy-sa"
+
+	cluster := &simplyblockv1alpha1.StorageCluster{
+		ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace},
+		Status:     simplyblockv1alpha1.StorageClusterStatus{UUID: clusterUUID},
+	}
+	existing := &simplyblockv1alpha1.StorageNodeSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "sn-existing",
+			Namespace:  namespace,
+			UID:        "uid-existing",
+			Finalizers: []string{utils.FinalizerStorageNodeSet},
+		},
+		Spec: simplyblockv1alpha1.StorageNodeSetSpec{ClusterName: clusterName, WorkerNodes: []string{}},
+	}
+	newSet := &simplyblockv1alpha1.StorageNodeSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "sn-new",
+			Namespace:  namespace,
+			UID:        "uid-new",
+			Finalizers: []string{utils.FinalizerStorageNodeSet},
+		},
+		Spec: simplyblockv1alpha1.StorageNodeSetSpec{ClusterName: clusterName, WorkerNodes: []string{}},
+	}
+	isController := true
+	legacySA := &corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      kube.LegacyStorageNodeSetServiceAccountName,
+			Namespace: namespace,
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: simplyblockv1alpha1.GroupVersion.String(),
+					Kind:       "StorageNodeSet",
+					Name:       existing.Name,
+					UID:        existing.UID,
+					Controller: &isController,
+				},
+			},
+		},
+	}
+
+	r := newStorageNodeSetStateTestReconciler(t, existing, newSet, cluster, legacySA)
+	for _, sn := range []*simplyblockv1alpha1.StorageNodeSet{existing, newSet} {
+		if _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(sn)}); err != nil {
+			t.Fatalf("reconcile %s returned error: %v", sn.Name, err)
+		}
+	}
+
+	var existingDS appsv1.DaemonSet
+	if err := r.Get(context.Background(), client.ObjectKey{Name: "simplyblock-storage-node-ds-sn-existing", Namespace: namespace}, &existingDS); err != nil {
+		t.Fatalf("failed to fetch existing DaemonSet: %v", err)
+	}
+	if got := existingDS.Spec.Template.Spec.ServiceAccountName; got != kube.LegacyStorageNodeSetServiceAccountName {
+		t.Fatalf("expected grandfathered StorageNodeSet to keep the legacy ServiceAccount name, got %q", got)
+	}
+	if err := r.Get(context.Background(), client.ObjectKey{Name: kube.LegacyStorageNodeSetClusterRoleBindingName(namespace)}, &rbacv1.ClusterRoleBinding{}); err != nil {
+		t.Fatalf("expected legacy ClusterRoleBinding to still exist: %v", err)
+	}
+
+	var newDS appsv1.DaemonSet
+	if err := r.Get(context.Background(), client.ObjectKey{Name: "simplyblock-storage-node-ds-sn-new", Namespace: namespace}, &newDS); err != nil {
+		t.Fatalf("failed to fetch new DaemonSet: %v", err)
+	}
+	if got, want := newDS.Spec.Template.Spec.ServiceAccountName, "simplyblock-storage-node-sa-sn-new"; got != want {
+		t.Fatalf("expected new StorageNodeSet to get its own ServiceAccount name, got %q want %q", got, want)
+	}
+
+	var newSA corev1.ServiceAccount
+	if err := r.Get(context.Background(), client.ObjectKey{Name: "simplyblock-storage-node-sa-sn-new", Namespace: namespace}, &newSA); err != nil {
+		t.Fatalf("expected new StorageNodeSet's own ServiceAccount to exist: %v", err)
+	}
+	if len(newSA.OwnerReferences) != 1 || newSA.OwnerReferences[0].UID != newSet.UID {
+		t.Fatalf("expected new StorageNodeSet's ServiceAccount to be owned by it, got %#v", newSA.OwnerReferences)
+	}
+
+	var refreshedLegacySA corev1.ServiceAccount
+	if err := r.Get(context.Background(), client.ObjectKey{Name: kube.LegacyStorageNodeSetServiceAccountName, Namespace: namespace}, &refreshedLegacySA); err != nil {
+		t.Fatalf("expected legacy ServiceAccount to still exist: %v", err)
+	}
+	if len(refreshedLegacySA.OwnerReferences) != 1 || refreshedLegacySA.OwnerReferences[0].UID != existing.UID {
+		t.Fatalf("expected legacy ServiceAccount ownership to remain with sn-existing, got %#v", refreshedLegacySA.OwnerReferences)
+	}
+}
+
+func TestStorageNodeSetReconcileCreatesPerStorageNodeSetClusterRoleBindings(t *testing.T) {
 	const clusterUUID1 = "cluster-uuid-one"
 	const clusterUUID2 = "cluster-uuid-two"
 
@@ -949,14 +1046,14 @@ func TestStorageNodeSetReconcileCreatesNamespaceSpecificClusterRoleBindings(t *t
 		}
 	}
 
-	for _, namespace := range []string{"cluster1", "cluster2"} {
+	for _, sn := range []*simplyblockv1alpha1.StorageNodeSet{sn1, sn2} {
 		binding := &rbacv1.ClusterRoleBinding{}
-		key := client.ObjectKey{Name: "simplyblock-storage-node-binding-" + namespace}
+		key := client.ObjectKey{Name: "simplyblock-storage-node-binding-" + sn.Namespace + "-" + sn.Name}
 		if err := r.Get(context.Background(), key, binding); err != nil {
 			t.Fatalf("failed to fetch ClusterRoleBinding %s: %v", key.Name, err)
 		}
-		if len(binding.Subjects) != 1 || binding.Subjects[0].Namespace != namespace {
-			t.Fatalf("expected binding %s to target namespace %s, got %#v", key.Name, namespace, binding.Subjects)
+		if len(binding.Subjects) != 1 || binding.Subjects[0].Namespace != sn.Namespace {
+			t.Fatalf("expected binding %s to target namespace %s, got %#v", key.Name, sn.Namespace, binding.Subjects)
 		}
 	}
 }
@@ -2104,7 +2201,7 @@ func TestStorageNodeSetDaemonSetTLSSecretRevisionAnnotation(t *testing.T) {
 			r.TLSEnabled = tc.tlsEnabled
 			r.TLSProvider = utils.TLSProviderCertManager
 
-			if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+			if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 				t.Fatalf("reconcileDaemonSet returned error: %v", err)
 			}
 
@@ -2143,7 +2240,7 @@ func TestStorageNodeSetDaemonSetReconcileRollsOnTLSSecretRevisionChange(t *testi
 	r.TLSEnabled = true
 	r.TLSProvider = utils.TLSProviderCertManager
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("first reconcileDaemonSet: %v", err)
 	}
 
@@ -2163,7 +2260,7 @@ func TestStorageNodeSetDaemonSetReconcileRollsOnTLSSecretRevisionChange(t *testi
 		t.Fatalf("rotate secret: %v", err)
 	}
 
-	if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+	if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 		t.Fatalf("second reconcileDaemonSet: %v", err)
 	}
 
@@ -2227,7 +2324,7 @@ func TestStorageNodeSetDaemonSetSBTLSServeEnv(t *testing.T) {
 			r.TLSEnabled = tc.tlsEnabled
 			r.TLSProvider = tc.tlsProvider
 
-			if err := r.reconcileDaemonSet(context.Background(), sn); err != nil {
+			if err := r.reconcileDaemonSet(context.Background(), sn, "sa-name"); err != nil {
 				t.Fatalf("reconcileDaemonSet returned error: %v", err)
 			}