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.

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

Fix copy chown settings to not default to real root #20446

Merged
merged 1 commit into from
Feb 20, 2016

Conversation

estesp
Copy link
Contributor

@estesp estesp commented Feb 18, 2016

Fixes: #20402

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)

@cpuguy83
Copy link
Member

Can you add a test?

@estesp
Copy link
Contributor Author

estesp commented Feb 18, 2016

@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 estesp force-pushed the fix-userns-cp branch 2 times, most recently from 93e9da6 to 87168a6 Compare February 18, 2016 22:38
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)
@thaJeztah
Copy link
Member

Adding this to the 1.10.2 milestone (if it's released), since userns is a new feature in 1.10

@thaJeztah thaJeztah added this to the 1.10.2 milestone Feb 18, 2016
@calavera
Copy link
Contributor

LGTM

1 similar comment
@tiborvass
Copy link
Contributor

LGTM


stat, err := system.Stat(filepath.Join(tmpVolDir, "file1"))
c.Assert(err, checker.IsNil)
uid, gid, err := getRootUIDGID()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use idtools.GetRootUIDGID here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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..

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

@tiborvass
Copy link
Contributor

Will investigate the userns CI as soon as i can

@estesp
Copy link
Contributor Author

estesp commented Feb 19, 2016

@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?

@tiborvass
Copy link
Contributor

@estesp no it doesnt seem specific to userns, but itd be great to have userns be green on this pr

@anusha-ragunathan
Copy link
Contributor

(*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)
20:55:47 /usr/local/go/src/os/exec/exec.go:380 +0x211
20:55:47 os/exec.(_Cmd).Run(0xc8204a5900, 0x0, 0x0)
20:55:47 /usr/local/go/src/os/exec/exec.go:258 +0x64
20:55:47 os/exec.(_Cmd).CombinedOutput(0xc8204a5900, 0x0, 0x0, 0x0, 0x0, 0x0)
20:55:47 /usr/local/go/src/os/exec/exec.go:424 +0x310
20:55:47 github.com/docker/docker/pkg/integration.RunCommandWithOutput(0xc8204a5900, 0x0, 0x0, 0x2, 0x0, 0x0)
20:55:47 /go/src/github.com/docker/docker/pkg/integration/utils.go:66 +0x41
20:55:47 github.com/docker/docker/integration-cli.runCommandWithOutput(0xc8204a5900, 0x0, 0x0, 0x15e, 0x0, 0x0)
20:55:47 github.com/docker/docker/integration-cli/_test/_obj_test/utils.go:39 +0x47
20:55:47 github.com/docker/docker/integration-cli.buildImageWithOut(0xe9f440, 0x10, 0xfe2900, 0x15e, 0xc820039701, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
20:55:47 github.com/docker/docker/integration-cli/_test/_obj_test/docker_utils.go:1675 +0xd8
20:55:47 github.com/docker/docker/integration-cli.buildImage(0xe9f440, 0x10, 0xfe2900, 0x15e, 0xc81ffdad01, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
20:55:47 github.com/docker/docker/integration-cli/_test/_obj_test/docker_utils.go:1710 +0xad
20:55:47 github.com/docker/docker/integration-cli.(_DockerSuite).TestBuildHistory(0x14823e8, 0xc82032aa50)
20:55:47 /go/src/github.com/docker/docker/integration-cli/docker_cli_history_test.go:48 +0x12d
20:55:47 reflect.Value.call(0xe03ec0, 0x14823e8, 0x18113, 0xe113d8, 0x4, 0xc82048bf10, 0x1, 0x1, 0x0, 0x0, ...)

Working up the stack shows that buildImage -> buildImageWithOut are called with valid args.
integration-cli.buildImageWithOut(0xe9f440, 0x10, 0xfe2900, 0x15e, 0xc820039701, 0x0, 0x0, 0x0, 0x0, 0x0, ...)

buildImageWithOut(name, dockerfile string, useCache bool, buildFlags ...string)

  • string name: 0xe9f440 (ptr), 0x10 (length).
    In our case, name is "testbuildhistory" - 16 = 0x10
  • string dockerfile: 0xfe2900 (ptr), 0x15e (length)
    In our case, this is the string representing the contents of the dockerfile.
    12 + 1(for \n) * 26 + 12 = 312 + 13 = 350
  • useCache: 0xc820039701. Bits are packed, so that last byte is what we are looking for.
    i.e useCache = 1, so we are good.

However, the build command doesnt complete on time (80 minutes in this case), the alarm kicks in and test panics.

@anusha-ragunathan
Copy link
Contributor

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants