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 #3087] Permission denied in building with docker on local VM #3152

Closed
wants to merge 1 commit into from

Conversation

safwanrahman
Copy link
Member

This is a common problem of Docker container. The build in docker container is running with root permission and creating the file as root. So the local user does not have permission to access the directory.
passing user while creating the container should fix the issue. So in the container, all the command will run as the uid of host user, and the host user will have the permission to access the directory.
@agjohnson r?

@ericholscher
Copy link
Member

This looks great. Docker is hard to test, but I do worry about changing something specific like this, so I'll need to test it locally before merging it.

@ericholscher
Copy link
Member

I'm curious if this fixes #2692 -- @humitos can you test it if possible?

@agjohnson
Copy link
Contributor

I would assume this approach wouldn't work for most cases. The host UID probably doesn't exist in the build container, and more than likely isn't the docs user uid. @humitos hit this problem running docker locally and his fix was to create a new dockerfile that extends our base dockerfile, and changes the docs user UID (and re-chown /home/docs) to his host UID. We should probably have a pattern for this for the docker image usage, or should have information on setting up volumes -- not sure which approach works better.

@safwanrahman
Copy link
Member Author

safwanrahman commented Oct 23, 2017

@agjohnson I think, this fixed for most of the cases. Most of the time, the host userid is 1000 and its exist in docker container.
The docs userid should not work because the file need to be created by the host userid permission. So the host user can delete it after build.
I will test with different UID and let you know the result

@safwanrahman
Copy link
Member Author

I have tested with a random UID and the container starts with the random UID though it does not exist in host machine.

@agjohnson
Copy link
Contributor

agjohnson commented Oct 23, 2017

Does a build happen successfully with a random UID? I can create the container, but I have no permissions:

% docker run --rm -t -u 2600 -i readthedocs/build:2.0 /bin/bash
I have no name!@72f1b223f80a:/home/docs$ pwd
/home/docs
I have no name!@72f1b223f80a:/home/docs$ id docs
uid=1005(docs) gid=205(docs) groups=205(docs)
I have no name!@72f1b223f80a:/home/docs$ mkdir foo
mkdir: cannot create directory ‘foo’: Permission denied
I have no name!@72f1b223f80a:/home/docs$ exit

@safwanrahman
Copy link
Member Author

In the build, we mount the host environment there. As the host uid and container uid are same, its possible to create a anything.
You can try
docker run --rm -t -u $UID -i readthedocs/build:2.0 /bin/bash and try to create a directory while you mount your local directory.

@safwanrahman
Copy link
Member Author

Does a build happen successfully with a random UID?

The build will not successfull with random UID, but if its host UID, it must be successful as I have tested.

@agjohnson
Copy link
Contributor

agjohnson commented Oct 23, 2017

I get problems with permissions with a mounted volume as well -- I'm on OSX and running docker in a vm, with shares of my home path to the VM, which is likely the problem here.

For example:
https://gist.github.com/agjohnson/11abb3e411bd6c3299845757261f408c

@safwanrahman
Copy link
Member Author

@agjohnson Before applying the patch, it was working? Can you give me a steps to reproduce? and what error you are getting? I have tested in Ubunutu and it works fine there.

@safwanrahman
Copy link
Member Author

safwanrahman commented Oct 23, 2017

So I have tried in my Macbook pro running OS X with the ubuntu:latest image, but there is no permission error in creating directory.
I have run following

safwans-MacBook-Pro:kuma safwan$ docker run --rm -t -u $UID -v ~/test:/test -i ubuntu /bin/bash
I have no name!@66c1d3e58a39:/$ cd test
I have no name!@66c1d3e58a39:/test$ ls
kubernetes-django  mozilla-django-oidc  origin
I have no name!@66c1d3e58a39:/test$ mkdir temp
I have no name!@66c1d3e58a39:/test$ ls
kubernetes-django  mozilla-django-oidc  origin  temp
I have no name!@66c1d3e58a39:/test$ exit

@agjohnson
Copy link
Contributor

My builds work fine without this patch, but because Vagrant remaps the permissions, and files are shared with the vm as 0777 permissions. There are a lot of places that we assume you are running as the docs user, not a non-existant user ID, beyond just file permissions.

I think the most correct fix is similar to @humitos fix at:
https://github.com/rtfd/readthedocs.org/pull/2692/files#diff-bbaadadacee305c8e9d6825771319cfbR56

I think the proper fix is using a custom Dockerfile to change the docs UID. To do this, the image would extend in the same manner, and the image would:

  • Get the host UID
  • Change to root users
  • Swap the docs UID for the host UID
  • Ensure new uid is in the same groups
  • chown /home/docs with UID
  • Change back to the docs user

@agjohnson
Copy link
Contributor

agjohnson commented Oct 23, 2017

We have a hardcoded user in the image though, so testing ubuntu:latest probably won't raise the same problem. The problems you'll see on linux aren't going to be permissions -- your fix gets around permissions by avoiding the docs user. We do want to execute commands as the docs user in the image though.

@safwanrahman
Copy link
Member Author

I am not actually comfortable to have device specific docker image. The image should be universal and the contributor should not build image in his host machine.

There are a lot of places that we assume you are running as the docs user

I strongly believe, this should be fixed first. The docker image should be platform independent. Can you speify where we are assuming so I can look over them and fix them.

@safwanrahman
Copy link
Member Author

We do want to execute commands as the docs user in the image though.

Whats the reason behind it?

@agjohnson
Copy link
Contributor

Agreed, I'd also prefer that this isn't the case, but an extension image is likely the quickest/easiest option. Realistically, we don't have a lot of bandwidth to help with overhauling our build image for local development. It should only take 1 extra steps for development setup with a custom extension image, setting the configuration to use the local extension image.

We require running as a docs user because the build commands shouldn't be able to use a root/privileged account. On the docker image, we install all versions of python and conda local for the user, so that the user has write to these paths. Without a lot of QA, i'm not sure if the user requires write access to the local python versions, my guess is no, but there could be issues from the wrong ownership here. There are also other places in production provisioning and our commercial hosting we assume the user's home is /home/docs.

Because of the complexity here, I lean towards the easy solution of a custom extension dockerfile. If you wanted to alter our base dockerfile, we'd need a good amount of QA across projects -- test conda projects, multiple python versions, and project options around pip/conda. We had talked a while back about some form of development docker image, but efforts there stalled and seem to be superseded by an extension image for local development

@safwanrahman
Copy link
Member Author

@agjohnson I understand your point. I will investigate more to find a way to fix it up by using the docs user. But I am not comfortable to have additional image for each user. Lest see what I can find.

@safwanrahman
Copy link
Member Author

safwanrahman commented Oct 24, 2017

@agjohnson So I have found a solution about changing the UID of docs user in the container. So, our python code will create a container, then create and run a exec instance for changing the UID of the docs user to match with host user. Then do the rest of the things.

But for this, we need to upgrade our docker-py package to at least 1.4.0 because passing user argument was introduced with this version.

Can I know the Docker and Docker API version installed in the production?

I have working on a patch, can you check @agjohnson and let me know if you have any concern about the way?

@agjohnson
Copy link
Contributor

After testing this a little I'm on the fence whether this is a good approach or not. The operation in your example is exactly what we need to, however the operation takes a significant amount of time. I'm testing local setup on a box now and it's been running for a while -- too long to be acceptable for each build. Because of this, we likely don't want to do this in production either. It should be a noop, as the UID isn't changing in production, but better to be explicit here.

I'm still leaning towards a custom image, compilation of the image would only happen once.

This is the image I'm building now:

FROM readthedocs/build:2.0

USER root
RUN usermod -u 1000 docs
USER docs

CMD ["/bin/bash"]

This would need to be configurable for users though. A new dev set up might be:

git clone https://github.com/rtfd/readthedocs.org
...
docker pull readthedocs/build:2.0
...
cd readthedocs.org/docker
docker build -t readthedocs/build:dev --build-arg UID=1000

We can likely automate the custom configuration step by assuming the image name is readthedocs/build:dev

@safwanrahman
Copy link
Member Author

@agjohnson I have also inspected it and found that its also taking too much time. I am going to investigate more to find why its taking too much time.

@humitos
Copy link
Member

humitos commented Nov 10, 2017

I'm happy that this problem came up again and there is another person trying to solve it :)

I think this issue is kind of complicated in terms of being sure that any change will work in all the situations that we need to work on, but I think it worth to make the effort if we want to open the door to new contributors. Probably, it will need contributors for testing and QA in these different cases, also.

I have been thinking on this last week and I think the idea that @safwanrahman is proposing is the way to go. I mean, having a generic docker image that works on production and on development without making any changes.

There are also other places in production provisioning and our commercial hosting we assume the user's home is /home/docs.

@agjohnson why do we need permissions under the /home/docs? Aren't all the things downloaded and created under user_builds, user_uploads, etc from the SITE_ROOT? https://github.com/rtfd/readthedocs.org/blob/f093076d75e5d5cf7239c66f416aa87273e2ed4d/readthedocs/settings/base.py#L170-L190

There are a lot of places that we assume you are running as the docs user, not a non-existant user ID, beyond just file permissions.

@agjohnson can you point me one of these to understand it better?

This post explains exactly the problem that we are having here: https://medium.com/@mccode/understanding-how-uid-and-gid-work-in-docker-containers-c37a01d01cf

The host UID probably doesn't exist in the build container, and more than likely isn't the docs user uid

After reading that post, I think this is not really important since you can execute a command with the UID from the host without needed to exist in the container.

Another similar idea to the one proposed here, instead of using os.getuid()) why not using a setting: DOCKER_USERNAME that will be docs-uid for production (in case that the project is ran by another user) and your-uid for development? I think this way all will continue working properly in production and allow us to work on development without modifying the image.

docker build -t readthedocs/build:dev --build-arg UID=1000

I think that most of the contributors will have the 1000 UID, so why not uploading this image to docker hub and use readthedocs/build:dev in the dev settings as default? It's not the final solution but an intermediate one and very easy to apply without breaking anything.

@humitos
Copy link
Member

humitos commented Nov 10, 2017

docker build -t readthedocs/build:dev --build-arg UID=1000

@agjohnson I modify your Dockerfile like this:

# Dockerfile.dev
# Read the Docs - Environment base
FROM readthedocs/build:2.0

ARG UID=1000
ARG GID=100
ARG GROUPNAME=users

# https://github.com/rtfd/readthedocs.org/pull/3152#issuecomment-339573168
USER root
# RUN groupadd --gid ${GID} ${GROUPNAME}
RUN usermod --uid ${UID} --gid ${GID} docs
USER docs

CMD ["/bin/bash"]

This way, any new contributor can build their own docker image with their own UID and GID. I commented the groupadd line since the users (mine) group is already created, but we probably make that command fail and continue in case it's already created.

If we are going on this way, I think we should include this Dockerfile in one of RTD repositories and add it in the documentation as a tip or something like that (as I did in my PR) maybe explaining the why and linking to these issues for a deeper understanding.

@agjohnson
Copy link
Contributor

@agjohnson why do we need permissions under the /home/docs? Aren't all the things downloaded and created under user_builds, user_uploads, etc from the SITE_ROOT?

Its not permissions, its the home path. We can't ensure tools relying on the user's home path will execute in the same way. We set up a number of things on the docs user, and so should try to execute as this UID.

I think that most of the contributors will have the 1000 UID, so why not uploading this image to docker hub and use readthedocs/build:dev in the dev settings as default? It's not the final solution but an intermediate one and very easy to apply without breaking anything.

This is not a huge hurdle, but we don't use UID 1000 in production either, so this introduces friction to getting a simple change out. We'd need to bring down each build server, alter the UID across TBs of docs and re-provision everything. UID 1000 also isn't the default minimum on RHEL/Centos, OSX, and a few others, so changing this on the build image would only b a partial fix still.

If we are going on this way, I think we should include this Dockerfile in one of RTD repositories and add it in the documentation as a tip or something like that (as I did in my PR) maybe explaining the why and linking to these issues for a deeper understanding.

Agreed. I still think this is the most simple solution. There is probably some application logic we can use to raise a warning that the UID doesn't match.

The simplest version of this seems to be a build configurable Dockerfile that we can store in docker/ or something. This could use build configuration arguments through docker to make this a one command setup. I didn't add the build configurable dockerfile at
#3152 (comment)
but the change is just adding an environment variable.

This doesn't make any more work for the developer -- that is, our built image is still downloaded and there is no compilation required. The additional step to alter the UID took a few minutes at most. This isn't even any additional steps, it just replaces:

docker pull readthedocs/build

with

docker build -t readthedocs/build:dev --build-arg UID=1000

Also, thanks for continue to track this down everyone, all of the ideas above are great ✨

Addressing this with more application logic is probably the most correct thing to do. But I think a custom Dockerfile like above is the best thing to do here -- it's simple, doesn't create more work, and offers the same solution to developers.

@safwanrahman
Copy link
Member Author

Its not permissions, its the home path. We can't ensure tools relying on the user's home path will execute in the same way. We set up a number of things on the docs user, and so should try to execute as this UID.

@agjohnson So we can set permission to the home directory so that all of the user can access it. Just simply run something like chmod -R 777 home/docs while building the image should make it work. What do you think?

@agjohnson
Copy link
Contributor

This would affect run the commands as the docs UID though, so wouldn't be enough.

@ericholscher
Copy link
Member

Seems like maybe this can be closed? I think there was a lot of discussion, but it didn't seem like this was the right solution.

@agjohnson
Copy link
Contributor

Yeah, think we can close 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.

4 participants