-
Notifications
You must be signed in to change notification settings - Fork 137
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 running as arbitrary uid #151
Conversation
ac99989
to
9a3af1d
Compare
Hey @willholley , will review this later today. Looks like |
@kocolosk you've been doing the reviews in this space, care to have a look? |
Will do |
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.
I think the changes in the docker-entrypoint
make sense -- we're basically putting the onus on the user to have all permissions configured correctly if they want to run under a custom UID.
I do worry that skipping the chown in the Dockerfile
will have an impact on startup times per #131. Is there a problem with keeping that line in there?
hopefully this is fixed in #152 |
* Adds guards around entrypoints commands that require root * Broaden permissions within the container filesystem to allow access by non-couchdb users. * Added an example to the documentation which specifies `--user`.
@willholley I'd like to merge this but @kocolosk 's comment is still unaddressed. We need to not reintroduce a regression here. Can you help? Thanks. |
@willholley merge at your convenience! I'll need to update the official docs upstream with your change, too, once this lands. |
2.3.1/Dockerfile
Outdated
find /opt/couchdb/etc -type d ! -perm 0755 -exec chmod -f 0755 '{}' +; \ | ||
find /opt/couchdb/etc -type f ! -perm 0644 -exec chmod -f 0644 '{}' +; \ | ||
# only local.d needs to be writable for the docker_entrypoint.sh | ||
chmod -f 0777 /opt/couchdb/etc/local.d |
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.
I would recommend moving this whole block up into the RUN
line that creates /opt/couchdb
in the first place while you're at it/here (there are some edge cases around chmod
/chown
in a separate layer on some graph drivers).
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.
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.
see his latest comment, looks like another change is req'd
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.
ok - suggested change has been pushed
There are some edge cases around chmod/chown in a separate layer on some graph drivers, so it's safer to keep this all in a single `RUN` block. Refs #151
There are some edge cases around chmod/chown in a separate layer on some graph drivers, so it's safer to keep this all in a single `RUN` block. Refs #151
There are some edge cases around chmod/chown in a separate layer on some graph drivers, so it's safer to keep this all in a single `RUN` block. Refs #151
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.
LGTM
Overview
Enables the CouchDB container to run as a non-root / user-specified uid. For now, I've only modified the 2.3.1 container but if the approach is accepted we can easily backport it.
Specifically, this:
--user
.Testing recommendations
Build the container locally:
Run the container as a custom user
or, run the container as a custom user with a mounted data volume:
Verify Couch is working:
GitHub issue number
Fixes #147
Related Pull Requests
Checklist