Run dockerfile commands with correct user. #12579

EnTeQuAk · 2019-10-10T10:47:22Z

This updates various things…

Remove the default usage of USER in the Dockerfile as this may
be too restrictive for daily usage
Use gosu and update the entrypoint to fix permissions and drop
properly into the olympia user
Use docker-compose run to actually make sure the entrypoint is run
correctly which isn't the case when using exec

Please do test these changes thoroughly, I only briefly ran them on macos and windows but I might not know what to look for specifically.

Things worth testing:

make initialize works as expected and doesn't trample directory permissions
make shell works and initializes itself with the olympia user
inside make shell a make update_deps works too because we're setting correct permissions
make update works as expected

I pushed a separate fixed-docker-perms tag to dockerhub and updated the docker-compose file to make testing easier.

This updates various things… * Remove the default usage of `USER` in the Dockerfile as this may be too restrictive for daily usage * Use `gosu` and update the entrypoint to fix permissions and drop properly into the `olympia` user * Use `docker-compose run` to actually make sure the entrypoint is run correctly which isn't the case when using `exec` Fixes #12366

diox · 2019-10-10T12:20:33Z

Makefile-os

@@ -11,17 +11,17 @@ help_submake:

 .PHONY: update_docker
 update_docker: ## update all the docker images
-	docker-compose exec worker make update_deps


Why the change from exec to run here and below ? The container should already be running, we don't want another one, we want to re-use the one we have.

As written in the description - I noticed that when using exec the entrypoint would simply not be executed which lead to many of the permission problems we've seen in folders on the hosts system.

run on the other hand does run the entrypoint properly. I haven't found another reasonable solution to this and I'm perfectly aware that it's not ideal but as the original web container stays up I don't think it's an issue.

Also, the storage layers (/deps/) seem to be shared between those two so that updates do happen on both afaik.

Happy to use exec if you know any way to make the entrypoint being executed properly.

run does not start supervisord (the CMD) again so that there aren't any conflicts from what I see.

Also, the storage layers (/deps/) seem to be shared between those two so that updates do happen on both afaik.

That doesn't seem to be true, no. I don't see where that would be set in the configuration, and it doesn't seem to be shared on my machine (I created a file in /deps using exec, then checked if it was there using run, it wasn't).

And it's not just about /deps: you might want to install a system package using make shell when you're adding a system dependency, or debugging something. That wouldn't work if it's a different container.

eviljeff · 2019-10-11T09:33:58Z

docker-compose.yml

@@ -23,7 +23,7 @@ x-env-mapping: &env
 services:
  worker: &worker
    <<: *env
-    image: addons/addons-server:latest
+    image: addons/addons-server:fixed-docker-perms


Is this a permanent change? How is :fixed-docker-perms being updated? (And could do with a better name if permanent)

No, as written in the description…

I pushed a separate fixed-docker-perms tag to dockerhub and updated the docker-compose file to make testing easier.

It's not permanent and will be switched back before I merge this branch.

diox · 2019-10-10T12:23:42Z

docker-compose.yml

@@ -23,7 +23,7 @@ x-env-mapping: &env
 services:
  worker: &worker
    <<: *env
-    image: addons/addons-server:latest
+    image: addons/addons-server:fixed-docker-perms


Need to revert that

Yes, before I merge that. For testing it's easier to leave it

diox · 2019-10-10T12:44:07Z

Makefile-os

@@ -11,17 +11,17 @@ help_submake:

 .PHONY: update_docker
 update_docker: ## update all the docker images
-	docker-compose exec worker make update_deps


Also, the storage layers (/deps/) seem to be shared between those two so that updates do happen on both afaik.

That doesn't seem to be true, no. I don't see where that would be set in the configuration, and it doesn't seem to be shared on my machine (I created a file in /deps using exec, then checked if it was there using run, it wasn't).

And it's not just about /deps: you might want to install a system package using make shell when you're adding a system dependency, or debugging something. That wouldn't work if it's a different container.

…fix-docker-builds

…erver into 12366-fix-docker-builds

EnTeQuAk · 2019-10-15T10:45:07Z

Alrighty, I updated the implementation slightly.

I created a olympia user in the Dockerfile
I commented out the entrypoint for now (see below for why)
I made use of the user option in docker-compose.yml

Now, this seems to work nicely on Linux and I'm not able to reproduce any of the filesystem permission issues we had in the past. I'm not quite sure though how other systems do the mapping so I need more testing for whether the uid/guid mapping is still needed or not.

@eviljeff (for Windows) and @muffinresearch (for OSX) are you able to checkout this branch and test if you still see issues with files owned by another account on your systems for files created inside the docker container?

If you don't, then we do have a fairly simple solution at hand.

muffinresearch · 2019-10-15T11:32:33Z

Alrighty, I updated the implementation slightly.
* I created a `olympia` user in the `Dockerfile`

* I commented out the entrypoint for now (see below for why)

* I made use of the `user` option in docker-compose.yml
Now, this seems to work nicely on Linux and I'm not able to reproduce any of the filesystem permission issues we had in the past. I'm not quite sure though how other systems do the mapping so I need more testing for whether the uid/guid mapping is still needed or not.

@eviljeff (for Windows) and @muffinresearch (for OSX) are you able to checkout this branch and test if you still see issues with files owned by another account on your systems for files created inside the docker container?

If you don't, then we do have a fairly simple solution at hand.

Yep will do.

muffinresearch

Uploading an add-on now works for me 👍

muffinresearch · 2019-10-15T11:40:48Z

Dockerfile

@@ -80,10 +83,15 @@ ENV PIP_SRC=/deps/src/
 ENV NPM_CONFIG_PREFIX=/deps/
 ENV SWIG_FEATURES="-D__x86_64__"

+RUN useradd -Md /code/ olympia


Possibly a bit counter-intuitive to set $HOME to /code even if we did this historically - does anything actually rely on that behaviour?

I don't think so and I'm not sure we actually need that. I could try without and see where that gets us

diox · 2019-10-15T11:58:10Z

scripts/start-docker.sh

+
+# Fix /deps/ folder so that we're able to update the image
+# with the `olympia` user
+if [[ $deps_uid -ne $uid ]]; then


I guess this doesn't hurt but why would the user be wrong?

With the recent changes, this entrypoint will hopefully be removed - I haven't done that yet while the testing isn't done.

So, don't worry about that just yet.

I put the chown in the Dockerfile instead which enforces the correct user assignment.

…erver into 12366-fix-docker-builds

…fix-docker-builds

diox

Tested locally, it works.

diox · 2019-10-16T13:30:57Z

docker-compose.yml

@@ -23,9 +23,13 @@ x-env-mapping: &env
 services:
  worker: &worker
    <<: *env
-    image: addons/addons-server:latest
+    build: .


As mentioned on IRC, we don't want to keep that, because this forces a local build instead of a pull. If someone wants to locally build an image, they should either use a custom file if they want to use docker-compose, or just use regular docker.

diox · 2019-10-16T13:31:51Z

docker-compose.yml

+    # We are defaulting to the olympia user to avoid any permission related
+    # issues and files owned by root. This can be overwritten through
+    # the --user option when calling docker-compose
+    user: olympia


Maybe add a simple rootshell target in Makefile-os ?

diox

Actually... make update_docker is broken for me:
https://gist.github.com/diox/040421aa6cb3db392e3e9ba6e5b2d3c1

pip is installing deps in /usr/local/lib so it won't work with the olympia user...

…fix-docker-builds

…nd PIP_USER

EnTeQuAk · 2019-10-21T04:54:21Z

I updated quite a bunch of things, I'll add more documentation later once I'm in the colo and will see if the circleci builds work for now. I think we got a tiny bit closer to a working docker environment (and dockerhub builds for that matter…)

…fix-docker-builds

…erver into 12366-fix-docker-builds

EnTeQuAk · 2019-10-21T09:03:15Z

Alright, quick update. I updated the configs to fix docker image build related issues which now requires GROUP_ID/USER_ID to be passed along when building the image manually. I'll update the docs accordingly once I ran a few more tests.

The rest is mostly the same from what already has been tested, we're now using user-local python installation and run pip accordingly which works around various issues during image building / make initialize and make update_docker.

STR for testing:

checkout this branch (and latest changes)
docker-compose pull to pull the latest build (build is done and uploaded now)
docker-compose up -d
make initialize (to re-initialize everything, updates deps etc which will shouldn't run into permission errors)
(alternatively just running make update_docker should be a good test-candidate too)
Then, upload an add-on, does this work? Do you see any root owned files, does accessing/downloading that file work?

EnTeQuAk · 2019-10-22T08:57:43Z

Dockerfile

-# Run everything as olympia user, by default.
-USER olympia
+ARG GROUP_ID=1000
+ARG USER_ID=1000


These require --build-arg GROUP_ID=$(id -g) --build-arg USER_ID=$(id -u) - I'll add docs about that later - I found that we don't have any docs at all about local building so I'll do that together with fixing circleci automatic builds (for local dev) as this may change requirements.

EnTeQuAk added 4 commits September 26, 2019 16:32

tmp

6412631

Correct if statement

747a477

Use separate tag for easier testing

1bc7fc3

EnTeQuAk requested review from eviljeff and diox October 10, 2019 11:49

diox reviewed Oct 10, 2019

View reviewed changes

eviljeff reviewed Oct 11, 2019

View reviewed changes

diox requested changes Oct 11, 2019

View reviewed changes

EnTeQuAk added 5 commits October 11, 2019 13:17

Merge branch 'master' of github.com:mozilla/addons-server into 12366-…

b9fb5b6

…fix-docker-builds

Merge branch 'master' of github.com:mozilla/addons-server into 12366-…

b60b532

…fix-docker-builds

Try running directly with user olympia

4dd8fdb

Merge branch '12366-fix-docker-builds' of github.com:mozilla/addons-s…

a405386

…erver into 12366-fix-docker-builds

Merge branch '12366-fix-docker-builds' of github.com:mozilla/addons-s…

cd3fb54

…erver into 12366-fix-docker-builds

EnTeQuAk force-pushed the 12366-fix-docker-builds branch from a405386 to cd3fb54 Compare October 15, 2019 10:10

Small cleanup

1166098

muffinresearch reviewed Oct 15, 2019

View reviewed changes

diox reviewed Oct 15, 2019

View reviewed changes

EnTeQuAk added 4 commits October 16, 2019 10:17

Merge branch '12366-fix-docker-builds' of github.com:mozilla/addons-s…

b32f01b

…erver into 12366-fix-docker-builds

Add docs

81e69d5

Merge branch 'master' of github.com:mozilla/addons-server into 12366-…

a8644f3

…fix-docker-builds

Fix comment

23e8588

diox approved these changes Oct 16, 2019

View reviewed changes

diox requested changes Oct 16, 2019

View reviewed changes

EnTeQuAk added 4 commits October 17, 2019 10:48

Merge branch 'master' of github.com:mozilla/addons-server into 12366-…

c327835

…fix-docker-builds

Add 'rootshell' command

70c6954

Build circleci for local dev

612846f

Fix pip execution by using 'python -m' to fetch the correct pip

56c10f1

EnTeQuAk added 2 commits October 21, 2019 06:49

Pick up upstream user/group for builds and implement PYTHONUSERBASE a…

ebde18b

…nd PIP_USER

Correctly pass USER_ID and GROUP_ID to Dockerfile when building

5f42bf1

EnTeQuAk added 5 commits October 21, 2019 06:56

Small cleanups

24214fa

Build our test branch

7efd7b1

Merge branch 'master' of github.com:mozilla/addons-server into 12366-…

2138430

…fix-docker-builds

Merge branch '12366-fix-docker-builds' of github.com:mozilla/addons-s…

f3efb69

…erver into 12366-fix-docker-builds

Fall back to manual uploads for testing

aa22fb0

EnTeQuAk commented Oct 22, 2019

View reviewed changes

Cleanup

853a6da

diox approved these changes Oct 22, 2019

View reviewed changes

Revert back to :latest tag

ef54110

EnTeQuAk merged commit b9ac5f4 into master Oct 22, 2019

EnTeQuAk deleted the 12366-fix-docker-builds branch October 22, 2019 09:24

EnTeQuAk mentioned this pull request Oct 23, 2019

Failed when working locally (make initialize) mozilla/addons#7075

Closed

b4handjr mentioned this pull request Nov 8, 2019

Circleci Integration tests are failing during setup mozilla/addons#13589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run dockerfile commands with correct user. #12579

Run dockerfile commands with correct user. #12579

EnTeQuAk commented Oct 10, 2019 •

edited

Loading

diox Oct 10, 2019

EnTeQuAk Oct 10, 2019 •

edited

Loading

diox Oct 10, 2019

eviljeff Oct 11, 2019

EnTeQuAk Oct 11, 2019

diox Oct 10, 2019

EnTeQuAk Oct 11, 2019

diox Oct 10, 2019

EnTeQuAk commented Oct 15, 2019 •

edited

Loading

muffinresearch commented Oct 15, 2019

muffinresearch left a comment

muffinresearch Oct 15, 2019

EnTeQuAk Oct 15, 2019

diox Oct 15, 2019

EnTeQuAk Oct 15, 2019

EnTeQuAk Oct 15, 2019

diox left a comment

diox Oct 16, 2019

diox Oct 16, 2019

diox left a comment •

edited

Loading

EnTeQuAk commented Oct 21, 2019

EnTeQuAk commented Oct 21, 2019 •

edited

Loading

EnTeQuAk Oct 22, 2019

Run dockerfile commands with correct user. #12579

Run dockerfile commands with correct user. #12579

Conversation

EnTeQuAk commented Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

EnTeQuAk Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnTeQuAk commented Oct 15, 2019 • edited Loading

muffinresearch commented Oct 15, 2019

muffinresearch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diox left a comment • edited Loading

Choose a reason for hiding this comment

EnTeQuAk commented Oct 21, 2019

EnTeQuAk commented Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

EnTeQuAk commented Oct 10, 2019 •

edited

Loading

EnTeQuAk Oct 10, 2019 •

edited

Loading

EnTeQuAk commented Oct 15, 2019 •

edited

Loading

diox left a comment •

edited

Loading

EnTeQuAk commented Oct 21, 2019 •

edited

Loading