Repository: kubernetes-sigs/azurefile-csi-driver
Pull request #465 (2020-11-01 14:47:49)

fix: static NFS pv config issue
<!--  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 bug

**What this PR does / why we need it**:
fix: static NFS pv config issue

**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**:
```
fix: static NFS pv config issue
```



Patch:
diff --git a/deploy/example/pv-azurefile-csi.yaml b/deploy/example/pv-azurefile-csi.yaml
index 6c5ebbcaa..e8e51b895 100644
--- a/deploy/example/pv-azurefile-csi.yaml
+++ b/deploy/example/pv-azurefile-csi.yaml
@@ -22,8 +22,9 @@ spec:
     readOnly: false
     volumeHandle: unique-volumeid  # make sure it's a unique id in the cluster
     volumeAttributes:
+      resourceGroup: EXISTING_RESOURCE_GROUP_NAME  # optional, only set this when storage account is not in the same resource group as agent node
+      storageAccount: EXISTING_STORAGE_ACCOUNT_NAME
       shareName: EXISTING_FILE_SHARE_NAME  # only file share name, don't use full path
-      server: SERVER_ADDRESS  # optional, provide a new address to replace default "accountname.file.core.windows.net"
     nodeStageSecretRef:
       name: azure-secret
       namespace: default
diff --git a/deploy/example/pv-azurefile-nfs.yaml b/deploy/example/pv-azurefile-nfs.yaml
new file mode 100644
index 000000000..3fdd3cbd5
--- /dev/null
+++ b/deploy/example/pv-azurefile-nfs.yaml
@@ -0,0 +1,20 @@
+---
+apiVersion: v1
+kind: PersistentVolume
+metadata:
+  name: pv-azurefile
+spec:
+  capacity:
+    storage: 10Gi
+  accessModes:
+    - ReadWriteMany
+  persistentVolumeReclaimPolicy: Retain  # if set as "Delete" file share would be removed in pvc deletion
+  csi:
+    driver: file.csi.azure.com
+    readOnly: false
+    volumeHandle: unique-volumeid  # make sure it's a unique id in the cluster
+    volumeAttributes:
+      resourceGroup: EXISTING_RESOURCE_GROUP_NAME  # optional, only set this when storage account is not in the same resource group as agent node
+      storageAccount: EXISTING_STORAGE_ACCOUNT_NAME
+      shareName: EXISTING_FILE_SHARE_NAME  # only file share name, don't use full path
+      protocol: nfs
diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md
index 9140c7240..5d2dbdd0b 100644
--- a/docs/driver-parameters.md
+++ b/docs/driver-parameters.md
@@ -22,12 +22,21 @@ diskName | existing VHD disk file name | `pvc-062196a6-6436-11ea-ab51-9efb888c0a
 fsType | File System Type | `nfs` | Yes | `nfs`
 
 ### Static Provision(bring your own file share)
-  > get a [example](../deploy/example/pv-azurefile-csi.yaml)
+  > get a [smb pv example](../deploy/example/pv-azurefile-csi.yaml)
+
+  > get a [nfs pv example](../deploy/example/pv-azurefile-nfs.yaml)
 
 Name | Meaning | Available Value | Mandatory | Default value
 --- | --- | --- | --- | ---
-volumeAttributes.sharename | Azure file share name | existing Azure file share name | Yes |
+volumeAttributes.resourceGroup | Azure resource group name | existing resource group name | No | if empty, driver will use the same resource group name as current k8s cluster
+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`
 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
+ ```
\ No newline at end of file
diff --git a/pkg/azurefile/azurefile.go b/pkg/azurefile/azurefile.go
index ea5bfffac..df2aa5f55 100644
--- a/pkg/azurefile/azurefile.go
+++ b/pkg/azurefile/azurefile.go
@@ -415,6 +415,26 @@ func IsCorruptedDir(dir string) bool {
 
 // GetAccountInfo get account info
 func (d *Driver) GetAccountInfo(volumeID string, secrets, reqContext map[string]string) (rgName, accountName, accountKey, fileShareName, diskName string, err error) {
+	var protocol string
+	for k, v := range reqContext {
+		switch strings.ToLower(k) {
+		case resourceGroupField:
+			rgName = v
+		case storageAccountField:
+			accountName = v
+		case shareNameField:
+			fileShareName = v
+		case diskNameField:
+			diskName = v
+		case protocolField:
+			protocol = v
+		}
+	}
+
+	if protocol == nfs {
+		return rgName, accountName, accountKey, fileShareName, diskName, err
+	}
+
 	if len(secrets) == 0 {
 		rgName, accountName, fileShareName, diskName, err = GetFileShareInfo(volumeID)
 		if err == nil {
@@ -430,14 +450,6 @@ func (d *Driver) GetAccountInfo(volumeID string, secrets, reqContext map[string]
 			}
 		}
 	} else {
-		for k, v := range reqContext {
-			switch strings.ToLower(k) {
-			case shareNameField:
-				fileShareName = v
-			case diskNameField:
-				diskName = v
-			}
-		}
 		accountName, accountKey, err = getStorageAccount(secrets)
 		// if volumeID is valid, get info from volumeID, ignore volumeID parsing error
 		if rg, _, fileShare, disk, err := GetFileShareInfo(volumeID); err == nil {


Test patch:
diff --git a/pkg/azurefile/azurefile_test.go b/pkg/azurefile/azurefile_test.go
index 08f01c237..10a30c912 100644
--- a/pkg/azurefile/azurefile_test.go
+++ b/pkg/azurefile/azurefile_test.go
@@ -530,45 +530,73 @@ func TestGetAccountInfo(t *testing.T) {
 	clientSet := fake.NewSimpleClientset()
 
 	tests := []struct {
-		volumeID   string
-		resGroup   string
-		secrets    map[string]string
-		reqContext map[string]string
-		expectErr  bool
-		err        error
+		volumeID            string
+		rgName              string
+		secrets             map[string]string
+		reqContext          map[string]string
+		expectErr           bool
+		err                 error
+		expectAccountName   string
+		expectFileShareName string
+		expectDiskName      string
 	}{
 		{
 			volumeID: "##",
-			resGroup: "",
+			rgName:   "",
 			secrets:  emptySecret,
 			reqContext: map[string]string{
 				shareNameField: "test_sharename",
 				diskNameField:  "test_diskname",
 			},
-			expectErr: false,
-			err:       nil,
+			expectErr:           false,
+			err:                 nil,
+			expectAccountName:   "",
+			expectFileShareName: "",
+			expectDiskName:      "",
 		},
 		{
 			volumeID: "vol_1##",
-			resGroup: "vol_1",
+			rgName:   "vol_1",
 			secrets:  validSecret,
 			reqContext: map[string]string{
 				shareNameField: "test_sharename",
 				diskNameField:  "test_diskname",
 			},
-			expectErr: false,
-			err:       nil,
+			expectErr:           false,
+			err:                 nil,
+			expectAccountName:   "testaccount",
+			expectFileShareName: "test_sharename",
+			expectDiskName:      "test_diskname",
 		},
 		{
-			volumeID: "vol_1##",
-			resGroup: "vol_1",
+			volumeID: "vol_2##",
+			rgName:   "vol_2",
 			secrets:  emptySecret,
 			reqContext: map[string]string{
 				shareNameField: "test_sharename",
 				diskNameField:  "test_diskname",
 			},
-			expectErr: false,
-			err:       nil,
+			expectErr:           false,
+			err:                 nil,
+			expectAccountName:   "",
+			expectFileShareName: "",
+			expectDiskName:      "",
+		},
+		{
+			volumeID: "uniqe-volumeid-nfs",
+			rgName:   "vol_nfs",
+			secrets:  emptySecret,
+			reqContext: map[string]string{
+				resourceGroupField:  "vol_nfs",
+				storageAccountField: "test_accountname",
+				shareNameField:      "test_sharename",
+				protocolField:       "nfs",
+			},
+			expectErr:           false,
+			err:                 nil,
+			expectAccountName:   "test_accountname",
+			expectFileShareName: "test_sharename",
+			expectDiskName:      "",
 		},
 	}
 
@@ -577,8 +605,8 @@ func TestGetAccountInfo(t *testing.T) {
 		d.cloud.StorageAccountClient = mockStorageAccountsClient
 		d.cloud.KubeClient = clientSet
 		d.cloud.Environment = azure2.Environment{StorageEndpointSuffix: "abc"}
-		mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), test.resGroup, gomock.Any()).Return(key, nil).AnyTimes()
-		_, _, _, _, _, err := d.GetAccountInfo(test.volumeID, test.secrets, test.reqContext)
+		mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), test.rgName, gomock.Any()).Return(key, nil).AnyTimes()
+		rgName, accountName, _, fileShareName, diskName, err := d.GetAccountInfo(test.volumeID, test.secrets, test.reqContext)
 		if test.expectErr && err == nil {
 			t.Errorf("Unexpected non-error")
 			continue
@@ -587,6 +615,13 @@ func TestGetAccountInfo(t *testing.T) {
 			t.Errorf("Unexpected error: %v", err)
 			continue
 		}
+
+		if err == nil {
+			assert.Equal(t, test.rgName, rgName, test.volumeID)
+			assert.Equal(t, test.expectAccountName, accountName, test.volumeID)
+			assert.Equal(t, test.expectFileShareName, fileShareName, test.volumeID)
+			assert.Equal(t, test.expectDiskName, diskName, test.volumeID)
+		}
 	}
 }