Fix copy chown settings to not default to real root #20446
Conversation
|
Can you add a test? |
estesp
force-pushed
the
fix-userns-cp
branch
from
c4e5e99
to
bf116c1
Compare
|
@cpuguy83 yup..was my plan, but being at a conference slowed me down :) Just updated as I had wanted a test which wasn't userns specific, so this test works in both CI flows. |
estesp
force-pushed
the
fix-userns-cp
branch
2 times, most recently
from
93e9da6
to
87168a6
Compare
This corrects `docker cp` behavior when user namespaces are enabled. Instead of chown'ing copied-in files to real root (0,0), the code queries for the remapped root uid & gid and sets the chown option properly. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
estesp
force-pushed
the
fix-userns-cp
branch
from
87168a6
to
40be5db
Compare
|
Adding this to the 1.10.2 milestone (if it's released), since userns is a new feature in 1.10 |
|
LGTM |
1 similar comment
|
LGTM |
|
|
||
| stat, err := system.Stat(filepath.Join(tmpVolDir, "file1")) | ||
| c.Assert(err, checker.IsNil) | ||
| uid, gid, err := getRootUIDGID() |
There was a problem hiding this comment.
You can use idtools.GetRootUIDGID here.
There was a problem hiding this comment.
Actually, that would require that I have the mappings from within the daemon, but the tests can be run as a separate client (and in this case they are)--so the only option is to parse the output of what the server will tell me about itself.. which in this case is the fact we know the root dir of the daemon contains the root uid, gid info..
There was a problem hiding this comment.
Makes sense.
TestDaemonUserNamespaceRootSetting is doing something similar and can be updated to use this new function.
Also, how is this testing against a userns enabled daemon, as mentioned in the function comment header?
There was a problem hiding this comment.
TestDaemonUserNamespaceRootSetting is doing something similar and can be updated to use this new function.
True--absolutely!
Also, how is this testing against a userns enabled daemon, as mentioned in the function comment header?
Because one of our runs of make test-integration-cli in CI sets the DOCKER_REMAP_ROOT setting and runs all tests against a daemon with userns enabled; appropriately named userns :)
|
Will investigate the userns CI as soon as i can |
|
@tiborvass I'm not sure how to dig further into that without a recreate outside of CI--I've seen it before; some form of hung process trying to run a command; but not enough info in the Goroutine dump to explain why. I'm also not sure if it only relates to userns CI runs, as I feel like I've seen the timeouts in other runs as well? |
|
@estesp no it doesnt seem specific to userns, but itd be great to have userns be green on this pr |
|
(*DockerSuite).TestBuildHistory(0x14823e8, 0xc82032aa50) hangs. Context: https://jenkins.dockerproject.org/job/Docker-PRs-userns/6445/consoleFull 20:55:47 os/exec.(_Cmd).Wait(0xc8204a5900, 0x0, 0x0) Working up the stack shows that buildImage -> buildImageWithOut are called with valid args. buildImageWithOut(name, dockerfile string, useCache bool, buildFlags ...string)
However, the build command doesnt complete on time (80 minutes in this case), the alarm kicks in and test panics. |
|
LGTM! |
Fix copy chown settings to not default to real root
Fixes: #20402
This corrects
docker cpbehavior when user namespaces are enabled.Instead of chown'ing copied-in files to real root (0,0), the code
queries for the remapped root uid & gid and sets the chown option
properly.
Docker-DCO-1.1-Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com (github: estesp)