Repository: kubernetes-csi/external-provisioner (go)

Issue:
A single "default" storage class secret
## Current Issue

Currently storage classes have each operation have a reference to the secret:

```yaml
kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: fast-storage-all
provisioner: csi-driver.team.example.com
parameters:
  type: pd-ssd
  csi.storage.k8s.io/provisioner-secret-name: ${pvc.name}
  csi.storage.k8s.io/provisioner-secret-namespace: ${pvc.namespace}-fast-storage
  csi.storage.k8s.io/node-publish-secret-name: ${pvc.name}-${pvc.annotations['team.example.com/key']}
  csi.storage.k8s.io/node-publish-secret-namespace: ${pvc.namespace}-fast-storage
```

See [Docs](https://kubernetes-csi.github.io/docs/secrets-and-credentials-storage-class.html#multiple-operation-secrets)

## Proposal 

To have a default that can be overridden by the specific operation secret references, that way as new operations are supported, we do not need to have a large set of repeating information for each operation. The new storageClass would look like this:

```yaml
kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: fast-storage-all
provisioner: csi-driver.team.example.com
parameters:
  type: pd-ssd
  csi.storage.k8s.io/secret-name: ${pvc.name}
  csi.storage.k8s.io/secret-namespace: ${pvc.namespace}-fast-storage
  csi.storage.k8s.io/node-publish-secret-name: ${pvc.name}-${pvc.annotations['team.example.com/key']}
  csi.storage.k8s.io/node-publish-secret-namespace: ${pvc.namespace}-fast-storage
``` 

Where

```
  csi.storage.k8s.io/secret-name: ${pvc.name}
  csi.storage.k8s.io/secret-namespace: ${pvc.namespace}-fast-storage
```

Are the new _default_ secrets if none are specified. 


Pull request:
issue-384, A single "default" storage class secret is added
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind feature

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #384

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Default StorageClass secrets are added:
 csi.storage.k8s.io/secret-name: ${pvc.name}
 csi.storage.k8s.io/secret-namespace: ${pvc.namespace}

Note: Default secrets for storage class feature does work only when both parameters are added.
```


Patch:
diff --git a/cmd/csi-provisioner/csi-provisioner.go b/cmd/csi-provisioner/csi-provisioner.go
index ea4bc7efd..d63976117 100644
--- a/cmd/csi-provisioner/csi-provisioner.go
+++ b/cmd/csi-provisioner/csi-provisioner.go
@@ -45,6 +45,7 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/informers"
 	v1 "k8s.io/client-go/listers/core/v1"
+	storagelistersv1 "k8s.io/client-go/listers/storage/v1"
 	storagelistersv1beta1 "k8s.io/client-go/listers/storage/v1beta1"
 	utilflag "k8s.io/component-base/cli/flag"
 	csitrans "k8s.io/csi-translation-lib"
@@ -192,13 +193,14 @@ func main() {
 		provisionerOptions = append(provisionerOptions, controller.AdditionalProvisionerNames([]string{supportsMigrationFromInTreePluginName}))
 	}
 
-	// Create informer to prevent hit the API server for all resource request
-	factory := informers.NewSharedInformerFactory(clientset, ctrl.ResyncPeriodOfCsiNodeInformer)
-	scLister := factory.Storage().V1().StorageClasses().Lister()
-
+	var scLister storagelistersv1.StorageClassLister
 	var csiNodeLister storagelistersv1beta1.CSINodeLister
 	var nodeLister v1.NodeLister
+	var factory informers.SharedInformerFactory
 	if ctrl.SupportsTopology(pluginCapabilities) {
+		// Create informer to prevent hit the API server for all resource request
+		factory = informers.NewSharedInformerFactory(clientset, ctrl.ResyncPeriodOfCsiNodeInformer)
+		scLister = factory.Storage().V1().StorageClasses().Lister()
 		csiNodeLister = factory.Storage().V1beta1().CSINodes().Lister()
 		nodeLister = factory.Core().V1().Nodes().Lister()
 	}
@@ -232,12 +234,14 @@ func main() {
 	)
 
 	run := func(context.Context) {
-		stopCh := context.Background().Done()
-		factory.Start(stopCh)
-		cacheSyncResult := factory.WaitForCacheSync(stopCh)
-		for _, v := range cacheSyncResult {
-			if !v {
-				klog.Fatalf("Failed to sync Informers!")
+		if factory != nil {
+			stopCh := context.Background().Done()
+			factory.Start(stopCh)
+			cacheSyncResult := factory.WaitForCacheSync(stopCh)
+			for _, v := range cacheSyncResult {
+				if !v {
+					klog.Fatalf("Failed to sync Informers!")
+				}
 			}
 		}
 
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index fe7f0b3f3..fdb9bc450 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -72,6 +72,9 @@ const (
 
 	prefixedFsTypeKey = csiParameterPrefix + "fstype"
 
+	prefixedDefaultSecretNameKey      = csiParameterPrefix + "secret-name"
+	prefixedDefaultSecretNamespaceKey = csiParameterPrefix + "secret-namespace"
+
 	prefixedProvisionerSecretNameKey      = csiParameterPrefix + "provisioner-secret-name"
 	prefixedProvisionerSecretNamespaceKey = csiParameterPrefix + "provisioner-secret-namespace"
 
@@ -128,6 +131,12 @@ const (
 )
 
 var (
+	defaultSecretParams = secretParamsMap{
+		name:               "Default",
+		secretNameKey:      prefixedDefaultSecretNameKey,
+		secretNamespaceKey: prefixedDefaultSecretNamespaceKey,
+	}
+
 	provisionerSecretParams = secretParamsMap{
 		name:                         "Provisioner",
 		deprecatedSecretNameKey:      provisionerSecretNameKey,
@@ -702,6 +711,8 @@ func removePrefixedParameters(param map[string]string) (map[string]string, error
 			case prefixedNodePublishSecretNamespaceKey:
 			case prefixedControllerExpandSecretNameKey:
 			case prefixedControllerExpandSecretNamespaceKey:
+			case prefixedDefaultSecretNameKey:
+			case prefixedDefaultSecretNamespaceKey:
 			default:
 				return map[string]string{}, fmt.Errorf("found unknown parameter key \"%s\" with reserved namespace %s", k, csiParameterPrefix)
 			}
@@ -1042,6 +1053,15 @@ func getSecretReference(secretParams secretParamsMap, storageClassParams map[str
 	if err != nil {
 		return nil, fmt.Errorf("failed to get name and namespace template from params: %v", err)
 	}
+
+	// if didn't find secrets for specific call, try to check default values
+	if nameTemplate == "" && namespaceTemplate == "" {
+		nameTemplate, namespaceTemplate, err = verifyAndGetSecretNameAndNamespaceTemplate(defaultSecretParams, storageClassParams)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get default name and namespace template from params: %v", err)
+		}
+	}
+
 	if nameTemplate == "" && namespaceTemplate == "" {
 		return nil, nil
 	}


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index 0436ee01d..60ca5f22f 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -213,6 +213,8 @@ func TestStripPrefixedCSIParams(t *testing.T) {
 				prefixedNodePublishSecretNamespaceKey:       "csiBar",
 				prefixedControllerExpandSecretNameKey:       "csiBar",
 				prefixedControllerExpandSecretNamespaceKey:  "csiBar",
+				prefixedDefaultSecretNameKey:                "csiBar",
+				prefixedDefaultSecretNamespaceKey:           "csiBar",
 			},
 			expectedParams: map[string]string{},
 		},
@@ -475,13 +477,17 @@ func provisionFromPVCCapabilities() (rpc.PluginCapabilitySet, rpc.ControllerCapa
 		}
 }
 
-// Minimal PVC required for tests to function
-func createFakePVC(requestBytes int64) *v1.PersistentVolumeClaim {
+func createFakeNamedPVC(requestBytes int64, name string, userAnnotations map[string]string) *v1.PersistentVolumeClaim {
+	annotations := map[string]string{annStorageProvisioner: driverName}
+	for k, v := range userAnnotations {
+		annotations[k] = v
+	}
+
 	return &v1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			UID:         "testid",
-			Name:        "fake-pvc",
-			Annotations: map[string]string{annStorageProvisioner: driverName},
+			Name:        name,
+			Annotations: annotations,
 		},
 		Spec: v1.PersistentVolumeClaimSpec{
 			Selector: nil, // Provisioner doesn't support selector
@@ -494,6 +500,11 @@ func createFakePVC(requestBytes int64) *v1.PersistentVolumeClaim {
 	}
 }
 
+// Minimal PVC required for tests to function
+func createFakePVC(requestBytes int64) *v1.PersistentVolumeClaim {
+	return createFakeNamedPVC(requestBytes, "fake-pvc", nil)
+}
+
 // createFakePVCWithVolumeMode returns PVC with VolumeMode
 func createFakePVCWithVolumeMode(requestBytes int64, volumeMode v1.PersistentVolumeMode) *v1.PersistentVolumeClaim {
 	claim := createFakePVC(requestBytes)
@@ -778,7 +789,7 @@ type provisioningTestcase struct {
 	volWithLessCap    bool
 	volWithZeroCap    bool
 	expectedPVSpec    *pvSpec
-	withSecretRefs    bool
+	clientSetObjects  []runtime.Object
 	createVolumeError error
 	expectErr         bool
 	expectState       controller.ProvisioningState
@@ -795,6 +806,47 @@ type pvSpec struct {
 	CSIPVS        *v1.CSIPersistentVolumeSource
 }
 
+const defaultSecretNsName = "default"
+
+func getDefaultStorageClassSecretParameters() map[string]string {
+	return map[string]string{
+		controllerPublishSecretNameKey:             "ctrlpublishsecret",
+		controllerPublishSecretNamespaceKey:        defaultSecretNsName,
+		nodeStageSecretNameKey:                     "nodestagesecret",
+		nodeStageSecretNamespaceKey:                defaultSecretNsName,
+		nodePublishSecretNameKey:                   "nodepublishsecret",
+		nodePublishSecretNamespaceKey:              defaultSecretNsName,
+		prefixedControllerExpandSecretNameKey:      "controllerexpandsecret",
+		prefixedControllerExpandSecretNamespaceKey: defaultSecretNsName,
+	}
+}
+
+func getDefaultSecretObjects() []runtime.Object {
+	return []runtime.Object{
+		&v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "ctrlpublishsecret",
+				Namespace: defaultSecretNsName,
+			},
+		}, &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "nodestagesecret",
+				Namespace: defaultSecretNsName,
+			},
+		}, &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "nodepublishsecret",
+				Namespace: defaultSecretNsName,
+			},
+		}, &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "controllerexpandsecret",
+				Namespace: defaultSecretNsName,
+			},
+		},
+	}
+}
+
 func TestProvision(t *testing.T) {
 	var requestedBytes int64 = 100
 	deletePolicy := v1.PersistentVolumeReclaimDelete
@@ -1093,12 +1145,12 @@ func TestProvision(t *testing.T) {
 			volOpts: controller.ProvisionOptions{
 				StorageClass: &storagev1.StorageClass{
 					ReclaimPolicy: &deletePolicy,
-					Parameters:    map[string]string{},
+					Parameters:    getDefaultStorageClassSecretParameters(),
 				},
 				PVName: "test-name",
 				PVC:    createFakePVC(requestedBytes),
 			},
-			withSecretRefs: true,
+			clientSetObjects: getDefaultSecretObjects(),
 			expectedPVSpec: &pvSpec{
 				Name: "test-testi",
 				Capacity: v1.ResourceList{
@@ -1132,6 +1184,108 @@ func TestProvision(t *testing.T) {
 			},
 			expectState: controller.ProvisioningFinished,
 		},
+		"provision with default secrets": {
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						prefixedDefaultSecretNameKey:      "default-secret",
+						prefixedDefaultSecretNamespaceKey: "default-ns",
+					},
+				},
+				PVName: "test-name",
+				PVC:    createFakePVC(requestedBytes),
+			},
+			clientSetObjects: []runtime.Object{&v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "default-secret",
+					Namespace: "default-ns",
+				},
+			}},
+			expectedPVSpec: &pvSpec{
+				Name: "test-testi",
+				Capacity: v1.ResourceList{
+					v1.ResourceName(v1.ResourceStorage): bytesToGiQuantity(requestedBytes),
+				},
+				ReclaimPolicy: v1.PersistentVolumeReclaimDelete,
+				CSIPVS: &v1.CSIPersistentVolumeSource{
+					Driver:       "test-driver",
+					VolumeHandle: "test-volume-id",
+					FSType:       "ext4",
+					VolumeAttributes: map[string]string{
+						"storage.kubernetes.io/csiProvisionerIdentity": "test-provisioner",
+					},
+					ControllerPublishSecretRef: &v1.SecretReference{
+						Name:      "default-secret",
+						Namespace: "default-ns",
+					},
+					NodeStageSecretRef: &v1.SecretReference{
+						Name:      "default-secret",
+						Namespace: "default-ns",
+					},
+					NodePublishSecretRef: &v1.SecretReference{
+						Name:      "default-secret",
+						Namespace: "default-ns",
+					},
+					ControllerExpandSecretRef: &v1.SecretReference{
+						Name:      "default-secret",
+						Namespace: "default-ns",
+					},
+				},
+			},
+			expectState: controller.ProvisioningFinished,
+		},
+		"provision with default secrets with template": {
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						prefixedDefaultSecretNameKey:      "${pvc.name}",
+						prefixedDefaultSecretNamespaceKey: "default-ns",
+					},
+				},
+				PVName: "test-name",
+				PVC:    createFakeNamedPVC(requestedBytes, "my-pvc", nil),
+			},
+			clientSetObjects: []runtime.Object{&v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "my-pvc",
+					Namespace: "default-ns",
+				},
+			}},
+			expectedPVSpec: &pvSpec{
+				Name: "test-testi",
+				Capacity: v1.ResourceList{
+					v1.ResourceName(v1.ResourceStorage): bytesToGiQuantity(requestedBytes),
+				},
+				ReclaimPolicy: v1.PersistentVolumeReclaimDelete,
+				CSIPVS: &v1.CSIPersistentVolumeSource{
+					Driver:       "test-driver",
+					VolumeHandle: "test-volume-id",
+					FSType:       "ext4",
+					VolumeAttributes: map[string]string{
+						"storage.kubernetes.io/csiProvisionerIdentity": "test-provisioner",
+					},
+					ControllerPublishSecretRef: &v1.SecretReference{
+						Name:      "my-pvc",
+						Namespace: "default-ns",
+					},
+					NodeStageSecretRef: &v1.SecretReference{
+						Name:      "my-pvc",
+						Namespace: "default-ns",
+					},
+					NodePublishSecretRef: &v1.SecretReference{
+						Name:      "my-pvc",
+						Namespace: "default-ns",
+					},
+					ControllerExpandSecretRef: &v1.SecretReference{
+						Name:      "my-pvc",
+						Namespace: "default-ns",
+					},
+				},
+			},
+			expectState: controller.ProvisioningFinished,
+		},
 		"provision with volume mode(Filesystem)": {
 			volOpts: controller.ProvisionOptions{
 				StorageClass: &storagev1.StorageClass{
@@ -1227,6 +1381,32 @@ func TestProvision(t *testing.T) {
 			expectErr:         true,
 			expectState:       controller.ProvisioningNoChange,
 		},
+		"fail to get secret reference for invalid default secret parameter template": {
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						prefixedDefaultSecretNameKey:      "default-${pvc.annotations['team.example.com/key']}",
+						prefixedDefaultSecretNamespaceKey: "default-ns",
+					},
+				},
+				PVName: "test-name",
+				PVC: createFakeNamedPVC(
+					requestedBytes,
+					"fake-pvc",
+					map[string]string{"team.example.com/key": "secret-from-annotation"},
+				),
+			},
+			clientSetObjects: []runtime.Object{&v1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "default-secret-from-annotation",
+					Namespace: "default-ns",
+				},
+			}},
+			getSecretRefErr: true,
+			expectErr:       true,
+			expectState:     controller.ProvisioningNoChange,
+		},
 		"fail vol with less capacity": {
 			volOpts: controller.ProvisionOptions{
 				StorageClass: &storagev1.StorageClass{
@@ -1435,33 +1615,7 @@ func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requested
 	defer mockController.Finish()
 	defer driver.Stop()
 
-	var clientSet kubernetes.Interface
-
-	if tc.withSecretRefs {
-		clientSet = fakeclientset.NewSimpleClientset(&v1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "ctrlpublishsecret",
-				Namespace: "default",
-			},
-		}, &v1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "nodestagesecret",
-				Namespace: "default",
-			},
-		}, &v1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "nodepublishsecret",
-				Namespace: "default",
-			},
-		}, &v1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "controllerexpandsecret",
-				Namespace: "default",
-			},
-		})
-	} else {
-		clientSet = fakeclientset.NewSimpleClientset()
-	}
+	clientSet := fakeclientset.NewSimpleClientset(tc.clientSetObjects...)
 
 	pluginCaps, controllerCaps := provisionCapabilities()
 	csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
@@ -1474,17 +1628,6 @@ func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requested
 		},
 	}
 
-	if tc.withSecretRefs {
-		tc.volOpts.StorageClass.Parameters[controllerPublishSecretNameKey] = "ctrlpublishsecret"
-		tc.volOpts.StorageClass.Parameters[controllerPublishSecretNamespaceKey] = "default"
-		tc.volOpts.StorageClass.Parameters[nodeStageSecretNameKey] = "nodestagesecret"
-		tc.volOpts.StorageClass.Parameters[nodeStageSecretNamespaceKey] = "default"
-		tc.volOpts.StorageClass.Parameters[nodePublishSecretNameKey] = "nodepublishsecret"
-		tc.volOpts.StorageClass.Parameters[nodePublishSecretNamespaceKey] = "default"
-		tc.volOpts.StorageClass.Parameters[prefixedControllerExpandSecretNameKey] = "controllerexpandsecret"
-		tc.volOpts.StorageClass.Parameters[prefixedControllerExpandSecretNamespaceKey] = "default"
-	}
-
 	if tc.notNilSelector {
 		tc.volOpts.PVC.Spec.Selector = &metav1.LabelSelector{}
 	} else if tc.makeVolumeNameErr {
@@ -2482,11 +2625,11 @@ type deleteTestcase struct {
 // TestDelete is a test of the delete operation
 func TestDelete(t *testing.T) {
 	tt := map[string]deleteTestcase{
-		"fail - nil PV": deleteTestcase{
+		"fail - nil PV": {
 			persistentVolume: nil,
 			expectErr:        true,
 		},
-		"fail - nil volume.Spec.CSI": deleteTestcase{
+		"fail - nil volume.Spec.CSI": {
 			persistentVolume: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pv",
@@ -2500,7 +2643,7 @@ func TestDelete(t *testing.T) {
 			},
 			expectErr: true,
 		},
-		"fail - pvc.annotations not supported": deleteTestcase{
+		"fail - pvc.annotations not supported for Create/Delete Volume Secret": {
 			persistentVolume: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pv",
@@ -2527,7 +2670,7 @@ func TestDelete(t *testing.T) {
 			},
 			expectErr: true,
 		},
-		"simple - valid case": deleteTestcase{
+		"simple - valid case": {
 			persistentVolume: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pv",
@@ -2551,7 +2694,7 @@ func TestDelete(t *testing.T) {
 			expectErr:  false,
 			mockDelete: true,
 		},
-		"simple - valid case with ClaimRef set": deleteTestcase{
+		"simple - valid case with ClaimRef set": {
 			persistentVolume: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pv",