Repository: kubernetes-csi/csi-driver-smb
Pull request #402 (2022-01-04 13:02:42)

feat: support mountOptions in DeleteVolume
<!--  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: support mountOptions in DeleteVolume

#### provide `mountOptions` for `DeleteVolume`
> since `DeleteVolumeRequest` does not provide `mountOptions`, following is the workaround to provide `mountOptions` for `DeleteVolume`
  - create a secret `smbcreds` with `mountOptions`
```console
kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD" --from-literal mountOptions="dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks"
```
 - set `csi.storage.k8s.io/provisioner-secret-name: "smbcreds"` in storage class
 - 
**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: support mountOptions in DeleteVolume
```


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

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/csi-driver-smb/pull/402#" 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-csi%2Fcsi-driver-smb).

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-csi/csi-driver-smb/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":[]} -->
coveralls: ## Pull Request Test Coverage Report for [Build 1653718142](https://coveralls.io/builds/45361211)

* **11** of **21**   **(52.38%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage decreased (**-0.7%**) to **85.383%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [pkg/smb/controllerserver.go](https://coveralls.io/builds/45361211/source?filename=pkg%2Fsmb%2Fcontrollerserver.go#L129) | 4 | 14 | 28.57%
<!-- | **Total:** | **11** | **21** | **52.38%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/45361211/badge)](https://coveralls.io/builds/45361211) |
| :-- | --: |
| Change from base [Build 1653700983](https://coveralls.io/builds/45361151): |  -0.7% |
| Covered Lines: | 847 |
| Relevant Lines: | 992 |

---
##### 💛  - [Coveralls](https://coveralls.io)

k8s-ci-robot: @andyzhangx: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-csi-driver-smb-external-e2e | 461aef42f52c44169d4e3de5674d3de6c138118c | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-driver-smb/402/pull-csi-driver-smb-external-e2e/1478355275437051904) | true | `/test pull-csi-driver-smb-external-e2e`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=csi-driver-smb&pr=402). [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-csi/csi-driver-smb/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 or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-csi-driver-smb-external-e2e | 461aef42f52c44169d4e3de5674d3de6c138118c | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-driver-smb/402/pull-csi-driver-smb-external-e2e/1478355275437051904) | true | `/test pull-csi-driver-smb-external-e2e`
pull-csi-driver-smb-e2e-windows-containerd | 461aef42f52c44169d4e3de5674d3de6c138118c | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-driver-smb/402/pull-csi-driver-smb-e2e-windows-containerd/1478355274505916416) | true | `/test pull-csi-driver-smb-e2e-windows-containerd`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=csi-driver-smb&pr=402). [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-csi/csi-driver-smb/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/Makefile b/Makefile
index 14807f96c..b728a16e7 100644
--- a/Makefile
+++ b/Makefile
@@ -213,7 +213,7 @@ clean:
 .PHONY: install-smb-provisioner
 install-smb-provisioner:
 	kubectl delete secret smbcreds --ignore-not-found
-	kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD"
+	kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD" --from-literal mountOptions="dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks"
 ifdef TEST_WINDOWS
 	kubectl apply -f deploy/example/smb-provisioner/smb-server-lb.yaml
 else
diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md
index 82b7d2f51..967effdc9 100644
--- a/docs/driver-parameters.md
+++ b/docs/driver-parameters.md
@@ -25,4 +25,14 @@ nodeStageSecretRef.namespace | namespace where the secret is | k8s namespace  |
  - Use `kubectl create secret` to create `smbcreds` secret to store Samba Server username, password
 ```console
 kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD"
-```
\ No newline at end of file
+```
+
+### Tips
+#### provide `mountOptions` for `DeleteVolume`
+> since `DeleteVolumeRequest` does not provide `mountOptions`, following is the workaround to provide `mountOptions` for `DeleteVolume`
+  - create a secret `smbcreds` with `mountOptions`
+```console
+kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD" --from-literal mountOptions="dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks"
+```
+
+ - set `csi.storage.k8s.io/provisioner-secret-name: "smbcreds"` in storage class
\ No newline at end of file
diff --git a/pkg/smb/controllerserver.go b/pkg/smb/controllerserver.go
index 1ad737b26..dc916553f 100644
--- a/pkg/smb/controllerserver.go
+++ b/pkg/smb/controllerserver.go
@@ -123,13 +123,26 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
 		return &csi.DeleteVolumeResponse{}, nil
 	}
 
+	var volCap *csi.VolumeCapability
+	mountOptions := getMountOptions(req.GetSecrets())
+	if mountOptions != "" {
+		klog.V(2).Infof("DeleteVolume: found mountOptions(%v) for volume(%s)", mountOptions, volumeID)
+		volCap = &csi.VolumeCapability{
+			AccessType: &csi.VolumeCapability_Mount{
+				Mount: &csi.VolumeCapability_MountVolume{
+					MountFlags: []string{mountOptions},
+				},
+			},
+		}
+	}
+
 	secrets := req.GetSecrets()
 	if len(secrets) > 0 {
 		if len(smbVol.uuid) > 0 {
 			klog.V(2).Infof("existing subDir(%s) is provided, skip subdirectory creation", smbVol.subDir)
 		} else {
 			// Mount smb base share so we can delete the subdirectory
-			if err = d.internalMount(ctx, smbVol, nil, secrets); err != nil {
+			if err = d.internalMount(ctx, smbVol, volCap, secrets); err != nil {
 				return nil, status.Errorf(codes.Internal, "failed to mount smb server: %v", err.Error())
 			}
 			defer func() {
@@ -146,7 +159,7 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
 			}
 		}
 	} else {
-		klog.Infof("DeleteVolume(%s) does not provide secrets", volumeID)
+		klog.V(2).Infof("DeleteVolume(%s) does not provide secrets", volumeID)
 	}
 
 	return &csi.DeleteVolumeResponse{}, nil
diff --git a/pkg/smb/smb.go b/pkg/smb/smb.go
index 3c86e4664..96d598e31 100644
--- a/pkg/smb/smb.go
+++ b/pkg/smb/smb.go
@@ -17,7 +17,7 @@ limitations under the License.
 package smb
 
 import (
-	"fmt"
+	"strings"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
 
@@ -35,6 +35,7 @@ const (
 	sourceField       = "source"
 	subDirField       = "subdir"
 	domainField       = "domain"
+	mountOptionsField = "mountoptions"
 	defaultDomainName = "AZURE"
 )
 
@@ -67,7 +68,7 @@ func (d *Driver) Run(endpoint, kubeconfig string, testMode bool) {
 	if err != nil {
 		klog.Fatalf("%v", err)
 	}
-	klog.Infof("\nDRIVER INFORMATION:\n-------------------\n%s\n\nStreaming logs below:", versionMeta)
+	klog.V(2).Infof("\nDRIVER INFORMATION:\n-------------------\n%s\n\nStreaming logs below:", versionMeta)
 
 	d.mounter, err = mounter.NewSafeMounter()
 	if err != nil {
@@ -109,6 +110,16 @@ func (d *Driver) Run(endpoint, kubeconfig string, testMode bool) {
 
 func IsCorruptedDir(dir string) bool {
 	_, pathErr := mount.PathExists(dir)
-	fmt.Printf("IsCorruptedDir(%s) returned with error: %v", dir, pathErr)
 	return pathErr != nil && mount.IsCorruptedMnt(pathErr)
 }
+
+// getMountOptions get mountOptions value from a map
+func getMountOptions(context map[string]string) string {
+	for k, v := range context {
+		switch strings.ToLower(k) {
+		case mountOptionsField:
+			return v
+		}
+	}
+	return ""
+}


Test patch:
diff --git a/pkg/smb/smb_test.go b/pkg/smb/smb_test.go
index d37e6e22e..408a91dbc 100644
--- a/pkg/smb/smb_test.go
+++ b/pkg/smb/smb_test.go
@@ -100,3 +100,39 @@ func TestRun(t *testing.T) {
 		t.Run(tc.name, tc.testFunc)
 	}
 }
+
+func TestGetMountOptions(t *testing.T) {
+	tests := []struct {
+		desc    string
+		context map[string]string
+		result  string
+	}{
+		{
+			desc:    "nil context",
+			context: nil,
+			result:  "",
+		},
+		{
+			desc:    "empty context",
+			context: map[string]string{},
+			result:  "",
+		},
+		{
+			desc:    "valid mountOptions",
+			context: map[string]string{"mountOptions": "dir_mode=0777"},
+			result:  "dir_mode=0777",
+		},
+		{
+			desc:    "valid mountOptions(lowercase)",
+			context: map[string]string{"mountoptions": "dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks"},
+			result:  "dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks",
+		},
+	}
+
+	for _, test := range tests {
+		result := getMountOptions(test.context)
+		if result != test.result {
+			t.Errorf("Unexpected result: %s, expected: %s", result, test.result)
+		}
+	}
+}