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

Adding ability to specify uid/gid when using goofys. #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RexMorgan
Copy link

@RexMorgan RexMorgan commented Aug 29, 2019

With this change, while setting up your StorageClass, you can specify the
uid and gid that goofys should use when mounting your file system. These
two new options are goofysuid and goofysgid.

This helps with #21 however it doesn't allow specifying specific directory or file permissions for the mounted volume.

The build for this commit is passing in gitlab https://gitlab.com/rexm/csi-s3/commit/b54578fc3ea647226d9338a87e2c4f41a8faef5f

If there's a good way to write tests for this or a better way to go about it, please let me know.

With this change, while setting up your StorageClass, you can specify the
uid and gid that goofys should use when mounting your file system. These
two new options are goofysuid and goofysgid.
@ctrox
Copy link
Owner

ctrox commented Aug 31, 2019

Thanks a lot for the PR!

Looking good as far as I can see, I'm guessing attrib contains the storageclass parameters?

attrib := req.GetVolumeContext()

This was not used before and just logged so far.

As for testing, this is a bit hard because currently it just runs the CSI sanity test suite for all the different mounters. What you could do is add your new uid/gid options here:

TestVolumeParameters: map[string]string{
"mounter": "goofys",
},

Like that at least we are sure the goofys mounting command does not return any errors but to test if it actually set the correct uid/gid we would have to somehow hook a function in there after the mount, not sure if this is possible with the sanity tests.

Also thanks for going through the trouble and cloning the repo to GitLab, I have setup a webhook here but sadly it looks like GitLab currently cannot deal with PRs as they are not mirrored to that repository.

@kevinfaveri
Copy link

Any news on that PR @ctrox?

@RexMorgan
Copy link
Author

Hi, this project ended up not being suited to my use case and I ended up going another route. Because of that, I haven’t moved forward with writing the tests and probably won’t have time to anytime soon.

I apologize if people were waiting for this to get merged.

@arugal
Copy link

arugal commented May 13, 2021

Hi @ctrox @RexMorgan I want this feature, can I open a new PR to complete this feature?

@ctrox
Copy link
Owner

ctrox commented May 23, 2021

@arugal Sure, go ahead!

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

Successfully merging this pull request may close these issues.

4 participants