Docs freshness updates to Docker run command docs#4144
Docs freshness updates to Docker run command docs#4144thaJeztah merged 1 commit intodocker:masterfrom
Conversation
|
Thank you for contributing! It appears your commit message is missing a DCO sign-off, We require all commit messages to have a There is no need to open a new pull request, but to fix this (and make CI pass), Unfortunately, it's not possible to do so through GitHub's web UI, so this needs You can find some instructions in the output of the DCO check (which can be found Steps to do so "roughly" come down to:
Sorry for the hassle (I wish GitHub would make this a bit easier to do), and let me know if you need help or more detailed instructions! |
2f2b297 to
6d66dda
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4144 +/- ##
==========================================
- Coverage 58.87% 58.86% -0.01%
==========================================
Files 570 572 +2
Lines 49558 49572 +14
==========================================
+ Hits 29178 29182 +4
- Misses 18616 18624 +8
- Partials 1764 1766 +2 |
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! Quite some improvements in here. While reading, I found some issues, and left some comments (some aren't new, but might be something we need to look into, which can be in a follow up)
Let me know if you want to go through some of them!
| filesystems). However, the `--privileged` flag will allow it to run: | ||
| This *doesn't* work, because by default, Docker drops most potentially dangerous kernel | ||
| capabilities, including `cap_sys_admin` (which is required to mount | ||
| filesystems). However, the `--privileged` flag allows it to run: |
There was a problem hiding this comment.
For a follow-up, we should make a pass at searching for --privileged examples. Using --privileged is a BIG hammer, and effectively disables all security that containers provide (making a privileged container effectively "not a container", or a container that "doesn't contain). For cases where we can we should document the minimum amount of (additional) privileges needed. (And have a canonical section on "privileged" with tons of
For this specific example, it looks like we need CAP_SYS_ADMIN, so the example can look like;
$ docker run -it --cap-add CAP_SYS_ADMIN ubuntu bash
root@50e3f57e16e6:/# mount -t tmpfs none /mnt
root@50e3f57e16e6:/# df -h
Filesystem Size Used Avail Use% Mounted on
none 1.9G 0 1.9G 0% /mntOr even (using alpine);
$ docker run -it --cap-add CAP_SYS_ADMIN alpine
/ # mount -t tmpfs none /mnt
/ # df -h
Filesystem Size Used Available Use% Mounted on
overlay 58.4G 27.8G 27.6G 50% /
tmpfs 64.0M 0 64.0M 0% /dev
shm 64.0M 0 64.0M 0% /dev/shm
/dev/vda1 58.4G 27.8G 27.6G 50% /etc/resolv.conf
/dev/vda1 58.4G 27.8G 27.6G 50% /etc/hostname
/dev/vda1 58.4G 27.8G 27.6G 50% /etc/hosts
tmpfs 64.0M 0 64.0M 0% /proc/kcore
tmpfs 64.0M 0 64.0M 0% /proc/keys
tmpfs 64.0M 0 64.0M 0% /proc/timer_list
tmpfs 3.8G 0 3.8G 0% /sys/firmware
none 3.8G 0 3.8G 0% /mntI should note that the existing example only shows the "essential" lines from the output (the mnt mount) for illustrational purposes.
- I guess we could make it
df -h | grep mnt(so that the output matches actual output) but we'd loose the headers :thinking_face: - Should we use a mount destination that more clearly stands out as "this is an example"? (
mkdir /example-mount && mount -t tmpfs none /example-mount)? Note that the existing example uses/mnt(which already exists), but other locations won't (so users would run into an error).
Some other bits (for follow-ups);
- Review the "Runtime privilege and Linux capabilities" to check if the list of capabilities is complete (recent kernel versions added some new capabilities).
- While we accept both
SYS_ADMINandCAP_SYS_ADMIN(as well as lowercase variants), I know that internally (at least daemon-side) we normalize toCAP_SOME_CAPABILITY(see https://github.com/moby/moby/blob/58c027ac8ba19a3fa339c65274ea6704ccbec977/oci/caps/utils.go#L56-L79), so I wonder if we should update examples (and the table) to use the same for consistency - Some examples use
ubuntu(ubuntu:latest); there's two issues there (possibly worth considering);- depending on the example,
ubuntumay not always be needed, and we could make the examples more lightweight by usingalpine(alpine'sapk add --no-cacheinstall is super fast as well, which makes it great to provide quick examples that need additional packages installed), and we should probably preferalpineoverbusybox(where we use it). - this might be a bit of a stretch, but
:latest(while it reduces maintenance on our side) rarely is "best practice". Now, this is a bit of a grey area; for quick "one-off" things (such asdocker run alpine echo hello), or other things one may do, I guess "it's fine" (don't want to be typing too much). On the other hand, building an image usingubuntu:latest(e.g.) is definitely not the way to go (latestmay be a new version of Ubuntu "tomorrow", and break your image!) so we need to find the right balance between that (show best practices where needed to make users familiar with them).
- depending on the example,
There was a problem hiding this comment.
@thaJeztah The existing text around this example says…
This doesn't work, because by default, Docker drops most potentially dangerous kernel
capabilities, includingCAP_SYS_ADMIN(which is required to mount
filesystems).
So is that also wrong? As the proposed example uses something the text explicitly says won't work.
And noted for all the follow up.
There was a problem hiding this comment.
@ChrisChinchilla the example is correct that it doesn't work. The issue here is a bit that we use --privileged to work around that, whereas (for this example) just adding the CAP_SYS_ADMIN capability would've worked.
--privileged is a bit of a big hammer (and should be avoided in most cases, as it removes all security that a container provides); granting CAP_SYS_ADMIN is just one thing it does (it grants "all capabilities", but also disables seccomp, selinux, apparmor, and grants access to some paths that are otherwise protected.
TL;DR it works, but should not be the recommended approach for this specific case
| filesystems). However, the `--privileged` flag will allow it to run: | ||
| This *doesn't* work, because by default, Docker drops most potentially dangerous kernel | ||
| capabilities, including `cap_sys_admin` (which is required to mount | ||
| filesystems). However, the `--privileged` flag allows it to run: |
There was a problem hiding this comment.
@ChrisChinchilla the example is correct that it doesn't work. The issue here is a bit that we use --privileged to work around that, whereas (for this example) just adding the CAP_SYS_ADMIN capability would've worked.
--privileged is a bit of a big hammer (and should be avoided in most cases, as it removes all security that a container provides); granting CAP_SYS_ADMIN is just one thing it does (it grants "all capabilities", but also disables seccomp, selinux, apparmor, and grants access to some paths that are otherwise protected.
TL;DR it works, but should not be the recommended approach for this specific case
thaJeztah
left a comment
There was a problem hiding this comment.
changes LGTM; could you squash the commits?
ac90042 to
cd6f2ee
Compare
|
@thaJeztah Done! |
|
oh! looks like the markdown needs to be regenerated; let me do so quickly |
Signed-off-by: Chris Chinchilla <chris.ward@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
cd6f2ee to
e693e7f
Compare
|
All green now 👍 |

This ended up being a lot more changes than expected, but is something of a refresh for consistency, style, understanding, and freshness.