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

Remove chown/chmod from entrypoint #109

Closed
wohali opened this issue Oct 11, 2018 · 3 comments
Closed

Remove chown/chmod from entrypoint #109

wohali opened this issue Oct 11, 2018 · 3 comments

Comments

@wohali
Copy link
Member

wohali commented Oct 11, 2018

As mentioned here

I experimented a bit more with this and I suspect we're carrying around some dead weight. The chown commands don't do anything on a modern Docker engine as far as I can tell - everything is already owned by user couchdb. The chmod commands do have an effect but honestly I'm wondering if these are even necessary:

Switch /opt/couchdb/data from 755 to 770
Switch the .ini files from 644 to 664
Switch the config directories from 755 to 775

I don't know what standard we used to come up with these settings but e.g. the docker.ini file that we create in docker-entrypoint.sh has 644 permissions so we're not even internally consistent.

I don't have a proper test matrix of Docker engine versions etc. to see if there was a point in time at which these settings were needed. I've also not tried Windows or anything else exotic.

@kocolosk here's my thinking on the above:

The chown commands don't do anything on a modern Docker engine as far as I can tell

I believe we had this in there because, if you are mounting an external volume, you might have all the files owned by the wrong user and CouchDB simply wouldn't operate. This is frightfully common.

I suspect if we remove this, we'll start getting bugs about how CouchDB can't read/write its data.

We could have the entryfile (or CouchDB!) complain if it can't read/write to that folder, would that be sufficient?

Switch /opt/couchdb/data from 755 to 770

The point of this I believe was to ensure that externally mounted volumes were correctly not allowing access to the data directory from anyone other than the couchdb user and group. Similar problem to the issue above.

Switch the .ini files from 644 to 664
Switch the config directories from 755 to 775

These were probably intended to ensure that the group could write to those files as well, but I suspect this can go. Why these are xx4 and xx5 instead of xx0, being inconsistent with /opt/couchdb/data, I don't know.

/cc @kocolosk @denyeart

@kocolosk
Copy link
Member

Ah, thanks @wohali as usual you are a few steps ahead of me on this topic.

I noticed that @yosifkit recently went through a bunch of official images and started doing a conditional chown in the entrypoint of only the files that were not already owned by the appropriate user. See e.g. redis/docker-library-redis#166. I think the original idea came from docker-library/postgres#143 where it was credited with a big savings in startup time. Seems like we could at least adopt this change without any regrets ...

@kocolosk
Copy link
Member

I also think we should probably set o= in the chmod for the data directory rather than specifying an explicit mode. No sense in making all the .couch and .view files executable.

@kocolosk
Copy link
Member

Another issue with the data directory is that the files created by CouchDB itself are not following the permissions indicated here in the entrypoint. CouchDB is creating files with 644 and directories with 755.

I guess one could with setfacl on the data directory to set these defaults correctly, but I'm not sure if ACLs are even going to be broadly supported by volume providers. I'd be fine to keep things simple and just chmod to 644/755 instead of 660/770, but I don't know what the real risk is here of allowing other users read access to the data files.

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