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

Add more flexible options #23

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

colearendt
Copy link

@colearendt colearendt commented Nov 12, 2018

  • Allows arbitrary number of SHARED_DIRECTORY inputs by using and parsing the docker CMD
  • Allows arbitrary NFS options being set by using NFS_OPTS environment variable
  • Tries valiantly to remain backwards compatible by:
    • Appending SHARED_DIRECTORY to the mount inputs
    • Using old patterns / defaults for READ_ONLY and SYNC if NFS_OPTS is not set

Would love to discuss if this is workable from your perspective and how committed you are to backwards compatibility. I think if backwards compatibility is not necessary, these deprecation warnings could be more forceful or you can simply bump the version and simplify the pattern by setting default NFS_OPTS and give users free reign to set their own options by overriding the environment variable.

NOTE: I submitted the PR to the byebyeconfd branch, since those changes looked desirable and these changes are related.

Related to #20
Related to #22

@sjiveson
Copy link
Owner

sjiveson commented Nov 19, 2018

Hey Cole, thank you for this, it looks great. I wasn't aware of the ability to use SET_OPTS et al.

I'd like to both understand more about these env vars and get the removal of confd done before I consider merging this. I can't say when that will be but hopefully in a week r s.

Backwards compatibility is definitely important btw.

Also can you explain why you think this helps with #22

@colearendt
Copy link
Author

Thanks for the response! So a bit of an overview how all of this works, in case it helps:

Previously: you use options to swap out a template using sed, and create a separate template for SHARED_DIRECTORY_2
Current approach: use an environment variable (NFS_OPTS or SET_OPTS) to set all NFS options for all shares (since you can have an arbitrary number). These shares are all added in sequence to the /etc/exports file

How backwards compatibility is maintained:

  • If SHARED_DIRECTORY is set, include it in the list of shares to export
  • If NFS_OPTS is not set, use the READ_ONLY AND SYNC env vars to append the defaults into the SET_OPTS environment variable, which will be used in place of NFS_OPTS for all mounts.
  • Note that SHARED_DIRECTORY_2 is not accounted for, as I had hoped that SHARED_DIRECTORY_2 would not need backwards compatibility 😄

Specifically, #22 is related because NFS_OPTS makes it very natural to change all NFS parameters at runtime, rather than requiring a rebuild of the docker image.

What are your thoughts on:

  • naming of env vars (i.e. should SET_OPTS be LEGACY_OPTS or something?)
  • should the warning messages be more explicit to point people to using NFS_OPTS?

@colearendt
Copy link
Author

colearendt commented Nov 20, 2018

Oh, also. ${PERMITTED} is used verbatim, since it is kinda independent of NFS_OPTS. It still defines PERMITTED=* if PERMITTED is not defined, again for backwards compatibility.

# In order to run a `READ_ONLY`, `SYNC` version in the new paradigm:
docker run \
  -e "PERMITTED=1.2.3.4" \
  -e "NFS_OPTS=ro,sync,fsid=0,no_subtree_check,no_auth_nlm,insecure,no_root_squash" \
  dockerimage /var/share

# In order to solve #22 , along with READ_WRITE and ASYNC
docker run \
  -e "PERMITTED=*" \
  -e "NFS_OPTS=rw,async,fsid=0,no_subtree_check,no_auth_nlm,insecure" \
  dockerimage /var/share /var/share2

If you like this approach, I am happy to do some work updating the README.

@sjiveson
Copy link
Owner

Thanks!

@sjiveson sjiveson closed this Feb 24, 2019
@colearendt
Copy link
Author

@sjiveson Do you mind clarifying a bit about why you closed? Did you decide that this functionality is not desirable?

@sjiveson
Copy link
Owner

Ah sorry, its been that long I forgot you did this PR against the byebyeconfd branch, which I just merged into the arm branch (months after originally merging to master) and then deleted.

@sjiveson sjiveson reopened this Feb 24, 2019
@sjiveson
Copy link
Owner

Just recreated the branch and managed to reopen, phew. Hopefully I'll actually find the time to give this the attention it deserves.

@colearendt
Copy link
Author

Thanks!! No worries! I’ll take some time to pull in the latest from master too. 😄

@KenjiTakahashi
Copy link

Hey guys, what is the status on this? I could use some of these changes, too. Thanks! (Yes, I know I can build my own image, but it is a bit troublesome ;-].)

@jforest
Copy link

jforest commented Jan 3, 2023

I would also love it if this could get merged.. it's a very useful update

@saymonaraujo
Copy link

This PR is mentioned multiple times in multiple issues. Is there any intention in create a version using this ?

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.

5 participants