-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow more control of the name of the directory created by the Access Point under Dynamic Provisioning #640
Allow more control of the name of the directory created by the Access Point under Dynamic Provisioning #640
Conversation
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Right, sorry about the PR wrangling, but now we're back I can continue the conversation from #568. @iAnomaly, @wolffberg - This is where the PR continues. @wolffberg I've addressed your points about validating the created EFS path by explicitly validating it before we send it off the API and have added tests to that effect. @wongma7 - I had a long think about your comments re: security and think I've come up with a compromise. My general view is always that things should be secure by default but should allow you to turn off those controls in the knowledge that you're explicitly opting in to having to manage those guarantees yourself. So to that end I've introduced another parameter I hope this is satisfactory and if not I'm more than happy to have a discussion about it. Further I realise some of the Go may not be 100% correct as I'm fairly new to the language and some of the most common idioms still elude me so feel free to comment away but hopefully this is a much better proposition than that proposed previously. |
This is awesome @jonathanrainer, really appreciate all your time on this and all the cases you covered with tests. I went looking for the omitted Thanks again, this will be an awesome flexible feature once merged. |
/assign wongma7 |
/ok-to-test I will not have time to review for a while but in principle yes I want this merged, thanks for patience |
5d38e7a
to
07598b5
Compare
019662a
to
07598b5
Compare
cb00f9c
to
30a8ea9
Compare
I've run into a use case where these changes would be highly desirable. Thanks team! |
f355417
to
f0af951
Compare
f0af951
to
5856780
Compare
if err == nil { | ||
klog.Infof("Using user-specified structure for access point directory.") | ||
rootDirName = val | ||
if value, ok := volumeParams[EnsureUniqueDirectory]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we release this new feature and customer upgraded to adopt this feature, controller can not really find the subPathPattern or EnsureUniqueDirectory in its existing volume param.
From the logic here, it seems that will still use PV name for access point directory like before correct?
So theoretically this should not break any existing customer use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's correct, once this is released a customer has to explicitly opt into it for it to become active, by setting a subPath pattern. This was one of the key reasons for using a newly defined key so that this behaviour is an enhancement rather than replacement for what the driver already does.
ensureUniqueDirectory
is the same, setting it on its own won't do anything, only when the combination of subPathPattern
and ensureUniqueDirectory
are set does it work. If neither are set then the code behaves exactly as it was before, with respect to the behaviour of basePath etc. So yes when this releases unless a customer actively changes their StorageClass to turn this new behaviour on they won't notice any difference.
aaa3f7e
to
856fc76
Compare
Hi Jonathan @jonathanrainer, Thanks for the patience and apologize about the delay, this PR lgtm, could you give it a rebase and squash into 1-3 commits? We can merge this afterwards and you can continue to revise PR 732 thanks! |
bump! |
Hi Jonathan @jonathanrainer, this PR lgtm, could you give it a pull + rebase and squash into 1-3 commits? We can merge this afterwards and you can continue to revise PR 732 thanks! |
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.
856fc76
to
81b7d70
Compare
@Ashley-wenyizha Hi, sorry for the delay, real life stuff got a bit on top of me! But have done what you wanted now and this is ready to merge as and when you'd like :) |
This ensures we can control when we want directories created under dynamic provisioning to be unique and when we don't (poweruser mode)
81b7d70
to
7f460f7
Compare
@Ashley-wenyizha FYI I had to make a small change to the sanity tests because the EFS API Request won't accept paths more than 100 characters but the sanity attempts to use a volume name of 128 characters. In this case I think it's fine because this is the mechanism by which you'd get around exactly this problem but if you think there's a better solution then let me know |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ashley-wenyizha, iAnomaly, jonathanrainer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is fantastic. Any idea what version we can expect this feature to be available in? Thanks for all the work that went into this. |
@jonathanrainer It looks like the new properties ( When will these properties be supported in a released appVersion? Also, note the typo in the helm docs: |
This is a continuation of #568, as that PR was opened incorrectly and messing around with it was much more trouble than just opening a new PR
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 subPathPattern. 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. To stop this causing problems with security, there is additionally a new featureFlag added called
ensureUniqueDirectories
which appends a UUID to the created structure so clashes will be avoided. This is true by default but allows users to explicitly opt into an unsafe mode if they wish to provide the guarantee that there will be not be clashes themselves rather than having the driver enforce it for them.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.
fixes #543