From 14275e0be09efddb2544b08b7415d3d3f01beaeb Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sat, 5 Mar 2022 13:23:45 +0000 Subject: [PATCH 1/2] feat: add mountPermissions parameter in storage class --- docs/driver-parameters.md | 3 +- pkg/nfs/controllerserver.go | 73 ++++++++++++++++++++++++------------- pkg/nfs/nfs.go | 5 ++- pkg/nfs/nodeserver.go | 15 ++++++-- test/e2e/e2e_suite_test.go | 1 + 5 files changed, 66 insertions(+), 31 deletions(-) diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md index d55c06ad..5523fc49 100644 --- a/docs/driver-parameters.md +++ b/docs/driver-parameters.md @@ -8,6 +8,7 @@ Name | Meaning | Example Value | Mandatory | Default value --- | --- | --- | --- | --- server | NFS Server address | domain name `nfs-server.default.svc.cluster.local`
or IP address `127.0.0.1` | Yes | share | NFS share path | `/` | Yes | +mountPermissions | mounted folder permissions. The default is `0777` | | No | ### PV/PVC usage (static provisioning) > [`PersistentVolume` example](../deploy/example/pv-nfs-csi.yaml) @@ -16,7 +17,7 @@ Name | Meaning | Example Value | Mandatory | Default value --- | --- | --- | --- | --- volumeAttributes.server | NFS Server address | domain name `nfs-server.default.svc.cluster.local`
or IP address `127.0.0.1` | Yes | volumeAttributes.share | NFS share path | `/` | Yes | - +volumeAttributes.mountPermissions | mounted folder permissions. The default is `0777` | | No | ### Tips #### provide `mountOptions` for `DeleteVolume` diff --git a/pkg/nfs/controllerserver.go b/pkg/nfs/controllerserver.go index 446033b3..63986cfb 100644 --- a/pkg/nfs/controllerserver.go +++ b/pkg/nfs/controllerserver.go @@ -21,6 +21,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "github.com/container-storage-interface/spec/lib/go/csi" @@ -78,8 +79,32 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol return nil, status.Error(codes.InvalidArgument, err.Error()) } + mountPermissions := cs.Driver.mountPermissions reqCapacity := req.GetCapacityRange().GetRequiredBytes() - nfsVol, err := cs.newNFSVolume(name, reqCapacity, req.GetParameters()) + parameters := req.GetParameters() + if parameters == nil { + parameters = make(map[string]string) + } + // validate parameters (case-insensitive) + for k, v := range parameters { + switch strings.ToLower(k) { + case paramServer: + // no op + case paramShare: + // no op + 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 in storage class", v)) + } + } + default: + return nil, status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid parameter %q in storage class", k)) + } + } + + nfsVol, err := cs.newNFSVolume(name, reqCapacity, parameters) if err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } @@ -89,7 +114,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol volCap = req.GetVolumeCapabilities()[0] } // Mount nfs base share so we can create a subdirectory - if err = cs.internalMount(ctx, nfsVol, volCap); err != nil { + if err = cs.internalMount(ctx, nfsVol, parameters, volCap); err != nil { return nil, status.Errorf(codes.Internal, "failed to mount nfs server: %v", err.Error()) } defer func() { @@ -98,7 +123,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol } }() - fileMode := os.FileMode(cs.Driver.mountPermissions) + fileMode := os.FileMode(mountPermissions) // Create subdirectory under base-dir internalVolumePath := cs.getInternalVolumePath(nfsVol) if err = os.Mkdir(internalVolumePath, fileMode); err != nil && !os.IsExist(err) { @@ -108,7 +133,16 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol if err = os.Chmod(internalVolumePath, fileMode); err != nil { klog.Warningf("failed to chmod subdirectory: %v", err.Error()) } - return &csi.CreateVolumeResponse{Volume: cs.nfsVolToCSI(nfsVol)}, nil + + parameters[paramServer] = nfsVol.server + parameters[paramShare] = cs.getVolumeSharePath(nfsVol) + return &csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + VolumeId: nfsVol.id, + CapacityBytes: 0, // by setting it to zero, Provisioner will use PVC requested size as PV size + VolumeContext: parameters, + }, + }, nil } // DeleteVolume delete a volume @@ -138,7 +172,7 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol } // mount nfs base share so we can delete the subdirectory - if err = cs.internalMount(ctx, nfsVol, volCap); err != nil { + if err = cs.internalMount(ctx, nfsVol, nil, volCap); err != nil { return nil, status.Errorf(codes.Internal, "failed to mount nfs server: %v", err.Error()) } defer func() { @@ -254,7 +288,7 @@ func (cs *ControllerServer) validateVolumeCapability(c *csi.VolumeCapability) er } // Mount nfs server at base-dir -func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, volCap *csi.VolumeCapability) error { +func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, volumeContext map[string]string, volCap *csi.VolumeCapability) error { sharePath := filepath.Join(string(filepath.Separator) + vol.baseDir) targetPath := cs.getInternalMountPath(vol) @@ -266,13 +300,16 @@ func (cs *ControllerServer) internalMount(ctx context.Context, vol *nfsVolume, v } } + if volumeContext == nil { + volumeContext = make(map[string]string) + } + volumeContext[paramServer] = vol.server + volumeContext[paramShare] = sharePath + klog.V(2).Infof("internally mounting %v:%v at %v", vol.server, sharePath, targetPath) _, err := cs.Driver.ns.NodePublishVolume(ctx, &csi.NodePublishVolumeRequest{ - TargetPath: targetPath, - VolumeContext: map[string]string{ - paramServer: vol.server, - paramShare: sharePath, - }, + TargetPath: targetPath, + VolumeContext: volumeContext, VolumeCapability: volCap, VolumeId: vol.id, }) @@ -303,8 +340,6 @@ func (cs *ControllerServer) newNFSVolume(name string, size int64, params map[str server = v case paramShare: baseDir = v - default: - return nil, fmt.Errorf("invalid parameter %q", k) } } @@ -347,18 +382,6 @@ func (cs *ControllerServer) getVolumeSharePath(vol *nfsVolume) string { return filepath.Join(string(filepath.Separator), vol.baseDir, vol.subDir) } -// Convert into nfsVolume into a csi.Volume -func (cs *ControllerServer) nfsVolToCSI(vol *nfsVolume) *csi.Volume { - return &csi.Volume{ - CapacityBytes: 0, // by setting it to zero, Provisioner will use PVC requested size as PV size - VolumeId: vol.id, - VolumeContext: map[string]string{ - paramServer: vol.server, - paramShare: cs.getVolumeSharePath(vol), - }, - } -} - // Given a nfsVolume, return a CSI volume id func (cs *ControllerServer) getVolumeIDFromNfsVol(vol *nfsVolume) string { idElements := make([]string, totalIDElements) diff --git a/pkg/nfs/nfs.go b/pkg/nfs/nfs.go index 8c3d4e74..0de496a1 100644 --- a/pkg/nfs/nfs.go +++ b/pkg/nfs/nfs.go @@ -55,8 +55,9 @@ const ( // The base directory must be a direct child of the root directory. // The root directory is omitted from the string, for example: // "base" instead of "/base" - paramShare = "share" - mountOptionsField = "mountoptions" + paramShare = "share" + mountOptionsField = "mountoptions" + mountPermissionsField = "mountpermissions" ) func NewDriver(options *DriverOptions) *Driver { diff --git a/pkg/nfs/nodeserver.go b/pkg/nfs/nodeserver.go index af5c8331..f354d301 100644 --- a/pkg/nfs/nodeserver.go +++ b/pkg/nfs/nodeserver.go @@ -19,6 +19,7 @@ package nfs import ( "fmt" "os" + "strconv" "strings" "github.com/container-storage-interface/spec/lib/go/csi" @@ -56,6 +57,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } var server, baseDir string + mountPermissions := ns.Driver.mountPermissions for k, v := range req.GetVolumeContext() { switch strings.ToLower(k) { case paramServer: @@ -66,6 +68,13 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis if v != "" { mountOptions = append(mountOptions, v) } + 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 in storage class", v)) + } + } } } @@ -80,7 +89,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis notMnt, err := ns.mounter.IsLikelyNotMountPoint(targetPath) if err != nil { if os.IsNotExist(err) { - if err := os.MkdirAll(targetPath, os.FileMode(ns.Driver.mountPermissions)); err != nil { + if err := os.MkdirAll(targetPath, os.FileMode(mountPermissions)); err != nil { return nil, status.Error(codes.Internal, err.Error()) } notMnt = true @@ -104,8 +113,8 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis return nil, status.Error(codes.Internal, err.Error()) } - klog.V(2).Infof("volumeID(%v): mount targetPath(%s) with permissions(0%o)", volumeID, targetPath, ns.Driver.mountPermissions) - if err := os.Chmod(targetPath, os.FileMode(ns.Driver.mountPermissions)); err != nil { + klog.V(2).Infof("volumeID(%v): mount targetPath(%s) with permissions(0%o)", volumeID, targetPath, mountPermissions) + if err := os.Chmod(targetPath, os.FileMode(mountPermissions)); err != nil { return nil, status.Error(codes.Internal, err.Error()) } return &csi.NodePublishVolumeResponse{}, nil diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 2660eb84..4dfd0b4d 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -50,6 +50,7 @@ var ( "share": nfsShare, "csi.storage.k8s.io/provisioner-secret-name": "mount-options", "csi.storage.k8s.io/provisioner-secret-namespace": "default", + "mountPermissions": "0755", } controllerServer *nfs.ControllerServer ) From 3c258fee98213a2ae51e41a7c0cfc2495949a0df Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sat, 5 Mar 2022 13:41:22 +0000 Subject: [PATCH 2/2] test: add unit test --- pkg/nfs/controllerserver_test.go | 32 ++++++++++++++++++++++++++++---- pkg/nfs/nodeserver.go | 2 +- pkg/nfs/nodeserver_test.go | 21 +++++++++++++++++++-- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/pkg/nfs/controllerserver_test.go b/pkg/nfs/controllerserver_test.go index d7fb6309..8b104101 100644 --- a/pkg/nfs/controllerserver_test.go +++ b/pkg/nfs/controllerserver_test.go @@ -100,16 +100,18 @@ func TestCreateVolume(t *testing.T) { }, }, Parameters: map[string]string{ - paramServer: testServer, - paramShare: testBaseDir, + paramServer: testServer, + paramShare: testBaseDir, + mountPermissionsField: "0750", }, }, resp: &csi.CreateVolumeResponse{ Volume: &csi.Volume{ VolumeId: newTestVolumeID, VolumeContext: map[string]string{ - paramServer: testServer, - paramShare: testShare, + paramServer: testServer, + paramShare: testShare, + mountPermissionsField: "0750", }, }, }, @@ -201,6 +203,28 @@ func TestCreateVolume(t *testing.T) { }, expectErr: true, }, + { + name: "[Error] invalid mountPermissions", + req: &csi.CreateVolumeRequest{ + Name: testCSIVolume, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + Parameters: map[string]string{ + paramServer: testServer, + paramShare: testBaseDir, + mountPermissionsField: "07ab", + }, + }, + expectErr: true, + }, } for _, test := range cases { diff --git a/pkg/nfs/nodeserver.go b/pkg/nfs/nodeserver.go index f354d301..86f1dab8 100644 --- a/pkg/nfs/nodeserver.go +++ b/pkg/nfs/nodeserver.go @@ -72,7 +72,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis 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 in storage class", v)) + return nil, status.Errorf(codes.InvalidArgument, fmt.Sprintf("invalid mountPermissions %s", v)) } } } diff --git a/pkg/nfs/nodeserver_test.go b/pkg/nfs/nodeserver_test.go index d417f098..a559c61e 100644 --- a/pkg/nfs/nodeserver_test.go +++ b/pkg/nfs/nodeserver_test.go @@ -41,8 +41,15 @@ func TestNodePublishVolume(t *testing.T) { } params := map[string]string{ - "server": "server", - "share": "share", + "server": "server", + "share": "share", + mountPermissionsField: "0755", + } + + invalidParams := map[string]string{ + "server": "server", + "share": "share", + mountPermissionsField: "07ab", } volumeCap := csi.VolumeCapability_AccessMode{Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER} @@ -112,6 +119,16 @@ func TestNodePublishVolume(t *testing.T) { Readonly: true}, expectedErr: nil, }, + { + desc: "[Error] invalid mountPermissions", + req: csi.NodePublishVolumeRequest{ + VolumeContext: invalidParams, + VolumeCapability: &csi.VolumeCapability{AccessMode: &volumeCap}, + VolumeId: "vol_1", + TargetPath: targetTest, + Readonly: true}, + expectedErr: status.Error(codes.InvalidArgument, "invalid mountPermissions 07ab"), + }, } // setup