Add "efs-dir" provsioning mode #1497

mpatlasov · 2024-11-08T01:03:21Z

This PR was inspired by Jonathan Rainer's PR, and Fabio Bertinatto's PR.

Is this a bug fix or adding new feature?

New feature, requested in #538 and #517 and possibly other issues, this comes up a lot as a feature people would like.

What is this PR about? / Why do we need it?

Since its creation the driver has supported Access Point Provisioning as its main means of dynamic provisioning. However this is problematic for a few reasons, it can cause issues with deleting access-points (as reported here) and it requires various AWS IAM Permissions that can be complicated to sort out and manage. Customers want a way to set their own permissions from the top level directory, but the mode doesn't allow to do that.

This PR allows a new provisioning mode called efs-dir so that instead of creating EFS access points, directories are created instead. This is achieved by creating a new Interface called Provisioner that is implemented by an AccessPointProvisioner (the original method) and a DirectoryProvisioner (the new method) this also allows in future for different kinds of provisioning to occur, maybe FileSystemProvisioning for example.

What testing is done?
New code is covered by unit-tests, they all pass when I run "go test" locally. New e2e tests are added covering create/delete operation in "efs-dir" mode. They also pass when I run them against OpenShift (OCP) cluster with new driver installed.

fixes #538

mpatlasov · 2024-11-13T00:59:07Z

/test pull-aws-efs-csi-driver-unit

seanzatzdev-amazon · 2024-12-03T21:07:13Z

Please rebase

mpatlasov · 2024-12-11T03:33:28Z

Update: last force-push rebased PR (only).

mpatlasov · 2024-12-11T21:39:29Z

Update: last force-push fixed e2e tests for "efs-dir".

mpatlasov · 2024-12-12T20:46:15Z

/test pull-aws-efs-csi-driver-unit

dankova22

@mpatlasov Thanks for the PR, we are considering supporting this feature. If could you address these comments and rebase, that would be great

dankova22 · 2025-01-07T15:04:13Z

docs/README.md

+| 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.                                                                                                                                                                 |


Can we update storage class parameters documentation that are not supported for efs-dir mode to more clearly state they are only applicable to access point provisioning?

dankova22 · 2025-01-07T15:11:25Z

pkg/driver/provisioner_dir.go

+	klog.V(5).Infof("Provisioning directory with permissions %s", perms)
+
+	provisionedDirectory := path.Join(target, provisionedPath)
+	err = d.osClient.MkDirAllWithPermsNoOwnership(provisionedDirectory, perms)


Can we add support for using static uid/gid from storage class?

I don't think dynamically selecting from a range as we do for access point provisioning makes sense here, but offering a static uid/gid option for the provisioned dir offers users an option who do not want permissive directory perms or to run their pods as root

dankova22 · 2025-01-07T15:20:10Z

pkg/driver/provisioner_dir.go

+			if value, ok := volumeParams[AzName]; ok {
+				azName = value
+			}
+			mountTarget, err := localCloud.DescribeMountTargets(ctx, fileSystemId, azName)


Could we reduce the provision function to only call DescribeMountTargets once instead of (max) 2?

Part of the advantage of using directory provisioning would be reducing API calls / chance of throttling, so we should try to limit these calls as much as possible.

We should be able to return the result from getMountOptions and reuse this for mountOptions and volContext

dankova22 · 2025-01-07T15:32:22Z

pkg/driver/provisioner_dir.go

+		} else {
+			// If it is nil then it's safe to try and delete the directory as it should now be empty
+			klog.V(5).Infof("Deleting temporary directory at '%s'", target)
+			if err := d.osClient.RemoveAll(target); err != nil {


Can we change this Remove instead of RemoveAll as extra safety precaution to ensure we are not deleting any unintended data?

This is unlikely as UUID is appended to target, but will ensure there is no accidental deletion if theres any other mounts in TempMountPathPrefix

dankova22 · 2025-01-07T15:45:48Z

Hi @mpatlasov Im interested to hear more about one of your concerns with current access point provisioning:

it can causes issues with user permissions around deleting provisioned directories

Is this referring to issues you have faced deleting access point root dirs outside of k8 or when deleting volumes with deleteAccessPointRootDir?

mpatlasov · 2025-01-08T01:33:41Z

Hi @dankova22 , thank you for reviewing this PR, highly appreciated! I've just rebased it, and not addressed your comments yet. Will do it soon.

mpatlasov · 2025-01-08T01:47:36Z

Hi @mpatlasov Im interested to hear more about one of your concerns with current access point provisioning:

it can causes issues with user permissions around deleting provisioned directories

Is this referring to issues you have faced deleting access point root dirs outside of k8 or when deleting volumes with deleteAccessPointRootDir?

This phrase in PR's description is copy/pasta from Jonathan Rainer's PR description. I thought it referred the issue #517:

What happened?
Access Points are not being deleted all PVCs in the namespace are deleted
What you expected to happen?
Access Points needs ro deleted when corresponding PVC is deleted

I have to remove "user permissions around deleting provisioned directories" from the description, sorry for that confusion.

mpatlasov · 2025-01-08T03:24:05Z

/hold

mpatlasov · 2025-01-10T00:36:21Z

/test pull-aws-efs-csi-driver-e2e

pkg/driver/provisioner_dir.go

mpatlasov · 2025-01-31T00:35:53Z

/unhold

dankova22 · 2025-02-21T22:44:56Z

Thanks again! Here is a patch for the documentation update:
update-readme-directory-provisioning.patch

Thanks Dan for [the patch](kubernetes-sigs#1497 (comment)).

mpatlasov · 2025-03-03T20:45:02Z

Hi @dankova22 , thanks for the doc patch, I've applied it now (sorry for delay - I was afk for last week).

mskanth972 · 2025-03-14T14:09:26Z

/lgtm

Use `buildDriver()` instead of `&Driver{...}`

Controller's CreateVolume and DeleteVolume now call method of Provisioner interface. All heavy-lifting logic of create/delete specific for access-point provsioning is hidden now in AccessPointProvisioner struct.

Implement `DirectoryProvisioner` struct and its `Provision/Delete` methods. Add `delete-provisioned-dir` command-line option to control DeleteVolume behavior in efs-dir mode.

The patch adds new unit-tests for `provisioner_ap.go` and `provisioner_dir.go`, and also adds a few unit-tests for `controller.go`.

…ioning

…fs-ap" mode.

Two issues fixed: * If Unmount() failed, return error message with unmountErr * If Delete() failed before defer func, and defer func failed, return error message with both errors

Thanks Dan for [the patch](kubernetes-sigs#1497 (comment)).

k8s-ci-robot · 2025-04-04T22:30:53Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2025-04-04T22:30:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mpatlasov
Once this PR has been reviewed and has the lgtm label, please ask for approval from mskanth972. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mpatlasov · 2025-04-04T22:31:29Z

Update: last force-push rebased PR (only).

k8s-ci-robot · 2025-04-04T22:36:57Z

@mpatlasov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-aws-efs-csi-driver-external-test-eks	`36cd25a`	link	true	`/test pull-aws-efs-csi-driver-external-test-eks`
pull-aws-efs-csi-driver-e2e	`36cd25a`	link	true	`/test pull-aws-efs-csi-driver-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

mpatlasov · 2025-04-04T22:43:07Z

pull-aws-efs-csi-driver-external-test-eks and pull-aws-efs-csi-driver-e2e are perma-failing:

ERROR: failed to solve: process "/bin/sh -c clean_install amazon-efs-utils true &&     clean_install crypto-policies true &&     install_binary         /usr/bin/cat         /usr/bin/cd         /usr/bin/df         /usr/bin/env         /usr/bin/find         /usr/bin/grep         /usr/bin/ln         /usr/bin/ls         /usr/bin/mount         /usr/bin/umount         /sbin/mount.nfs4         /usr/bin/openssl         /usr/bin/sed         /usr/bin/stat         /usr/bin/stunnel         /usr/sbin/tcpdump         /usr/bin/which &&     cleanup \"efs-csi\"" did not complete successfully: exit code: 1
make[1]: *** [Makefile:86: .image-30814-linux-amd64-amazon] Error 1
make[1]: Leaving directory '/home/prow/go/src/github.com/kubernetes-sigs/aws-efs-csi-driver'
make: *** [Makefile:119: test-e2e] Error 2

This is not relevant to PR.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 8, 2024

k8s-ci-robot requested review from d-nishi and leakingtapan November 8, 2024 01:03

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 8, 2024

rubroboletus mentioned this pull request Nov 8, 2024

in helm chart add proxy settings as secret #1499

Open

mpatlasov force-pushed the aws-efs-dir-mode branch from 8002947 to 8e6f341 Compare November 12, 2024 22:50

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2024

mpatlasov force-pushed the aws-efs-dir-mode branch from 8e6f341 to 025940c Compare December 11, 2024 03:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2024

mpatlasov force-pushed the aws-efs-dir-mode branch from 025940c to 175644b Compare December 11, 2024 21:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2024

dankova22 reviewed Jan 7, 2025

View reviewed changes

mpatlasov force-pushed the aws-efs-dir-mode branch from 175644b to ad2a406 Compare January 8, 2025 01:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2025

mpatlasov force-pushed the aws-efs-dir-mode branch from 9cf3968 to ad2a406 Compare January 9, 2025 00:02

mpatlasov force-pushed the aws-efs-dir-mode branch from 040821c to 1f9e756 Compare January 29, 2025 03:06

github-advanced-security bot found potential problems Jan 29, 2025

View reviewed changes

pkg/driver/provisioner_dir.go Fixed Show fixed Hide fixed

pkg/driver/provisioner_dir.go Fixed Show fixed Hide fixed

mpatlasov force-pushed the aws-efs-dir-mode branch from 1f9e756 to ff3ba10 Compare January 30, 2025 21:54

mpatlasov added a commit to mpatlasov/aws-efs-csi-driver that referenced this pull request Mar 3, 2025

Add new section (Directory Provisioning Mode) to README

c02affe

Thanks Dan for [the patch](kubernetes-sigs#1497 (comment)).

k8s-ci-robot assigned mskanth972 Mar 14, 2025

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 14, 2025

mpatlasov added 15 commits April 2, 2025 15:08

Simplify controller_test.go

9734322

Use `buildDriver()` instead of `&Driver{...}`

Introduce Provisioner interface

6d8505f

Controller's CreateVolume and DeleteVolume now call method of Provisioner interface. All heavy-lifting logic of create/delete specific for access-point provsioning is hidden now in AccessPointProvisioner struct.

Add directory provisioning mode

096cd0e

Implement `DirectoryProvisioner` struct and its `Provision/Delete` methods. Add `delete-provisioned-dir` command-line option to control DeleteVolume behavior in efs-dir mode.

Add unit-tests for directory and access-point provisioning modes

833b1a8

The patch adds new unit-tests for `provisioner_ap.go` and `provisioner_dir.go`, and also adds a few unit-tests for `controller.go`.

Update charts, examples and documentation

2f1bbb5

Add e2e tests for "efs-dir" provisioning mode.

5515641

Avoid redundant call DescribeMountTargets in dir-mode Provision()

9bd7a7d

Add support for static uid/gid from storage class for dir-mode provis…

bd07ecd

…ioning

In dir-mode Delete(), remove temporary directory only if empty

06a35d3

Update README to highlight storage class parameters usable only in "e…

0043707

…fs-ap" mode.

In dir-mode Provision(), remove temporary directory only if empty

b1e0732

Improve logging in defer func of Delete() for "efs-dir" mode

e672642

Two issues fixed: * If Unmount() failed, return error message with unmountErr * If Delete() failed before defer func, and defer func failed, return error message with both errors

Add e2e test writing to the fs in "dir-mode" provsioning case.

145b083

Add new section (Directory Provisioning Mode) to README

3cf6597

Thanks Dan for [the patch](kubernetes-sigs#1497 (comment)).

Remove deprecated framework.ExpectEqual from e2e.go

36cd25a

mpatlasov force-pushed the aws-efs-dir-mode branch from c4cea00 to 36cd25a Compare April 4, 2025 22:30

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2025

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2025

Add "efs-dir" provsioning mode #1497

Are you sure you want to change the base?

Add "efs-dir" provsioning mode #1497

Uh oh!

Conversation

mpatlasov commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpatlasov commented Nov 13, 2024

Uh oh!

seanzatzdev-amazon commented Dec 3, 2024

Uh oh!

mpatlasov commented Dec 11, 2024

Uh oh!

mpatlasov commented Dec 11, 2024

Uh oh!

mpatlasov commented Dec 12, 2024

Uh oh!

dankova22 left a comment

Choose a reason for hiding this comment

Uh oh!

dankova22 Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

dankova22 Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

dankova22 Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

dankova22 Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

dankova22 commented Jan 7, 2025

Uh oh!

mpatlasov commented Jan 8, 2025

Uh oh!

mpatlasov commented Jan 8, 2025

Uh oh!

mpatlasov commented Jan 8, 2025

Uh oh!

mpatlasov commented Jan 10, 2025

Uh oh!

Uh oh!

Uh oh!

mpatlasov commented Jan 31, 2025

Uh oh!

dankova22 commented Feb 21, 2025

Uh oh!

mpatlasov commented Mar 3, 2025

Uh oh!

mskanth972 commented Mar 14, 2025

Uh oh!

k8s-ci-robot commented Apr 4, 2025

Uh oh!

k8s-ci-robot commented Apr 4, 2025

Uh oh!

mpatlasov commented Apr 4, 2025

Uh oh!

k8s-ci-robot commented Apr 4, 2025

Uh oh!

mpatlasov commented Apr 4, 2025

Uh oh!

Uh oh!

mpatlasov commented Nov 8, 2024 •

edited

Loading