From c9bff0180715f616b3121544f93b621cb3a13838 Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Mon, 24 Aug 2026 17:49:22 +0200 Subject: [PATCH 1/2] fix(storagenodeset): give each StorageNodeSet its own ServiceAccount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The storage-node ServiceAccount, ClusterRole, and ClusterRoleBinding were shared by every StorageNodeSet in a namespace, but reconcileRBAC gave the ServiceAccount a controller ownerReference to whichever StorageNodeSet reconciled it last. Deleting that one StorageNodeSet — including as an ordinary part of cluster expansion, adding a StorageNodeSet and then removing it — let the garbage collector cascade-delete the ServiceAccount out from under every other StorageNodeSet's already-running pods. Their mounted tokens are bound to the deleted ServiceAccount's UID, which the API server rejects (401) until kubelet's next token refresh (default ~48min). Scope the ServiceAccount and ClusterRoleBinding one-to-one per StorageNodeSet instead of sharing them, so each carries a correct ownerReference and deleting one StorageNodeSet can never affect another's. The ClusterRole stays shared and unowned, since its Rules are the same for every StorageNodeSet regardless of who reconciles it. Co-Authored-By: Claude Sonnet 5 --- atlas-lib/kube/names.go | 16 +++++++++++++ .../simplyblockstoragenodeset_controller.go | 12 ++++++++-- ...lockstoragenodeset_controller_unit_test.go | 12 +++++----- operator/internal/utils/storage_nodeset_ds.go | 23 ++++++++++++------- .../internal/utils/storage_nodeset_ds_test.go | 15 ++++++++---- 5 files changed, 57 insertions(+), 21 deletions(-) diff --git a/atlas-lib/kube/names.go b/atlas-lib/kube/names.go index 3222b8bb7..a84a20839 100644 --- a/atlas-lib/kube/names.go +++ b/atlas-lib/kube/names.go @@ -59,6 +59,22 @@ 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 +} + // 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 diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller.go b/operator/internal/controller/simplyblockstoragenodeset_controller.go index 5e6e9df1c..e1815f793 100644 --- a/operator/internal/controller/simplyblockstoragenodeset_controller.go +++ b/operator/internal/controller/simplyblockstoragenodeset_controller.go @@ -1099,8 +1099,16 @@ func (r *StorageNodeSetReconciler) reconcileWorkerNodes( // reconcileRBAC ensures the ServiceAccount, ClusterRole, and ClusterRoleBinding // required by the storage-node DaemonSet are present and up to date. +// +// 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) error { - sa := utils.BuildStorageNodeSetServiceAccount(snCR.Namespace) + sa := utils.BuildStorageNodeSetServiceAccount(snCR) if err := controllerutil.SetControllerReference(snCR, sa, r.Scheme); err != nil { return fmt.Errorf("failed to set ServiceAccount owner reference: %w", err) } @@ -1121,7 +1129,7 @@ func (r *StorageNodeSetReconciler) reconcileRBAC(ctx context.Context, snCR *simp return fmt.Errorf("failed to apply ClusterRole: %w", err) } - crb := utils.BuildStorageNodeSetClusterRoleBinding(snCR.Namespace) + crb := utils.BuildStorageNodeSetClusterRoleBinding(snCR) desiredCRBSubjects := crb.Subjects desiredCRBRoleRef := crb.RoleRef if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, crb, func() error { diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go b/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go index 59a7637eb..27a3b8135 100644 --- a/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go +++ b/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go @@ -900,7 +900,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 +911,7 @@ func TestStorageNodeSetReconcileServiceAccountHasOwnerReference(t *testing.T) { } } -func TestStorageNodeSetReconcileCreatesNamespaceSpecificClusterRoleBindings(t *testing.T) { +func TestStorageNodeSetReconcileCreatesPerStorageNodeSetClusterRoleBindings(t *testing.T) { const clusterUUID1 = "cluster-uuid-one" const clusterUUID2 = "cluster-uuid-two" @@ -949,14 +949,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) } } } diff --git a/operator/internal/utils/storage_nodeset_ds.go b/operator/internal/utils/storage_nodeset_ds.go index 67d57c26d..1af39c996 100644 --- a/operator/internal/utils/storage_nodeset_ds.go +++ b/operator/internal/utils/storage_nodeset_ds.go @@ -289,7 +289,7 @@ fi` Annotations: podAnnotations, }, Spec: corev1.PodSpec{ - ServiceAccountName: "simplyblock-storage-node-sa", + ServiceAccountName: kube.StorageNodeSetServiceAccountName(sn.Name), HostNetwork: true, Tolerations: sn.Spec.Tolerations, NodeSelector: map[string]string{ @@ -403,15 +403,19 @@ func buildStorageNodeSetTLSVolume(tlsProvider string) corev1.Volume { } } -func BuildStorageNodeSetServiceAccount(namespace string) *corev1.ServiceAccount { +// BuildStorageNodeSetServiceAccount returns the ServiceAccount owned by sn and +// mounted by its storage-node DaemonSet pods. It is named and owned +// per-StorageNodeSet, not shared, so deleting one StorageNodeSet can never +// garbage-collect another's ServiceAccount out from under its running pods. +func BuildStorageNodeSetServiceAccount(sn *simplyblockv1alpha1.StorageNodeSet) *corev1.ServiceAccount { return &corev1.ServiceAccount{ TypeMeta: metav1.TypeMeta{ Kind: "ServiceAccount", APIVersion: "v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: "simplyblock-storage-node-sa", - Namespace: namespace, + Name: kube.StorageNodeSetServiceAccountName(sn.Name), + Namespace: sn.Namespace, }, } } @@ -607,20 +611,23 @@ func NodeHostnameLabel(nodeName string) string { return label } -func BuildStorageNodeSetClusterRoleBinding(namespace string) *rbacv1.ClusterRoleBinding { +// BuildStorageNodeSetClusterRoleBinding returns the ClusterRoleBinding that +// grants sn's own ServiceAccount the shared ClusterRole. Named per- +// StorageNodeSet to match its per-StorageNodeSet ServiceAccount. +func BuildStorageNodeSetClusterRoleBinding(sn *simplyblockv1alpha1.StorageNodeSet) *rbacv1.ClusterRoleBinding { return &rbacv1.ClusterRoleBinding{ TypeMeta: metav1.TypeMeta{ Kind: "ClusterRoleBinding", APIVersion: "rbac.authorization.k8s.io/v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("simplyblock-storage-node-binding-%s", namespace), + Name: kube.StorageNodeSetClusterRoleBindingName(sn.Namespace, sn.Name), }, Subjects: []rbacv1.Subject{ { Kind: "ServiceAccount", - Name: "simplyblock-storage-node-sa", - Namespace: namespace, + Name: kube.StorageNodeSetServiceAccountName(sn.Name), + Namespace: sn.Namespace, }, }, RoleRef: rbacv1.RoleRef{ diff --git a/operator/internal/utils/storage_nodeset_ds_test.go b/operator/internal/utils/storage_nodeset_ds_test.go index 1447053f3..81f49acc0 100644 --- a/operator/internal/utils/storage_nodeset_ds_test.go +++ b/operator/internal/utils/storage_nodeset_ds_test.go @@ -18,19 +18,24 @@ func TestStorageNodeSetAPIAddress(t *testing.T) { } } -func TestBuildStorageNodeSetClusterRoleBindingNameIncludesNamespace(t *testing.T) { - cluster1Binding := BuildStorageNodeSetClusterRoleBinding("cluster1") - cluster2Binding := BuildStorageNodeSetClusterRoleBinding("cluster2") +func TestBuildStorageNodeSetClusterRoleBindingNameIncludesNamespaceAndName(t *testing.T) { + sn1 := &simplyblockv1alpha1.StorageNodeSet{ObjectMeta: metav1.ObjectMeta{Name: "sn-a", Namespace: "cluster1"}} + sn2 := &simplyblockv1alpha1.StorageNodeSet{ObjectMeta: metav1.ObjectMeta{Name: "sn-b", Namespace: "cluster2"}} + cluster1Binding := BuildStorageNodeSetClusterRoleBinding(sn1) + cluster2Binding := BuildStorageNodeSetClusterRoleBinding(sn2) if cluster1Binding.Name == cluster2Binding.Name { - t.Fatalf("expected per-namespace ClusterRoleBinding names, got %q", cluster1Binding.Name) + t.Fatalf("expected per-StorageNodeSet ClusterRoleBinding names, got %q", cluster1Binding.Name) } - if cluster1Binding.Name != "simplyblock-storage-node-binding-cluster1" { + if cluster1Binding.Name != "simplyblock-storage-node-binding-cluster1-sn-a" { t.Fatalf("unexpected cluster1 ClusterRoleBinding name %q", cluster1Binding.Name) } if len(cluster1Binding.Subjects) != 1 || cluster1Binding.Subjects[0].Namespace != "cluster1" { t.Fatalf("expected cluster1 service account subject, got %#v", cluster1Binding.Subjects) } + if cluster1Binding.Subjects[0].Name != "simplyblock-storage-node-sa-sn-a" { + t.Fatalf("expected per-StorageNodeSet service account subject, got %#v", cluster1Binding.Subjects) + } } func TestBuildSpdkProxyEndpointSlice_DottedNodeNameTruncates(t *testing.T) { From aae9066c6cf539d40ed7d6f26d44c41a2125fb09 Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Tue, 25 Aug 2026 13:09:57 +0200 Subject: [PATCH 2/2] fix(storagenodeset): grandfather the legacy shared ServiceAccount name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fix ships in the next release, not the one currently being cut, so existing StorageNodeSets already running under the shared-ServiceAccount design (already deployed with the current release) must not have their ServiceAccount name change — that would change the DaemonSet pod template and force an unwanted rolling restart of already-running storage nodes on upgrade. reconcileRBAC now resolves, per StorageNodeSet, whether to keep the legacy shared names or use the new per-StorageNodeSet ones: if a legacy ServiceAccount already exists and its ownerReference already names this StorageNodeSet, it keeps the legacy names untouched; otherwise it gets its own per-StorageNodeSet ServiceAccount and ClusterRoleBinding. Since the legacy design's ownerReference already names exactly one StorageNodeSet at any moment, this deterministically grandfathers exactly the one that already owned it, with zero disruption for the common single-StorageNodeSet- per-namespace case, while any other pre-existing StorageNodeSet in a multi-StorageNodeSet namespace (the scenario the original bug required) is split off onto its own correctly-owned ServiceAccount. Co-Authored-By: Claude Sonnet 5 --- atlas-lib/kube/names.go | 14 +++ .../simplyblockstoragenodeset_controller.go | 71 +++++++++-- ...lockstoragenodeset_controller_unit_test.go | 115 ++++++++++++++++-- operator/internal/utils/storage_nodeset_ds.go | 31 ++--- .../internal/utils/storage_nodeset_ds_test.go | 24 ++-- 5 files changed, 205 insertions(+), 50 deletions(-) diff --git a/atlas-lib/kube/names.go b/atlas-lib/kube/names.go index a84a20839..f5e81dc92 100644 --- a/atlas-lib/kube/names.go +++ b/atlas-lib/kube/names.go @@ -75,6 +75,20 @@ func StorageNodeSetClusterRoleBindingName(namespace, storageNodeSetName 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 diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller.go b/operator/internal/controller/simplyblockstoragenodeset_controller.go index e1815f793..41473bdca 100644 --- a/operator/internal/controller/simplyblockstoragenodeset_controller.go +++ b/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 @@ -1107,17 +1109,22 @@ func (r *StorageNodeSetReconciler) reconcileWorkerNodes( // 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) error { - sa := utils.BuildStorageNodeSetServiceAccount(snCR) +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)) @@ -1126,10 +1133,10 @@ 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) + crb := utils.BuildStorageNodeSetClusterRoleBinding(snCR, crbName, saName) desiredCRBSubjects := crb.Subjects desiredCRBRoleRef := crb.RoleRef if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, crb, func() error { @@ -1137,9 +1144,51 @@ func (r *StorageNodeSetReconciler) reconcileRBAC(ctx context.Context, snCR *simp 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) diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go b/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go index 27a3b8135..8fada0c4a 100644 --- a/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go +++ b/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) } @@ -911,6 +913,101 @@ func TestStorageNodeSetReconcileServiceAccountHasOwnerReference(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" @@ -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) } diff --git a/operator/internal/utils/storage_nodeset_ds.go b/operator/internal/utils/storage_nodeset_ds.go index 1af39c996..7bc65a476 100644 --- a/operator/internal/utils/storage_nodeset_ds.go +++ b/operator/internal/utils/storage_nodeset_ds.go @@ -45,7 +45,7 @@ var defaultContainerResources = corev1.ResourceRequirements{ }, } -func BuildStorageNodeSetDaemonSet(sn *simplyblockv1alpha1.StorageNodeSet, tlsEnabled bool, tlsMutualEnabled bool, tlsProvider, tlsSecretResourceVersion string) *appsv1.DaemonSet { +func BuildStorageNodeSetDaemonSet(sn *simplyblockv1alpha1.StorageNodeSet, serviceAccountName string, tlsEnabled bool, tlsMutualEnabled bool, tlsProvider, tlsSecretResourceVersion string) *appsv1.DaemonSet { labels := map[string]string{ kube.LabelApp: kube.AppStorageNode, @@ -289,7 +289,7 @@ fi` Annotations: podAnnotations, }, Spec: corev1.PodSpec{ - ServiceAccountName: kube.StorageNodeSetServiceAccountName(sn.Name), + ServiceAccountName: serviceAccountName, HostNetwork: true, Tolerations: sn.Spec.Tolerations, NodeSelector: map[string]string{ @@ -403,18 +403,19 @@ func buildStorageNodeSetTLSVolume(tlsProvider string) corev1.Volume { } } -// BuildStorageNodeSetServiceAccount returns the ServiceAccount owned by sn and -// mounted by its storage-node DaemonSet pods. It is named and owned -// per-StorageNodeSet, not shared, so deleting one StorageNodeSet can never -// garbage-collect another's ServiceAccount out from under its running pods. -func BuildStorageNodeSetServiceAccount(sn *simplyblockv1alpha1.StorageNodeSet) *corev1.ServiceAccount { +// BuildStorageNodeSetServiceAccount returns the ServiceAccount mounted by +// sn's storage-node DaemonSet pods, under the given name. The caller resolves +// name to either the per-StorageNodeSet name or, for whichever StorageNodeSet +// already owned it pre-migration, the legacy shared name — see +// StorageNodeSetReconciler.resolveStorageNodeSetRBACNames. +func BuildStorageNodeSetServiceAccount(sn *simplyblockv1alpha1.StorageNodeSet, name string) *corev1.ServiceAccount { return &corev1.ServiceAccount{ TypeMeta: metav1.TypeMeta{ Kind: "ServiceAccount", APIVersion: "v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: kube.StorageNodeSetServiceAccountName(sn.Name), + Name: name, Namespace: sn.Namespace, }, } @@ -611,22 +612,24 @@ func NodeHostnameLabel(nodeName string) string { return label } -// BuildStorageNodeSetClusterRoleBinding returns the ClusterRoleBinding that -// grants sn's own ServiceAccount the shared ClusterRole. Named per- -// StorageNodeSet to match its per-StorageNodeSet ServiceAccount. -func BuildStorageNodeSetClusterRoleBinding(sn *simplyblockv1alpha1.StorageNodeSet) *rbacv1.ClusterRoleBinding { +// BuildStorageNodeSetClusterRoleBinding returns the ClusterRoleBinding named +// name that grants serviceAccountName the shared ClusterRole. The caller +// resolves both to either the per-StorageNodeSet names or, for whichever +// StorageNodeSet already owned them pre-migration, the legacy shared names — +// see StorageNodeSetReconciler.resolveStorageNodeSetRBACNames. +func BuildStorageNodeSetClusterRoleBinding(sn *simplyblockv1alpha1.StorageNodeSet, name, serviceAccountName string) *rbacv1.ClusterRoleBinding { return &rbacv1.ClusterRoleBinding{ TypeMeta: metav1.TypeMeta{ Kind: "ClusterRoleBinding", APIVersion: "rbac.authorization.k8s.io/v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: kube.StorageNodeSetClusterRoleBindingName(sn.Namespace, sn.Name), + Name: name, }, Subjects: []rbacv1.Subject{ { Kind: "ServiceAccount", - Name: kube.StorageNodeSetServiceAccountName(sn.Name), + Name: serviceAccountName, Namespace: sn.Namespace, }, }, diff --git a/operator/internal/utils/storage_nodeset_ds_test.go b/operator/internal/utils/storage_nodeset_ds_test.go index 81f49acc0..8f8b3303a 100644 --- a/operator/internal/utils/storage_nodeset_ds_test.go +++ b/operator/internal/utils/storage_nodeset_ds_test.go @@ -18,23 +18,15 @@ func TestStorageNodeSetAPIAddress(t *testing.T) { } } -func TestBuildStorageNodeSetClusterRoleBindingNameIncludesNamespaceAndName(t *testing.T) { - sn1 := &simplyblockv1alpha1.StorageNodeSet{ObjectMeta: metav1.ObjectMeta{Name: "sn-a", Namespace: "cluster1"}} - sn2 := &simplyblockv1alpha1.StorageNodeSet{ObjectMeta: metav1.ObjectMeta{Name: "sn-b", Namespace: "cluster2"}} - cluster1Binding := BuildStorageNodeSetClusterRoleBinding(sn1) - cluster2Binding := BuildStorageNodeSetClusterRoleBinding(sn2) +func TestBuildStorageNodeSetClusterRoleBindingUsesGivenNames(t *testing.T) { + sn := &simplyblockv1alpha1.StorageNodeSet{ObjectMeta: metav1.ObjectMeta{Name: "sn-a", Namespace: "cluster1"}} + binding := BuildStorageNodeSetClusterRoleBinding(sn, "binding-name", "sa-name") - if cluster1Binding.Name == cluster2Binding.Name { - t.Fatalf("expected per-StorageNodeSet ClusterRoleBinding names, got %q", cluster1Binding.Name) + if binding.Name != "binding-name" { + t.Fatalf("expected ClusterRoleBinding name %q, got %q", "binding-name", binding.Name) } - if cluster1Binding.Name != "simplyblock-storage-node-binding-cluster1-sn-a" { - t.Fatalf("unexpected cluster1 ClusterRoleBinding name %q", cluster1Binding.Name) - } - if len(cluster1Binding.Subjects) != 1 || cluster1Binding.Subjects[0].Namespace != "cluster1" { - t.Fatalf("expected cluster1 service account subject, got %#v", cluster1Binding.Subjects) - } - if cluster1Binding.Subjects[0].Name != "simplyblock-storage-node-sa-sn-a" { - t.Fatalf("expected per-StorageNodeSet service account subject, got %#v", cluster1Binding.Subjects) + if len(binding.Subjects) != 1 || binding.Subjects[0].Name != "sa-name" || binding.Subjects[0].Namespace != "cluster1" { + t.Fatalf("expected subject sa-name in namespace cluster1, got %#v", binding.Subjects) } } @@ -106,7 +98,7 @@ func TestBuildStorageNodeSetDaemonSetUserResourcesOverrideDefaults(t *testing.T) }, } - ds := BuildStorageNodeSetDaemonSet(sn, false, false, "", "") + ds := BuildStorageNodeSetDaemonSet(sn, "sa-name", false, false, "", "") main := ds.Spec.Template.Spec.Containers[0] mainMem := main.Resources.Limits[corev1.ResourceMemory]