-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Prefer loading docker-init
from an appropriate "libexec" directory
#45198
Conversation
daemon/config/config_windows.go
Outdated
// LookupInitPath returns an absolute path to docker-init (unimplemented on Windows) | ||
func (conf *Config) LookupInitPath() (string, error) { | ||
return "", errors.New("docker-init is unimplemented on Windows") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*Config).LookupInitPath
is only referenced from build-constrained files so this stub should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, neat! I will update. 🙇
The `docker-init` binary is not intended to be a user-facing command, and as such it is more appropriate for it to be found in `/usr/libexec` (or similar) than in `PATH` (see the FHS, especially https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html and https://refspecs.linuxfoundation.org/FHS_2.3/fhs-2.3.html#USRLIBLIBRARIESFORPROGRAMMINGANDPA). This adjusts the logic for using that configuration option to take this into account and appropriately search for `docker-init` (or the user's configured alternative) in these directories before falling back to the existing `PATH` lookup behavior. This behavior _used_ to exist for the old `dockerinit` binary (of a similar name and used in a similar way but for an alternative purpose), but that behavior was removed in 4357ed4 when that older `dockerinit` was also removed. Most of this reasoning _also_ applies to `docker-proxy` (and various `containerd-xxx` binaries such as the shims), but this change does not affect those. It would be relatively straightforward to adapt `LookupInitPath` to be a more generic function such as `libexecLookupPath` or similar if we wanted to explore that. See https://github.com/docker/cli/blob/14482589df194a86b2ee07df643ba3277b40df7d/cli-plugins/manager/manager_unix.go for the related path list in the CLI which loads CLI plugins from a similar set of paths (with a similar rationale - plugin binaries are not typically intended to be run directly by users but rather invoked _via_ the CLI binary). Signed-off-by: Tianon Gravi <admwiggin@gmail.com>
bfbb10a
to
6caaa8c
Compare
Force push just removed the unnecessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Once we have this in, I guess we also need changes in;
Do any of the SELinux and/or apparmor profiles need updates for this? (changed location of the binary)? |
Agreed, the first two we should change for sure (although this change is intentionally backwards compatible, so none of those changes are necessarily urgent 🙇).
I don't think so, since they only confine the container, and this gets bind-mounted inside in the same location as before (and I believe the bind mount path is the one that gets applied in the enforcement, not the original path). There's no reference to "docker-init" in either profile right now (unless it's obfuscated in a regex or something). 👀 |
Ah, yes you're probably right; we don't run this binary on the host, so probably fine |
I was waiting for Cory to review as well, but I see I missed that he already did. Let's merge this one |
The
docker-init
binary is not intended to be a user-facing command, and as such it is more appropriate for it to be found in/usr/libexec
(or similar) than inPATH
(see the FHS, especially https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html and https://refspecs.linuxfoundation.org/FHS_2.3/fhs-2.3.html#USRLIBLIBRARIESFORPROGRAMMINGANDPA).This adjusts the logic for using that configuration option to take this into account and appropriately search for
docker-init
(or the user's configured alternative) in these directories before falling back to the existingPATH
lookup behavior.This behavior used to exist for the old
dockerinit
binary (of a similar name and used in a similar way but for an alternative purpose), but that behavior was removed in 4357ed4 when that olderdockerinit
was also removed.Most of this reasoning also applies to
docker-proxy
(and variouscontainerd-xxx
binaries such as the shims), but this change does not affect those. It would be relatively straightforward to adaptLookupInitPath
to be a more generic function such aslibexecLookupPath
or similar if we wanted to explore that.See https://github.com/docker/cli/blob/14482589df194a86b2ee07df643ba3277b40df7d/cli-plugins/manager/manager_unix.go for the related path list in the CLI which loads CLI plugins from a similar set of paths (with a similar rationale - plugin binaries are not typically intended to be run directly by users but rather invoked via the CLI binary).
Refs #19490, #2217, docker/cli#1564