Allow more control of the name of the directory created by the Access Point under Dynamic Provisioning

[jonathanrainer]

Is this a bug fix or adding new feature?
This adds a new feature, mostly in response to discussions on Issue #543

What is this PR about? / Why do we need it?
At the moment if you use dynamic provisioning the name of the folder created on the EFS is the name of the generated PV however in some use cases this isn't desirable and leads to a lot of confusion. Under this scheme the name of the directory can be controlled by users using a new parameter rootDirectoryPath and deprecating the old basePath. This allows users a higher level of control over the directories that are created as they can specify any combination of fixed strings and a set of predefined variables based on what the CSI Spec supports.

What testing is done?
I have added to the unit tests for the module and I believe this is sufficient because all that is being changed is the directory being added to the AccessPointOpts struct. There might want to be some E2E testing about what happens if that directory already exists but I'm happy to take pointers on that as this is my first PR to the repo.

[k8s-ci-robot]

Welcome @jonathanrainer!

It looks like this is your first PR to kubernetes-sigs/aws-efs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-efs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @jonathanrainer. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

[wongma7]

/ok-to-test

will have to incorporate namespace in the naming somehow to avoid conflicts

[jonathanrainer]

Have pushed a new version that instead of enforcing the use of the PVC Name gives the users the ability to decide how they want to structure the access point directory. It's a bit more generic and has more potential for error admittedly but I think it allows more control. If you think this isn't a good idea we can revert and I can add the Namespace in somewhere else but I think this strikes a nice balance.

[jonathanrainer]

/retest

[jonathanrainer]

/retest

[iAnomaly]

Appreciate the work here @jonathanrainer! I have some opinions if you'd care for them:

1. This one might be controversial since it breaks existing behavior, but personally I think existing basePath should be repurposed instead of adding yet another parameter. The reasoning here is that baseBath could be expanded to accept arbitrary string path including some predefined "variables" similar you've suggested. Maybe something like $pv.name and $pvc.namespace.
2. Then the default for basePath should become pv.name

This accomplishes what #543 asked for but is even more generic such that users could also set basePath: "" if they wanted the root to be used (which ironically is what the currently incorrect documentation says): 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

Again, to be clear this changes the current behavior of basePath so likely this would require another SemVer major bump, but I think it supports much more flexibility including the ability to just use the root path "/" without pvc.name appended at all. What do you think about this part @wongma7?

In summary,

1. Would be better IMO if you could repurpose basePath instead of adding a new parameter.
2. Would be better IMO if you could support arbitrary string with your desired "variables" supported similar to interpolation instead of just taking a comma delineated list of "variables" like "pvcName" and "pvcNamespace".

Thoughts?

[jonathanrainer]

I think all that sounds reasonable I can have a go at something like that tomorrow and see what I come up with. The only problem I can forsee is how you communicate the variables vs. the fixed part of the string, I guess you could use Go's templating syntax but if you're using Helm to install this it would be a bit odd getting it to generate more templating code but perhaps that's unavoidable.

I'm more than open to the idea but I'd appreciate some pointers/ideas on the syntax.

[jonathanrainer]

Also thinking about it maybe the best idea is to remove "basePath" if and swap it for something like "pathTemplate"? I think that's much clearer as to what that parameter does, that way we can support both and then deprecate basePath if we want to or go the whole hog and deprecate it in one fell swoop if people want to take the major version bump.

[iAnomaly]

I think all that sounds reasonable I can have a go at something like that tomorrow and see what I come up with. The only problem I can forsee is how you communicate the variables vs. the fixed part of the string, I guess you could use Go's templating syntax but if you're using Helm to install this it would be a bit odd getting it to generate more templating code but perhaps that's unavoidable.

I'm more than open to the idea but I'd appreciate some pointers/ideas on the syntax.

Ya, I'm still thinking about this part for sure. Go templating feels too heavy here and conflicts with Helm seem guaranteed. Something like https://pkg.go.dev/fmt feels much closer but still thinking.

Also thinking about it maybe the best idea is to remove "basePath" if and swap it for something like "pathTemplate"? I think that's much clearer as to what that parameter does, that way we can support both and then deprecate basePath if we want to or go the whole hog and deprecate it in one fell swoop if people want to take the major version bump.

"Template" doesn't feel right to me, but totally understand if repurposing basePath is too controversial/backwards incompatible. I'd vote for either rootDirectoryPath since that's what the AWS EFS Console calls it or even just path would be clear in this context, again defaulting to /%pv.name like the undocumented behavior of subPath behaves today.

[jonathanrainer]

@iAnomaly Had a first pass at it, the templating syntax I ended up at was /{pv.name} and so on. It doesn't feel 100% there yet to me and I'm very open to suggestions but at least this gives something concrete to talk about.

[iAnomaly]

@iAnomaly Had a first pass at it, the templating syntax I ended up at was /{pv.name} and so on. It doesn't feel 100% there yet to me and I'm very open to suggestions but at least this gives something concrete to talk about.

Looking really good IMO @jonathanrainer! Now that I think of it, since we're creating a whole new parameter rootDirectoryPath maybe we should take the opportunity to default to / instead of /{pv.name} since that's the AWS EFS Console experience? What do you think?

[jonathanrainer]

I think I was going for parity with existing behaviour so people can migrate more easily. It's easy to change though and I'm happy to.

@wongma7 what do you think? This is a pretty big change for the parameters, don't want to push too far down this route if you think it's a bad idea

[wolffberg]

If we want backwards compatibility we could keep basePath as is and join another path from a new parameter like subPath, subPathTemplate or similar.
This would also make it easier to avoid name clashes if you for some reason want to use the EFS share for other things than your PVCs as we still can't get the cluster name from the K8s API.

[wongma7]

I like wolffberg's idea, it is easy to reason about.

My highest priority is backwards compatibility, I think it is too soon for a major version update and an upgrade story that entails recreating storageclasses is hard to reason about and could be troublesome.

Another option is to maintain both basePath and rootDirectoryPath and make them mutually exclusive. I don't like this because it is confusing and I want to avoid doing validation of storageclass in general (because validation cannot occur at storageclass creation time, it can only happen later at pv provision time, it is unpleasant UX). But I mention it for completeness

Also, I slightly prefer reusing go templating syntax over coming up with something new. helm should let us escape expressions. Another option is to copy https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner pathPattern parameter templating syntax. I don't know how they arrived at that syntax. But there's probably a nonzero overlap of users who would benefit from consistency since https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner does technically work with EFS too.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jonathan Rainer (bab36f5, 7a0f7a3, 6cf828e, d282382)

[jonathanrainer]

/check-cla

[jonathanrainer]

Ok, sorry about the mess with this one, I forgot that fiddling with master was probably going to affect the PR but anyway.

I've re-written this to keep basePath around in line with @wolffberg's suggestion and have incorporated the syntax from nfs-subdir-external-provisioner as a starting point. Their version is more expansive because you're allowed to use things like annotations and arbitrary labels and if we wanted to add that in the future we could but it looked quite complicated and I think it would be scope creep for what we're doing here.

Let me know what you think and we can work from there.

[iAnomaly]

Thanks for continuing to work through this @jonathanrainer. My only question is how does the current implementation support a desired constructed path of just /? Reading through the code it seems like the way to achieve that would be with:

basePath: nil (omit this StorageClass parameter)
subPathPattern: ""

But it looks like subPathPattern doesn't currently support a length == 0 case? https://github.com/kubernetes-sigs/aws-efs-csi-driver/pull/568/files#diff-4e4f8ad2f057d794bd5842ed50e358451e6e1b3b20e5f2836289665bdfbf5625R252

[jonathanrainer]

@iAnomaly Thanks for the feedback! You're right that is certainly a use-case I overlooked, I've now updated the code so it's possible to have a 0-length pattern and added a test to show that it gives you the correct constructed directory (i.e. /) once it's been through the process.

[wongma7]

since it's possible now for names to collide whereas before we could assume that the path would be unique since volName would be the pvc+uuid (and volName is a PV name which is cluster-scoped)*, I'm wondering whether we need to handle it. Especially if deleteAccessPointRootDir is set to false then we could have a situation where say I have iteration 1 of pod+pvc-a writing data to directory pvc-a and then iteration 2 of pod+pvc-a (not necessarily in the same namespace as iteration 1) unexpectedly reading/writing to the same directory pvc-a.

It puts quite a burden on administrators to set up the storageclasses 'correctly' so that boundaries are respected. If I just lazily create a storageclass with subpath /{PVC.name} then users from namespace A or B could both access that which is probably not what I intend.

Just want to start a discussion for now, I anticipate we might get blocked releasing this on security grounds in its current form.

[jonathanrainer]

That's reasonable, and I'm certainly not an expert in this stuff which is why I rely on other people who are! In which case, since the EBS driver already does this do they not have the same problem? And what do you think is the best way to deal with a situation like this?

• Can we query the EFS to check if any access points overlap? How similar do we need to check that they are? i.e. It might be completely legitimate to have 2 access points pointing at the same directory if they had different GUID/UID properties for example
• Were this to be violated what's an acceptable response, should the driver just return an error, as it does if you misconfigure the subPathPattern?

[wolffberg]

I think the path should still contain the UUID to prevent collisions e.g. /cluster/namespace/pvc-1d8451fe-ee96-421a-b55f-c90a346e1215

Also, I just found this paragraph on the EFS docs that might require a check or two.

You enable this feature by setting the access point Path attribute when creating an access point. The Path attribute is the full path of the root directory of the file system for all file system requests made through this access point. The full path can't exceed 100 characters in length. It can include up to four subdirectories.

[k8s-ci-robot]

@jonathanrainer: 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-verify	317eb22	link	true	/test pull-aws-efs-csi-driver-verify
pull-aws-efs-csi-driver-unit	317eb22	link	true	/test pull-aws-efs-csi-driver-unit

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.

Details

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/test-infra repository. I understand the commands that are listed here.

[jonathanrainer]

This PR being linked to master is making it difficult to manage. I'm going to close this PR now and re-open it properly so re-bases etc. will not cause me to spend in-ordinate amounts of time fixing it.