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

Fix docker user for development #279

Merged
merged 3 commits into from Jul 4, 2018
Merged

Conversation

jmaupetit
Copy link
Contributor

@jmaupetit jmaupetit commented Jul 3, 2018

Purpose

While running Richie locally for development, we use to mount local volumes to running containers. As containers may create files, those last may have inappropriate permissions depending on the user running in the container (e.g. root).

Proposal

To avoid permission issues we need to map local user with the running container user:

  • in the Makefile
  • by adding sugar scripts to launch docker run|exec commands

Sneak peek

screenshot from 2018-07-03 17-27-32

# This Makefile is only meant to be used for DEVELOPMENT purpose as we are
# changing the user id that will run in the container.
#
# PLEASE DO NOT USE IT FOR YOUR CI/PRODUCTION/WHATEVER...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it what we want for the CI and production as well (as opposed to running as root)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We surely do not want to (shouldn't?) run with the root user in most cases, you are right.

Remember that only the dev flavor of Richie's image runs with the root user (we use the UID=10000 in "production"), and, we use the entrypoint trick to allow running commands in Richie containers whatever the container user will be. This allows us to map local user ID with container user ID and mount local volumes without falling into file permission issues.

In the CI, we do not want to mount volumes as we want to test our newly built docker image. Hence, if we run as a non-root user, we will fall into file permission issues in the container itself (tests, coverage, etc. will try to create files in /app, a directory that belongs to the root user). One way to solve this would be to change the ownership of the /app directory from root to a lambda user. But I think it's not a good practice: I prefer having a running user that has no write permissions at all in the container.

To make a long story short, I think running tests with the root user in the CI is a lesser evil than allowing the container running user to write in the application's directory.

WDYT?

Copy link
Contributor

@sampaccoud sampaccoud Jul 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that you should run the CI in conditions as close as possible to the production.
If you run as root and your app writes inside the container to places where it won't be allowed to write in production, then your tests could miss something big...
So maybe we should make the same mounts as in production and run as an unpriviledged user to precisely check that our container works in "readonly" mode.

On more way to look at it is that this seemingly small difference is the whole reason why we have to modify images in https://www.github.com/openshift-docker. The original images from which we build in this repo would pass your CI with a root user but would fail to work in production.

This being said, maybe we can decide that what we are testing here is the code and that the images will be tested through infrastructure tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that you should run the CI in conditions as close as possible to the production.

True.

If you run as root and your app writes inside the container to places where it won't be allowed to write in production, then your tests could miss something big...

Also true. Moreover the dev flavor of the docker image that is used for tests has many additional dependencies installed; potentially changing the app behaviour.

So maybe we should make the same mounts as in production and run as an unpriviledged user to precisely check that our container works in "readonly" mode.

Good idea. But, I was talking about mounting the code as volume for tests. This is not what we want.

This being said, maybe we can decide that what we are testing here is the code and that the images will be tested through infrastructure tests.

That would be an interesting approach.

Conclusion: we need to switch to a random user also for tests (read-only mode); I hope we will be able to configure our (test|lint)-suite to fit with this requirement. I'll declare a new issue for this.

@sampaccoud
Copy link
Contributor

I think we should add some documentation to explain how to use these commands :-)
I re-discovered reading this PR that there are some commands bin/pylint, bin/test,...

@jmaupetit
Copy link
Contributor Author

Where do you think this documentation should live in: README or docs? Do you want this documentation to be part of this PR or a new one?

@sampaccoud
Copy link
Contributor

sampaccoud commented Jul 4, 2018

It should probably be here https://github.com/openfun/richie/blob/master/docs/docker_development.md.
I think you should be the person doing it and it's best to do it while it's fresh in your head 😉

To avoid permission issues during development, we enforce the docker
user ID to match the local user ID that is running docker commands. By
doing so, new files created in local volumes will belong the local user
instead of root:root.

Nota bene:

* this Makefile should only be use for development
* our user id enforcement only works for commands launched thanks to
  this Makefile
* Add sugar scripts to ease running docker run and docker exec commands
  with the appropriate user ID (matching the local user ID)
* Improve log messages with emojis 🎉
@jmaupetit jmaupetit force-pushed the fix-docker-user-for-dev branch 3 times, most recently from f204918 to bb1ff4c Compare July 4, 2018 09:10
@jmaupetit
Copy link
Contributor Author

@sampaccoud I'll wait your review on the docs to merge this work.

### Using sugar scripts

While developing using Docker, you will fall into permission issues if you mount
the code directory as a volume in the container. One way to solve this is to use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more explanation?

Indeed, the Docker engine will, by default, run the containers with your root user. Any file created or updated by the app container on your host, as a result of the volume mounts, will be owned by your root user.

Add documentation about sugar scripts usage and mention them in the
README.
@jmaupetit jmaupetit merged commit 385a82e into master Jul 4, 2018
@jmaupetit jmaupetit deleted the fix-docker-user-for-dev branch July 4, 2018 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants