Partial refactor of UID/GID usage to a new struct#33362
Partial refactor of UID/GID usage to a new struct#33362tonistiigi merged 7 commits intomoby:masterfrom
Conversation
dnephin
changed the title
dnephin
force-pushed
the
refactor-uid-maps
branch
2 times, most recently
from
cbc3ddf to
adf35c7
Compare
dnephin
force-pushed
the
refactor-uid-maps
branch
2 times, most recently
from
ae1dce3 to
ae41030
Compare
daemon/archive.go
Outdated
There was a problem hiding this comment.
Any reason to ignore the error here (and in lots of other calls to RootPair) ?
There was a problem hiding this comment.
Mainly just because that's what the old code did: https://github.com/moby/moby/pull/33362/files#diff-1a1f3e7ad9b1d7584e2d3e7d0c4c3db9L969
According to the comment the default values are correct if there is no remapping.
I'm not sure what error is possible here. I'll take another look. Maybe it doesn't make sense to return an error at all.
There was a problem hiding this comment.
The only way for this function to return an error is if the idMaps are non-nil, and they don't have an entry for the root user. I don't know if there is any case where that can happen.
Maybe it would be better to error out in the NewIDMappings() during startup. Instead of checking the same error condition every time it's used. The mapping never changes so it seems like it would make more sense that way.
There was a problem hiding this comment.
Looking at the setup code again I notice this feature is not support on windows, so we don't have to worry about that case.
Remapping to the root user is already checked in NewIDMappings(), so I'm going to remove the error on RootPair().
dnephin
force-pushed
the
refactor-uid-maps
branch
2 times, most recently
from
3d8e034 to
80152ac
Compare
|
This looks reasonable to me. Just tried to use the labels to get a single "userns" build for validating the changes against the integration tests, but not sure if Gordon is doing anything :) |
|
LGTM I made a config change to allow userns rebuilding but that service needs to be redeployed before it becomes available. |
|
So can someone manually force a userns Jenkins run just for completeness? |
|
I manually started a job for this PR with userns enabled (https://jenkins.dockerproject.org/job/Docker-PRs-userns/14852). There's one failure with plugins: 18:54:52 ----------------------------------------------------------------------
18:54:52 FAIL: docker_cli_plugins_test.go:429: DockerSuite.TestPluginUpgrade
18:54:52
18:54:52 docker_cli_plugins_test.go:436:
18:54:52 dockerCmd(c, "run", "--rm", "-v", "bananas:/apple", "busybox", "sh", "-c", "touch /apple/core")
18:54:52 /go/src/github.com/docker/docker/pkg/testutil/cmd/command.go:64:
18:54:52 t.Fatalf("at %s:%d - %s", filepath.Base(file), line, err.Error())
18:54:52 ... Error: at cli.go:48 -
18:54:52 Command: /usr/local/bin/docker run --rm -v bananas:/apple busybox sh -c touch /apple/core
18:54:52 ExitCode: 126
18:54:52 Error: exit status 126
18:54:52 Stdout:
18:54:52 Stderr: container_linux.go:262: starting container process caused "process_linux.go:339: container init caused \"rootfs_linux.go:57: mounting \\\"/var/lib/docker/427680.427680/plugins/f57dfa7dac0a7dbf8b6be302b1e4ed3af4df1794a9e20f519fc129846d9c0e84/rootfs/mnt/volumes/bananas/_data\\\" to rootfs \\\"/var/lib/docker/427680.427680/vfs/dir/e9aabbbfec67f77b0d87dc843a70ccd6c0c5647bc1d4a390a3fcff0cc0b94265\\\" at \\\"/apple\\\" caused \\\"stat /var/lib/docker/427680.427680/plugins/f57dfa7dac0a7dbf8b6be302b1e4ed3af4df1794a9e20f519fc129846d9c0e84/rootfs/mnt/volumes/bananas/_data: permission denied\\\"\""
18:54:52 /usr/local/bin/docker: Error response from daemon: oci runtime error: container_linux.go:262: starting container process caused "process_linux.go:339: container init caused \"rootfs_linux.go:57: mounting \\\"/var/lib/docker/427680.427680/plugins/f57dfa7dac0a7dbf8b6be302b1e4ed3af4df1794a9e20f519fc129846d9c0e84/rootfs/mnt/volumes/bananas/_data\\\" to rootfs \\\"/var/lib/docker/427680.427680/vfs/dir/e9aabbbfec67f77b0d87dc843a70ccd6c0c5647bc1d4a390a3fcff0cc0b94265\\\" at \\\"/apple\\\" caused \\\"stat /var/lib/docker/427680.427680/plugins/f57dfa7dac0a7dbf8b6be302b1e4ed3af4df1794a9e20f519fc129846d9c0e84/rootfs/mnt/volumes/bananas/_data: permission denied\\\"\"".
18:54:52
18:54:52
18:54:52 Failures:
18:54:52 ExitCode was 126 expected 0
18:54:52 Expected no error
18:54:52
18:54:52
18:54:52
18:54:52 ---------------------------------------------------------------------- |
|
Thanks @mlaventure . I tried running that test (locally) with master and it fails the same way. I'm doing a full run on jenkins here: https://jenkins.dockerproject.org/job/Docker-PRs-userns/14854/console to show the failure. It looks like Tonis ran the userns-master build, but it skipped the test with "docker_cli_plugins_test.go:429: DockerSuite.TestPluginUpgrade (unmatched requirement Network)". It looks like the last run of userns was September 2016, which is before this test was added. I think this test may never have worked with userns. |
|
Yes, this test won't be successful on userns because of an external volume that won't be mapped to have write perms. from the remapped UID running in the container. For most tests like this, we skip for userns rather than complicate the test setup. Thanks for executing a test run @mlaventure |
|
Moved to code-review. Tests are green |
|
LGTM ping @tiborvass |
Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
The test was failing because TarOptions was using a non-pointer for ChownOpts, which meant the check for nil was never true, and createTarFile was never using the hdr.UID/GID Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
There is no case which would resolve in this error. The root user always exists, and if the id maps are empty, the default value of 0 is correct. Signed-off-by: Daniel Nephin <[email protected]>
dnephin
force-pushed
the
refactor-uid-maps
branch
from
80152ac to
93fbdb6
Compare
Existing code which uses []IDMap relies on zero-valued fields to be valid, empty mappings. So if we want to successfully finish the refactoring started in moby#33362, their replacement therefore also needs to have a useful zero value which represents an empty mapping. Ideally this would be accomplished by changing IdentityMapping to be a pass-by-value type so that there would be no nil pointers to worry about, but *IdentityMapping values are already used extensively within BuildKit so we cannot do that without also refactoring BuildKit in lockstep. We can, however, do the next best thing: modify all the associated functions so that they are well-behaved when called on a nil pointer. Signed-off-by: Cory Snider <[email protected]>
9 participants
Related to #32989 and #32904
Improve the interfaces exposed by
idtoolswith some new structs.In order to move
CopyOnBuildinto the builder we need access to id mappings. Instead of passing around slices, expose a struct with the necessary behaviour.This PR converts all uses of idtools, except for graphdrivers.