From d2344ce6420328c3751e8b2d8950343fd1c60105 Mon Sep 17 00:00:00 2001
From: andyzhangx <xiazhang@microsoft.com>
Date: Fri, 13 May 2022 13:45:47 +0000
Subject: [PATCH] chore: refine bypass chmod code

---
 pkg/nfs/nodeserver.go | 12 +--------
 pkg/nfs/utils.go      | 19 +++++++++++++++
 pkg/nfs/utils_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 11 deletions(-)

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
+}
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
+}