Fix media uploads for Docker in rootless mode #50608
Conversation
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
|
I managed to get e2e tests working in rootless docker with this change on my machine, but I'm getting this error on GitHub doing the same thing. @noahtallen I'm no Docker expert, is there any reason this would be failing here? Or is there a better way to change permissions in Docker? |
|
Hm, I'm no expert either, but throwing out some ideas:
I'm also curious, what errors do you see in rootless mode? I also don't have a good way to test rootless mode unfortunately. thanks for opening a PR! |
ajlende
force-pushed
the
fix/wp-env-rootless-docker
branch
from
3886006 to
71d97f6
Compare
The DOCKERFILE is running the commands as root, so chown is necessary to move ownership to the host user.
That's what I started with, but I kept getting the same error until I created the dir and chowned it.
The
Updated the description. I should have had that added before :) |
ajlende
requested review from
noahtallen,
ObliviousHarmony and
t-hamano
as code owners
|
Flaky tests detected in c44b630. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5049246189
|
|
I made it past |
ajlende
force-pushed
the
fix/wp-env-rootless-docker
branch
from
8d9cf28 to
fe62240
Compare
There was a problem hiding this comment.
This is interesting @ajlende. Given the existence of this particular problem, however, I think there will be other permission problems when running Docker in rootless mode to consider. All of the issues fixed by #49962 would be experienced in rootless mode.
The problem is that the uid and gid in rootless mode are in the host user's namespace rather than being mapped in the same way. This means that root in the container is actually the user's uid and your host user's uid in the container is an offset from the host user's uid. The end result is that the "user" in the container doesn't have write access to any of the files.
This PR works because you're forcing ownership of /var/www/html/wp-content/uploads to the user namespace uid, but, I wonder if there's another approach where we can preserve the fixes provided by the original PR? What if we set the userns_mode to host in the docker-compose.yml file? The Docker daemon would still not be running as root, but, the uid mapping should work again, right? This would also only apply to the wp-env containers.
| @@ -54,6 +54,11 @@ function getMounts( | |||
| ? `user-home:/home/${ hostUsername }` | |||
| : `tests-user-home:/home/${ hostUsername }`; | |||
|
|
|||
| const userUploadsMount = | |||
There was a problem hiding this comment.
What happens if there is a mappings entry for /var/www/html/wp-content/uploads already?
There was a problem hiding this comment.
I'm sorry, I don't really know that much about docker so I may be misunderstanding. This function creates a new Set() which, by definition, doesn't contain duplicates, so I don't think there could already be an entry for it?
There was a problem hiding this comment.
Let's take, for example, a wp-env.json file containing this:
{
"mappings": {
"/var/www/html/wp-content/uploads": "./env/uploads"
}
}This would translate to a unique entry in the Set, but, be a duplicate mapping for /var/www/html/wp-content/upload.
|
The tests are all consistently passing locally, but consistently failing on GitHub. I'm not really sure how to debug this. |
|
Also, #32519 should actually already be fixed. The |
|
Thanks for the quick review @ObliviousHarmony!
I just tried that on a fresh branch off of trunk, and I still end up with the original error where uploads aren't allowed.
I was wondering if we should set up permissions as described in the Hardening WordPress docs, but I had something working locally with just the uploads change and figured it would be better to start small and fix my one issue first. |
We did some further debugging together and weren't able to find a way to make this work. I did some further research and it looks like this is unfortunately unavoidable. If you're running in rootless then using your host
Honestly, I would prefer if we avoided added any permission hacks specific for Rootless Docker. There's a problem here in how permissions work between these two deployments, but, I've got an idea! In #50814 (comment) it was highlighted that there's a problem with Windows caused by the usage of |
|
Let me take a pass at solving both of these problems in one go. Could you share the output of |
[
{
"Name": "default",
"Metadata": {},
"Endpoints": {
"docker": {
"Host": "unix:///run/user/1000/docker.sock",
"SkipTLSVerify": false
}
},
"TLSMaterial": {},
"Storage": {
"MetadataPath": "\u003cIN MEMORY\u003e",
"TLSPath": "\u003cIN MEMORY\u003e"
}
}
] |
|
What about |
Nice, I love the idea of solving both problems at once! |
It does seem to list |
|
I ran into what should have been a pretty obvious problem with running everything as I'll let @noahtallen weigh in, but, I don't think rootless Docker is something we should try and go to extreme lengths to support. Ultimately it boils down to everything being owned by Ideally |
|
I mostly agree with @ObliviousHarmony. I don't think |
|
Thanks for the help and teaching me some things about wp-env. Since this isn't as simple as I first thought, we'll close this PR, and I'll take @ObliviousHarmony's suggestion to just modify my containers manually for now. |
What?
Fixes media uploads for Docker running in rootless mode.
Why?
Running Docker in rootless mode provides an extra layer of security by not running the daemon with the root user.
May also solve #32519 because it's using a volume for the uploads now.
How?
Testing Instructions
wp-env startTesting Instructions for Keyboard
Screenshots or screencast