From 31d4da2285ae50a8996620ffc9a6076e29b46d9d Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Mon, 18 Oct 2021 09:40:14 +0100 Subject: [PATCH 01/13] Adds subPathPattern to compliment basePath Adding subPathPattern allows us to support more use cases than before because now users can specify a template they wish to be created when the access point is created. This template matches the form of the nfs-subdir-provioner which aids consistency. This defaults to /${.PV.name} to be in keeping with current behaviour but allows much more flexibility. --- pkg/driver/controller.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index e2b8d147b..7f7226df7 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "os" + "path" "strconv" "strings" @@ -45,7 +46,11 @@ const ( GidMax = "gidRangeEnd" MountTargetIp = "mounttargetip" ProvisioningMode = "provisioningMode" + PvName = "csi.storage.k8s.io/pv/name" + PvcName = "csi.storage.k8s.io/pvc/name" + PvcNamespace = "csi.storage.k8s.io/pvc/namespace" RoleArn = "awsRoleArn" + SubPathPattern = "subPathPattern" TempMountPathPrefix = "/var/lib/csi/pv" Uid = "uid" ) @@ -234,7 +239,25 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } rootDirName := volName - rootDir := basePath + "/" + rootDirName + // Check if a custom structure should be imposed on the access point directory + if value, ok := volumeParams[SubPathPattern]; ok { + if len(value) == 0 { + return nil, status.Errorf(codes.InvalidArgument, "No pattern specified for subPath.") + } + // Try and construct the root directory and check it only contains supported components + val, err := interpolateRootDirectoryName(value, volumeParams) + if err == nil { + klog.Infof("Using user-specified structure for access point directory.") + rootDirName = val + } else { + return nil, err + } + } else { + klog.Infof("Using PV name for access point directory.") + } + + rootDir := path.Join("/", basePath, rootDirName) + klog.Infof("Using %v as the access point directory.", rootDir) accessPointsOptions.Uid = int64(uid) accessPointsOptions.Gid = int64(gid) From 6fee80acfb12ee5d260efc09360c593416ed25ac Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Mon, 18 Oct 2021 09:40:14 +0100 Subject: [PATCH 02/13] Adding functions to interpolate names from template These functions allow us to take a pattern and turn it into a valid directory name. These can then be called in the method that builds up the directory name to be sent to the API. --- pkg/driver/controller.go | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 7f7226df7..be0bb81b0 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path" + "sort" "strconv" "strings" @@ -60,6 +61,13 @@ var ( controllerCaps = []csi.ControllerServiceCapability_RPC_Type{ csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, } + // subPathPatternComponents shows the elements that we allow to be in the construction of the root directory + // of the access point, as well as the values we need to extract them from the Volume Parameters. + subPathPatternComponents = map[string]string{ + ".PVC.name": PvcName, + ".PVC.namespace": PvcNamespace, + ".PV.name": PvName, + } ) func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { @@ -494,3 +502,39 @@ func getCloud(secrets map[string]string, driver *Driver) (cloud.Cloud, string, e return localCloud, roleArn, nil } + +func interpolateRootDirectoryName(rootDirectoryPath string, volumeParams map[string]string) (string, error) { + r := strings.NewReplacer(createListOfVariableSubstitutions(volumeParams)...) + result := r.Replace(rootDirectoryPath) + + // Check if any templating characters still exist + if strings.Contains(result, "${") || strings.Contains(result, "}") { + return "", status.Errorf(codes.InvalidArgument, + "Path specified \"%v\" contains invalid elements. Can only contain %v", rootDirectoryPath, + getSupportedComponentNames()) + } + return result, nil +} + +func createListOfVariableSubstitutions(volumeParams map[string]string) []string { + variableSubstitutions := make([]string, 2*len(subPathPatternComponents)) + i := 0 + for key, volumeParamsKey := range subPathPatternComponents { + variableSubstitutions[i] = "${" + key + "}" + variableSubstitutions[i+1] = volumeParams[volumeParamsKey] + i += 2 + } + return variableSubstitutions +} + +func getSupportedComponentNames() []string { + keys := make([]string, len(subPathPatternComponents)) + + i := 0 + for key := range subPathPatternComponents { + keys[i] = key + i++ + } + sort.Strings(keys) + return keys +} From bc4c46365f01833d955ad2fccecae5d85daaf825 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Mon, 18 Oct 2021 09:40:14 +0100 Subject: [PATCH 03/13] Updates testing Adds happy path testing for subPathPattern including whether it plays nicely with existing basePath parameter --- pkg/driver/controller_test.go | 314 ++++++++++++++++++++++++++++++++++ 1 file changed, 314 insertions(+) diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 96f4790a9..248cdf495 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -3,6 +3,9 @@ package driver import ( "context" "errors" + "fmt" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "testing" "github.com/container-storage-interface/spec/lib/go/csi" @@ -391,6 +394,223 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Success: Normal flow with a valid directory structure set", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + pvName := "foo" + pvcName := "bar" + directoryCreated := fmt.Sprintf("/%s/%s", pvName, pvcName) + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "${.PV.name}/${.PVC.name}", + PvName: pvName, + PvcName: pvcName, + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if accessPointOpts.DirectoryPath != directoryCreated { + t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + directoryCreated, + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, + { + name: "Success: Normal flow with a valid directory structure set, using a single element", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + pvcName := "foo" + directoryCreated := fmt.Sprintf("/%s", pvcName) + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "${.PVC.name}", + PvcName: pvcName, + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if accessPointOpts.DirectoryPath != directoryCreated { + t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + directoryCreated, + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, + { + name: "Success: Normal flow with a valid directory structure set, and a basePath", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + pvcName := "foo" + basePath := "bash" + directoryCreated := fmt.Sprintf("/%s/%s", basePath, pvcName) + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "${.PVC.name}", + BasePath: basePath, + PvcName: pvcName, + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if accessPointOpts.DirectoryPath != directoryCreated { + t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + directoryCreated, + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, { name: "Fail: Volume name missing", testFunc: func(t *testing.T) { @@ -1360,6 +1580,100 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Fail: accessPointDirStructure is specified but empty", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + DirectoryPerms: "777", + SubPathPattern: "", + }, + } + + ctx := context.Background() + + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("Did not throw InvalidArgument error, instead threw %v", err) + } + mockCtl.Finish() + }, + }, + { + name: "Fail: accessPointDirStructure is specified but uses unsupported attributes", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + accessPointStructure := "${.PVC.name}/${foo}" + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + DirectoryPerms: "777", + SubPathPattern: accessPointStructure, + }, + } + + ctx := context.Background() + + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("Did not throw InvalidArgument error, instead threw %v", err) + } + mockCtl.Finish() + }, + }, } for _, tc := range testCases { From e62c49c0f06314517773707738f6f79ebffe8186 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Mon, 18 Oct 2021 09:40:14 +0100 Subject: [PATCH 04/13] Updating documentation and examples --- charts/aws-efs-csi-driver/values.yaml | 1 + docs/README.md | 2 +- examples/kubernetes/dynamic_provisioning/README.md | 7 ++++++- .../dynamic_provisioning/specs/storageclass.yaml | 3 ++- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/charts/aws-efs-csi-driver/values.yaml b/charts/aws-efs-csi-driver/values.yaml index 511d70e35..cfcbdc7d1 100644 --- a/charts/aws-efs-csi-driver/values.yaml +++ b/charts/aws-efs-csi-driver/values.yaml @@ -136,5 +136,6 @@ storageClasses: [] # gidRangeStart: "1000" # gidRangeEnd: "2000" # basePath: "/dynamic_provisioning" +# subPathPattern: "/subPath" # reclaimPolicy: Delete # volumeBindingMode: Immediate diff --git a/docs/README.md b/docs/README.md index b6bfe5f46..afd268765 100644 --- a/docs/README.md +++ b/docs/README.md @@ -36,8 +36,8 @@ The following CSI interfaces are implemented: | gidRangeStart | | 50000 | true | Start range of the POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. Not used if uid/gid is set. | | gidRangeEnd | | 7000000 | true | End range of the POSIX group Id. Not used if uid/gid is set. | | basePath | | | true | Path under which access points for dynamic provisioning is created. If this parameter is not specified, access points are created under the root directory of the file system | +| subPathPattern | | `/${.PV.name}` | true | The template used to construct the subPath under which each of the access points created under Dynamic Provisioning. Can be made up of fixed strings and limited variables, is akin to the 'subPathPattern' variable on the [nfs-subdir-external-provisioner](https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner) chart. Supports `.PVC.name`,`.PVC.namespace` and `.PV.name`| | az | | "" | true | Used for cross-account mount. `az` under storage class parameter is optional. If specified, mount target associated with the az will be used for cross-account mount. If not specified, a random mount target will be picked for cross account mount | - **Notes**: * Custom Posix group Id range for Access Point root directory must include both `gidRangeStart` and `gidRangeEnd` parameters. These parameters are optional only if both are omitted. If you specify one, the other becomes mandatory. * When using a custom Posix group ID range, there is a possibility for the driver to run out of available POSIX group Ids. We suggest ensuring custom group ID range is large enough or create a new storage class with a new file system to provision additional volumes. diff --git a/examples/kubernetes/dynamic_provisioning/README.md b/examples/kubernetes/dynamic_provisioning/README.md index abcf5b76d..5759149a2 100644 --- a/examples/kubernetes/dynamic_provisioning/README.md +++ b/examples/kubernetes/dynamic_provisioning/README.md @@ -20,13 +20,18 @@ parameters: gidRangeStart: "1000" gidRangeEnd: "2000" basePath: "/dynamic_provisioning" + subPathPattern: "${.PVC.namespace}/${PVC.name}" ``` * provisioningMode - The type of volume to be provisioned by efs. Currently, only access point based provisioning is supported `efs-ap`. * fileSystemId - The file system under which Access Point is created. * directoryPerms - Directory Permissions of the root directory created by Access Point. * gidRangeStart (Optional) - Starting range of Posix Group ID to be applied onto the root directory of the access point. Default value is 50000. * gidRangeEnd (Optional) - Ending range of Posix Group ID. Default value is 7000000. -* basePath (Optional) - Path on the file system under which access point root directory is created. If path is not provided, access points root directory are created under the root of the file system. +* basePath (Optional) - Path on the file system under which access point root directory is created. If path is not + provided, access points root directory are created under the root of the file system. +* subPathPattern (Optional) - A pattern that describes the subPath under which an access point should be created. So in + the example given above if the PVC namespace is `foo` and the PVC name is `pvc-123-456` the access point would be + created at `/dynamic_provisioner/foo/pvc-123-456`. ### Deploy the Example Create storage class, persistent volume claim (PVC) and the pod which consumes PV: diff --git a/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml b/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml index 0fcb0cc8f..ab2d8910c 100644 --- a/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml +++ b/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml @@ -9,4 +9,5 @@ parameters: directoryPerms: "700" gidRangeStart: "1000" # optional gidRangeEnd: "2000" # optional - basePath: "/dynamic_provisioning" # optional \ No newline at end of file + basePath: "/dynamic_provisioning" # optional + subPathPattern: "${.PVC.namespace}/${.PVC.name}" # optional \ No newline at end of file From 99f2f8767e4f30f827b23b96b195b2a0b32771fb Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Tue, 7 Dec 2021 06:37:37 +0000 Subject: [PATCH 05/13] Removes 0-length check to allow setting "/" as access point directory Removing the 0-length check allows us to support not setting a basePath but having an empty pattern, as distinct from not setting subPathPattern, which means "/" can be used as a valid accessPoint path. --- pkg/driver/controller.go | 11 ++-- pkg/driver/controller_test.go | 113 ++++++++++++++++++++-------------- 2 files changed, 71 insertions(+), 53 deletions(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index be0bb81b0..ff964f5b8 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -203,10 +203,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) accessPointsOptions.DirectoryPerms = value } - if value, ok := volumeParams[BasePath]; ok { - basePath = value - } - // Storage class parameter `az` will be used to fetch preferred mount target for cross account mount. // If the `az` storage class parameter is not provided, a random mount target will be picked for mounting. // This storage class parameter different from `az` mount option provided by efs-utils https://github.com/aws/efs-utils/blob/v1.31.1/src/mount_efs/__init__.py#L195 @@ -246,12 +242,13 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) gid = allocatedGid } + if value, ok := volumeParams[BasePath]; ok { + basePath = value + } + rootDirName := volName // Check if a custom structure should be imposed on the access point directory if value, ok := volumeParams[SubPathPattern]; ok { - if len(value) == 0 { - return nil, status.Errorf(codes.InvalidArgument, "No pattern specified for subPath.") - } // Try and construct the root directory and check it only contains supported components val, err := interpolateRootDirectoryName(value, volumeParams) if err == nil { diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 248cdf495..ed63598eb 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -611,6 +611,73 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Success: Normal flow with an empty subPath Pattern and no basePath", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "", + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if accessPointOpts.DirectoryPath != "/" { + t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + "/", + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, { name: "Fail: Volume name missing", testFunc: func(t *testing.T) { @@ -1580,52 +1647,6 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, - { - name: "Fail: accessPointDirStructure is specified but empty", - testFunc: func(t *testing.T) { - mockCtl := gomock.NewController(t) - mockCloud := mocks.NewMockCloud(mockCtl) - - driver := &Driver{ - endpoint: endpoint, - cloud: mockCloud, - gidAllocator: NewGidAllocator(), - } - - req := &csi.CreateVolumeRequest{ - Name: volumeName, - VolumeCapabilities: []*csi.VolumeCapability{ - stdVolCap, - }, - CapacityRange: &csi.CapacityRange{ - RequiredBytes: capacityRange, - }, - Parameters: map[string]string{ - ProvisioningMode: "efs-ap", - FsId: fsId, - DirectoryPerms: "777", - SubPathPattern: "", - }, - } - - ctx := context.Background() - - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) - - _, err := driver.CreateVolume(ctx, req) - if err == nil { - t.Fatal("CreateVolume did not fail") - } - if status.Code(err) != codes.InvalidArgument { - t.Fatalf("Did not throw InvalidArgument error, instead threw %v", err) - } - mockCtl.Finish() - }, - }, { name: "Fail: accessPointDirStructure is specified but uses unsupported attributes", testFunc: func(t *testing.T) { From 24a20a2ca803a7bb947b191a17f9b4f81009b8bf Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Sat, 25 Jun 2022 21:11:25 +0100 Subject: [PATCH 06/13] Adding ensureUniqueDirectory This ensures we can control when we want directories created under dynamic provisioning to be unique and when we don't (poweruser mode). This also adds some successful tests, next commit will add the failure cases to make sure all bases are covered. --- charts/aws-efs-csi-driver/values.yaml | 1 + docs/README.md | 27 ++-- .../specs/storageclass.yaml | 3 +- go.mod | 1 + pkg/driver/controller.go | 70 +++++++--- pkg/driver/controller_test.go | 124 +++++++++++++++--- 6 files changed, 172 insertions(+), 54 deletions(-) diff --git a/charts/aws-efs-csi-driver/values.yaml b/charts/aws-efs-csi-driver/values.yaml index cfcbdc7d1..8fc7b43a0 100644 --- a/charts/aws-efs-csi-driver/values.yaml +++ b/charts/aws-efs-csi-driver/values.yaml @@ -137,5 +137,6 @@ storageClasses: [] # gidRangeEnd: "2000" # basePath: "/dynamic_provisioning" # subPathPattern: "/subPath" +# ensureUniqueDirectory: true # reclaimPolicy: Delete # volumeBindingMode: Immediate diff --git a/docs/README.md b/docs/README.md index afd268765..d2a2c718a 100644 --- a/docs/README.md +++ b/docs/README.md @@ -26,18 +26,19 @@ The following CSI interfaces are implemented: * Identity Service: GetPluginInfo, GetPluginCapabilities, Probe ### Storage Class Parameters for Dynamic Provisioning -| Parameters | Values | Default | Optional | Description | -|---------------------|--------|---------|-----------|-------------| -| provisioningMode | efs-ap | | false | Type of volume provisioned by efs. Currently, Access Points are supported. | -| fileSystemId | | | false | File System under which access points are created. | -| directoryPerms | | | false | Directory permissions for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | -| uid | | | true | POSIX user Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | -| gid | | | true | POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | -| gidRangeStart | | 50000 | true | Start range of the POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. Not used if uid/gid is set. | -| gidRangeEnd | | 7000000 | true | End range of the POSIX group Id. Not used if uid/gid is set. | -| basePath | | | true | Path under which access points for dynamic provisioning is created. If this parameter is not specified, access points are created under the root directory of the file system | -| subPathPattern | | `/${.PV.name}` | true | The template used to construct the subPath under which each of the access points created under Dynamic Provisioning. Can be made up of fixed strings and limited variables, is akin to the 'subPathPattern' variable on the [nfs-subdir-external-provisioner](https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner) chart. Supports `.PVC.name`,`.PVC.namespace` and `.PV.name`| -| az | | "" | true | Used for cross-account mount. `az` under storage class parameter is optional. If specified, mount target associated with the az will be used for cross-account mount. If not specified, a random mount target will be picked for cross account mount | +| Parameters | Values | Default | Optional | Description | +|-----------------------|--------|-----------------|----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| provisioningMode | efs-ap | | false | Type of volume provisioned by efs. Currently, Access Points are supported. | +| fileSystemId | | | false | File System under which access points are created. | +| directoryPerms | | | false | Directory permissions for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | +| uid | | | true | POSIX user Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | +| gid | | | true | POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | +| gidRangeStart | | 50000 | true | Start range of the POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. Not used if uid/gid is set. | +| gidRangeEnd | | 7000000 | true | End range of the POSIX group Id. Not used if uid/gid is set. | +| basePath | | | true | Path under which access points for dynamic provisioning is created. If this parameter is not specified, access points are created under the root directory of the file system | +| subPathPattern | | `/${.PV.name}` | true | The template used to construct the subPath under which each of the access points created under Dynamic Provisioning. Can be made up of fixed strings and limited variables, is akin to the 'subPathPattern' variable on the [nfs-subdir-external-provisioner](https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner) chart. Supports `.PVC.name`,`.PVC.namespace` and `.PV.name` | +| ensureUniqueDirectory | | true | true | **NOTE: Only set this to false if you're sure this is the behaviour you want**.
Used when dynamic provisioning is enabled, if set to true, appends the a UID to the pattern specified in `subPathPattern` to ensure that access points will not accidentally point at the same directory. | +| az | | "" | true | Used for cross-account mount. `az` under storage class parameter is optional. If specified, mount target associated with the az will be used for cross-account mount. If not specified, a random mount target will be picked for cross account mount | **Notes**: * Custom Posix group Id range for Access Point root directory must include both `gidRangeStart` and `gidRangeEnd` parameters. These parameters are optional only if both are omitted. If you specify one, the other becomes mandatory. * When using a custom Posix group ID range, there is a possibility for the driver to run out of available POSIX group Ids. We suggest ensuring custom group ID range is large enough or create a new storage class with a new file system to provision additional volumes. @@ -102,7 +103,7 @@ The following sections are Kubernetes specific. If you are a Kubernetes user, us ### Installation #### Set up driver permission: The driver requires IAM permission to talk to Amazon EFS to manage the volume on user's behalf. There are several methods to grant driver IAM permission: -* Using IAM Role for Service Account (Recommended if you're using EKS): create an [IAM Role for service accounts](https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html) with the [required permissions](./iam-policy-example.json). Uncomment annotations and put the IAM role ARN in [service-account manifest](../deploy/kubernetes/base/serviceaccount-csi-controller.yaml) +* Using IAM Role for Service Account (Recommended if you're using EKS): create an [IAM Role for service accounts](https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html) with the [required permissions](./iam-policy-example.json). Uncomment annotations and put the IAM role ARN in [service-account manifest](../deploy/kubernetes/base/controller-serviceaccount.yaml) * Using IAM [instance profile](https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2_instance-profiles.html) - grant all the worker nodes with [required permissions](./iam-policy-example.json) by attaching policy to the instance profile of the worker. #### Deploy the driver: diff --git a/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml b/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml index ab2d8910c..14c9a8efc 100644 --- a/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml +++ b/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml @@ -10,4 +10,5 @@ parameters: gidRangeStart: "1000" # optional gidRangeEnd: "2000" # optional basePath: "/dynamic_provisioning" # optional - subPathPattern: "${.PVC.namespace}/${.PVC.name}" # optional \ No newline at end of file + subPathPattern: "${.PVC.namespace}/${.PVC.name}" # optional + ensureUniqueDirectory: "true" # optional \ No newline at end of file diff --git a/go.mod b/go.mod index 5bdc9bffa..3adbb533c 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/onsi/ginkgo v1.14.0 github.com/onsi/gomega v1.10.1 google.golang.org/grpc v1.38.0 + github.com/google/uuid v1.1.2 k8s.io/api v0.22.2 k8s.io/apimachinery v0.22.2 k8s.io/client-go v0.22.2 diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index ff964f5b8..ab3361dc3 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -19,6 +19,7 @@ package driver import ( "context" "fmt" + "github.com/google/uuid" "os" "path" "sort" @@ -33,27 +34,28 @@ import ( ) const ( - AccessPointMode = "efs-ap" - AzName = "az" - BasePath = "basePath" - DefaultGidMin = 50000 - DefaultGidMax = 7000000 - DefaultTagKey = "efs.csi.aws.com/cluster" - DefaultTagValue = "true" - DirectoryPerms = "directoryPerms" - FsId = "fileSystemId" - Gid = "gid" - GidMin = "gidRangeStart" - GidMax = "gidRangeEnd" - MountTargetIp = "mounttargetip" - ProvisioningMode = "provisioningMode" - PvName = "csi.storage.k8s.io/pv/name" - PvcName = "csi.storage.k8s.io/pvc/name" - PvcNamespace = "csi.storage.k8s.io/pvc/namespace" - RoleArn = "awsRoleArn" - SubPathPattern = "subPathPattern" - TempMountPathPrefix = "/var/lib/csi/pv" - Uid = "uid" + AccessPointMode = "efs-ap" + AzName = "az" + BasePath = "basePath" + DefaultGidMin = 50000 + DefaultGidMax = 7000000 + DefaultTagKey = "efs.csi.aws.com/cluster" + DefaultTagValue = "true" + DirectoryPerms = "directoryPerms" + EnsureUniqueDirectory = "ensureUniqueDirectory" + FsId = "fileSystemId" + Gid = "gid" + GidMin = "gidRangeStart" + GidMax = "gidRangeEnd" + MountTargetIp = "mounttargetip" + ProvisioningMode = "provisioningMode" + PvName = "csi.storage.k8s.io/pv/name" + PvcName = "csi.storage.k8s.io/pvc/name" + PvcNamespace = "csi.storage.k8s.io/pvc/namespace" + RoleArn = "awsRoleArn" + SubPathPattern = "subPathPattern" + TempMountPathPrefix = "/var/lib/csi/pv" + Uid = "uid" ) var ( @@ -254,6 +256,17 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) if err == nil { klog.Infof("Using user-specified structure for access point directory.") rootDirName = val + if value, ok := volumeParams[EnsureUniqueDirectory]; ok { + if ensureUniqueDirectory, _ := strconv.ParseBool(value); !ensureUniqueDirectory { + klog.Infof("Not appending PVC UID to path.") + } else { + klog.Infof("Appending PVC UID to path.") + rootDirName = fmt.Sprintf("%s-%s", val, uuid.New().String()) + } + } else { + klog.Infof("Appending PVC UID to path.") + rootDirName = fmt.Sprintf("%s-%s", val, uuid.New().String()) + } } else { return nil, err } @@ -262,6 +275,9 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } rootDir := path.Join("/", basePath, rootDirName) + if ok, err := validateEfsPathRequirements(rootDir); !ok { + return nil, err + } klog.Infof("Using %v as the access point directory.", rootDir) accessPointsOptions.Uid = int64(uid) @@ -535,3 +551,15 @@ func getSupportedComponentNames() []string { sort.Strings(keys) return keys } + +func validateEfsPathRequirements(proposedPath string) (bool, error) { + if len(proposedPath) > 100 { + // Check the proposed path is 100 characters or less + return false, status.Errorf(codes.InvalidArgument, "Proposed path '%s' exceeds EFS limit of 100 characters", proposedPath) + } else if strings.Count(proposedPath, "/") > 5 { + // Check the proposed path contains at most 4 subdirectories + return false, status.Errorf(codes.InvalidArgument, "Proposed path '%s' EFS limit of 4 subdirectories", proposedPath) + } else { + return true, nil + } +} diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index ed63598eb..c1624a72c 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -4,8 +4,10 @@ import ( "context" "errors" "fmt" + "github.com/google/uuid" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "regexp" "testing" "github.com/container-storage-interface/spec/lib/go/csi" @@ -443,8 +445,8 @@ func TestCreateVolume(t *testing.T) { mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { - if accessPointOpts.DirectoryPath != directoryCreated { - t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { + t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, accessPointOpts.DirectoryPath) } @@ -514,8 +516,8 @@ func TestCreateVolume(t *testing.T) { mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { - if accessPointOpts.DirectoryPath != directoryCreated { - t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { + t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", directoryCreated, accessPointOpts.DirectoryPath) } @@ -564,14 +566,89 @@ func TestCreateVolume(t *testing.T) { RequiredBytes: capacityRange, }, Parameters: map[string]string{ - ProvisioningMode: "efs-ap", - FsId: fsId, - GidMin: "1000", - GidMax: "2000", - DirectoryPerms: "777", - SubPathPattern: "${.PVC.name}", - BasePath: basePath, - PvcName: pvcName, + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "${.PVC.name}", + BasePath: basePath, + EnsureUniqueDirectory: "true", + PvcName: pvcName, + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { + t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", + directoryCreated, + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, + { + name: "Success: Normal flow with a valid directory structure set, and a basePath, and uniqueness guarantees turned off", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + pvcName := "foo" + basePath := "bash" + directoryCreated := fmt.Sprintf("/%s/%s", basePath, pvcName) + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "${.PVC.name}", + BasePath: basePath, + EnsureUniqueDirectory: "false", + PvcName: pvcName, }, } @@ -612,7 +689,7 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Success: Normal flow with an empty subPath Pattern and no basePath", + name: "Success: Normal flow with an empty subPath Pattern, no basePath and uniqueness guarantees turned off", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -633,12 +710,13 @@ func TestCreateVolume(t *testing.T) { RequiredBytes: capacityRange, }, Parameters: map[string]string{ - ProvisioningMode: "efs-ap", - FsId: fsId, - GidMin: "1000", - GidMax: "2000", - DirectoryPerms: "777", - SubPathPattern: "", + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "", + EnsureUniqueDirectory: "false", }, } @@ -2267,3 +2345,11 @@ func TestControllerGetCapabilities(t *testing.T) { t.Fatalf("ControllerGetCapabilities failed: %v", err) } } + +func verifyPathWhenUUIDIncluded(pathToVerify string, expectedPathWithoutUUID string) bool { + r := regexp.MustCompile("(.*)-([0-9A-fA-F]+-[0-9A-fA-F]+-[0-9A-fA-F]+-[0-9A-fA-F]+-[0-9A-fA-F]+$)") + matches := r.FindStringSubmatch(pathToVerify) + doesPathMatchWithUuid := matches[1] == expectedPathWithoutUUID + _, err := uuid.Parse(matches[2]) + return err == nil && doesPathMatchWithUuid +} From dff2a2b68e48f5c9a21ffd0fee47bb9ca3b3f578 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Wed, 23 Feb 2022 06:15:57 +0000 Subject: [PATCH 07/13] Adding negative test cases and fixing as appropriate Added a fix around the handling for misconfiguration of the ensureUniqueDirectory whereby if it's misconfigured it will fall through to its default value. --- pkg/driver/controller.go | 2 +- pkg/driver/controller_test.go | 175 +++++++++++++++++++++++++++++++++- 2 files changed, 173 insertions(+), 4 deletions(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index ab3361dc3..9ff408871 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -257,7 +257,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) klog.Infof("Using user-specified structure for access point directory.") rootDirName = val if value, ok := volumeParams[EnsureUniqueDirectory]; ok { - if ensureUniqueDirectory, _ := strconv.ParseBool(value); !ensureUniqueDirectory { + if ensureUniqueDirectory, err := strconv.ParseBool(value); !ensureUniqueDirectory && err == nil { klog.Infof("Not appending PVC UID to path.") } else { klog.Infof("Appending PVC UID to path.") diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index c1624a72c..05b55cd98 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -688,6 +688,79 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Success: Normal flow with a valid directory structure set, but ensuring uniqueness is set incorrectly, so default of true is used." + + "", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + pvcName := "foo" + directoryCreated := fmt.Sprintf("/%s", pvcName) + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "${.PVC.name}", + EnsureUniqueDirectory: "banana", + PvcName: pvcName, + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if !verifyPathWhenUUIDIncluded(accessPointOpts.DirectoryPath, directoryCreated) { + t.Fatalf("Root directory mismatch. Expected: %v (with UID appended), actual: %v", + directoryCreated, + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, { name: "Success: Normal flow with an empty subPath Pattern, no basePath and uniqueness guarantees turned off", testFunc: func(t *testing.T) { @@ -1726,12 +1799,108 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: accessPointDirStructure is specified but uses unsupported attributes", + name: "Fail: subPathPattern is specified but uses unsupported attributes", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + subPathPattern := "${.PVC.name}/${foo}" + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + DirectoryPerms: "777", + SubPathPattern: subPathPattern, + }, + } + + ctx := context.Background() + + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("Did not throw InvalidArgument error, instead threw %v", err) + } + mockCtl.Finish() + }, + }, + { + name: "Fail: resulting accessPointDirectory is too over 100 characters", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + subPathPattern := "this-directory-name-is-far-too-long-for-any-practical-purposes-and-only-serves-to-prove-a-point" + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + DirectoryPerms: "777", + SubPathPattern: subPathPattern, + }, + } + + ctx := context.Background() + + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("Did not throw InvalidArgument error, instead threw %v", err) + } + mockCtl.Finish() + }, + }, + { + name: "Fail: resulting accessPointDirectory contains over 4 subdirectories", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) - accessPointStructure := "${.PVC.name}/${foo}" + subPathPattern := "a/b/c/d/e/f" driver := &Driver{ endpoint: endpoint, @@ -1751,7 +1920,7 @@ func TestCreateVolume(t *testing.T) { ProvisioningMode: "efs-ap", FsId: fsId, DirectoryPerms: "777", - SubPathPattern: accessPointStructure, + SubPathPattern: subPathPattern, }, } From a0cffd95eac6415578cd4f75fa2c252c82093229 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Wed, 23 Feb 2022 06:21:45 +0000 Subject: [PATCH 08/13] Updating README or dynamic provisioning example --- examples/kubernetes/dynamic_provisioning/README.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/examples/kubernetes/dynamic_provisioning/README.md b/examples/kubernetes/dynamic_provisioning/README.md index 5759149a2..d6b165a07 100644 --- a/examples/kubernetes/dynamic_provisioning/README.md +++ b/examples/kubernetes/dynamic_provisioning/README.md @@ -21,6 +21,7 @@ parameters: gidRangeEnd: "2000" basePath: "/dynamic_provisioning" subPathPattern: "${.PVC.namespace}/${PVC.name}" + ensureUniqueDirectories: true ``` * provisioningMode - The type of volume to be provisioned by efs. Currently, only access point based provisioning is supported `efs-ap`. * fileSystemId - The file system under which Access Point is created. @@ -31,7 +32,11 @@ parameters: provided, access points root directory are created under the root of the file system. * subPathPattern (Optional) - A pattern that describes the subPath under which an access point should be created. So in the example given above if the PVC namespace is `foo` and the PVC name is `pvc-123-456` the access point would be - created at `/dynamic_provisioner/foo/pvc-123-456`. + created at `/dynamic_provisioner/foo/pvc-123-456-`. (The UUID being appended is due to ensureUniqueDirectories below) +* ensureUniqueDirectories (Optional) - A boolean that ensures that, if set, a UUID is appended to the final element of + any dynamically provisioned path, as in the above example. This can be turned off but this requires you as the + administrator to ensure that your storage classes are set up correctly. Otherwise, it's possible that 2 pods could + end up writing to the same directory by accident. **Please think very carefully before setting this to false!** ### Deploy the Example Create storage class, persistent volume claim (PVC) and the pod which consumes PV: From 29c21fa89160718697144a1c3fba1c32af341f90 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Sat, 25 Jun 2022 21:11:56 +0100 Subject: [PATCH 09/13] Running go mod tidy properly --- go.mod | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 3adbb533c..a10a0b54b 100644 --- a/go.mod +++ b/go.mod @@ -4,12 +4,12 @@ require ( github.com/aws/aws-sdk-go v1.40.29 github.com/container-storage-interface/spec v1.5.0 github.com/golang/mock v1.4.4 + github.com/google/uuid v1.1.2 github.com/kubernetes-csi/csi-test v1.1.1 github.com/mitchellh/go-ps v0.0.0-20170309133038-4fdf99ab2936 github.com/onsi/ginkgo v1.14.0 github.com/onsi/gomega v1.10.1 google.golang.org/grpc v1.38.0 - github.com/google/uuid v1.1.2 k8s.io/api v0.22.2 k8s.io/apimachinery v0.22.2 k8s.io/client-go v0.22.2 @@ -34,7 +34,6 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/google/go-cmp v0.5.5 // indirect github.com/google/gofuzz v1.1.0 // indirect - github.com/google/uuid v1.1.2 // indirect github.com/googleapis/gnostic v0.5.5 // indirect github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect github.com/imdario/mergo v0.3.5 // indirect From 04c55215936a3847e21f37eb2f38aa58291439f5 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Wed, 27 Apr 2022 21:28:42 +0100 Subject: [PATCH 10/13] Fixing small typo --- docs/README.md | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/docs/README.md b/docs/README.md index d2a2c718a..a59d5b370 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1,4 +1,5 @@ [![Build Status](https://travis-ci.org/kubernetes-sigs/aws-efs-csi-driver.svg?branch=master)](https://travis-ci.org/kubernetes-sigs/aws-efs-csi-driver) +[![Build Status](https://travis-ci.org/kubernetes-sigs/aws-efs-csi-driver.svg?branch=master)](https://travis-ci.org/kubernetes-sigs/aws-efs-csi-driver) [![Coverage Status](https://coveralls.io/repos/github/kubernetes-sigs/aws-efs-csi-driver/badge.svg?branch=master)](https://coveralls.io/github/kubernetes-sigs/aws-efs-csi-driver?branch=master) [![Go Report Card](https://goreportcard.com/badge/github.com/kubernetes-sigs/aws-efs-csi-driver)](https://goreportcard.com/report/github.com/kubernetes-sigs/aws-efs-csi-driver) @@ -26,19 +27,19 @@ The following CSI interfaces are implemented: * Identity Service: GetPluginInfo, GetPluginCapabilities, Probe ### Storage Class Parameters for Dynamic Provisioning -| Parameters | Values | Default | Optional | Description | -|-----------------------|--------|-----------------|----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| provisioningMode | efs-ap | | false | Type of volume provisioned by efs. Currently, Access Points are supported. | -| fileSystemId | | | false | File System under which access points are created. | -| directoryPerms | | | false | Directory permissions for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | -| uid | | | true | POSIX user Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | -| gid | | | true | POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | -| gidRangeStart | | 50000 | true | Start range of the POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. Not used if uid/gid is set. | -| gidRangeEnd | | 7000000 | true | End range of the POSIX group Id. Not used if uid/gid is set. | -| basePath | | | true | Path under which access points for dynamic provisioning is created. If this parameter is not specified, access points are created under the root directory of the file system | +| Parameters | Values | Default | Optional | Description | +|-----------------------|--------|-----------------|----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| provisioningMode | efs-ap | | false | Type of volume provisioned by efs. Currently, Access Points are supported. | +| fileSystemId | | | false | File System under which access points are created. | +| directoryPerms | | | false | Directory permissions for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | +| uid | | | true | POSIX user Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | +| gid | | | true | POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. | +| gidRangeStart | | 50000 | true | Start range of the POSIX group Id to be applied for [Access Point root directory](https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#enforce-root-directory-access-point) creation. Not used if uid/gid is set. | +| gidRangeEnd | | 7000000 | true | End range of the POSIX group Id. Not used if uid/gid is set. | +| basePath | | | true | Path under which access points for dynamic provisioning is created. If this parameter is not specified, access points are created under the root directory of the file system | | subPathPattern | | `/${.PV.name}` | true | The template used to construct the subPath under which each of the access points created under Dynamic Provisioning. Can be made up of fixed strings and limited variables, is akin to the 'subPathPattern' variable on the [nfs-subdir-external-provisioner](https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner) chart. Supports `.PVC.name`,`.PVC.namespace` and `.PV.name` | -| ensureUniqueDirectory | | true | true | **NOTE: Only set this to false if you're sure this is the behaviour you want**.
Used when dynamic provisioning is enabled, if set to true, appends the a UID to the pattern specified in `subPathPattern` to ensure that access points will not accidentally point at the same directory. | -| az | | "" | true | Used for cross-account mount. `az` under storage class parameter is optional. If specified, mount target associated with the az will be used for cross-account mount. If not specified, a random mount target will be picked for cross account mount | +| ensureUniqueDirectory | | true | true | **NOTE: Only set this to false if you're sure this is the behaviour you want**.
Used when dynamic provisioning is enabled, if set to true, appends a UUID to the pattern specified in `subPathPattern` to ensure that access points will not accidentally point at the same directory. | +| az | | "" | true | Used for cross-account mount. `az` under storage class parameter is optional. If specified, mount target associated with the az will be used for cross-account mount. If not specified, a random mount target will be picked for cross account mount | **Notes**: * Custom Posix group Id range for Access Point root directory must include both `gidRangeStart` and `gidRangeEnd` parameters. These parameters are optional only if both are omitted. If you specify one, the other becomes mandatory. * When using a custom Posix group ID range, there is a possibility for the driver to run out of available POSIX group Ids. We suggest ensuring custom group ID range is large enough or create a new storage class with a new file system to provision additional volumes. From b7c3a7f9c8c79021a10e51db400c733fe16fec78 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Wed, 1 Jun 2022 13:28:57 +0100 Subject: [PATCH 11/13] Adding errant dot --- examples/kubernetes/dynamic_provisioning/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/kubernetes/dynamic_provisioning/README.md b/examples/kubernetes/dynamic_provisioning/README.md index d6b165a07..df1fe9439 100644 --- a/examples/kubernetes/dynamic_provisioning/README.md +++ b/examples/kubernetes/dynamic_provisioning/README.md @@ -20,7 +20,7 @@ parameters: gidRangeStart: "1000" gidRangeEnd: "2000" basePath: "/dynamic_provisioning" - subPathPattern: "${.PVC.namespace}/${PVC.name}" + subPathPattern: "${.PVC.namespace}/${.PVC.name}" ensureUniqueDirectories: true ``` * provisioningMode - The type of volume to be provisioned by efs. Currently, only access point based provisioning is supported `efs-ap`. From 585678018a77c9517af1a2a432ee371e562ce959 Mon Sep 17 00:00:00 2001 From: Jonathan Rainer Date: Wed, 15 Jun 2022 07:27:28 +0100 Subject: [PATCH 12/13] Adding extra test case to cover the basePath = '/' case This has come up in several issues on the repo where people set basePath to `/` and this leads to problems. This proves this PR resolves that issue and doesn't lead to the problems of a path like `//` --- pkg/driver/controller_test.go | 75 ++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 05b55cd98..6b0ddeea4 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -4,14 +4,16 @@ import ( "context" "errors" "fmt" + "regexp" + "testing" + "github.com/google/uuid" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "regexp" - "testing" "github.com/container-storage-interface/spec/lib/go/csi" "github.com/golang/mock/gomock" + "github.com/kubernetes-sigs/aws-efs-csi-driver/pkg/cloud" "github.com/kubernetes-sigs/aws-efs-csi-driver/pkg/driver/mocks" ) @@ -829,6 +831,75 @@ func TestCreateVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "Success: Normal flow with an empty subPath Pattern, and basePath set to /", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + tags: parseTagsFromStr(""), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + SubPathPattern: "", + BasePath: "/", + EnsureUniqueDirectory: "false", + }, + } + + ctx := context.Background() + fileSystem := &cloud.FileSystem{ + FileSystemId: fsId, + } + accessPoint := &cloud.AccessPoint{ + AccessPointId: apId, + FileSystemId: fsId, + } + mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + + mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any()).Return(accessPoint, nil). + Do(func(ctx context.Context, volumeName string, accessPointOpts *cloud.AccessPointOptions) { + if accessPointOpts.DirectoryPath != "/" { + t.Fatalf("Root directory mismatch. Expected: %v, actual: %v", + "/", + accessPointOpts.DirectoryPath) + } + }) + + res, err := driver.CreateVolume(ctx, req) + + if err != nil { + t.Fatalf("CreateVolume failed: %v", err) + } + + if res.Volume == nil { + t.Fatal("Volume is nil") + } + + if res.Volume.VolumeId != volumeId { + t.Fatalf("Volume Id mismatched. Expected: %v, Actual: %v", volumeId, res.Volume.VolumeId) + } + + mockCtl.Finish() + }, + }, { name: "Fail: Volume name missing", testFunc: func(t *testing.T) { From cbf81298274226b1598c52beeb35303bd850cb0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Portela=20Afonso?= Date: Thu, 15 Sep 2022 15:31:46 +0100 Subject: [PATCH 13/13] feat: enable support for single AP per SC --- charts/aws-efs-csi-driver/values.yaml | 1 + .../specs/storageclass.yaml | 1 + .../specs/storageclass.yaml | 1 + pkg/driver/controller.go | 34 +++++++++++++------ 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/charts/aws-efs-csi-driver/values.yaml b/charts/aws-efs-csi-driver/values.yaml index 8fc7b43a0..76950565a 100644 --- a/charts/aws-efs-csi-driver/values.yaml +++ b/charts/aws-efs-csi-driver/values.yaml @@ -130,6 +130,7 @@ storageClasses: [] # mountOptions: # - tls # parameters: +# accessPointID: fsap-0024a40ac730a6cc8 # provisioningMode: efs-ap # fileSystemId: fs-1122aabb # directoryPerms: "700" diff --git a/examples/kubernetes/cross_account_mount/specs/storageclass.yaml b/examples/kubernetes/cross_account_mount/specs/storageclass.yaml index 0ec079aa7..93a5fc04d 100644 --- a/examples/kubernetes/cross_account_mount/specs/storageclass.yaml +++ b/examples/kubernetes/cross_account_mount/specs/storageclass.yaml @@ -4,6 +4,7 @@ metadata: name: efs-sc provisioner: efs.csi.aws.com parameters: + accessPointID: fsap-0024a40ac730a6cc8 # optional provisioningMode: efs-ap fileSystemId: fs-92107410 directoryPerms: "700" diff --git a/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml b/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml index 14c9a8efc..6f32a3171 100644 --- a/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml +++ b/examples/kubernetes/dynamic_provisioning/specs/storageclass.yaml @@ -4,6 +4,7 @@ metadata: name: efs-sc provisioner: efs.csi.aws.com parameters: + accessPointID: fsap-0024a40ac730a6cc8 # optional provisioningMode: efs-ap fileSystemId: fs-92107410 directoryPerms: "700" diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 9ff408871..a771cf464 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -19,13 +19,14 @@ package driver import ( "context" "fmt" - "github.com/google/uuid" "os" "path" "sort" "strconv" "strings" + "github.com/google/uuid" + "github.com/container-storage-interface/spec/lib/go/csi" "github.com/kubernetes-sigs/aws-efs-csi-driver/pkg/cloud" "google.golang.org/grpc/codes" @@ -35,6 +36,7 @@ import ( const ( AccessPointMode = "efs-ap" + AccessPointID = "accessPointId" AzName = "az" BasePath = "basePath" DefaultGidMin = 50000 @@ -93,6 +95,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } var ( + accessPointId *string azName string basePath string err error @@ -195,6 +198,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } } + // Check if using single Access Point + if value, ok := volumeParams[AccessPointID]; ok { + accessPointId = &value + } + // Assign default GID ranges if not provided if gidMin == 0 && gidMax == 0 { gidMin = DefaultGidMin @@ -284,16 +292,20 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) accessPointsOptions.Gid = int64(gid) accessPointsOptions.DirectoryPath = rootDir - accessPointId, err := localCloud.CreateAccessPoint(ctx, volName, accessPointsOptions) - if err != nil { - d.gidAllocator.releaseGid(accessPointsOptions.FileSystemId, gid) - if err == cloud.ErrAccessDenied { - return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err) - } - if err == cloud.ErrAlreadyExists { - return nil, status.Errorf(codes.AlreadyExists, "Access Point already exists") + if accessPointId == nil { + newAccessPointId, err := localCloud.CreateAccessPoint(ctx, volName, accessPointsOptions) + if err != nil { + d.gidAllocator.releaseGid(accessPointsOptions.FileSystemId, gid) + if err == cloud.ErrAccessDenied { + return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err) + } + if err == cloud.ErrAlreadyExists { + return nil, status.Errorf(codes.AlreadyExists, "Access Point already exists") + } + return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err) } - return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err) + + accessPointId = &newAccessPointId.AccessPointId } volContext := map[string]string{} @@ -311,7 +323,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return &csi.CreateVolumeResponse{ Volume: &csi.Volume{ CapacityBytes: volSize, - VolumeId: accessPointsOptions.FileSystemId + "::" + accessPointId.AccessPointId, + VolumeId: accessPointsOptions.FileSystemId + "::" + *accessPointId, VolumeContext: volContext, }, }, nil