Skip to content

Fix running in privileged mode against a daemon with --default-cgroupns-mode=host#39578

Merged
AkihiroSuda merged 2 commits intomoby:masterfrom
rgulewich:cgroupns-test-req
Aug 25, 2019
Merged

Fix running in privileged mode against a daemon with --default-cgroupns-mode=host#39578
AkihiroSuda merged 2 commits intomoby:masterfrom
rgulewich:cgroupns-test-req

Conversation

@rgulewich
Copy link
Contributor

@rgulewich rgulewich commented Jul 19, 2019 •
edited
Loading

As per #38377 (comment)

Two changes:

  • Flip the cgroup ns check in TestCgroupNamespacesRunPrivileged(): it requires cgroup namespaces to be enabled, not the other way around.
  • Explicitly set the cgroup namespace mode to host when running with --privileged, rather than falling back to the host default

@rgulewich rgulewich mentioned this pull request Jul 19, 2019
Merged
@rgulewich
Copy link
Contributor Author

rgulewich commented Jul 19, 2019

@tonistiigi - This should address the test failures you were talking about in the comment linked above.

thaJeztah
thaJeztah approved these changes Jul 19, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tonistiigi
Copy link
Member

tonistiigi commented Jul 20, 2019

@rgulewich failing in CI for the same test

@andrewhsu
Copy link
Contributor

andrewhsu commented Jul 24, 2019

@rgulewich i see in the experimental PR check with commit 86abfc6:

--- FAIL: TestCgroupNamespacesRunPrivileged (0.73s)
    run_cgroupns_linux_test.go:30: Creating a new daemon
    daemon.go:336: [dab0eb268a84f] waiting for daemon to start
    daemon.go:336: [dab0eb268a84f] waiting for daemon to start
    daemon.go:364: [dab0eb268a84f] daemon started
    run_cgroupns_linux_test.go:37: assertion failed: error is not nil: Error response from daemon: privileged mode is incompatible with private cgroup namespaces.  You must run the container in the host cgroup namespace when running privileged mode

@rgulewich rgulewich force-pushed the cgroupns-test-req branch from 86abfc6 to 6c141aa Compare July 29, 2019 22:34
@rgulewich rgulewich changed the title Change TestCgroupNamespacesRunPrivileged requirement Fix running in privileged mode against a daemon with --default-cgroupns-mode=host Jul 30, 2019
@rgulewich
Copy link
Contributor Author

rgulewich commented Jul 30, 2019

@tonistiigi / @andrewhsu / @thaJeztah - Updated, and TestCgroupNamespacesRunPrivileged() is now passing. The failing test doesn't seem to be related to the change, as far as I can tell.

@thaJeztah
Copy link
Member

thaJeztah commented Jul 30, 2019

@psftw FYI looks like that machine might be out of space? https://ci.docker.com/public/blue/organizations/jenkins/moby/detail/PR-39578/2/pipeline

--- FAIL: TestBuildWithHugeFile (74.82s)
    build_test.go:468: assertion failed: string "{\"stream\":\"Step 1/2 : FROM busybox\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e f4279da41337\\n\"}\r\n{\"stream\":\"Step 2/2 : RUN for g in $(seq 0 8); do dd if=/dev/urandom of=rnd bs=1K count=1 seek=$((1024*1024*g)) status=none; done \\u0026\\u0026     ls -la rnd \\u0026\\u0026 du -sk rnd\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in c3496a2abe39\\n\"}\r\n{\"stream\":\"-rw-r--r--    1 root     root     8589935616 Jul 29 23:17 rnd\\n\"}\r\n{\"stream\":\"36\\trnd\\n\"}\r\n{\"stream\":\"Removing intermediate container c3496a2abe39\\n\"}\r\n{\"errorDetail\":{\"message\":\"ApplyLayer exit status 1 stdout:  stderr: write /rnd: no space left on device\"},\"error\":\"ApplyLayer exit status 1 stdout:  stderr: write /rnd: no space left on device\"}\r\n" does not contain "Successfully built"

@rgulewich
Change TestCgroupNamespacesRunPrivileged requirement
96f6c81 
This test requires cgroup namespaces to be enabled, not the other way
around.

Signed-off-by: Rob Gulewich <rgulewich@netflix.com>
@rgulewich
Explicity set Cgroup NS mode to "host" when running privileged
530f2d6 
Signed-off-by: Rob Gulewich <rgulewich@netflix.com>
@rgulewich
Copy link
Contributor Author

rgulewich commented Aug 24, 2019

@tonistiigi / @andrewhsu / @thaJeztah / @cpuguy83 - Any chance someone could take a look at this? The Windows test failure looks to be unrelated.

tonistiigi
tonistiigi approved these changes Aug 24, 2019
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

cc @AkihiroSuda

@AkihiroSuda AkihiroSuda merged commit cd1356d into moby:master Aug 25, 2019
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

@cpuguy83 cpuguy83

Labels

area/security/userns area/testing status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

7 participants

@rgulewich @tonistiigi @andrewhsu @thaJeztah @AkihiroSuda @cpuguy83 @GordonTheTurtle