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

User namespaces: graduate from experimental #19187

Merged
merged 3 commits into from Jan 12, 2016
Merged

Conversation

estesp
Copy link
Contributor

@estesp estesp commented Jan 8, 2016

Moves experimental user namespaces to master in preparation for the 1.10 release. Adds documentation to regular docker daemon command docs. Properly warns of incompatible flags for namespace sharing + privileged.

Closes: #18448
Closes: #18430
Closes: #18247

@estesp
Copy link
Contributor Author

estesp commented Jan 8, 2016

ping @thaJeztah : I spent some extra time trying to make the docs more readable/clear. Feel free to start reviewing/providing feedback as I think the docs may need more help than the code, which is basically just a migration of the flag and settings.

@thaJeztah
Copy link
Member

ping @moxiegirl as well 😄

@vdemeester
Copy link
Member

😻

@jessfraz
Copy link
Contributor

jessfraz commented Jan 8, 2016

yayayayayayayayayayayayyayaaya! LGTM

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 8, 2016

yolo
LGTM

@icecrime
Copy link
Contributor

icecrime commented Jan 8, 2016

Looks YOLO To Me 👍

Adds the `--userns-remap` flag to the master build

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
@estesp
Copy link
Contributor Author

estesp commented Jan 8, 2016

@jfrazelle just FYI--updated the patch to stop hack/make.sh from adding experimental build tag when DOCKER_REMAP_ROOT is set, given it will now be "testable" from master. Let me know if you see any issues with that, but the CI userns run should verify that's correct.

@jessfraz
Copy link
Contributor

jessfraz commented Jan 8, 2016

sweet thats perfect thanks!

On Fri, Jan 8, 2016 at 12:10 PM, Phil Estes notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle just FYI--updated the patch to
stop hack/make.sh from adding experimental build tag when
DOCKER_REMAP_ROOT is set, given it will now be "testable" from master.
Let me know if you see any issues with that, but the CI userns run
should verify that's correct.


Reply to this email directly or view it on GitHub
#19187 (comment).

@thaJeztah
Copy link
Member

@estesp don't forget to update the daemon man page as well, to include the new flag; https://github.com/docker/docker/blob/master/man/docker-daemon.8.md

@HackToday
Copy link
Contributor

@thaJeztah do you know how to test it when it was not merged ? Do we have binary build for that ? Or need I cherry-pick these code in my repo and build binary by myself ? Thanks

@vdemeester
Copy link
Member

@HackToday You can try out a pull request by checking it out using git and then build it (using make binary for example 😉)

@HackToday
Copy link
Contributor

Thanks @vdemeester I tried this clone
https://github.com/estesp/docker/tree/lets-do-this

Since I did not figure it out just cherry-pick easy ways to do that :(

@HackToday
Copy link
Contributor

Found following potential issues:

  1. use host with --uts=host or --ipc=host failed with following strange error message:

root@003d1da3734a:/go/src/github.com/docker/docker# docker run -it --uts=host myhtop pidtest-1
mount: permission denied
Could not mount /sys/kernel/security.
AppArmor detection and --privileged mode might break.
mount: permission denied
root@003d1da3734a:/go/src/github.com/docker/docker# docker run -it --ipc=host myhtop pidtest-1
mount: permission denied
Could not mount /sys/kernel/security.
AppArmor detection and --privileged mode might break.
mount: permission denied

  1. for --ipc use container:ID

root@003d1da3734a:/go/src/github.com/docker/docker# docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
5040226021d9 busybox "sh" 23 minutes ago Up 23 minutes testok
2dc6bb7c5ebb busybox "sh" 25 minutes ago Up 25 minutes goofy_northcutt
root@003d1da3734a:/go/src/github.com/docker/docker#
root@003d1da3734a:/go/src/github.com/docker/docker#
root@003d1da3734a:/go/src/github.com/docker/docker# docker run --name testsecond -it --ipc=container:testok busybox sh
docker: Error response from daemon: Container command could not be invoked..
root@003d1da3734a:/go/src/github.com/docker/docker# docker ps -a
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
98268acac542 busybox "sh" 8 seconds ago Created testsecond
5040226021d9 busybox "sh" 23 minutes ago Up 23 minutes testok
2dc6bb7c5ebb busybox "sh" 25 minutes ago Up 25 minutes goofy_northcutt

And the container was created.

@HackToday
Copy link
Contributor

And another issue:

When run with docker run --read-only -v /icanwrite busybox sh still have long go trace message like below:

5: main
Package: main
File: docker.go@18


6: main
Package: runtime
File: proc.go@111


7: goexit
Package: runtime
File: asm_amd64.s@1721
docker: Error response from daemon: Cannot start container c8bd26e061fe5d4b229581c912b0204c0dee4b3bebf82e72940895591bf74f09: [9] System error: operation not permitted.

It seems still an issue mentioned like this one #18430 (although the old one is about pid namespace)

return warnings, fmt.Errorf("Cannot share network namespaces with user namespaces enabled.")
}
if hostConfig.PidMode.IsHost() {
return warnings, fmt.Errorf("Cannot use the host PID namespace with user namespaces enabled.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest replace use with share, as keep consistent above network namespace error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@estesp
Copy link
Contributor Author

estesp commented Jan 11, 2016

And another issue:

When run with docker run --read-only -v /icanwrite busybox sh still have long go trace message like below:

I have added an early error similar to the namespace sharing errors when using --read-only=true. The docs mention this is not possible with user namespaces, but adding the error makes it much clearer to the user. Thanks.

@estesp
Copy link
Contributor Author

estesp commented Jan 11, 2016

@thaJeztah I have made the modifications you suggested as well as added the additional info to the docker-daemon man page. Please let me know if the man page additions are sufficient of if the more complete detail from the markdown docs should be added here as well.

@HackToday I have made corrections to the docs per your suggestions and added an error message when --read-only is in use (to prevent the long complicated Go dump output). Your other issues (with shared ipc/uts) seem to be related to something your specific container is doing at startup? If I run a simple "busybox" or "ubuntu" image, I don't get these errors. I can look into it further, but maybe you can try with a standard image and then understand what your "myhtop" container is trying to do

## Daemon user namespace options

Linux kernel [user namespace support](http://man7.org/linux/man-pages/man7/user_namespaces.7.html) provides additional security by enabling
a process--and therefore a container--to have a unique range of user and
Copy link
Contributor

Choose a reason for hiding this comment

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

single dashes if you must use dashes.

- and therefore a container - 

@jamtur01
Copy link
Contributor

Nice Docs - minor edits.

@estesp
Copy link
Contributor Author

estesp commented Jan 12, 2016

Thanks for the docs review @jamtur01. I have updated the PR per your comments.

@jamtur01
Copy link
Contributor

One minor fix but otherwise Docs LGTM. Thanks!

@estesp
Copy link
Contributor Author

estesp commented Jan 12, 2016

Great! @jamtur01 I've used "superuser" instead, and the PR is updated. Thanks!

This prevents strange errors and clarifies which namespace options are
incompatible with user namespaces (at this time).

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
Remove the experimental docs for user namespaces and add similar content
to the `docker daemon` command documentation.

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
@cyphar
Copy link
Contributor

cyphar commented Jan 12, 2016

I think that #19251 should be considered before we make this non-experimental (quite a few systems don't have the latest version of shadow).

@HackToday
Copy link
Contributor

LGTM

@cyphar
Copy link
Contributor

cyphar commented Jan 12, 2016

@HackToday "LGTM"s should only be used by Docker maintainers (they are used by other maintainers to figure out whether there is a consensus on a feature being merged). It's just confusing to have non-maintainers LGTM things.

@HackToday
Copy link
Contributor

@cyphar sure, Ignore LGTM, I will +1 about this

@HackToday
Copy link
Contributor

👍

@cpuguy83
Copy link
Member

ping @thaJeztah for docs signoff

@thaJeztah
Copy link
Member

docs LGTM, let's merge!

thaJeztah added a commit that referenced this pull request Jan 12, 2016
User namespaces: graduate from experimental
@thaJeztah thaJeztah merged commit c72be04 into moby:master Jan 12, 2016
@tiborvass
Copy link
Contributor

\o/

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.

None yet