From bf1080a539e296efd9abd5255e2d0a17ec10d959 Mon Sep 17 00:00:00 2001 From: M Date: Mon, 14 Feb 2022 22:41:00 +0530 Subject: [PATCH 01/11] NFSv4 acl support --- docker-files/Dockerfile.centos | 2 +- docker-files/Dockerfile.ubi | 2 +- docker-files/Dockerfile.ubi.alt | 2 + docker-files/Dockerfile.ubi.min | 2 + go.mod | 2 +- go.sum | 2 + helm/csi-powerstore/templates/controller.yaml | 2 + helm/csi-powerstore/values.yaml | 17 ++ pkg/array/array.go | 1 + pkg/common/common.go | 4 + pkg/common/envvars.go | 3 + pkg/controller/controller.go | 25 ++ pkg/controller/controller_test.go | 125 ++++++++++ pkg/controller/publisher.go | 1 + pkg/node/acl.go | 132 ++++++++++ pkg/node/acl_test.go | 233 ++++++++++++++++++ pkg/node/node_test.go | 2 + pkg/node/stager.go | 27 +- samples/secret/secret.yaml | 17 ++ samples/storageclass/powerstore-nfs.yaml | 17 ++ 20 files changed, 611 insertions(+), 7 deletions(-) create mode 100644 pkg/node/acl.go create mode 100644 pkg/node/acl_test.go diff --git a/docker-files/Dockerfile.centos b/docker-files/Dockerfile.centos index e7bf05ae..37659682 100644 --- a/docker-files/Dockerfile.centos +++ b/docker-files/Dockerfile.centos @@ -14,7 +14,7 @@ LABEL vendor="Dell Inc." \ COPY licenses /licenses # dependencies, following by cleaning the cache -RUN echo "%_netsharedpath /sys:/proc" >> /etc/rpm/macros.dist && yum update -y && yum install -y e2fsprogs xfsprogs nfs-utils which device-mapper-multipath \ +RUN echo "%_netsharedpath /sys:/proc" >> /etc/rpm/macros.dist && yum update -y && yum install -y e2fsprogs xfsprogs nfs-utils nfs4-acl-tools acl which device-mapper-multipath \ && \ yum clean all \ && \ diff --git a/docker-files/Dockerfile.ubi b/docker-files/Dockerfile.ubi index bc6bb48a..d60704ff 100644 --- a/docker-files/Dockerfile.ubi +++ b/docker-files/Dockerfile.ubi @@ -15,7 +15,7 @@ COPY licenses /licenses # dependencies, following by cleaning the cache RUN yum update -y \ && \ - yum install -y e2fsprogs xfsprogs nfs-utils which device-mapper-multipath \ + yum install -y e2fsprogs xfsprogs nfs-utils nfs4-acl-tools acl which device-mapper-multipath \ && \ yum clean all \ && \ diff --git a/docker-files/Dockerfile.ubi.alt b/docker-files/Dockerfile.ubi.alt index 79487196..db31cc3d 100644 --- a/docker-files/Dockerfile.ubi.alt +++ b/docker-files/Dockerfile.ubi.alt @@ -25,6 +25,8 @@ RUN yum -y update && \ e2fsprogs.x86_64 \ xfsprogs.x86_64 \ nfs-utils.x86_64 \ + nfs4-acl-tools \ + acl \ which \ device-mapper-multipath \ && \ diff --git a/docker-files/Dockerfile.ubi.min b/docker-files/Dockerfile.ubi.min index 1ca53fd6..87b16c8b 100644 --- a/docker-files/Dockerfile.ubi.min +++ b/docker-files/Dockerfile.ubi.min @@ -19,6 +19,8 @@ RUN microdnf update -y \ e2fsprogs \ xfsprogs \ nfs-utils \ + nfs4-acl-tools \ + acl \ which \ device-mapper-multipath \ && \ diff --git a/go.mod b/go.mod index 12fbe464..8b5f54ee 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/dell/gocsi v1.5.0 github.com/dell/gofsutil v1.7.1-0.20220204052137-9928a2dc48d8 github.com/dell/goiscsi v1.2.0 - github.com/dell/gopowerstore v1.6.1-0.20211223095101-c47391fc979f + github.com/dell/gopowerstore v1.6.1-0.20220214164327-12bc4206c198 github.com/fsnotify/fsnotify v1.4.9 github.com/golang/mock v1.4.4 github.com/golang/protobuf v1.5.2 diff --git a/go.sum b/go.sum index 1058bd97..867d3a56 100644 --- a/go.sum +++ b/go.sum @@ -85,6 +85,8 @@ github.com/dell/goiscsi v1.2.0 h1:ocQs4pz2Fw2vr73RVAQBKwpN468SK4TZHPLhU7/FB9A= github.com/dell/goiscsi v1.2.0/go.mod h1:MfuMjbKWsh/MOb0VDW20C+LFYRIOfWKGiAxWkeM5TKo= github.com/dell/gopowerstore v1.6.1-0.20211223095101-c47391fc979f h1:F+aAuMlcTUV/F6eWYoM94+I6MR94gVE3g8ZY65DGzbE= github.com/dell/gopowerstore v1.6.1-0.20211223095101-c47391fc979f/go.mod h1:0ziQJ1iuZYDV+P53ua+VeH+rIylYT9WNjGSI/7aPly0= +github.com/dell/gopowerstore v1.6.1-0.20220214164327-12bc4206c198 h1:yOIMDcJ9fjuBHytDf32rjowoL8WM1VPuB7Yh8TYG1eU= +github.com/dell/gopowerstore v1.6.1-0.20220214164327-12bc4206c198/go.mod h1:0ziQJ1iuZYDV+P53ua+VeH+rIylYT9WNjGSI/7aPly0= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no= github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= diff --git a/helm/csi-powerstore/templates/controller.yaml b/helm/csi-powerstore/templates/controller.yaml index 543c350f..3cd85b58 100644 --- a/helm/csi-powerstore/templates/controller.yaml +++ b/helm/csi-powerstore/templates/controller.yaml @@ -272,6 +272,8 @@ spec: value: {{ .Values.driverName }} - name: X_CSI_POWERSTORE_EXTERNAL_ACCESS value: {{ .Values.externalAccess }} + - name: X_CSI_NFS_ACLS + value: "{{ .Values.nfsAcls }}" - name: X_CSI_POWERSTORE_CONFIG_PATH value: /powerstore-config/config - name: X_CSI_POWERSTORE_CONFIG_PARAMS_PATH diff --git a/helm/csi-powerstore/values.yaml b/helm/csi-powerstore/values.yaml index 54271607..83760468 100644 --- a/helm/csi-powerstore/values.yaml +++ b/helm/csi-powerstore/values.yaml @@ -34,6 +34,23 @@ externalAccess: # Default value: None imagePullPolicy: IfNotPresent +# nfsAcls: enables setting ACLs for NFS mount directory +# This value acts as default value for nfsAcls, if not specified for an array config in secret +# ACLs can be specified in three formats: +# 1) Unix mode +# 2) POSIX acl +# 3) NFSv4 acl (specific to NFSv4 shares only) +# Allowed values: +# 1) Unix mode: valid octal mode number +# Examples: "0777", "777", "0755" +# 2) POSIX acls: valid POSIX acls, seperated by comma +# Examples: "group::rwx", "user::rwx,group::rwx" +# 3) NFSv4 acls: valid NFSv4 acls, seperated by comma +# Examples: "A::OWNER@:RWX,A::GROUP@:RWX", "A::OWNER@:rxtncy" +# Optional: true +# Default value: "0777" +nfsAcls: "0777" + # controller: configure controller specific parameters controller: # controllerCount: defines the number of csi-powerscale controller pods to deploy to diff --git a/pkg/array/array.go b/pkg/array/array.go index 9ba6897d..454e9bf7 100644 --- a/pkg/array/array.go +++ b/pkg/array/array.go @@ -116,6 +116,7 @@ type PowerStoreArray struct { BlockProtocol common.TransportType `yaml:"blockProtocol"` Insecure bool `yaml:"skipCertificateValidation"` IsDefault bool `yaml:"isDefault"` + NfsAcls string `yaml:"nfsAcls"` Client gopowerstore.Client IP string diff --git a/pkg/common/common.go b/pkg/common/common.go index 8ae1ee01..d761da4b 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -72,6 +72,10 @@ const ( KeyArrayVolumeName = "Name" // KeyProtocol key value to check in request parameters for volume name KeyProtocol = "Protocol" + // KeyNfsACL key value to specify NFS ACLs for NFS volume + KeyNfsACL = "nfsAcls" + // KeyNasName key value to specify NAS server name + KeyNasName = "nasName" // VerboseName longer description of the driver VerboseName = "CSI Driver for Dell EMC PowerStore" // FcTransport indicates that FC is chosen as a SCSI transport protocol diff --git a/pkg/common/envvars.go b/pkg/common/envvars.go index 19c6d8e4..5b84b36b 100644 --- a/pkg/common/envvars.go +++ b/pkg/common/envvars.go @@ -82,4 +82,7 @@ const ( // EnvIsHealthMonitorEnabled specifies if health monitor is enabled. EnvIsHealthMonitorEnabled = "X_CSI_HEALTH_MONITOR_ENABLED" + + // EnvNfsAcls specifies acls to be set on NFS mount directory + EnvNfsAcls = "X_CSI_NFS_ACLS" ) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 97801ac6..44ca956f 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -23,6 +23,7 @@ import ( "context" "errors" "fmt" + "os" "sort" "strconv" @@ -58,6 +59,7 @@ type Service struct { Fs fs.Interface externalAccess string + nfsAcls string array.Locker @@ -85,6 +87,16 @@ func (s *Service) Init() error { s.isHealthMonitorEnabled, _ = strconv.ParseBool(isHealthMonitorEnabled) } + if nfsAcls, ok := csictx.LookupEnv(ctx, common.EnvNfsAcls); ok { + if nfsAcls == "" { + nfsAcls = fmt.Sprintf("%s", os.ModePerm) // Default to 0777 + } else { + s.nfsAcls = nfsAcls + } + } else { + nfsAcls = fmt.Sprintf("%s", os.ModePerm) // Default to 0777 + } + return nil } @@ -132,6 +144,7 @@ func (s *Service) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest var creator VolumeCreator var protocol string + nfsAcls := s.nfsAcls if useNFS { protocol = "nfs" nasParamsName, ok := params[KeyNasName] @@ -144,6 +157,12 @@ func (s *Service) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest nasName: arr.GetNasName(), } } + + if params[common.KeyNfsACL] != "" { + nfsAcls = params[common.KeyNfsACL] // Storage class takes precedence + } else if arr.NfsAcls != "" { + nfsAcls = arr.NfsAcls // Secrets next + } } else { protocol = "scsi" creator = &SCSICreator{} @@ -308,6 +327,12 @@ func (s *Service) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest volumeResponse.VolumeContext[common.KeyArrayID] = arr.GetGlobalID() volumeResponse.VolumeContext[common.KeyArrayVolumeName] = req.Name volumeResponse.VolumeContext[common.KeyProtocol] = protocol + + if useNFS { + volumeResponse.VolumeContext[common.KeyNfsACL] = nfsAcls + volumeResponse.VolumeContext[common.KeyNasName] = arr.GetNasName() + } + volumeResponse.VolumeId = volumeResponse.VolumeId + "/" + arr.GetGlobalID() + "/" + protocol volumeResponse.AccessibleTopology = topology return &csi.CreateVolumeResponse{ diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index a2905b32..6edb315e 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -75,6 +75,8 @@ const ( validReplicationPrefix = "/" + controller.KeyReplicationEnabled validVolumeGroupName = "VGName" validRemoteSystemGlobalID = "PS111111111111" + validNfsAcls = "A::OWNER@:RWX" + validNfsServerID = "24aefac2-a796-47dc-886a-c73ff8c1a671" ) var ( @@ -119,6 +121,7 @@ func setVariables() { arrays[secondValidID] = second csictx.Setenv(context.Background(), common.EnvReplicationPrefix, "replication.storage.dell.com") + csictx.Setenv(context.Background(), common.EnvNfsAcls, "A::OWNER@:RWX") ctrlSvc = &controller.Service{Fs: fsMock} ctrlSvc.SetArrays(arrays) @@ -388,6 +391,124 @@ var _ = Describe("CSIControllerService", func() { common.KeyArrayVolumeName: "my-vol", common.KeyProtocol: "nfs", common.KeyArrayID: secondValidID, + common.KeyNfsACL: "A::OWNER@:RWX", + common.KeyNasName: validNasName, + }, + }, + })) + }) + }) + + When("creating nfs volume with NFS acls in array config and storage class", func() { + It("should successfully create nfs volume with storage class NFS acls in volume response", func() { + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, nil) + clientMock.On("CreateFS", mock.Anything, mock.Anything).Return(gopowerstore.CreateResponse{ID: validBaseVolID}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + ctrlSvc.Arrays()[secondValidID].NfsAcls = "A::GROUP@:RWX" + + req := getTypicalCreateVolumeNFSRequest("my-vol", validVolSize) + req.Parameters[common.KeyNfsACL] = "0777" + req.Parameters[common.KeyArrayID] = secondValidID + + res, err := ctrlSvc.CreateVolume(context.Background(), req) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + CapacityBytes: validVolSize, + VolumeId: filepath.Join(validBaseVolID, secondValidID, "nfs"), + VolumeContext: map[string]string{ + common.KeyArrayVolumeName: "my-vol", + common.KeyProtocol: "nfs", + common.KeyArrayID: secondValidID, + common.KeyNfsACL: "0777", + common.KeyNasName: validNasName, + }, + }, + })) + }) + }) + + When("creating nfs volume with NFS acls in array config and not in storage class", func() { + It("should successfully create nfs volume with array config NFS acls in volume response", func() { + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, nil) + clientMock.On("CreateFS", mock.Anything, mock.Anything).Return(gopowerstore.CreateResponse{ID: validBaseVolID}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + ctrlSvc.Arrays()[secondValidID].NfsAcls = "A::GROUP@:RWX" + + req := getTypicalCreateVolumeNFSRequest("my-vol", validVolSize) + req.Parameters[common.KeyArrayID] = secondValidID + + res, err := ctrlSvc.CreateVolume(context.Background(), req) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + CapacityBytes: validVolSize, + VolumeId: filepath.Join(validBaseVolID, secondValidID, "nfs"), + VolumeContext: map[string]string{ + common.KeyArrayVolumeName: "my-vol", + common.KeyProtocol: "nfs", + common.KeyArrayID: secondValidID, + common.KeyNfsACL: "A::GROUP@:RWX", + common.KeyNasName: validNasName, + }, + }, + })) + }) + }) + + When("creating nfs volume with NFS acls not in array config and not in storage class", func() { + It("should successfully create nfs volume with default NFS acls in volume response", func() { + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, nil) + clientMock.On("CreateFS", mock.Anything, mock.Anything).Return(gopowerstore.CreateResponse{ID: validBaseVolID}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + req := getTypicalCreateVolumeNFSRequest("my-vol", validVolSize) + req.Parameters[common.KeyArrayID] = secondValidID + + res, err := ctrlSvc.CreateVolume(context.Background(), req) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + CapacityBytes: validVolSize, + VolumeId: filepath.Join(validBaseVolID, secondValidID, "nfs"), + VolumeContext: map[string]string{ + common.KeyArrayVolumeName: "my-vol", + common.KeyProtocol: "nfs", + common.KeyArrayID: secondValidID, + common.KeyNfsACL: "A::OWNER@:RWX", + common.KeyNasName: validNasName, + }, + }, + })) + }) + }) + + When("creating nfs volume with NFS acls in not in secrets", func() { + It("should successfully create nfs volume with default NFS acls in volume response", func() { + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, nil) + clientMock.On("CreateFS", mock.Anything, mock.Anything).Return(gopowerstore.CreateResponse{ID: validBaseVolID}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + req := getTypicalCreateVolumeNFSRequest("my-vol", validVolSize) + req.Parameters[common.KeyArrayID] = secondValidID + + csictx.Setenv(context.Background(), common.EnvNfsAcls, "") + _ = ctrlSvc.Init() + + res, err := ctrlSvc.CreateVolume(context.Background(), req) + Expect(err).To(BeNil()) + Expect(res).To(Equal(&csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + CapacityBytes: validVolSize, + VolumeId: filepath.Join(validBaseVolID, secondValidID, "nfs"), + VolumeContext: map[string]string{ + common.KeyArrayVolumeName: "my-vol", + common.KeyProtocol: "nfs", + common.KeyArrayID: secondValidID, + common.KeyNfsACL: "A::OWNER@:RWX", + common.KeyNasName: validNasName, }, }, })) @@ -457,6 +578,8 @@ var _ = Describe("CSIControllerService", func() { common.KeyArrayVolumeName: "my-vol", common.KeyProtocol: "nfs", common.KeyArrayID: secondValidID, + common.KeyNfsACL: "A::OWNER@:RWX", + common.KeyNasName: validNasName, }, }, })) @@ -1533,6 +1656,7 @@ var _ = Describe("CSIControllerService", func() { "ExportID": nfsID, "allowRoot": "", "HostIP": "127.0.0.1", + "nfsAcls": "", }, })) }) @@ -1654,6 +1778,7 @@ var _ = Describe("CSIControllerService", func() { "allowRoot": "", "HostIP": "127.0.0.1", "NatIP": "10.0.0.1", + "nfsAcls": "", }, })) }) diff --git a/pkg/controller/publisher.go b/pkg/controller/publisher.go index 5c5168a6..61946dcb 100644 --- a/pkg/controller/publisher.go +++ b/pkg/controller/publisher.go @@ -267,6 +267,7 @@ func (n *NfsPublisher) Publish(ctx context.Context, req *csi.ControllerPublishVo } publishContext[common.KeyExportID] = export.ID publishContext[common.KeyAllowRoot] = req.VolumeContext[common.KeyAllowRoot] + publishContext[common.KeyNfsACL] = req.VolumeContext[common.KeyNfsACL] return &csi.ControllerPublishVolumeResponse{PublishContext: publishContext}, nil } diff --git a/pkg/node/acl.go b/pkg/node/acl.go new file mode 100644 index 00000000..2c05b2e1 --- /dev/null +++ b/pkg/node/acl.go @@ -0,0 +1,132 @@ +/* + * + * Copyright © 2022 Dell Inc. or its subsidiaries. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package node + +import ( + "context" + "fmt" + "os/exec" + "regexp" + "strings" + + "github.com/dell/gopowerstore" + log "github.com/sirupsen/logrus" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +var ( + execCommand = executeCommand +) + +func validateAndSetACLs(ctx context.Context, nasName string, client gopowerstore.Client, acls string, dir string) (bool, error) { + aclsConfigured := false + if nfsv4ACLs(acls) { + if nfsv4NasServer(ctx, client, nasName) { + if err := setNfsv4Acls(acls, dir); err != nil { + log.Error(fmt.Sprintf("can't assign NFSv4 ACLs to folder %s: %s", dir, err.Error())) + return false, err + } + aclsConfigured = true + } else { + return false, status.Errorf(codes.Internal, "can't assign NFSv4 ACLs to folder %s: NAS server is not NFSv4 enabled", dir) + } + } else if posixACL(acls) { + if err := setPosixAcls(acls, dir); err != nil { + log.Error(fmt.Sprintf("can't assign POSIX ACLs to folder %s: %s", dir, err.Error())) + return false, err + } + aclsConfigured = true + } else { + return false, status.Errorf(codes.Internal, "can't assign ACLs to folder %s: unknown ACL format %s", dir, acls) + } + + return aclsConfigured, nil +} + +func posixMode(acls string) bool { + if matched, _ := regexp.Match(`\d{3,4}`, []byte(acls)); matched { + return true + } + return false +} + +func posixACL(acls string) bool { + if matched, _ := regexp.Match(`[\w*:\w*:\w*,*]+`, []byte(acls)); matched { + return true + } + return false +} + +func setPosixAcls(acls string, dir string) error { + aclList := strings.Split(acls, ",") + for _, acl := range aclList { + command := []string{"setfacl", "-m", + strings.TrimSpace(acl), + strings.TrimSpace(dir)} + log.Info("POSIX ACL command: " + strings.Join(command, " ") + "\n") + outStr, err := execCommand(command) + log.Info("POSIX ACL output: " + string(outStr) + "\n") + if err != nil { + return err + } + } + + return nil +} + +func nfsv4ACLs(acls string) bool { + if strings.HasPrefix(acls, "A:") || strings.HasPrefix(acls, "D:") { + return true + } + return false +} + +func setNfsv4Acls(acls string, dir string) error { + command := []string{"nfs4_setfacl", "-s", acls, dir} + log.Info("NFSv4 ACL command: " + strings.Join(command, " ") + "\n") + outStr, err := execCommand(command) + log.Info("NFSv4 ACL output: " + string(outStr) + "\n") + return err +} + +func executeCommand(command []string) ([]byte, error) { + cmd := exec.Command(command[0], command[1:]...) // #nosec G204 + return cmd.Output() +} + +func nfsv4NasServer(ctx context.Context, client gopowerstore.Client, nasName string) bool { + nfsv4Enabled := false + nas, err := gopowerstore.Client.GetNASByName(client, ctx, nasName) + if err == nil { + nfsServer, err := gopowerstore.Client.GetNfsServer(client, ctx, nas.NfsServers[0].Id) + if err == nil { + if nfsServer.IsNFSv4Enabled { + nfsv4Enabled = true + } else { + log.Error(fmt.Sprintf("NFS v4 not enabled on NAS server: %s\n", nasName)) + } + } else { + log.Error(fmt.Sprintf("can't fetch nfs server with id %s: %s", nas.NfsServers[0].Id, err.Error())) + } + } else { + log.Error(fmt.Sprintf("can't determine nfsv4 enabled: %s", err.Error())) + } + return nfsv4Enabled +} diff --git a/pkg/node/acl_test.go b/pkg/node/acl_test.go new file mode 100644 index 00000000..7c38779e --- /dev/null +++ b/pkg/node/acl_test.go @@ -0,0 +1,233 @@ +package node + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/dell/gopowerstore" + gopowerstoremock "github.com/dell/gopowerstore/mocks" + "github.com/stretchr/testify/mock" +) + +func TestPosixMode_Success(t *testing.T) { + isPosixMode := posixMode("0755") + expected := true + if isPosixMode != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isPosixMode)) + } +} + +func TestPosixMode_Fail(t *testing.T) { + isPosixMode := posixMode("abcd") + expected := false + if isPosixMode != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isPosixMode)) + } +} + +func TestPosixAcl_Success(t *testing.T) { + isPosixACL := posixACL("u::rwx") + expected := true + if isPosixACL != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isPosixACL)) + } +} + +func TestPosixAcl_Fail(t *testing.T) { + isPosixACL := posixACL("abcd") + expected := false + if isPosixACL != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isPosixACL)) + } +} + +func TestNfsv4Acl_Success(t *testing.T) { + isNfsv4ACLs := nfsv4ACLs("A::OWNER@:RWX") + expected := true + if isNfsv4ACLs != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNfsv4ACLs)) + } +} + +func TestNfsv4Acl_Fail(t *testing.T) { + isNfsv4ACLs := nfsv4ACLs("abcd") + expected := false + if isNfsv4ACLs != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNfsv4ACLs)) + } +} + +func TestNfsv4NasServer_Success(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + expected := true + if isNFSv4Enabled != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) + } +} + +func TestNfsv4NasServer_Err_GetNASByName(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, errors.New("GetNASByName_fail")) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + expected := false + if isNFSv4Enabled != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) + } +} + +func TestNfsv4NasServer_Err_GetNfsServer(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, errors.New("GetNfsServer_fail")) + + isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + expected := false + if isNFSv4Enabled != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) + } +} + +func TestNfsv4NasServer_Fail(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: false}, nil) + + isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + expected := false + if isNFSv4Enabled != expected { + t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) + } +} + +func TestValidateAndSetNfsACLs_Success_nfsv4Acls(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + execCommand = executeCommandMock + defer execCommandReset() + + aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "A::OWNER@:RWX", "dir1") + + if err != nil || aclConfigured == false { + t.Errorf(fmt.Sprintf("expected: true, actual: %v err: %s", aclConfigured, err.Error())) + } +} + +func TestValidateAndSetNfsACLs_Success_posixAcls(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + execCommand = executeCommandMock + defer execCommandReset() + + aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "u::rwx", "dir1") + + if err != nil || aclConfigured == false { + t.Errorf(fmt.Sprintf("expected: true, actual: %v err: %s", aclConfigured, err.Error())) + } +} + +func TestValidateAndSetNfsACLs_Fail_InvalidAcls(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + + execCommand = executeCommandMock + defer execCommandReset() + + aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "abcd", "dir1") + + if err != nil || aclConfigured == true { + t.Errorf(fmt.Sprintf("expected: false, actual: %v err: %s", aclConfigured, err.Error())) + } +} + +func TestValidateAndSetNfsACLs_Fail_GetNfsServerFail(t *testing.T) { + clientMock = new(gopowerstoremock.Client) + + nfsServers := []gopowerstore.NFSServerInstance{ + { + Id: validNfsServerID, + IsNFSv4Enabled: true, + }, + } + + clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) + clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, errors.New("GetNfsServer_fail")) + + execCommand = executeCommandMock + defer execCommandReset() + + aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "u::rwx", "dir1") + + if err != nil || aclConfigured == true { + t.Errorf(fmt.Sprintf("expected: false, actual: %v err: %s", aclConfigured, err.Error())) + } +} + +func executeCommandMock(command []string) ([]byte, error) { + return []byte("executed command successfully"), nil +} + +func execCommandReset() { + execCommand = executeCommand +} diff --git a/pkg/node/node_test.go b/pkg/node/node_test.go index eb22818f..d977b8a3 100644 --- a/pkg/node/node_test.go +++ b/pkg/node/node_test.go @@ -80,6 +80,8 @@ const ( firstValidIP = "gid1" secondValidIP = "gid2" validNasName = "my-nas-name" + validNasID = "e8f4c5f8-c2fc-4df4-bd99-c292c12b55be" + validNfsServerID = "e8f4c5f8-c2fc-4dd2-bd99-c292c12b55be" validEphemeralName = "ephemeral-39bb1b5f-5624-490d-9ece-18f7b28a904e/gid1/scsi" ephemerallockfile = "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/ephemeral/39bb1b5f-5624-490d-9ece-18f7b28a904e/gid1/scsi/id" ) diff --git a/pkg/node/stager.go b/pkg/node/stager.go index da02eaa5..193cff51 100644 --- a/pkg/node/stager.go +++ b/pkg/node/stager.go @@ -128,6 +128,7 @@ func (n *NFSStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest, exportID := req.PublishContext[common.KeyExportID] nfsExport := req.PublishContext[common.KeyNfsExportPath] allowRoot := req.PublishContext[common.KeyAllowRoot] + nasName := req.PublishContext[common.KeyNasName] natIP := "" if ip, ok := req.PublishContext[common.KeyNatIP]; ok { @@ -141,6 +142,8 @@ func (n *NFSStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest, logFields["ExportID"] = exportID logFields["HostIP"] = hostIP logFields["NatIP"] = natIP + logFields["NFSv4ACLs"] = req.PublishContext[common.KeyNfsACL] + logFields["NasName"] = nasName ctx = common.SetLogFields(ctx, logFields) found, err := isReadyToPublishNFS(ctx, stagingPath, fs) @@ -165,15 +168,31 @@ func (n *NFSStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest, } // Create folder with 1777 in nfs share so every user can use it - log.WithFields(logFields).Info("creating common folder") if err := fs.MkdirAll(filepath.Join(stagingPath, commonNfsVolumeFolder), 0750); err != nil { return nil, status.Errorf(codes.Internal, "can't create common folder %s: %s", filepath.Join(stagingPath, "volume"), err.Error()) } - if err := fs.Chmod(filepath.Join(stagingPath, commonNfsVolumeFolder), os.ModeSticky|os.ModePerm); err != nil { - return nil, status.Errorf(codes.Internal, - "can't change permissions of folder %s: %s", filepath.Join(stagingPath, "volume"), err.Error()) + mode := os.ModePerm + acls := req.PublishContext[common.KeyNfsACL] + aclsConfigured := false + if acls != "" { + if posixMode(acls) { + perm, _ := strconv.ParseUint(acls, 8, 64) + mode = os.FileMode(perm) + } else { + aclsConfigured, err = validateAndSetACLs(ctx, nasName, n.array.GetClient(), acls, filepath.Join(stagingPath, commonNfsVolumeFolder)) + if err != nil || !aclsConfigured { + return nil, err + } + } + } + + if !aclsConfigured { + if err := fs.Chmod(filepath.Join(stagingPath, commonNfsVolumeFolder), os.ModeSticky|mode); err != nil { + return nil, status.Errorf(codes.Internal, + "can't change permissions of folder %s: %s", filepath.Join(stagingPath, "volume"), err.Error()) + } } if allowRoot == "false" { diff --git a/samples/secret/secret.yaml b/samples/secret/secret.yaml index 80887277..ccf350c3 100644 --- a/samples/secret/secret.yaml +++ b/samples/secret/secret.yaml @@ -51,6 +51,23 @@ arrays: # Default Value: None nasName: "nas-server" + # nfsAcls: enables setting ACLs for NFS mount directory + # This value will be used if a storage class does not have the nfsAcls parameter specified + # ACLs can be specified in three formats: + # 1) Unix mode + # 2) POSIX acl + # 3) NFSv4 acl (specific to NFSv4 shares only) + # Allowed values: + # 1) Unix mode: valid octal mode number + # Examples: "0777", "777", "0755" + # 2) POSIX acls: valid POSIX acls, seperated by comma + # Examples: "group::rwx", "user::rwx,group::rwx" + # 3) NFSv4 acls: valid NFSv4 acls, seperated by comma + # Examples: "A::OWNER@:RWX,A::GROUP@:RWX", "A::OWNER@:rxtncy" + # Optional: true + # Default value: "0777" + # nfsAcls: "0777" + - endpoint: "https://11.0.0.1/api/rest" globalID: "unique" username: "user" diff --git a/samples/storageclass/powerstore-nfs.yaml b/samples/storageclass/powerstore-nfs.yaml index a8b0fdc6..e6cf04ba 100644 --- a/samples/storageclass/powerstore-nfs.yaml +++ b/samples/storageclass/powerstore-nfs.yaml @@ -34,6 +34,23 @@ parameters: # Default value: false allowRoot: "false" + # nfsAcls: enables setting ACLs for NFS mount directory + # This value overrides the nfsAcls attribute of corresponding array config in secret, if present + # ACLs can be specified in three formats: + # 1) Unix mode + # 2) POSIX acl + # 3) NFSv4 acl (specific to NFSv4 shares only) + # Allowed values: + # 1) Unix mode: valid octal mode number + # Examples: "0777", "777", "0755" + # 2) POSIX acls: valid POSIX acls, seperated by comma + # Examples: "group::rwx", "user::rwx,group::rwx" + # 3) NFSv4 acls: valid NFSv4 acls, seperated by comma + # Examples: "A::OWNER@:RWX,A::GROUP@:RWX", "A::OWNER@:rxtncy" + # Optional: true + # Default value: "0777" + # nfsAcls: "0777" + # reclaimPolicy: PVs that are dynamically created by a StorageClass will have the reclaim policy specified here # Allowed values: # Reclaim: retain the PV after PVC deletion From b4bde21debe98ab7afe3249ffeeef41bb2a4b5ea Mon Sep 17 00:00:00 2001 From: M Date: Tue, 15 Feb 2022 15:51:45 +0530 Subject: [PATCH 02/11] Adding input validations, go mod tidy --- go.sum | 2 -- helm/csi-powerstore/values.yaml | 13 +++---- pkg/node/acl.go | 42 +++++------------------ pkg/node/acl_test.go | 43 ++---------------------- samples/secret/secret.yaml | 13 +++---- samples/storageclass/powerstore-nfs.yaml | 13 +++---- 6 files changed, 25 insertions(+), 101 deletions(-) diff --git a/go.sum b/go.sum index 867d3a56..979b6c7b 100644 --- a/go.sum +++ b/go.sum @@ -83,8 +83,6 @@ github.com/dell/gofsutil v1.7.1-0.20220204052137-9928a2dc48d8/go.mod h1:0tAefmK/ github.com/dell/goiscsi v1.1.0/go.mod h1:MfuMjbKWsh/MOb0VDW20C+LFYRIOfWKGiAxWkeM5TKo= github.com/dell/goiscsi v1.2.0 h1:ocQs4pz2Fw2vr73RVAQBKwpN468SK4TZHPLhU7/FB9A= github.com/dell/goiscsi v1.2.0/go.mod h1:MfuMjbKWsh/MOb0VDW20C+LFYRIOfWKGiAxWkeM5TKo= -github.com/dell/gopowerstore v1.6.1-0.20211223095101-c47391fc979f h1:F+aAuMlcTUV/F6eWYoM94+I6MR94gVE3g8ZY65DGzbE= -github.com/dell/gopowerstore v1.6.1-0.20211223095101-c47391fc979f/go.mod h1:0ziQJ1iuZYDV+P53ua+VeH+rIylYT9WNjGSI/7aPly0= github.com/dell/gopowerstore v1.6.1-0.20220214164327-12bc4206c198 h1:yOIMDcJ9fjuBHytDf32rjowoL8WM1VPuB7Yh8TYG1eU= github.com/dell/gopowerstore v1.6.1-0.20220214164327-12bc4206c198/go.mod h1:0ziQJ1iuZYDV+P53ua+VeH+rIylYT9WNjGSI/7aPly0= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= diff --git a/helm/csi-powerstore/values.yaml b/helm/csi-powerstore/values.yaml index 83760468..ca9cfccb 100644 --- a/helm/csi-powerstore/values.yaml +++ b/helm/csi-powerstore/values.yaml @@ -34,18 +34,15 @@ externalAccess: # Default value: None imagePullPolicy: IfNotPresent -# nfsAcls: enables setting ACLs for NFS mount directory +# nfsAcls: enables setting permissions on NFS mount directory # This value acts as default value for nfsAcls, if not specified for an array config in secret -# ACLs can be specified in three formats: -# 1) Unix mode -# 2) POSIX acl -# 3) NFSv4 acl (specific to NFSv4 shares only) +# Permissions can be specified in two formats: +# 1) Unix mode (NFSv3) +# 2) NFSv4 acl (NFSv4) # Allowed values: # 1) Unix mode: valid octal mode number # Examples: "0777", "777", "0755" -# 2) POSIX acls: valid POSIX acls, seperated by comma -# Examples: "group::rwx", "user::rwx,group::rwx" -# 3) NFSv4 acls: valid NFSv4 acls, seperated by comma +# 2) NFSv4 acls: valid NFSv4 acls, seperated by comma # Examples: "A::OWNER@:RWX,A::GROUP@:RWX", "A::OWNER@:rxtncy" # Optional: true # Default value: "0777" diff --git a/pkg/node/acl.go b/pkg/node/acl.go index 2c05b2e1..9cbd2c38 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -47,14 +47,8 @@ func validateAndSetACLs(ctx context.Context, nasName string, client gopowerstore } else { return false, status.Errorf(codes.Internal, "can't assign NFSv4 ACLs to folder %s: NAS server is not NFSv4 enabled", dir) } - } else if posixACL(acls) { - if err := setPosixAcls(acls, dir); err != nil { - log.Error(fmt.Sprintf("can't assign POSIX ACLs to folder %s: %s", dir, err.Error())) - return false, err - } - aclsConfigured = true } else { - return false, status.Errorf(codes.Internal, "can't assign ACLs to folder %s: unknown ACL format %s", dir, acls) + return false, status.Errorf(codes.Internal, "can't assign ACLs to folder %s: invalid NFSv4 ACL format %s", dir, acls) } return aclsConfigured, nil @@ -67,35 +61,15 @@ func posixMode(acls string) bool { return false } -func posixACL(acls string) bool { - if matched, _ := regexp.Match(`[\w*:\w*:\w*,*]+`, []byte(acls)); matched { - return true - } - return false -} - -func setPosixAcls(acls string, dir string) error { - aclList := strings.Split(acls, ",") - for _, acl := range aclList { - command := []string{"setfacl", "-m", - strings.TrimSpace(acl), - strings.TrimSpace(dir)} - log.Info("POSIX ACL command: " + strings.Join(command, " ") + "\n") - outStr, err := execCommand(command) - log.Info("POSIX ACL output: " + string(outStr) + "\n") - if err != nil { - return err - } - } - - return nil -} - func nfsv4ACLs(acls string) bool { - if strings.HasPrefix(acls, "A:") || strings.HasPrefix(acls, "D:") { - return true + aclsList := strings.Split(acls, ",") + for _, acl := range aclsList { + matched, err := regexp.Match(`([AD]:\w*:\w*@\w*:\w*)`, []byte(acl)) + if !matched || err != nil { + return false + } } - return false + return true } func setNfsv4Acls(acls string, dir string) error { diff --git a/pkg/node/acl_test.go b/pkg/node/acl_test.go index 7c38779e..a8d96612 100644 --- a/pkg/node/acl_test.go +++ b/pkg/node/acl_test.go @@ -27,22 +27,6 @@ func TestPosixMode_Fail(t *testing.T) { } } -func TestPosixAcl_Success(t *testing.T) { - isPosixACL := posixACL("u::rwx") - expected := true - if isPosixACL != expected { - t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isPosixACL)) - } -} - -func TestPosixAcl_Fail(t *testing.T) { - isPosixACL := posixACL("abcd") - expected := false - if isPosixACL != expected { - t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isPosixACL)) - } -} - func TestNfsv4Acl_Success(t *testing.T) { isNfsv4ACLs := nfsv4ACLs("A::OWNER@:RWX") expected := true @@ -155,29 +139,6 @@ func TestValidateAndSetNfsACLs_Success_nfsv4Acls(t *testing.T) { } } -func TestValidateAndSetNfsACLs_Success_posixAcls(t *testing.T) { - clientMock = new(gopowerstoremock.Client) - - nfsServers := []gopowerstore.NFSServerInstance{ - { - Id: validNfsServerID, - IsNFSv4Enabled: true, - }, - } - - clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) - clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) - - execCommand = executeCommandMock - defer execCommandReset() - - aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "u::rwx", "dir1") - - if err != nil || aclConfigured == false { - t.Errorf(fmt.Sprintf("expected: true, actual: %v err: %s", aclConfigured, err.Error())) - } -} - func TestValidateAndSetNfsACLs_Fail_InvalidAcls(t *testing.T) { clientMock = new(gopowerstoremock.Client) @@ -196,7 +157,7 @@ func TestValidateAndSetNfsACLs_Fail_InvalidAcls(t *testing.T) { aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "abcd", "dir1") - if err != nil || aclConfigured == true { + if err == nil || aclConfigured != false { t.Errorf(fmt.Sprintf("expected: false, actual: %v err: %s", aclConfigured, err.Error())) } } @@ -219,7 +180,7 @@ func TestValidateAndSetNfsACLs_Fail_GetNfsServerFail(t *testing.T) { aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "u::rwx", "dir1") - if err != nil || aclConfigured == true { + if err == nil || aclConfigured != false { t.Errorf(fmt.Sprintf("expected: false, actual: %v err: %s", aclConfigured, err.Error())) } } diff --git a/samples/secret/secret.yaml b/samples/secret/secret.yaml index ccf350c3..54326d01 100644 --- a/samples/secret/secret.yaml +++ b/samples/secret/secret.yaml @@ -51,18 +51,15 @@ arrays: # Default Value: None nasName: "nas-server" - # nfsAcls: enables setting ACLs for NFS mount directory + # nfsAcls: enables setting permissions on NFS mount directory # This value will be used if a storage class does not have the nfsAcls parameter specified - # ACLs can be specified in three formats: - # 1) Unix mode - # 2) POSIX acl - # 3) NFSv4 acl (specific to NFSv4 shares only) + # Permissions can be specified in two formats: + # 1) Unix mode (NFSv3) + # 2) NFSv4 acl (NFSv4) # Allowed values: # 1) Unix mode: valid octal mode number # Examples: "0777", "777", "0755" - # 2) POSIX acls: valid POSIX acls, seperated by comma - # Examples: "group::rwx", "user::rwx,group::rwx" - # 3) NFSv4 acls: valid NFSv4 acls, seperated by comma + # 2) NFSv4 acls: valid NFSv4 acls, seperated by comma # Examples: "A::OWNER@:RWX,A::GROUP@:RWX", "A::OWNER@:rxtncy" # Optional: true # Default value: "0777" diff --git a/samples/storageclass/powerstore-nfs.yaml b/samples/storageclass/powerstore-nfs.yaml index e6cf04ba..6b12250d 100644 --- a/samples/storageclass/powerstore-nfs.yaml +++ b/samples/storageclass/powerstore-nfs.yaml @@ -34,18 +34,15 @@ parameters: # Default value: false allowRoot: "false" - # nfsAcls: enables setting ACLs for NFS mount directory + # nfsAcls: enables setting permissions on NFS mount directory # This value overrides the nfsAcls attribute of corresponding array config in secret, if present - # ACLs can be specified in three formats: - # 1) Unix mode - # 2) POSIX acl - # 3) NFSv4 acl (specific to NFSv4 shares only) + # Permissions can be specified in two formats: + # 1) Unix mode (NFSv3) + # 2) NFSv4 acl (NFSv4) # Allowed values: # 1) Unix mode: valid octal mode number # Examples: "0777", "777", "0755" - # 2) POSIX acls: valid POSIX acls, seperated by comma - # Examples: "group::rwx", "user::rwx,group::rwx" - # 3) NFSv4 acls: valid NFSv4 acls, seperated by comma + # 2) NFSv4 acls: valid NFSv4 acls, seperated by comma # Examples: "A::OWNER@:RWX,A::GROUP@:RWX", "A::OWNER@:rxtncy" # Optional: true # Default value: "0777" From e3f7f0775fc25244806933187e4d98d12b11fd35 Mon Sep 17 00:00:00 2001 From: M Date: Wed, 16 Feb 2022 09:45:22 +0530 Subject: [PATCH 03/11] NFSv4 acl support --- pkg/node/acl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/node/acl.go b/pkg/node/acl.go index 9cbd2c38..5a6dc010 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -64,7 +64,7 @@ func posixMode(acls string) bool { func nfsv4ACLs(acls string) bool { aclsList := strings.Split(acls, ",") for _, acl := range aclsList { - matched, err := regexp.Match(`([AD]:\w*:\w*@\w*:\w*)`, []byte(acl)) + matched, err := regexp.Match(`([ADUL]:\w*:\w*[@]\w*:\w*)`, []byte(acl)) if !matched || err != nil { return false } From 558bdcc1a28d0af4e3723f6265a2f88250ed42f7 Mon Sep 17 00:00:00 2001 From: M Date: Wed, 16 Feb 2022 16:41:16 +0530 Subject: [PATCH 04/11] NFSv4 acl support --- pkg/node/acl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/node/acl.go b/pkg/node/acl.go index 5a6dc010..e321907e 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -64,7 +64,7 @@ func posixMode(acls string) bool { func nfsv4ACLs(acls string) bool { aclsList := strings.Split(acls, ",") for _, acl := range aclsList { - matched, err := regexp.Match(`([ADUL]:\w*:\w*[@]\w*:\w*)`, []byte(acl)) + matched, err := regexp.Match(`([ADUL]:\w*:\w*[@]*\w*:\w*)`, []byte(acl)) if !matched || err != nil { return false } From 597c15cfffc910cd7318d9b4ba5cefaab941531a Mon Sep 17 00:00:00 2001 From: M Date: Thu, 17 Feb 2022 15:37:49 +0530 Subject: [PATCH 05/11] NFSv4 acl support --- pkg/node/acl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/node/acl.go b/pkg/node/acl.go index e321907e..7ef52587 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -64,7 +64,7 @@ func posixMode(acls string) bool { func nfsv4ACLs(acls string) bool { aclsList := strings.Split(acls, ",") for _, acl := range aclsList { - matched, err := regexp.Match(`([ADUL]:\w*:\w*[@]*\w*:\w*)`, []byte(acl)) + matched, err := regexp.Match(`([ADUL]:\w*:[\w.]*[@]*[\w.]*:\w*)`, []byte(acl)) if !matched || err != nil { return false } From 201870cf3bdd089ad19e5625c0158c0575c3b601 Mon Sep 17 00:00:00 2001 From: M Date: Fri, 18 Feb 2022 16:43:53 +0530 Subject: [PATCH 06/11] Addressing review comments --- helm/csi-powerstore/values.yaml | 2 +- pkg/controller/controller.go | 8 ++------ pkg/controller/controller_test.go | 8 +++++--- pkg/node/acl.go | 4 ++-- pkg/node/stager.go | 8 ++++++-- samples/secret/secret.yaml | 2 +- samples/storageclass/powerstore-nfs.yaml | 2 +- 7 files changed, 18 insertions(+), 16 deletions(-) diff --git a/helm/csi-powerstore/values.yaml b/helm/csi-powerstore/values.yaml index ca9cfccb..d2c41278 100644 --- a/helm/csi-powerstore/values.yaml +++ b/helm/csi-powerstore/values.yaml @@ -35,7 +35,7 @@ externalAccess: imagePullPolicy: IfNotPresent # nfsAcls: enables setting permissions on NFS mount directory -# This value acts as default value for nfsAcls, if not specified for an array config in secret +# This value acts as default value for NFS ACL (nfsAcls), if not specified for an array config in secret # Permissions can be specified in two formats: # 1) Unix mode (NFSv3) # 2) NFSv4 acl (NFSv4) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 44ca956f..3ab01a75 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -23,7 +23,6 @@ import ( "context" "errors" "fmt" - "os" "sort" "strconv" @@ -87,14 +86,11 @@ func (s *Service) Init() error { s.isHealthMonitorEnabled, _ = strconv.ParseBool(isHealthMonitorEnabled) } + s.nfsAcls = "" if nfsAcls, ok := csictx.LookupEnv(ctx, common.EnvNfsAcls); ok { - if nfsAcls == "" { - nfsAcls = fmt.Sprintf("%s", os.ModePerm) // Default to 0777 - } else { + if nfsAcls != "" { s.nfsAcls = nfsAcls } - } else { - nfsAcls = fmt.Sprintf("%s", os.ModePerm) // Default to 0777 } return nil diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 6edb315e..bc4c1edf 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -485,8 +485,8 @@ var _ = Describe("CSIControllerService", func() { }) }) - When("creating nfs volume with NFS acls in not in secrets", func() { - It("should successfully create nfs volume with default NFS acls in volume response", func() { + When("creating nfs volume with NFS acls in not in secrets & default", func() { + It("should successfully create nfs volume with empty NFS acls in volume response", func() { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, nil) clientMock.On("CreateFS", mock.Anything, mock.Anything).Return(gopowerstore.CreateResponse{ID: validBaseVolID}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) @@ -494,7 +494,9 @@ var _ = Describe("CSIControllerService", func() { req := getTypicalCreateVolumeNFSRequest("my-vol", validVolSize) req.Parameters[common.KeyArrayID] = secondValidID + ctrlSvc.Arrays()[secondValidID].NfsAcls = "" csictx.Setenv(context.Background(), common.EnvNfsAcls, "") + _ = ctrlSvc.Init() res, err := ctrlSvc.CreateVolume(context.Background(), req) @@ -507,7 +509,7 @@ var _ = Describe("CSIControllerService", func() { common.KeyArrayVolumeName: "my-vol", common.KeyProtocol: "nfs", common.KeyArrayID: secondValidID, - common.KeyNfsACL: "A::OWNER@:RWX", + common.KeyNfsACL: "", common.KeyNasName: validNasName, }, }, diff --git a/pkg/node/acl.go b/pkg/node/acl.go index 7ef52587..3026f2cc 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -74,9 +74,9 @@ func nfsv4ACLs(acls string) bool { func setNfsv4Acls(acls string, dir string) error { command := []string{"nfs4_setfacl", "-s", acls, dir} - log.Info("NFSv4 ACL command: " + strings.Join(command, " ") + "\n") + log.Infof("NFSv4 ACL command: %s \n", strings.Join(command, " ")) outStr, err := execCommand(command) - log.Info("NFSv4 ACL output: " + string(outStr) + "\n") + log.Infof("NFSv4 ACL output: %s \n", string(outStr)) return err } diff --git a/pkg/node/stager.go b/pkg/node/stager.go index 193cff51..4dcee468 100644 --- a/pkg/node/stager.go +++ b/pkg/node/stager.go @@ -178,8 +178,12 @@ func (n *NFSStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest, aclsConfigured := false if acls != "" { if posixMode(acls) { - perm, _ := strconv.ParseUint(acls, 8, 64) - mode = os.FileMode(perm) + perm, err := strconv.ParseUint(acls, 8, 64) + if err == nil { + mode = os.FileMode(perm) + } else { + log.WithFields(logFields).Warn("can't parse file mode, invalid mode specified. Default mode permissions will be set.") + } } else { aclsConfigured, err = validateAndSetACLs(ctx, nasName, n.array.GetClient(), acls, filepath.Join(stagingPath, commonNfsVolumeFolder)) if err != nil || !aclsConfigured { diff --git a/samples/secret/secret.yaml b/samples/secret/secret.yaml index 54326d01..84a75379 100644 --- a/samples/secret/secret.yaml +++ b/samples/secret/secret.yaml @@ -52,7 +52,7 @@ arrays: nasName: "nas-server" # nfsAcls: enables setting permissions on NFS mount directory - # This value will be used if a storage class does not have the nfsAcls parameter specified + # This value will be used if a storage class does not have the NFS ACL (nfsAcls) parameter specified # Permissions can be specified in two formats: # 1) Unix mode (NFSv3) # 2) NFSv4 acl (NFSv4) diff --git a/samples/storageclass/powerstore-nfs.yaml b/samples/storageclass/powerstore-nfs.yaml index 6b12250d..495696ef 100644 --- a/samples/storageclass/powerstore-nfs.yaml +++ b/samples/storageclass/powerstore-nfs.yaml @@ -35,7 +35,7 @@ parameters: allowRoot: "false" # nfsAcls: enables setting permissions on NFS mount directory - # This value overrides the nfsAcls attribute of corresponding array config in secret, if present + # This value overrides the NFS ACL (nfsAcls) attribute of corresponding array config in secret, if present # Permissions can be specified in two formats: # 1) Unix mode (NFSv3) # 2) NFSv4 acl (NFSv4) From c3e36b89d5f5335d91a20269c052dd802f77e566 Mon Sep 17 00:00:00 2001 From: M Date: Mon, 21 Feb 2022 17:48:09 +0530 Subject: [PATCH 07/11] Addressing review comments --- mocks/NFSv4ACLsInterface.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 mocks/NFSv4ACLsInterface.go diff --git a/mocks/NFSv4ACLsInterface.go b/mocks/NFSv4ACLsInterface.go new file mode 100644 index 00000000..138d6968 --- /dev/null +++ b/mocks/NFSv4ACLsInterface.go @@ -0,0 +1,24 @@ +// Code generated by mockery v1.0.0. DO NOT EDIT. + +package mocks + +import mock "github.com/stretchr/testify/mock" + +// NFSv4ACLsInterface is an autogenerated mock type for the NFSv4ACLsInterface type +type NFSv4ACLsInterface struct { + mock.Mock +} + +// SetNfsv4Acls provides a mock function with given fields: acls, dir +func (_m *NFSv4ACLsInterface) SetNfsv4Acls(acls string, dir string) error { + ret := _m.Called(acls, dir) + + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(acls, dir) + } else { + r0 = ret.Error(0) + } + + return r0 +} From 2621ccd4f0d96eb3e9e8bfa8410924d935295f86 Mon Sep 17 00:00:00 2001 From: M Date: Mon, 21 Feb 2022 17:56:59 +0530 Subject: [PATCH 08/11] Fixing formatting errors --- mocks/NFSv4ACLsInterface.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/mocks/NFSv4ACLsInterface.go b/mocks/NFSv4ACLsInterface.go index 138d6968..32be3ebe 100644 --- a/mocks/NFSv4ACLsInterface.go +++ b/mocks/NFSv4ACLsInterface.go @@ -1,24 +1,24 @@ // Code generated by mockery v1.0.0. DO NOT EDIT. - + package mocks import mock "github.com/stretchr/testify/mock" // NFSv4ACLsInterface is an autogenerated mock type for the NFSv4ACLsInterface type type NFSv4ACLsInterface struct { - mock.Mock + mock.Mock } // SetNfsv4Acls provides a mock function with given fields: acls, dir func (_m *NFSv4ACLsInterface) SetNfsv4Acls(acls string, dir string) error { - ret := _m.Called(acls, dir) + ret := _m.Called(acls, dir) - var r0 error - if rf, ok := ret.Get(0).(func(string, string) error); ok { - r0 = rf(acls, dir) - } else { - r0 = ret.Error(0) - } + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(acls, dir) + } else { + r0 = ret.Error(0) + } - return r0 + return r0 } From 49be4ad665cb24620d280a67c47c824387861653 Mon Sep 17 00:00:00 2001 From: M Date: Mon, 21 Feb 2022 22:51:17 +0530 Subject: [PATCH 09/11] Addressing review comments --- helm/csi-powerstore/values.yaml | 3 ++- pkg/node/acl.go | 27 ++++++++++++------------ pkg/node/stager.go | 2 +- samples/secret/secret.yaml | 3 ++- samples/storageclass/powerstore-nfs.yaml | 3 ++- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/helm/csi-powerstore/values.yaml b/helm/csi-powerstore/values.yaml index d2c41278..5ce73426 100644 --- a/helm/csi-powerstore/values.yaml +++ b/helm/csi-powerstore/values.yaml @@ -38,7 +38,8 @@ imagePullPolicy: IfNotPresent # This value acts as default value for NFS ACL (nfsAcls), if not specified for an array config in secret # Permissions can be specified in two formats: # 1) Unix mode (NFSv3) -# 2) NFSv4 acl (NFSv4) +# 2) NFSv4 ACLs (NFSv4) +# NFSv4 ACLs are supported on NFSv4 share only. # Allowed values: # 1) Unix mode: valid octal mode number # Examples: "0777", "777", "0755" diff --git a/pkg/node/acl.go b/pkg/node/acl.go index 3026f2cc..e6a2a255 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -31,15 +31,17 @@ import ( "google.golang.org/grpc/status" ) -var ( - execCommand = executeCommand -) +type NFSv4ACLsInterface interface { + SetNfsv4Acls(acls string, dir string) error +} -func validateAndSetACLs(ctx context.Context, nasName string, client gopowerstore.Client, acls string, dir string) (bool, error) { +type NFSv4ACLs struct{} + +func validateAndSetACLs(ctx context.Context, s NFSv4ACLsInterface, nasName string, client gopowerstore.Client, acls string, dir string) (bool, error) { aclsConfigured := false if nfsv4ACLs(acls) { - if nfsv4NasServer(ctx, client, nasName) { - if err := setNfsv4Acls(acls, dir); err != nil { + if isNfsv4Enabled(ctx, client, nasName) { + if err := s.SetNfsv4Acls(acls, dir); err != nil { log.Error(fmt.Sprintf("can't assign NFSv4 ACLs to folder %s: %s", dir, err.Error())) return false, err } @@ -72,20 +74,17 @@ func nfsv4ACLs(acls string) bool { return true } -func setNfsv4Acls(acls string, dir string) error { +func (n *NFSv4ACLs) SetNfsv4Acls(acls string, dir string) error { command := []string{"nfs4_setfacl", "-s", acls, dir} log.Infof("NFSv4 ACL command: %s \n", strings.Join(command, " ")) - outStr, err := execCommand(command) + // arguments for exec.Command() are validated in caller + cmd := exec.Command(command[0], command[1:]...) // #nosec G204 + outStr, err := cmd.Output() log.Infof("NFSv4 ACL output: %s \n", string(outStr)) return err } -func executeCommand(command []string) ([]byte, error) { - cmd := exec.Command(command[0], command[1:]...) // #nosec G204 - return cmd.Output() -} - -func nfsv4NasServer(ctx context.Context, client gopowerstore.Client, nasName string) bool { +func isNfsv4Enabled(ctx context.Context, client gopowerstore.Client, nasName string) bool { nfsv4Enabled := false nas, err := gopowerstore.Client.GetNASByName(client, ctx, nasName) if err == nil { diff --git a/pkg/node/stager.go b/pkg/node/stager.go index 4dcee468..f4ec9ae7 100644 --- a/pkg/node/stager.go +++ b/pkg/node/stager.go @@ -185,7 +185,7 @@ func (n *NFSStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest, log.WithFields(logFields).Warn("can't parse file mode, invalid mode specified. Default mode permissions will be set.") } } else { - aclsConfigured, err = validateAndSetACLs(ctx, nasName, n.array.GetClient(), acls, filepath.Join(stagingPath, commonNfsVolumeFolder)) + aclsConfigured, err = validateAndSetACLs(ctx, &NFSv4ACLs{}, nasName, n.array.GetClient(), acls, filepath.Join(stagingPath, commonNfsVolumeFolder)) if err != nil || !aclsConfigured { return nil, err } diff --git a/samples/secret/secret.yaml b/samples/secret/secret.yaml index 84a75379..f6b2d448 100644 --- a/samples/secret/secret.yaml +++ b/samples/secret/secret.yaml @@ -55,7 +55,8 @@ arrays: # This value will be used if a storage class does not have the NFS ACL (nfsAcls) parameter specified # Permissions can be specified in two formats: # 1) Unix mode (NFSv3) - # 2) NFSv4 acl (NFSv4) + # 2) NFSv4 ACLs (NFSv4) + # NFSv4 ACLs are supported on NFSv4 share only. # Allowed values: # 1) Unix mode: valid octal mode number # Examples: "0777", "777", "0755" diff --git a/samples/storageclass/powerstore-nfs.yaml b/samples/storageclass/powerstore-nfs.yaml index 495696ef..7ebf46c3 100644 --- a/samples/storageclass/powerstore-nfs.yaml +++ b/samples/storageclass/powerstore-nfs.yaml @@ -38,7 +38,8 @@ parameters: # This value overrides the NFS ACL (nfsAcls) attribute of corresponding array config in secret, if present # Permissions can be specified in two formats: # 1) Unix mode (NFSv3) - # 2) NFSv4 acl (NFSv4) + # 2) NFSv4 ACLs (NFSv4) + # NFSv4 ACLs are supported on NFSv4 share only. # Allowed values: # 1) Unix mode: valid octal mode number # Examples: "0777", "777", "0755" From b0af8863582f1eb79910994483324af531450bd0 Mon Sep 17 00:00:00 2001 From: M Date: Mon, 21 Feb 2022 22:56:55 +0530 Subject: [PATCH 10/11] Fixing linting errors --- pkg/node/acl.go | 3 +++ pkg/node/acl_test.go | 40 ++++++++++++++++------------------------ 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/pkg/node/acl.go b/pkg/node/acl.go index e6a2a255..93609ce3 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -31,10 +31,13 @@ import ( "google.golang.org/grpc/status" ) +// NFSv4ACLsInterface contains method definition to set NFSv4 ACLs type NFSv4ACLsInterface interface { + // SetNfsv4Acls sets NFSv4 ACLs SetNfsv4Acls(acls string, dir string) error } +// NFSv4ACLs implements setting NFSv4 ACLs type NFSv4ACLs struct{} func validateAndSetACLs(ctx context.Context, s NFSv4ACLsInterface, nasName string, client gopowerstore.Client, acls string, dir string) (bool, error) { diff --git a/pkg/node/acl_test.go b/pkg/node/acl_test.go index a8d96612..b66ecbd3 100644 --- a/pkg/node/acl_test.go +++ b/pkg/node/acl_test.go @@ -6,11 +6,14 @@ import ( "fmt" "testing" + "github.com/dell/csi-powerstore/mocks" "github.com/dell/gopowerstore" gopowerstoremock "github.com/dell/gopowerstore/mocks" "github.com/stretchr/testify/mock" ) +var nfsv4ACLsMock *mocks.NFSv4ACLsInterface + func TestPosixMode_Success(t *testing.T) { isPosixMode := posixMode("0755") expected := true @@ -56,7 +59,7 @@ func TestNfsv4NasServer_Success(t *testing.T) { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) - isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + isNFSv4Enabled := isNfsv4Enabled(context.Background(), clientMock, validNasName) expected := true if isNFSv4Enabled != expected { t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) @@ -69,7 +72,7 @@ func TestNfsv4NasServer_Err_GetNASByName(t *testing.T) { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID}, errors.New("GetNASByName_fail")) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) - isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + isNFSv4Enabled := isNfsv4Enabled(context.Background(), clientMock, validNasName) expected := false if isNFSv4Enabled != expected { t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) @@ -89,7 +92,7 @@ func TestNfsv4NasServer_Err_GetNfsServer(t *testing.T) { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, errors.New("GetNfsServer_fail")) - isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + isNFSv4Enabled := isNfsv4Enabled(context.Background(), clientMock, validNasName) expected := false if isNFSv4Enabled != expected { t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) @@ -109,7 +112,7 @@ func TestNfsv4NasServer_Fail(t *testing.T) { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: false}, nil) - isNFSv4Enabled := nfsv4NasServer(context.Background(), clientMock, validNasName) + isNFSv4Enabled := isNfsv4Enabled(context.Background(), clientMock, validNasName) expected := false if isNFSv4Enabled != expected { t.Errorf(fmt.Sprintf("expected: %v, actual: %v", expected, isNFSv4Enabled)) @@ -118,6 +121,7 @@ func TestNfsv4NasServer_Fail(t *testing.T) { func TestValidateAndSetNfsACLs_Success_nfsv4Acls(t *testing.T) { clientMock = new(gopowerstoremock.Client) + nfsv4ACLsMock = new(mocks.NFSv4ACLsInterface) nfsServers := []gopowerstore.NFSServerInstance{ { @@ -126,13 +130,11 @@ func TestValidateAndSetNfsACLs_Success_nfsv4Acls(t *testing.T) { }, } + nfsv4ACLsMock.On("SetNfsv4Acls", mock.Anything, mock.Anything).Return(nil) clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) - execCommand = executeCommandMock - defer execCommandReset() - - aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "A::OWNER@:RWX", "dir1") + aclConfigured, err := validateAndSetACLs(context.Background(), nfsv4ACLsMock, validNasName, clientMock, "A::OWNER@:RWX", "dir2") if err != nil || aclConfigured == false { t.Errorf(fmt.Sprintf("expected: true, actual: %v err: %s", aclConfigured, err.Error())) @@ -141,6 +143,7 @@ func TestValidateAndSetNfsACLs_Success_nfsv4Acls(t *testing.T) { func TestValidateAndSetNfsACLs_Fail_InvalidAcls(t *testing.T) { clientMock = new(gopowerstoremock.Client) + nfsv4ACLsMock = new(mocks.NFSv4ACLsInterface) nfsServers := []gopowerstore.NFSServerInstance{ { @@ -151,11 +154,9 @@ func TestValidateAndSetNfsACLs_Fail_InvalidAcls(t *testing.T) { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, nil) + nfsv4ACLsMock.On("setNfsv4Acls", mock.Anything, mock.Anything).Return(nil) - execCommand = executeCommandMock - defer execCommandReset() - - aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "abcd", "dir1") + aclConfigured, err := validateAndSetACLs(context.Background(), nfsv4ACLsMock, validNasName, clientMock, "abcd", "dir1") if err == nil || aclConfigured != false { t.Errorf(fmt.Sprintf("expected: false, actual: %v err: %s", aclConfigured, err.Error())) @@ -164,6 +165,7 @@ func TestValidateAndSetNfsACLs_Fail_InvalidAcls(t *testing.T) { func TestValidateAndSetNfsACLs_Fail_GetNfsServerFail(t *testing.T) { clientMock = new(gopowerstoremock.Client) + nfsv4ACLsMock = new(mocks.NFSv4ACLsInterface) nfsServers := []gopowerstore.NFSServerInstance{ { @@ -174,21 +176,11 @@ func TestValidateAndSetNfsACLs_Fail_GetNfsServerFail(t *testing.T) { clientMock.On("GetNASByName", mock.Anything, validNasName).Return(gopowerstore.NAS{ID: validNasID, NfsServers: nfsServers}, nil) clientMock.On("GetNfsServer", mock.Anything, mock.Anything).Return(gopowerstore.NFSServerInstance{Id: validNfsServerID, IsNFSv4Enabled: true}, errors.New("GetNfsServer_fail")) + nfsv4ACLsMock.On("setNfsv4Acls", mock.Anything, mock.Anything).Return(nil) - execCommand = executeCommandMock - defer execCommandReset() - - aclConfigured, err := validateAndSetACLs(context.Background(), validNasName, clientMock, "u::rwx", "dir1") + aclConfigured, err := validateAndSetACLs(context.Background(), nfsv4ACLsMock, validNasName, clientMock, "A::OWNER@:RWX", "dir1") if err == nil || aclConfigured != false { t.Errorf(fmt.Sprintf("expected: false, actual: %v err: %s", aclConfigured, err.Error())) } } - -func executeCommandMock(command []string) ([]byte, error) { - return []byte("executed command successfully"), nil -} - -func execCommandReset() { - execCommand = executeCommand -} From aaecd6bda66450cdb1c9bb91839c22fe8dcc682b Mon Sep 17 00:00:00 2001 From: M Date: Mon, 21 Feb 2022 23:01:00 +0530 Subject: [PATCH 11/11] Fixing linting errors --- pkg/node/acl.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/node/acl.go b/pkg/node/acl.go index 93609ce3..34703bd6 100644 --- a/pkg/node/acl.go +++ b/pkg/node/acl.go @@ -77,6 +77,7 @@ func nfsv4ACLs(acls string) bool { return true } +// SetNfsv4Acls sets NFSv4 ACLS func (n *NFSv4ACLs) SetNfsv4Acls(acls string, dir string) error { command := []string{"nfs4_setfacl", "-s", acls, dir} log.Infof("NFSv4 ACL command: %s \n", strings.Join(command, " "))