Skip to content
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

NFS export enhancements (and API in general) #59

Open
dsonck92 opened this issue Aug 3, 2020 · 2 comments
Open

NFS export enhancements (and API in general) #59

dsonck92 opened this issue Aug 3, 2020 · 2 comments

Comments

@dsonck92
Copy link
Contributor

dsonck92 commented Aug 3, 2020

As the latest pull requests have been merged in (#55 #56 #58) NFS got a lot more usable. As I'm working on a kubernetes NFS provisioner (to be precise, a targetd provisioner with iSCSI and NFS support) it's pretty much feature complete. However, there are some issues that might need some thought or suggestions:

  • When an NFS share is created, it's suggested to export the filesystem (in essence subvolume) directly as per man exports. However, as targetd created filesystem volumes are owned by root after creation, this creates permission issues on nfs clients. These can be manually resolved with chown, but it might be useful to allow NFS to specify the (numeric) user to apply before exporting. (Add chown option to nfs_export_add #60)
  • Currently the NFS shares are by path, and as such targetd is essentially a thin remote NFS administration API. It might be interesting to look into exporting volumes specifically, much like iSCSI already does. This could be in addition to the regular API to facilitate exporting arbitrary NFS paths in case the user doesn't care for pools and wants to export random folders.
  • As a security note, there are currently no limitations on NFS paths. Granted, if you give some client access to targetd, you essentially give it root and is stated in the README.md, however, it might be useful to put some limitation options for NFS. For instance "only allow from pools" or "only allow below this path". This is just a train of thought that may or may not be out of scope though.

Some general usability points I came across while working with the FS api:

  • When creating an fs, there is no feedback other than "failed". Meanwhile, the API assumes UUIDs for most of its interaction. It might be interesting to consider returning information about the creation like uuid. That way, clients don't need to call fs_list just to try and map the "just now created" filesystem to the uuid to use for subsequent interactions.
    I think this can be extended in general to any api call. JSONRPC allows multiple parameters and multiple results, yet, only a few actually return data where there is useful data to be given.
@tasleson
Copy link
Member

tasleson commented Aug 7, 2020

I think these are all reasonable improvement suggestions. As long as we maintain API compatibility we can certainly introduce new functionality. The last point was very apparent when writing the unit tests and I see no reason we couldn't just return all the data about the newly created item. The existing API would probably get sluggish in large configurations. It's good to avoid round tripping in the design. Another option to consider:

  • Allow query operations to have a filtering syntax to allow the service to do the filtering, to reduce the amount of data returned in requests.

Pull requests welcome

@dsonck92
Copy link
Contributor Author

dsonck92 commented Aug 9, 2020

Alright, I will pick up some issues then and create some pull requests. In particular the export owner and the feedback part are something I run against as I literally chowned things before the remote system would properly start

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants