Merge pull request #843 from andyzhangx/mount-with-0777

fix: mount with 0777 in volume creation and deletion

Author: andyzhangx

.github/workflows/trivy.yaml

@@ -12,7 +12,7 @@ jobs:
       - name: Set up Go 1.x
         uses: actions/setup-go@v5
         with:
-          go-version: 1.22.5
+          go-version: 1.23.1
         id: go
 
       - name: Checkout code

pkg/smb/controllerserver.go

@@ -19,7 +19,6 @@ package smb
 import (
 	"context"
 	"fmt"
-	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -92,12 +91,17 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	}
 
 	volCap := volumeCapabilities[0]
-	if volCap.GetMount() != nil && !createSubDir {
+	if volCap.GetMount() != nil {
 		options := volCap.GetMount().GetMountFlags()
-		if hasGuestMountOptions(options) {
+		if !createSubDir && hasGuestMountOptions(options) {
 			klog.V(2).Infof("guest mount option(%v) is provided, create subdirectory", options)
 			createSubDir = true
 		}
+		// set default file/dir mode
+		volCap.GetMount().MountFlags = appendMountOptions(options, map[string]string{
+			fileMode: defaultFileMode,
+			dirMode:  defaultDirMode,
+		})
 	}
 
 	if acquired := d.volumeLocks.TryAcquire(name); !acquired {
@@ -159,18 +163,22 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
 	}
 	defer d.volumeLocks.Release(volumeID)
 
-	var volCap *csi.VolumeCapability
 	secrets := req.GetSecrets()
 	mountOptions := getMountOptions(secrets)
 	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},
-				},
+	}
+	// set default file/dir mode
+	volCap := &csi.VolumeCapability{
+		AccessType: &csi.VolumeCapability_Mount{
+			Mount: &csi.VolumeCapability_MountVolume{
+				MountFlags: appendMountOptions([]string{mountOptions},
+					map[string]string{
+						fileMode: defaultFileMode,
+						dirMode:  defaultDirMode,
+					}),
 			},
-		}
+		},
 	}
 
 	if smbVol.onDelete == "" {
@@ -224,14 +232,6 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
 				return nil, status.Errorf(codes.Internal, "archive subdirectory(%s, %s) failed with %v", internalVolumePath, archivedInternalVolumePath, err)
 			}
 		} else {
-			if _, err2 := os.Lstat(internalVolumePath); err2 == nil {
-				err2 := filepath.WalkDir(internalVolumePath, func(path string, _ fs.DirEntry, _ error) error {
-					return os.Chmod(path, 0777)
-				})
-				if err2 != nil {
-					klog.Errorf("failed to chmod subdirectory: %v", err2)
-				}
-			}
 			klog.V(2).Infof("Removing subdirectory at %v", internalVolumePath)
 			if err = os.RemoveAll(internalVolumePath); err != nil {
 				return nil, status.Errorf(codes.Internal, "failed to delete subdirectory: %v", err)

pkg/smb/smb.go

@@ -52,6 +52,10 @@ const (
 	DefaultKrb5CacheDirectory = "/var/lib/kubelet/kerberos/"
 	retain                    = "retain"
 	archive                   = "archive"
+	fileMode                  = "file_mode"
+	dirMode                   = "dir_mode"
+	defaultFileMode           = "0777"
+	defaultDirMode            = "0777"
 )
 
 var supportedOnDeleteValues = []string{"", "delete", retain, archive}
@@ -234,3 +238,28 @@ func validateOnDeleteValue(onDelete string) error {
 
 	return fmt.Errorf("invalid value %s for OnDelete, supported values are %v", onDelete, supportedOnDeleteValues)
 }
+
+// appendMountOptions appends extra mount options to the given mount options
+func appendMountOptions(mountOptions []string, extraMountOptions map[string]string) []string {
+	// stores the mount options already included in mountOptions
+	included := make(map[string]bool)
+	for _, mountOption := range mountOptions {
+		for k := range extraMountOptions {
+			if strings.HasPrefix(mountOption, k) {
+				included[k] = true
+			}
+		}
+	}
+
+	allMountOptions := mountOptions
+	for k, v := range extraMountOptions {
+		if _, isIncluded := included[k]; !isIncluded {
+			if v != "" {
+				allMountOptions = append(allMountOptions, fmt.Sprintf("%s=%s", k, v))
+			} else {
+				allMountOptions = append(allMountOptions, k)
+			}
+		}
+	}
+	return allMountOptions
+}

pkg/smb/smb_test.go

@@ -338,3 +338,50 @@ func TestValidateOnDeleteValue(t *testing.T) {
 		}
 	}
 }
+
+func TestAppendMountOptions(t *testing.T) {
+	tests := []struct {
+		desc     string
+		options  []string
+		newOpts  map[string]string
+		expected []string
+	}{
+		{
+			desc:     "empty options",
+			options:  nil,
+			newOpts:  map[string]string{},
+			expected: nil,
+		},
+		{
+			desc:     "empty newOpts",
+			options:  []string{"a", "b"},
+			newOpts:  map[string]string{},
+			expected: []string{"a", "b"},
+		},
+		{
+			desc:     "empty newOpts",
+			options:  []string{"a", "b"},
+			newOpts:  map[string]string{"c": "d"},
+			expected: []string{"a", "b", "c=d"},
+		},
+		{
+			desc:     "duplicate newOpts",
+			options:  []string{"a", "b", "c=d"},
+			newOpts:  map[string]string{"c": "d"},
+			expected: []string{"a", "b", "c=d"},
+		},
+		{
+			desc:     "normal newOpts",
+			options:  []string{"a", "b"},
+			newOpts:  map[string]string{"c": "d", "e": "f"},
+			expected: []string{"a", "b", "c=d", "e=f"},
+		},
+	}
+
+	for _, test := range tests {
+		result := appendMountOptions(test.options, test.newOpts)
+		if !reflect.DeepEqual(result, test.expected) {
+			t.Errorf("test[%s]: unexpected output: %v, expected result: %v", test.desc, result, test.expected)
+		}
+	}
+}