Merge pull request #619 from andyzhangx/mountPermissions

andyzhangx · web-flow · commit 4875556c1d70 · 2022-03-07T11:03:37.000+08:00
feat: add mountPermissions parameter in storage class
diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md
@@ -23,6 +23,8 @@ storeAccountKey | whether store account key to k8s secret <br><br> Note:  <br> `
 secretName | specify secret name to store account key | | No |
 secretNamespace | specify the namespace of secret to store account key | `default`,`kube-system`, etc | No | `default`
 isHnsEnabled | enable `Hierarchical namespace` for Azure DataLake storage account | `true`,`false` | No | `false`
+--- | **Following parameters are only for NFS protocol** | --- | --- |
+mountPermissions | mounted folder permissions | `0777` | No |
 
  - `fsGroup` securityContext setting
 
@@ -62,6 +64,8 @@ volumeAttributes.secretName | secret name that stores storage account name and k
 volumeAttributes.secretNamespace | secret namespace | `default`,`kube-system`, etc | No | `default`
 nodeStageSecretRef.name | secret name that stores(check below examples):<br>`azurestorageaccountkey`<br>`azurestorageaccountsastoken`<br>`msisecret`<br>`azurestoragespnclientsecret` | existing Kubernetes secret name |  No  |
 nodeStageSecretRef.namespace | secret namespace | k8s namespace  |  Yes  |
+--- | **Following parameters are only for NFS protocol** | --- | --- |
+volumeAttributes.mountPermissions | mounted folder permissions | `0777` | No |
 --- | **Following parameters are only for NFS vnet setting** | --- | --- |
 vnetResourceGroup | specify vnet resource group where virtual network is | existing resource group name | No | if empty, driver will use the `vnetResourceGroup` value in azure cloud config file
 vnetName | virtual network name | existing virtual network name | No | if empty, driver will use the `vnetName` value in azure cloud config file
diff --git a/pkg/blob/blob.go b/pkg/blob/blob.go
@@ -83,6 +83,7 @@ const (
 	vnetResourceGroupField       = "vnetresourcegroup"
 	vnetNameField                = "vnetname"
 	subnetNameField              = "subnetname"
+	mountPermissionsField        = "mountpermissions"
 
 	// See https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#container-names
 	containerNameMinLength = 3
diff --git a/pkg/blob/controllerserver.go b/pkg/blob/controllerserver.go
@@ -19,6 +19,7 @@ package blob
 import (
 	"context"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"google.golang.org/grpc/codes"
@@ -130,8 +131,15 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 			vnetName = v
 		case subnetNameField:
 			subnetName = v
+		case mountPermissionsField:
+			// only do validations here, used in NodeStageVolume, NodePublishVolume
+			if v != "" {
+				if _, err := strconv.ParseUint(v, 8, 32); err != nil {
+					return nil, status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid mountPermissions %s in storage class", v))
+				}
+			}
 		default:
-			return nil, fmt.Errorf("invalid parameter %s in storage class", k)
+			return nil, status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid parameter %q in storage class", k))
 		}
 	}
 
@@ -226,7 +234,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 				})
 				d.volLockMap.UnlockEntry(lockKey)
 				if err != nil {
-					return nil, status.Errorf(codes.Internal, "failed to ensure storage account: %v", err)
+					return nil, status.Errorf(codes.Internal, "ensure storage account failed with %v", err)
 				}
 				d.accountSearchCache.Set(lockKey, accountName)
 				d.volMap.Store(volName, accountName)
diff --git a/pkg/blob/controllerserver_test.go b/pkg/blob/controllerserver_test.go
@@ -122,13 +122,14 @@ func TestCreateVolume(t *testing.T) {
 				d := NewFakeDriver()
 				d.cloud = &azure.Cloud{}
 				mp := make(map[string]string)
-				mp["protocol"] = "unit-test"
+				mp[protocolField] = "unit-test"
 				mp[skuNameField] = "unit-test"
 				mp[storageAccountTypeField] = "unit-test"
 				mp[locationField] = "unit-test"
 				mp[storageAccountField] = "unit-test"
 				mp[resourceGroupField] = "unit-test"
-				mp["containername"] = "unit-test"
+				mp[containerNameField] = "unit-test"
+				mp[mountPermissionsField] = "0750"
 				req := &csi.CreateVolumeRequest{
 					Name:               "unit-test",
 					VolumeCapabilities: stdVolumeCapabilities,
@@ -150,8 +151,9 @@ func TestCreateVolume(t *testing.T) {
 				d := NewFakeDriver()
 				d.cloud = &azure.Cloud{}
 				mp := make(map[string]string)
-				mp["tags"] = "unit-test"
+				mp[tagsField] = "unit-test"
 				mp[storageAccountTypeField] = "premium"
+				mp[mountPermissionsField] = "0700"
 				req := &csi.CreateVolumeRequest{
 					Name:               "unit-test",
 					VolumeCapabilities: stdVolumeCapabilities,
@@ -177,7 +179,8 @@ func TestCreateVolume(t *testing.T) {
 				mp[locationField] = "unit-test"
 				mp[storageAccountField] = "unit-test"
 				mp[resourceGroupField] = "unit-test"
-				mp["containername"] = "unit-test"
+				mp[containerNameField] = "unit-test"
+				mp[mountPermissionsField] = "0755"
 				req := &csi.CreateVolumeRequest{
 					Name:               "unit-test",
 					VolumeCapabilities: stdVolumeCapabilities,
@@ -196,7 +199,7 @@ func TestCreateVolume(t *testing.T) {
 				}
 				mockStorageAccountsClient.EXPECT().ListByResourceGroup(gomock.Any(), gomock.Any()).Return(nil, rerr).AnyTimes()
 				_, err := d.CreateVolume(context.Background(), req)
-				expectedErr := status.Errorf(codes.Internal, "failed to ensure storage account: could not list storage accounts for account type : Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: test")
+				expectedErr := status.Errorf(codes.Internal, "ensure storage account failed with could not list storage accounts for account type : Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: test")
 				if !reflect.DeepEqual(err, expectedErr) {
 					t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr)
 				}
@@ -223,7 +226,29 @@ func TestCreateVolume(t *testing.T) {
 					controllerServiceCapability,
 				}
 
-				expectedErr := fmt.Errorf("invalid parameter %s in storage class", "invalidparameter")
+				expectedErr := status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid parameter %q in storage class", "invalidparameter"))
+				_, err := d.CreateVolume(context.Background(), req)
+				if !reflect.DeepEqual(err, expectedErr) {
+					t.Errorf("Unexpected error: %v", err)
+				}
+			},
+		},
+		{
+			name: "invalid mountPermissions",
+			testFunc: func(t *testing.T) {
+				d := NewFakeDriver()
+				mp := make(map[string]string)
+				mp[mountPermissionsField] = "0abc"
+				req := &csi.CreateVolumeRequest{
+					Name:               "unit-test",
+					VolumeCapabilities: stdVolumeCapabilities,
+					Parameters:         mp,
+				}
+				d.Cap = []*csi.ControllerServiceCapability{
+					controllerServiceCapability,
+				}
+
+				expectedErr := status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid %s %s in storage class", "mountPermissions", "0abc"))
 				_, err := d.CreateVolume(context.Background(), req)
 				if !reflect.DeepEqual(err, expectedErr) {
 					t.Errorf("Unexpected error: %v", err)
diff --git a/pkg/blob/nodeserver.go b/pkg/blob/nodeserver.go
@@ -18,10 +18,12 @@ package blob
 
 import (
 	"fmt"
+	"io/fs"
 	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"time"
 
@@ -70,20 +72,30 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
 		return nil, status.Error(codes.InvalidArgument, "Target path not provided")
 	}
 
+	mountPermissions := d.mountPermissions
 	context := req.GetVolumeContext()
-	if context != nil && strings.EqualFold(context[ephemeralField], trueValue) {
-		context[secretNamespaceField] = context[podNamespaceField]
-		// only get storage account from secret
-		context[getAccountKeyFromSecretField] = trueValue
-		context[storageAccountField] = ""
-		klog.V(2).Infof("NodePublishVolume: ephemeral volume(%s) mount on %s, VolumeContext: %v", volumeID, target, context)
-		_, err := d.NodeStageVolume(ctx, &csi.NodeStageVolumeRequest{
-			StagingTargetPath: target,
-			VolumeContext:     context,
-			VolumeCapability:  volCap,
-			VolumeId:          volumeID,
-		})
-		return &csi.NodePublishVolumeResponse{}, err
+	if context != nil {
+		if strings.EqualFold(context[ephemeralField], trueValue) {
+			context[secretNamespaceField] = context[podNamespaceField]
+			// only get storage account from secret
+			context[getAccountKeyFromSecretField] = trueValue
+			context[storageAccountField] = ""
+			klog.V(2).Infof("NodePublishVolume: ephemeral volume(%s) mount on %s, VolumeContext: %v", volumeID, target, context)
+			_, err := d.NodeStageVolume(ctx, &csi.NodeStageVolumeRequest{
+				StagingTargetPath: target,
+				VolumeContext:     context,
+				VolumeCapability:  volCap,
+				VolumeId:          volumeID,
+			})
+			return &csi.NodePublishVolumeResponse{}, err
+		}
+
+		if perm := context[mountPermissionsField]; perm != "" {
+			var err error
+			if mountPermissions, err = strconv.ParseUint(perm, 8, 32); err != nil {
+				return nil, status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid mountPermissions %s", perm))
+			}
+		}
 	}
 
 	source := req.GetStagingTargetPath()
@@ -96,7 +108,7 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
 		mountOptions = append(mountOptions, "ro")
 	}
 
-	mnt, err := d.ensureMountPoint(target)
+	mnt, err := d.ensureMountPoint(target, fs.FileMode(mountPermissions))
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not mount target %q: %v", target, err)
 	}
@@ -108,7 +120,7 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
 	klog.V(2).Infof("NodePublishVolume: volume %s mounting %s at %s with mountOptions: %v", volumeID, source, target, mountOptions)
 	if d.enableBlobMockMount {
 		klog.Warningf("NodePublishVolume: mock mount on volumeID(%s), this is only for TESTING!!!", volumeID)
-		if err := volumehelper.MakeDir(target, os.FileMode(d.mountPermissions)); err != nil {
+		if err := volumehelper.MakeDir(target, os.FileMode(mountPermissions)); err != nil {
 			klog.Errorf("MakeDir failed on target: %s (%v)", target, err)
 			return nil, err
 		}
@@ -199,21 +211,13 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 	}
 	defer d.volumeLocks.Release(volumeID)
 
-	mnt, err := d.ensureMountPoint(targetPath)
-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "Could not mount target %q: %v", targetPath, err)
-	}
-	if mnt {
-		klog.V(2).Infof("NodeStageVolume: volume %s is already mounted on %s", volumeID, targetPath)
-		return &csi.NodeStageVolumeResponse{}, nil
-	}
-
 	mountFlags := req.GetVolumeCapability().GetMount().GetMountFlags()
 	attrib := req.GetVolumeContext()
 	secrets := req.GetSecrets()
 
 	var serverAddress, storageEndpointSuffix, protocol, ephemeralVolMountOptions string
 	var ephemeralVol, isHnsEnabled bool
+	mountPermissions := d.mountPermissions
 	for k, v := range attrib {
 		switch strings.ToLower(k) {
 		case serverNameField:
@@ -228,9 +232,25 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 			ephemeralVolMountOptions = v
 		case isHnsEnabledField:
 			isHnsEnabled = strings.EqualFold(v, trueValue)
+		case mountPermissionsField:
+			if v != "" {
+				var err error
+				if mountPermissions, err = strconv.ParseUint(v, 8, 32); err != nil {
+					return nil, status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid mountPermissions %s", v))
+				}
+			}
 		}
 	}
 
+	mnt, err := d.ensureMountPoint(targetPath, fs.FileMode(mountPermissions))
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not mount target %q: %v", targetPath, err)
+	}
+	if mnt {
+		klog.V(2).Infof("NodeStageVolume: volume %s is already mounted on %s", volumeID, targetPath)
+		return &csi.NodeStageVolumeResponse{}, nil
+	}
+
 	_, accountName, _, containerName, authEnv, err := d.GetAuthEnv(ctx, volumeID, protocol, attrib, secrets)
 	if err != nil {
 		return nil, err
@@ -262,10 +282,10 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 		}
 
 		// set permissions for NFSv3 root folder
-		if err := os.Chmod(targetPath, os.FileMode(d.mountPermissions)); err != nil {
+		if err := os.Chmod(targetPath, os.FileMode(mountPermissions)); err != nil {
 			return nil, status.Error(codes.Internal, fmt.Sprintf("Chmod(%s) failed with %v", targetPath, err))
 		}
-		klog.V(2).Infof("volume(%s) mount %q on %q with 0%o succeeded", volumeID, source, targetPath, d.mountPermissions)
+		klog.V(2).Infof("volume(%s) mount %q on %q with 0%o succeeded", volumeID, source, targetPath, mountPermissions)
 
 		return &csi.NodeStageVolumeResponse{}, nil
 	}
@@ -295,7 +315,7 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 	authEnv = append(authEnv, "AZURE_STORAGE_ACCOUNT="+accountName, "AZURE_STORAGE_BLOB_ENDPOINT="+serverAddress)
 	if d.enableBlobMockMount {
 		klog.Warningf("NodeStageVolume: mock mount on volumeID(%s), this is only for TESTING!!!", volumeID)
-		if err := volumehelper.MakeDir(targetPath, os.FileMode(d.mountPermissions)); err != nil {
+		if err := volumehelper.MakeDir(targetPath, os.FileMode(mountPermissions)); err != nil {
 			klog.Errorf("MakeDir failed on target: %s (%v)", targetPath, err)
 			return nil, err
 		}
@@ -454,7 +474,7 @@ func (d *Driver) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeS
 
 // ensureMountPoint: create mount point if not exists
 // return <true, nil> if it's already a mounted point otherwise return <false, nil>
-func (d *Driver) ensureMountPoint(target string) (bool, error) {
+func (d *Driver) ensureMountPoint(target string, perm os.FileMode) (bool, error) {
 	notMnt, err := d.mounter.IsLikelyNotMountPoint(target)
 	if err != nil && !os.IsNotExist(err) {
 		if IsCorruptedDir(target) {
@@ -500,7 +520,7 @@ func (d *Driver) ensureMountPoint(target string) (bool, error) {
 		notMnt = true
 		return !notMnt, err
 	}
-	if err := volumehelper.MakeDir(target, os.FileMode(d.mountPermissions)); err != nil {
+	if err := volumehelper.MakeDir(target, perm); err != nil {
 		klog.Errorf("MakeDir failed on target: %s (%v)", target, err)
 		return !notMnt, err
 	}
diff --git a/pkg/blob/nodeserver_test.go b/pkg/blob/nodeserver_test.go
@@ -122,7 +122,7 @@ func TestEnsureMountPoint(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		_, err := d.ensureMountPoint(test.target)
+		_, err := d.ensureMountPoint(test.target, 0777)
 		if !reflect.DeepEqual(err, test.expectedErr) {
 			t.Errorf("[%s]: Unexpected Error: %v, expected error: %v", test.desc, err, test.expectedErr)
 		}
@@ -186,11 +186,16 @@ func TestNodePublishVolume(t *testing.T) {
 		},
 		{
 			desc: "Valid request read only",
-			req: csi.NodePublishVolumeRequest{VolumeCapability: &csi.VolumeCapability{AccessMode: &volumeCap},
+			req: csi.NodePublishVolumeRequest{
+				VolumeCapability:  &csi.VolumeCapability{AccessMode: &volumeCap},
 				VolumeId:          "vol_1",
 				TargetPath:        targetTest,
 				StagingTargetPath: sourceTest,
-				Readonly:          true},
+				VolumeContext: map[string]string{
+					mountPermissionsField: "0755",
+				},
+				Readonly: true,
+			},
 			expectedErr: nil,
 		},
 		{
@@ -211,6 +216,19 @@ func TestNodePublishVolume(t *testing.T) {
 				Readonly:          true},
 			expectedErr: nil,
 		},
+		{
+			desc: "[Error] invalid mountPermissions",
+			req: csi.NodePublishVolumeRequest{
+				VolumeCapability:  &csi.VolumeCapability{AccessMode: &volumeCap},
+				VolumeId:          "vol_1",
+				TargetPath:        targetTest,
+				StagingTargetPath: sourceTest,
+				VolumeContext: map[string]string{
+					mountPermissionsField: "07ab",
+				},
+			},
+			expectedErr: status.Error(codes.InvalidArgument, fmt.Sprintf("invalid mountPermissions %s", "07ab")),
+		},
 	}
 
 	// Setup
@@ -419,6 +437,25 @@ func TestNodeStageVolume(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "[Error] invalid mountPermissions",
+			testFunc: func(t *testing.T) {
+				req := &csi.NodeStageVolumeRequest{
+					VolumeId:          "unit-test",
+					StagingTargetPath: "unit-test",
+					VolumeCapability:  &csi.VolumeCapability{AccessMode: &volumeCap},
+					VolumeContext: map[string]string{
+						mountPermissionsField: "07ab",
+					},
+				}
+				d := NewFakeDriver()
+				_, err := d.NodeStageVolume(context.TODO(), req)
+				expectedErr := status.Error(codes.InvalidArgument, fmt.Sprintf("invalid mountPermissions %s", "07ab"))
+				if !reflect.DeepEqual(err, expectedErr) {
+					t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr)
+				}
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, tc.testFunc)
diff --git a/test/e2e/dynamic_provisioning_test.go b/test/e2e/dynamic_provisioning_test.go
@@ -520,8 +520,9 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
 			CSIDriver: testDriver,
 			Pods:      pods,
 			StorageClassParameters: map[string]string{
-				"skuName":  "Premium_LRS",
-				"protocol": "nfs",
+				"skuName":          "Premium_LRS",
+				"protocol":         "nfs",
+				"mountPermissions": "0755",
 			},
 		}
 		test.Run(cs, ns)
