provider: avoid CSI secret name collisions

Previously, when fetching a storage claim config, a remote consumer was given information for creating the Ceph CSI Secrets that was identical to the secret names on the provider side. Running a consumer in the same namespace as the provider introduced namespace collisions. Hilariously, by virtue of being the same information, things just kinda worked out anyway. Still, this is likely going to produce massive problems later, so better to fix it now. Signed-off-by: Jose A. Rivera <[email protected]>
red-hat-storage · Mar 29, 2024 · 77ce704 · 77ce704
1 parent 89a9b71
commit 77ce704
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 85 deletions.
diff --git a/services/provider/server/server.go b/services/provider/server/server.go
@@ -393,6 +393,10 @@ func (s *OCSProviderServer) getCephClientInformation(ctx context.Context, name s
 	return cephClient.Status.Info["secretName"], cephClient.Annotations[controllers.StorageCephUserTypeAnnotation], nil
 }
 
+func scrCephClientCsiSecretName(secretType, suffix string) string {
+	return fmt.Sprintf("ceph-client-%s-%s", secretType, suffix)
+}
+
 func (s *OCSProviderServer) getOnboardingValidationKey(ctx context.Context) (*rsa.PublicKey, error) {
 	pubKeySecret := &corev1.Secret{}
 	err := s.client.Get(ctx, types.NamespacedName{Name: onboardingTicketKeySecret, Namespace: s.namespace}, pubKeySecret)
@@ -553,13 +557,18 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 	var storageClassName string
 	storageClassData := map[string]string{}
 
+	storageClassRequestHash := strings.TrimPrefix(storageClassRequest.Name, "storageclassrequest-")
+	nodeSecretName := scrCephClientCsiSecretName("node", storageClassRequestHash)
+	provisionerSecretName := scrCephClientCsiSecretName("provisioner", storageClassRequestHash)
+
 	for _, cephRes := range storageClassRequest.Status.CephResources {
 		switch cephRes.Kind {
 		case "CephClient":
-			clientSecretName, _, err := s.getCephClientInformation(ctx, cephRes.Name)
+			clientSecretName, clientUserType, err := s.getCephClientInformation(ctx, cephRes.Name)
 			if err != nil {
 				return nil, status.Error(codes.Internal, err.Error())
 			}
+			extSecretName := scrCephClientCsiSecretName(clientUserType, storageClassRequestHash)
 
 			cephUserSecret := &v1.Secret{}
 			err = s.client.Get(ctx, types.NamespacedName{Name: clientSecretName, Namespace: s.namespace}, cephUserSecret)
@@ -574,10 +583,8 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 				keyProp = "adminKey"
 			}
 			extR = append(extR, &pb.ExternalResource{
-				// a common suffix '.csi' is being added to distinguish secrets that are created
-				// by ocs-client-operator vs rook-operator when both these operators are deployed in same namespace
 				// TODO: need to transform existing secrets during migration manually
-				Name: clientSecretName + ".csi",
+				Name: extSecretName,
 				Kind: "Secret",
 				Data: mustMarshal(map[string]string{
 					idProp:  cephRes.Name,
@@ -590,16 +597,6 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 			storageClassData["clusterID"] = s.namespace
 			storageClassData["radosnamespace"] = cephRes.Name
 
-			nodeCephClientSecret, _, err := s.getCephClientInformation(ctx, cephRes.CephClients["node"])
-			if err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-
-			provisionerCephClientSecret, _, err := s.getCephClientInformation(ctx, cephRes.CephClients["provisioner"])
-			if err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-
 			rns := &rookCephv1.CephBlockPoolRadosNamespace{}
 			err = s.client.Get(ctx, types.NamespacedName{Name: cephRes.Name, Namespace: s.namespace}, rns)
 			if err != nil {
@@ -610,9 +607,9 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 			storageClassData["imageFeatures"] = "layering,deep-flatten,exclusive-lock,object-map,fast-diff"
 			storageClassData["csi.storage.k8s.io/fstype"] = "ext4"
 			storageClassData["imageFormat"] = "2"
-			storageClassData["csi.storage.k8s.io/provisioner-secret-name"] = provisionerCephClientSecret
-			storageClassData["csi.storage.k8s.io/node-stage-secret-name"] = nodeCephClientSecret
-			storageClassData["csi.storage.k8s.io/controller-expand-secret-name"] = provisionerCephClientSecret
+			storageClassData["csi.storage.k8s.io/provisioner-secret-name"] = provisionerSecretName
+			storageClassData["csi.storage.k8s.io/node-stage-secret-name"] = nodeSecretName
+			storageClassData["csi.storage.k8s.io/controller-expand-secret-name"] = provisionerSecretName
 			if storageClassRequest.Spec.EncryptionMethod != "" {
 				storageClassData["encrypted"] = "true"
 				storageClassData["encryptionKMSID"] = storageClassRequest.Spec.EncryptionMethod
@@ -623,7 +620,7 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 				Kind: "VolumeSnapshotClass",
 				Data: mustMarshal(map[string]string{
 					"clusterID": storageClassData["clusterID"],
-					"csi.storage.k8s.io/snapshotter-secret-name": provisionerCephClientSecret,
+					"csi.storage.k8s.io/snapshotter-secret-name": provisionerSecretName,
 				})})
 
 		case "CephFilesystemSubVolumeGroup":
@@ -633,24 +630,14 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 				return nil, status.Errorf(codes.Internal, "failed to get %s cephFilesystemSubVolumeGroup. %v", cephRes.Name, err)
 			}
 
-			nodeCephClientSecret, _, err := s.getCephClientInformation(ctx, cephRes.CephClients["node"])
-			if err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-
-			provisionerCephClientSecret, _, err := s.getCephClientInformation(ctx, cephRes.CephClients["provisioner"])
-			if err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-
 			storageClassName = "cephfs"
 			storageClassData["clusterID"] = getSubVolumeGroupClusterID(subVolumeGroup)
 			storageClassData["subvolumegroupname"] = subVolumeGroup.Name
 			storageClassData["fsName"] = subVolumeGroup.Spec.FilesystemName
 			storageClassData["pool"] = subVolumeGroup.GetLabels()[v1alpha1.CephFileSystemDataPoolLabel]
-			storageClassData["csi.storage.k8s.io/provisioner-secret-name"] = provisionerCephClientSecret
-			storageClassData["csi.storage.k8s.io/node-stage-secret-name"] = nodeCephClientSecret
-			storageClassData["csi.storage.k8s.io/controller-expand-secret-name"] = provisionerCephClientSecret
+			storageClassData["csi.storage.k8s.io/provisioner-secret-name"] = provisionerSecretName
+			storageClassData["csi.storage.k8s.io/node-stage-secret-name"] = nodeSecretName
+			storageClassData["csi.storage.k8s.io/controller-expand-secret-name"] = provisionerSecretName
 
 			extR = append(extR, &pb.ExternalResource{
 				Name: cephRes.Name,
@@ -664,7 +651,7 @@ func (s *OCSProviderServer) GetStorageClassClaimConfig(ctx context.Context, req
 				Kind: "VolumeSnapshotClass",
 				Data: mustMarshal(map[string]string{
 					"clusterID": getSubVolumeGroupClusterID(subVolumeGroup),
-					"csi.storage.k8s.io/snapshotter-secret-name": provisionerCephClientSecret,
+					"csi.storage.k8s.io/snapshotter-secret-name": provisionerSecretName,
 				})})
 		}
 	}

diff --git a/services/provider/server/server_test.go b/services/provider/server/server_test.go
@@ -367,7 +367,7 @@ func createCephClientAndSecret(name string, server *OCSProviderServer) (*rookCep
 	}
 
 	secret := &v1.Secret{
-		ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("rook-ceph-client-%s", name), Namespace: server.namespace},
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: server.namespace},
 		Data: map[string][]byte{
 			name: []byte("AQADw/hhqBOcORAAJY3fKIvte++L/zYhASjYPQ=="),
 		},
@@ -503,29 +503,29 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 					"imageFeatures":             "layering,deep-flatten,exclusive-lock,object-map,fast-diff",
 					"csi.storage.k8s.io/fstype": "ext4",
 					"imageFormat":               "2",
-					"csi.storage.k8s.io/provisioner-secret-name":       "rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e",
-					"csi.storage.k8s.io/node-stage-secret-name":        "rook-ceph-client-995e66248ad3e8642de868f461cdd827",
-					"csi.storage.k8s.io/controller-expand-secret-name": "rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e",
+					"csi.storage.k8s.io/provisioner-secret-name":       "ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715",
+					"csi.storage.k8s.io/node-stage-secret-name":        "ceph-client-node-fd5db4ce26aec80f63574cc5c8eec715",
+					"csi.storage.k8s.io/controller-expand-secret-name": "ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715",
 				},
 			},
 			"ceph-rbd-volumesnapshotclass": {
 				Name: "ceph-rbd",
 				Kind: "VolumeSnapshotClass",
 				Data: map[string]string{
 					"clusterID": serverNamespace,
-					"csi.storage.k8s.io/snapshotter-secret-name": "rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e",
+					"csi.storage.k8s.io/snapshotter-secret-name": "ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715",
 				},
 			},
-			"rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e": {
-				Name: "rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e",
+			"ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715": {
+				Name: "ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715",
 				Kind: "Secret",
 				Data: map[string]string{
 					"userID":  "3de200d5c23524a4612bde1fdbeb717e",
 					"userKey": "AQADw/hhqBOcORAAJY3fKIvte++L/zYhASjYPQ==",
 				},
 			},
-			"rook-ceph-client-995e66248ad3e8642de868f461cdd827": {
-				Name: "rook-ceph-client-995e66248ad3e8642de868f461cdd827",
+			"ceph-client-node-fd5db4ce26aec80f63574cc5c8eec715": {
+				Name: "ceph-client-node-fd5db4ce26aec80f63574cc5c8eec715",
 				Kind: "Secret",
 				Data: map[string]string{
 					"userID":  "995e66248ad3e8642de868f461cdd827",
@@ -543,29 +543,29 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 					"fsName":             "myfs",
 					"subvolumegroupname": "cephFilesystemSubVolumeGroup",
 					"pool":               "",
-					"csi.storage.k8s.io/provisioner-secret-name":       "rook-ceph-client-4ffcb503ff8044c8699dac415f82d604",
-					"csi.storage.k8s.io/node-stage-secret-name":        "rook-ceph-client-1b042fcc8812fe4203689eec38fdfbfa",
-					"csi.storage.k8s.io/controller-expand-secret-name": "rook-ceph-client-4ffcb503ff8044c8699dac415f82d604",
+					"csi.storage.k8s.io/provisioner-secret-name":       "ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447",
+					"csi.storage.k8s.io/node-stage-secret-name":        "ceph-client-node-79e35b1c313f42d129f35fe66752c447",
+					"csi.storage.k8s.io/controller-expand-secret-name": "ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447",
 				},
 			},
 			"cephfs-volumesnapshotclass": {
 				Name: "cephfs",
 				Kind: "VolumeSnapshotClass",
 				Data: map[string]string{
 					"clusterID": "8d26c7378c1b0ec9c2455d1c3601c4cd",
-					"csi.storage.k8s.io/snapshotter-secret-name": "rook-ceph-client-4ffcb503ff8044c8699dac415f82d604",
+					"csi.storage.k8s.io/snapshotter-secret-name": "ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447",
 				},
 			},
-			"rook-ceph-client-4ffcb503ff8044c8699dac415f82d604": {
-				Name: "rook-ceph-client-4ffcb503ff8044c8699dac415f82d604",
+			"ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447": {
+				Name: "ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447",
 				Kind: "Secret",
 				Data: map[string]string{
 					"adminID":  "4ffcb503ff8044c8699dac415f82d604",
 					"adminKey": "AQADw/hhqBOcORAAJY3fKIvte++L/zYhASjYPQ==",
 				},
 			},
-			"rook-ceph-client-1b042fcc8812fe4203689eec38fdfbfa": {
-				Name: "rook-ceph-client-1b042fcc8812fe4203689eec38fdfbfa",
+			"ceph-client-node-79e35b1c313f42d129f35fe66752c447": {
+				Name: "ceph-client-node-79e35b1c313f42d129f35fe66752c447",
 				Kind: "Secret",
 				Data: map[string]string{
 					"adminID":  "1b042fcc8812fe4203689eec38fdfbfa",
@@ -712,14 +712,14 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 		},
 		Status: &rookCephv1.CephClientStatus{
 			Info: map[string]string{
-				"secretName": "rook-ceph-client-995e66248ad3e8642de868f461cdd827",
+				"secretName": "ceph-client-node-fd5db4ce26aec80f63574cc5c8eec715",
 			},
 		},
 	}
 
 	secret := &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "rook-ceph-client-995e66248ad3e8642de868f461cdd827",
+			Name:      "ceph-client-node-fd5db4ce26aec80f63574cc5c8eec715",
 			Namespace: server.namespace,
 		},
 		Data: map[string][]byte{
@@ -742,14 +742,14 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 		},
 		Status: &rookCephv1.CephClientStatus{
 			Info: map[string]string{
-				"secretName": "rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e",
+				"secretName": "ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715",
 			},
 		},
 	}
 
 	secret = &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "rook-ceph-client-3de200d5c23524a4612bde1fdbeb717e",
+			Name:      "ceph-client-provisioner-fd5db4ce26aec80f63574cc5c8eec715",
 			Namespace: server.namespace,
 		},
 		Data: map[string][]byte{
@@ -772,14 +772,14 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 		},
 		Status: &rookCephv1.CephClientStatus{
 			Info: map[string]string{
-				"secretName": "rook-ceph-client-1b042fcc8812fe4203689eec38fdfbfa",
+				"secretName": "ceph-client-node-79e35b1c313f42d129f35fe66752c447",
 			},
 		},
 	}
 
 	secret = &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "rook-ceph-client-1b042fcc8812fe4203689eec38fdfbfa",
+			Name:      "ceph-client-node-79e35b1c313f42d129f35fe66752c447",
 			Namespace: server.namespace,
 		},
 		Data: map[string][]byte{
@@ -802,14 +802,14 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 		},
 		Status: &rookCephv1.CephClientStatus{
 			Info: map[string]string{
-				"secretName": "rook-ceph-client-4ffcb503ff8044c8699dac415f82d604",
+				"secretName": "ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447",
 			},
 		},
 	}
 
 	secret = &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "rook-ceph-client-4ffcb503ff8044c8699dac415f82d604",
+			Name:      "ceph-client-provisioner-79e35b1c313f42d129f35fe66752c447",
 			Namespace: server.namespace,
 		},
 		Data: map[string][]byte{
@@ -858,10 +858,6 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 			name = fmt.Sprintf("%s-volumesnapshotclass", name)
 		} else if extResource.Kind == "StorageClass" {
 			name = fmt.Sprintf("%s-storageclass", name)
-		} else if extResource.Kind == "Secret" {
-			var found bool
-			name, found = strings.CutSuffix(name, ".csi")
-			assert.True(t, found)
 		}
 		mockResource, ok := mockBlockPoolClaimExtR[name]
 		if !ok {
@@ -873,12 +869,7 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, string(extResource.Data), string(data))
 		assert.Equal(t, extResource.Kind, mockResource.Kind)
-		if extResource.Kind == "Secret" {
-			name, _ := strings.CutSuffix(name, ".csi")
-			assert.Equal(t, name, mockResoruce.Name)
-		} else {
-			assert.Equal(t, extResource.Name, mockResource.Name)
-		}
+		assert.Equal(t, extResource.Name, mockResource.Name)
 	}
 
 	// get the storage class request config for share filesystem
@@ -897,10 +888,6 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 			name = fmt.Sprintf("%s-volumesnapshotclass", name)
 		} else if extResource.Kind == "StorageClass" {
 			name = fmt.Sprintf("%s-storageclass", name)
-		} else if extResource.Kind == "Secret" {
-			var found bool
-			name, found = strings.CutSuffix(name, ".csi")
-			assert.True(t, found)
 		}
 		mockResource, ok := mockShareFilesystemClaimExtR[name]
 		if !ok {
@@ -912,25 +899,24 @@ func TestOCSProviderServerGetStorageClassClaimConfig(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, string(extResource.Data), string(data))
 		assert.Equal(t, extResource.Kind, mockResource.Kind)
-		if extResource.Kind == "Secret" {
-			name, _ := strings.CutSuffix(name, ".csi")
-			assert.Equal(t, name, mockResoruce.Name)
-		} else {
-			assert.Equal(t, extResource.Name, mockResoruce.Name)
-			assert.Equal(t, extResource.Name, mockResource.Name)
-		}
+		assert.Equal(t, extResource.Name, mockResource.Name)
 	}
 
 	// When ceph resources is empty
 	for _, i := range sharedFilesystemClaimResource.Status.CephResources {
-		if i.Kind == "CephClient" {
-			cephClient, secret := createCephClientAndSecret(i.Name, server)
-			cephClient.Status = &rookCephv1.CephClientStatus{
-				Info: map[string]string{
-					"secretName": fmt.Sprintf("rook-ceph-client-%s", i.Name),
-				},
+		if i.Kind == "CephFilesystemSubVolumeGroup" {
+			scrNameHash := strings.TrimPrefix(sharedFilesystemClaimResource.Name, "storageclassrequest-")
+			for cephClientUserType, cephClientName := range i.CephClients {
+				cephClient, secret := createCephClientAndSecret(cephClientName, server)
+				secret.Name = scrCephClientCsiSecretName(cephClientUserType, scrNameHash)
+				cephClient.Status = &rookCephv1.CephClientStatus{
+					Info: map[string]string{
+						"secretName": secret.Name,
+					},
+				}
+				assert.NoError(t, client.Delete(ctx, secret))
 			}
-			assert.NoError(t, client.Delete(ctx, secret))
+			break
 		}
 	}