reactioncommerce / docker-base

Apache License 2.0
4 stars 11 forks source link

fix: ensure fix-volumes uses node user on Mac #19

Closed aldeed closed 4 years ago

aldeed commented 4 years ago

Problem

Using stat -c "%u:%g" . to determine the UID:GID does not work when the host machine is Mac OSX and the working directory (.) is itself a volume linked to the host machine. On Mac, the command will always show root (0:0) as the owner because all volumes are always owned by root. This is true even if the underlying image has this directory pre-created with different ownership. The volume ownership masks the ownership built into the image.

Solution

Since the true purpose of fix-volumes.sh is to ensure that all volumes are owned by the same user who will run the command, my fix is to pass this user name into the script as an argument. Currently we could assume "node" user, but the script is more versatile if it accepts the user name as an argument because only the calling Dockerfile/entrypoint truly knows which user it will be running the command as.

Versioning

Because all images are rebuilt and retagged on every merge to trunk, all versions of node-dev images are currently affected by this bug. So in addition to making a v4, which makes it easy to force-pull this change into any project, I also updated all existing images to pass the new argument to fix-volumes.sh

Testing

  1. Run ./dockerfiles.sh build images/node-dev/12.10.0-v4/Dockerfile in this repo
  2. In API repo, latest release-3.0.0 branch, change the image to reactioncommerce/node-dev:12.10.0-v4 in docker-compose.dev.yml
  3. If you haven't already, ln -s docker-compose.dev.yml docker-compose.override.yml in the API project.
  4. In API project, dc down -v, or at least delete the node_modules volume with something like docker volume rm reaction_api_node_modules
  5. In API project, dc up -d mongo
  6. In API project, dc up api.

In the logs you should see something like this:

api_1    | fix-volumes: Fixing all volumes to be owned by node
api_1    | fix-volumes: Fixing volume /usr/local/src/app/node_modules (before=0:0 after=1000:1000)…✓

And NPM packages should install without errors and the API should start.

focusaurus commented 4 years ago

Is there any issue on mac? We used to just exit if we see uid 0 and do nothing, and is that not OK for mac?

aldeed commented 4 years ago

@focusaurus Yes the issue is that stat -c "%u:%g" . is always 0:0 so it exits. But we don't want it to exit. The app runs as node so the volumes all need to be changed to be owned by node.

aldeed commented 4 years ago

@focusaurus I'm not sure which exact change broke it on Mac, but the API was still back on -v1 and updating to -v3 it no longer works because of the issue I described.

focusaurus commented 4 years ago

Yeah I implemented --force initially because we only do a cheap check of the top-level dir of each mount. You would need that in the case that the mount point itself is OK but some subdirectory within it has the wrong owner, so --force was "just make it work I don't care how long it takes". It was intended for customer support scenario where regular fix-volumes.sh was already running but not chowning anything because the wrongness was only in subdirectories. I think it's useful still but don't want to block the fix. We cloud trigger it with a command line flag with a bit more parsing code or an env var or something eventually.

aldeed commented 4 years ago

I tested this on linux and I believe I was able to reproduce the issue and confirm the fix still works for linux, too. Here’s what I did:

sudo useradd testuser # create test user
sudo passwd testuser # set password for test user
usermod -a -G docker testuser # add test user to docker group so docker-compose command will work
chmod 644 .env # make .env visible to test user because we create it for current user only
(check uid/gid, 1001:1001 in my case)
sudo chown 1001:1001 . # change ownership of the api project root to new user/group
su - testuser # switch to new user
docker-compose up api # start api as test user

And then I saw this in the logs:

Fixing volume /usr/local/src/app/node_modules (before=1000:129 after=1001:1001)…✓