diff --git a/atlas-lib/kube/names.go b/atlas-lib/kube/names.go index 3222b8bb7..f5e81dc92 100644 --- a/atlas-lib/kube/names.go +++ b/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 diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller.go b/operator/internal/controller/simplyblockstoragenodeset_controller.go index 5e6e9df1c..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 @@ -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,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.Namespace) + crb := utils.BuildStorageNodeSetClusterRoleBinding(snCR, crbName, saName) desiredCRBSubjects := crb.Subjects desiredCRBRoleRef := crb.RoleRef if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, crb, func() error { @@ -1129,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 59a7637eb..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) } @@ -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) } diff --git a/operator/internal/utils/storage_nodeset_ds.go b/operator/internal/utils/storage_nodeset_ds.go index 67d57c26d..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: "simplyblock-storage-node-sa", + ServiceAccountName: serviceAccountName, HostNetwork: true, Tolerations: sn.Spec.Tolerations, NodeSelector: map[string]string{ @@ -403,15 +403,20 @@ func buildStorageNodeSetTLSVolume(tlsProvider string) corev1.Volume { } } -func BuildStorageNodeSetServiceAccount(namespace string) *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: "simplyblock-storage-node-sa", - Namespace: namespace, + Name: name, + Namespace: sn.Namespace, }, } } @@ -607,20 +612,25 @@ func NodeHostnameLabel(nodeName string) string { return label } -func BuildStorageNodeSetClusterRoleBinding(namespace string) *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: fmt.Sprintf("simplyblock-storage-node-binding-%s", namespace), + Name: name, }, Subjects: []rbacv1.Subject{ { Kind: "ServiceAccount", - Name: "simplyblock-storage-node-sa", - Namespace: namespace, + Name: serviceAccountName, + 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..8f8b3303a 100644 --- a/operator/internal/utils/storage_nodeset_ds_test.go +++ b/operator/internal/utils/storage_nodeset_ds_test.go @@ -18,18 +18,15 @@ func TestStorageNodeSetAPIAddress(t *testing.T) { } } -func TestBuildStorageNodeSetClusterRoleBindingNameIncludesNamespace(t *testing.T) { - cluster1Binding := BuildStorageNodeSetClusterRoleBinding("cluster1") - cluster2Binding := BuildStorageNodeSetClusterRoleBinding("cluster2") +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-namespace 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" { - 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 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) } } @@ -101,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]