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.

Sign up for GitHub

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

wp-env runs docker commands with same UID as host to prevent filesystem permission issues #42867

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

wp-env runs docker commands with same UID as host to prevent filesystem permission issues #42867

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ca7945
Update run.js
Luc45 Aug 1, 2022
e2ee21e
Linting
Luc45 Aug 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/env/lib/commands/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
const { spawn } = require( 'child_process' );
const os = require('os');

/**
* Internal dependencies
Expand Down Expand Up @@ -53,6 +54,8 @@ function spawnCommandDirectly( { container, command, config, spinner } ) {
'-f',
config.dockerComposeConfigPath,
'run',
'-u',
Copy link
Member

Choose a reason for hiding this comment

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

So I think this just changes the user when running something like npx wp-env run wordpress bash. I don't think this change would impact permissions as wp-env starts up. 🤔

Another change I was just working on in #42826 will use a git checkout for the main WordPress version rather than whatever Docker provides. I wonder if that would help the permissions issue as well...

I also experimented with file permissions in #40864.

But I was unfortunately never able to replicate the bug (even on Linux!), so I'm not sure what the best path is forwards! (I'm also curious if the steps on this page helped at all with permissions issues: https://docs.docker.com/engine/install/linux-postinstall/)

Could you describe the issues you're seeing?

Copy link
Author

@Luc45 Luc45 Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is some scripting involved, so it's hard to get an accurate step-by-step, but it goes something like this:

  • wp-env start
  • wp-env run tests-cli "wp theme install twentynineteen --activate"
  • wp-env destroy
  • <file permission issues>

Even though, I could not reproduce it neither running these commands in a clean wp-env (this repo).

I'm not sure when the permission errors kicks-in, as it shouldn't happen if the destroy also happens inside the container. But if the destroy calls rm -rf on the host, then it should have permission errors.

This is the error that I can reproduce when using wp-env integrated with our tooling:

image

Is that call to rm -rf plugins happening on the host machine? If so, that's a likely cause of permission issues

Copy link
Author

Choose a reason for hiding this comment

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

Ops, my bad. rm -rf plugins is called on our end of the tooling.

This issue could theoretically be closed as a non-issue, but it's nice to have the UID mapping being done correctly in Linux as well.

Copy link
Author

@Luc45 Luc45 Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've seen the #40864, I believe that mapping the host UID with the UID running inside the docker container is the most appopriate solution, as per this PR.

I believe that adding this UID mapping behind a feature flag, and optionally providing the ability to override Docker's UID with a environment variable will be enough to resolve the permission issues, as someone could do this if needed:

WP_ENV_UID=$(id -u):$(id -g) ./bin/wp-env run tests-cli "wp plugin download woocommerce"
("woocommerce" files will have correct permissions between host and Docker)

The Docker Desktop VM for Mac and Windows does something similar to this behind the scenes, but it's automatically managed for you, in Linux it has to be explicit. But if the docker-compose run call is behind an abstraction, such as wp-env's one, then the abstraction should allow to passthrough these arguments, ideally.

Copy link
Author

Choose a reason for hiding this comment

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

A cleaner approach would be to add support for a -u or --user flag, mimicking docker-compose

Copy link
Member

Choose a reason for hiding this comment

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

Good points! I also like the idea of keeping the user information consistent.

The reason I'm not sure this PR completely solves it is because when wp-env starts up, it's running docker compose commands directly, without using the built-in run command. The run command modified in this PR only changes the user settings for npx wp-env run .... But when everything is installed with npx wp-env start, it's still using the default settings for everything.

os.userInfo().uid,
'--rm',
container,
...command.split( ' ' ), // The command will fail if passed as a complete string.
Expand Down