Repository: kubernetes-sigs/azurefile-csi-driver
Pull request #519 (2021-01-07 13:54:09)

feat: add secretName parameter
<!--  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. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. 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**:
feat: add secretName parameter
This PR is mainly for fixing Azure File CSI driver migration issue(workaround node_authorizer error in csi migration scenario), another benefit is add parity feature(`secretName`) in storage class with in-tree driver.

**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 #

**Requirements**:
- [ ] uses [conventional commit messages](https://www.conventionalcommits.org/)
  <!-- Common commit types:
        build: Build 🏭
        chore: Maintenance 🔧
        ci: Continuous Integration 💜
        docs: Documentation 📘
        feat: Features 🌈
        fix: Bug Fixes 🐞
        perf: Performance Improvements 🚀
        refactor: Code Refactoring 💎
        revert: Revert Change ◀️
        style: Code Style 🎶
        security: Security Fix 🛡️
        test: Testing 💚 -->
- [ ] includes documentation
- [ ] adds unit tests
- [ ] tested upgrade from previous version

**Special notes for your reviewer**:


**Release note**:
```
feat: add secretName parameter
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/azurefile-csi-driver/pull/519#" title="Author self-approved">andyzhangx</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fazurefile-csi-driver).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/OWNERS)~~ [andyzhangx]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
k8s-ci-robot: @andyzhangx: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-azurefile-csi-driver-verify | f23af5fcdcbd196bdc6e8c09df453211ad505845 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azurefile-csi-driver/519/pull-azurefile-csi-driver-verify/1347179725860114432) | `/test pull-azurefile-csi-driver-verify`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=azurefile-csi-driver&pr=519). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aandyzhangx). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/azurefile-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @andyzhangx: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-azurefile-csi-driver-verify | f23af5fcdcbd196bdc6e8c09df453211ad505845 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azurefile-csi-driver/519/pull-azurefile-csi-driver-verify/1347179725860114432) | `/test pull-azurefile-csi-driver-verify`
pull-azurefile-csi-driver-sanity | f23af5fcdcbd196bdc6e8c09df453211ad505845 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azurefile-csi-driver/519/pull-azurefile-csi-driver-sanity/1347179725897863168) | `/test pull-azurefile-csi-driver-sanity`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=azurefile-csi-driver&pr=519). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aandyzhangx). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/azurefile-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md
index 4059fc50e..21e96803b 100644
--- a/docs/driver-parameters.md
+++ b/docs/driver-parameters.md
@@ -14,7 +14,8 @@ resourceGroup | specify the resource group in which Azure file share will be cre
 shareName | specify Azure file share name | existing or new Azure file name | No | if empty, driver will generate an Azure file share name
 server | specify Azure storage account server address | existing server address, e.g. `accountname.privatelink.file.core.windows.net` | No | if empty, driver will use default `accountname.file.core.windows.net` or other sovereign cloud account address
 storeAccountKey | whether store account key to k8s secret | `true`,`false` | No | `true`
-secretNamespace | specify the namespace of secret to store account key | `default`,`kube-system`,etc | No | `default`
+secretName | specify secret name to store account key | | No |
+secretNamespace | specify the namespace of secret to store account key | `default`,`kube-system`, etc | No | `default`
 tags | [tags](https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/tag-resources) would be created in newly created storage account | tag format: 'foo=aaa,bar=bbb' | No | ""
 --- | **Following parameters are only for [VHD disk feature](../deploy/example/disk)** | --- | --- |
 fsType | File System Type | `ext4`, `ext3`, `ext2`, `xfs` | Yes | `ext4`
@@ -33,11 +34,11 @@ volumeAttributes.resourceGroup | Azure resource group name | existing resource g
 volumeAttributes.storageAccount | existing storage account name | existing storage account name | Yes |
 volumeAttributes.shareName | Azure file share name | existing Azure file share name | Yes |
 volumeAttributes.protocol | specify file share protocol | `smb`, `nfs` | No | `smb`
-volumeAttributes.server | specify Azure storage account server address | existing server address, e.g. `accountname.privatelink.file.core.windows.net` | No | if empty, driver will use default `accountname.file.core.windows.net` or other sovereign cloud account address
+server | specify Azure storage account server address | existing server address, e.g. `accountname.privatelink.file.core.windows.net` | No | if empty, driver will use default `accountname.file.core.windows.net` or other sovereign cloud account address
 nodeStageSecretRef.name | secret name that stores storage account name and key | existing secret name |  Yes  |
 nodeStageSecretRef.namespace | namespace where the secret is | k8s namespace  |  Yes  |
 
  - create a Kubernetes secret for `nodeStageSecretRef.name`
  ```console
-kubectl create secret generic azure-secret --from-literal azurestorageaccountkey="xxx" --type=Opaque
+kubectl create secret generic azure-secret --from-literal accountkey="xxx" --type=Opaque
  ```
diff --git a/pkg/azurefile/azurefile.go b/pkg/azurefile/azurefile.go
index 4e6da2bbe..62492b2f6 100644
--- a/pkg/azurefile/azurefile.go
+++ b/pkg/azurefile/azurefile.go
@@ -85,6 +85,7 @@ const (
 	resourceGroupField       = "resourcegroup"
 	locationField            = "location"
 	secretNamespaceField     = "secretnamespace"
+	secretNameField          = "secretname"
 	storeAccountKeyField     = "storeaccountkey"
 	storeAccountKeyFalse     = "false"
 	defaultSecretAccountName = "azurestorageaccountname"
@@ -458,11 +459,18 @@ func (d *Driver) GetAccountInfo(volumeID string, secrets, reqContext map[string]
 	}
 
 	if len(secrets) == 0 {
-		if accountName != "" {
-			// read from k8s secret first
-			accountKey, err = d.GetStorageAccesskeyFromSecret(accountName, reqContext[secretNamespaceField])
-			if err != nil && d.cloud.StorageAccountClient != nil {
-				klog.V(2).Infof("could not get account(%s) key from secret, error: %v, use cluster identity to get account key instead", accountName, err)
+		secretNamespace, ok := reqContext[secretNamespaceField]
+		if !ok {
+			secretNamespace = defaultSecretNamespace
+		}
+		secretName, ok := reqContext[secretNamespace]
+		if !ok && accountName != "" {
+			secretName = fmt.Sprintf(secretNameTemplate, accountName)
+		}
+		if secretName != "" {
+			accountKey, err = d.GetStorageAccesskeyFromSecret(secretName, secretNamespace)
+			if err != nil && d.cloud.StorageAccountClient != nil && accountName != "" {
+				klog.V(2).Infof("could not get account(%s) key from secret(%s), error: %v, use cluster identity to get account key instead", accountName, secretName, err)
 				accountKey, err = d.cloud.GetStorageAccesskey(accountName, rgName)
 			}
 		}
@@ -524,17 +532,23 @@ func (d *Driver) ResizeFileShare(resourceGroup, accountName, shareName string, s
 	return d.cloud.ResizeFileShare(resourceGroup, accountName, shareName, sizeGiB)
 }
 
-// GetStorageAccesskey get Azure storage account key
-func (d *Driver) GetStorageAccesskey(accountOptions *azure.AccountOptions, secrets map[string]string, secretNamespace string) (string, error) {
+// GetStorageAccesskey get Azure storage account key from
+// 	1. secrets (if not empty)
+// 	2. use k8s client identity to read from k8s secret
+// 	3. use cluster identity to get from storage account directly
+func (d *Driver) GetStorageAccesskey(accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) {
 	if len(secrets) > 0 {
 		_, accountKey, err := getStorageAccount(secrets)
 		return accountKey, err
 	}
 
 	// read from k8s secret first
-	accountKey, err := d.GetStorageAccesskeyFromSecret(accountOptions.Name, secretNamespace)
+	if secretName == "" {
+		secretName = fmt.Sprintf(secretNameTemplate, accountOptions.Name)
+	}
+	accountKey, err := d.GetStorageAccesskeyFromSecret(secretName, secretNamespace)
 	if err != nil {
-		klog.V(2).Infof("could not get account(%s) key from secret, error: %v, use cluster identity to get account key instead", accountOptions.Name, err)
+		klog.V(2).Infof("could not get account(%s) key from secret(%s), error: %v, use cluster identity to get account key instead", accountOptions.Name, secretName, err)
 		return d.cloud.GetStorageAccesskey(accountOptions.Name, accountOptions.ResourceGroup)
 	}
 
@@ -542,12 +556,11 @@ func (d *Driver) GetStorageAccesskey(accountOptions *azure.AccountOptions, secre
 }
 
 // GetStorageAccesskeyFromSecret get storage account key from k8s secret
-func (d *Driver) GetStorageAccesskeyFromSecret(accountName, secretNamespace string) (string, error) {
+func (d *Driver) GetStorageAccesskeyFromSecret(secretName, secretNamespace string) (string, error) {
 	if d.cloud.KubeClient == nil {
-		return "", fmt.Errorf("could not get account(%s) key from secret: KubeClient is nil", accountName)
+		return "", fmt.Errorf("could not get account key from secret(%s): KubeClient is nil", secretName)
 	}
 
-	secretName := fmt.Sprintf(secretNameTemplate, accountName)
 	if secretNamespace == "" {
 		secretNamespace = defaultSecretNamespace
 	}
diff --git a/pkg/azurefile/controllerserver.go b/pkg/azurefile/controllerserver.go
index 0dda52048..847dcb405 100644
--- a/pkg/azurefile/controllerserver.go
+++ b/pkg/azurefile/controllerserver.go
@@ -91,7 +91,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	if parameters == nil {
 		parameters = make(map[string]string)
 	}
-	var sku, resourceGroup, location, account, fileShareName, diskName, fsType, storeAccountKey, secretNamespace, protocol, customTags string
+	var sku, resourceGroup, location, account, fileShareName, diskName, fsType, storeAccountKey, secretName, secretNamespace, protocol, customTags string
 
 	// Apply ProvisionerParameters (case-insensitive). We leave validation of
 	// the values to the cloud provider.
@@ -115,6 +115,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 			fsType = v
 		case storeAccountKeyField:
 			storeAccountKey = v
+		case secretNameField:
+			secretName = v
 		case secretNamespaceField:
 			secretNamespace = v
 		case protocolField:
@@ -252,7 +254,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 
 	if isDiskFsType(fsType) && diskName == "" {
 		if accountKey == "" {
-			if accountKey, err = d.GetStorageAccesskey(accountOptions, req.GetSecrets(), secretNamespace); err != nil {
+			if accountKey, err = d.GetStorageAccesskey(accountOptions, req.GetSecrets(), secretName, secretNamespace); err != nil {
 				return nil, fmt.Errorf("failed to GetStorageAccesskey on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
 			}
 		}
@@ -276,16 +278,16 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 
 	if storeAccountKey != storeAccountKeyFalse && len(req.GetSecrets()) == 0 {
 		if accountKey == "" {
-			if accountKey, err = d.GetStorageAccesskey(accountOptions, req.GetSecrets(), secretNamespace); err != nil {
+			if accountKey, err = d.GetStorageAccesskey(accountOptions, req.GetSecrets(), secretName, secretNamespace); err != nil {
 				return nil, fmt.Errorf("failed to GetStorageAccesskey on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
 			}
 		}
-		secretName, err := setAzureCredentials(d.cloud.KubeClient, accountName, accountKey, secretNamespace)
+		storeSecretName, err := setAzureCredentials(d.cloud.KubeClient, accountName, accountKey, secretName, secretNamespace)
 		if err != nil {
 			return nil, status.Errorf(codes.Internal, "failed to store storage account key: %v", err)
 		}
-		if secretName != "" {
-			klog.V(2).Infof("store account key to k8s secret(%v) in %s namespace", secretName, secretNamespace)
+		if storeSecretName != "" {
+			klog.V(2).Infof("store account key to k8s secret(%v) in %s namespace", storeSecretName, secretNamespace)
 		}
 	}
 
diff --git a/pkg/azurefile/utils.go b/pkg/azurefile/utils.go
index b1cc2d8fb..1f412f2ac 100644
--- a/pkg/azurefile/utils.go
+++ b/pkg/azurefile/utils.go
@@ -77,7 +77,7 @@ func (lm *lockMap) unlockEntry(entry string) {
 	lm.mutexMap[entry].Unlock()
 }
 
-func setAzureCredentials(kubeClient kubernetes.Interface, accountName, accountKey, secretNamespace string) (string, error) {
+func setAzureCredentials(kubeClient kubernetes.Interface, accountName, accountKey, secretName, secretNamespace string) (string, error) {
 	if kubeClient == nil {
 		klog.Warningf("could not create secret: kubeClient is nil")
 		return "", nil
@@ -88,10 +88,12 @@ func setAzureCredentials(kubeClient kubernetes.Interface, accountName, accountKe
 	if secretNamespace == "" {
 		secretNamespace = defaultSecretNamespace
 	}
-	secretName := fmt.Sprintf(secretNameTemplate, accountName)
+	if secretName == "" {
+		secretName = fmt.Sprintf(secretNameTemplate, accountName)
+	}
 	secret := &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace: defaultSecretNamespace,
+			Namespace: secretNamespace,
 			Name:      secretName,
 		},
 		Data: map[string][]byte{


Test patch:
diff --git a/pkg/azurefile/controllerserver_test.go b/pkg/azurefile/controllerserver_test.go
index 5f5e5cfc5..0f5857530 100644
--- a/pkg/azurefile/controllerserver_test.go
+++ b/pkg/azurefile/controllerserver_test.go
@@ -497,9 +497,9 @@ func TestCreateVolume(t *testing.T) {
 
 				ctx := context.Background()
 
-				expectedErr := status.Error(codes.Internal, "failed to store storage account key: couldn't create secret request namespace does not match object namespace, request: \"secretnamespace\" object: \"default\"")
+				// expectedErr := status.Error(codes.Internal, "failed to store storage account key: couldn't create secret request namespace does not match object namespace, request: \"secretnamespace\" object: \"default\"")
 				_, err := d.CreateVolume(ctx, req)
-				if !reflect.DeepEqual(err, expectedErr) {
+				if !reflect.DeepEqual(err, nil) {
 					t.Errorf("Unexpected error: %v", err)
 				}
 			},
@@ -1617,7 +1617,7 @@ func TestSnapshotExists(t *testing.T) {
 		d.cloud.StorageAccountClient = mockStorageAccountsClient
 		d.cloud.KubeClient = clientSet
 		d.cloud.Environment = azure2.Environment{StorageEndpointSuffix: "abc"}
-		mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), "vol_1", gomock.Any()).Return(test.key, nil).AnyTimes()
+		mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), "", gomock.Any()).Return(test.key, nil).AnyTimes()
 
 		_, _, err := d.snapshotExists(context.Background(), test.sourceVolumeID, "sname", validSecret)
 		if !reflect.DeepEqual(err, test.expectedErr) {
diff --git a/pkg/azurefile/utils_test.go b/pkg/azurefile/utils_test.go
index c85925324..04f8592d0 100644
--- a/pkg/azurefile/utils_test.go
+++ b/pkg/azurefile/utils_test.go
@@ -107,6 +107,7 @@ func TestSetAzureCredentials(t *testing.T) {
 		kubeClient      kubernetes.Interface
 		accountName     string
 		accountKey      string
+		secretName      string
 		secretNamespace string
 		expectedName    string
 		expectedErr     error
@@ -144,10 +145,20 @@ func TestSetAzureCredentials(t *testing.T) {
 			expectedName: "azure-storage-account-testName-secret",
 			expectedErr:  nil,
 		},
+		{
+			desc:            "[success] normal scenario using secretName",
+			kubeClient:      fakeClient,
+			accountName:     "testName",
+			accountKey:      "testKey",
+			secretName:      "secretName",
+			secretNamespace: "secretNamespace",
+			expectedName:    "secretName",
+			expectedErr:     nil,
+		},
 	}
 
 	for _, test := range tests {
-		result, err := setAzureCredentials(test.kubeClient, test.accountName, test.accountKey, test.secretNamespace)
+		result, err := setAzureCredentials(test.kubeClient, test.accountName, test.accountKey, test.secretName, test.secretNamespace)
 		if result != test.expectedName || !reflect.DeepEqual(err, test.expectedErr) {
 			t.Errorf("desc: %s,\n input: kubeClient(%v), accountName(%v), accountKey(%v),\n setAzureCredentials result: %v, expectedName: %v err: %v, expectedErr: %v",
 				test.desc, test.kubeClient, test.accountName, test.accountKey, result, test.expectedName, err, test.expectedErr)
diff --git a/test/e2e/dynamic_provisioning_test.go b/test/e2e/dynamic_provisioning_test.go
index 24f99273b..80ba2e59f 100644
--- a/test/e2e/dynamic_provisioning_test.go
+++ b/test/e2e/dynamic_provisioning_test.go
@@ -119,9 +119,13 @@ var _ = ginkgo.Describe("Dynamic Provisioning", func() {
 			},
 		}
 		test := testsuites.DynamicallyProvisionedCmdVolumeTest{
-			CSIDriver:              testDriver,
-			Pods:                   pods,
-			StorageClassParameters: map[string]string{"skuName": "Standard_LRS"},
+			CSIDriver: testDriver,
+			Pods:      pods,
+			StorageClassParameters: map[string]string{
+				"skuName":         "Standard_LRS",
+				"secretName":      "sercet-test",
+				"sectetNamespace": "test",
+			},
 		}
 		test.Run(cs, ns)
 	})