Repository: kubernetes-csi/csi-driver-nfs
Pull request #341 (2022-05-13 13:46:28)

chore: refine bypass chmod code
<!--  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 cleanup

**What this PR does / why we need it**:
chore: refine bypass chmod code

**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 #

**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
none
```



Patch:
diff --git a/pkg/nfs/nodeserver.go b/pkg/nfs/nodeserver.go
index 4ddb9e28..d1558d6f 100644
--- a/pkg/nfs/nodeserver.go
+++ b/pkg/nfs/nodeserver.go
@@ -121,19 +121,9 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	}
 
 	if performChmodOp {
-		info, err := os.Lstat(targetPath)
-		if err != nil {
+		if err := chmodIfPermissionMismatch(targetPath, os.FileMode(mountPermissions)); err != nil {
 			return nil, status.Error(codes.Internal, err.Error())
 		}
-		perm := info.Mode() & os.ModePerm
-		if perm != os.FileMode(mountPermissions) {
-			klog.V(2).Infof("volumeID(%v): chmod targetPath(%s, mode:0%o) with permissions(0%o)", volumeID, targetPath, info.Mode(), mountPermissions)
-			if err := os.Chmod(targetPath, os.FileMode(mountPermissions)); err != nil {
-				return nil, status.Error(codes.Internal, err.Error())
-			}
-		} else {
-			klog.V(2).Infof("skip chmod on targetPath(%s) since mode is already 0%o)", targetPath, info.Mode())
-		}
 	} else {
 		klog.V(2).Infof("skip chmod on targetPath(%s) since mountPermissions is set as 0", targetPath)
 	}
diff --git a/pkg/nfs/utils.go b/pkg/nfs/utils.go
index d0810bb5..0798c2ec 100644
--- a/pkg/nfs/utils.go
+++ b/pkg/nfs/utils.go
@@ -18,6 +18,7 @@ package nfs
 
 import (
 	"fmt"
+	"os"
 	"strings"
 	"sync"
 
@@ -132,3 +133,21 @@ func getMountOptions(context map[string]string) string {
 	}
 	return ""
 }
+
+// chmodIfPermissionMismatch only perform chmod when permission mismatches
+func chmodIfPermissionMismatch(targetPath string, mode os.FileMode) error {
+	info, err := os.Lstat(targetPath)
+	if err != nil {
+		return err
+	}
+	perm := info.Mode() & os.ModePerm
+	if perm != mode {
+		klog.V(2).Infof("chmod targetPath(%s, mode:0%o) with permissions(0%o)", targetPath, info.Mode(), mode)
+		if err := os.Chmod(targetPath, mode); err != nil {
+			return err
+		}
+	} else {
+		klog.V(2).Infof("skip chmod on targetPath(%s) since mode is already 0%o)", targetPath, info.Mode())
+	}
+	return nil
+}


Test patch:
diff --git a/pkg/nfs/utils_test.go b/pkg/nfs/utils_test.go
index 9a8f0c9d..85612049 100644
--- a/pkg/nfs/utils_test.go
+++ b/pkg/nfs/utils_test.go
@@ -18,6 +18,9 @@ package nfs
 
 import (
 	"fmt"
+	"os"
+	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -154,3 +157,57 @@ func TestGetMountOptions(t *testing.T) {
 		}
 	}
 }
+
+func TestChmodIfPermissionMismatch(t *testing.T) {
+	permissionMatchingPath, _ := getWorkDirPath("permissionMatchingPath")
+	_ = makeDir(permissionMatchingPath)
+	defer os.RemoveAll(permissionMatchingPath)
+
+	permissionMismatchPath, _ := getWorkDirPath("permissionMismatchPath")
+	_ = os.MkdirAll(permissionMismatchPath, os.FileMode(0721))
+	defer os.RemoveAll(permissionMismatchPath)
+
+	tests := []struct {
+		desc          string
+		path          string
+		mode          os.FileMode
+		expectedError error
+	}{
+		{
+			desc:          "Invalid path",
+			path:          "invalid-path",
+			mode:          0755,
+			expectedError: fmt.Errorf("CreateFile invalid-path: The system cannot find the file specified"),
+		},
+		{
+			desc:          "permission matching path",
+			path:          permissionMatchingPath,
+			mode:          0755,
+			expectedError: nil,
+		},
+		{
+			desc:          "permission mismatch path",
+			path:          permissionMismatchPath,
+			mode:          0755,
+			expectedError: nil,
+		},
+	}
+
+	for _, test := range tests {
+		err := chmodIfPermissionMismatch(test.path, test.mode)
+		if !reflect.DeepEqual(err, test.expectedError) {
+			if err == nil || test.expectedError == nil && !strings.Contains(err.Error(), test.expectedError.Error()) {
+				t.Errorf("test[%s]: unexpected error: %v, expected error: %v", test.desc, err, test.expectedError)
+			}
+		}
+	}
+}
+
+// getWorkDirPath returns the path to the current working directory
+func getWorkDirPath(dir string) (string, error) {
+	path, err := os.Getwd()
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("%s%c%s", path, os.PathSeparator, dir), nil
+}