-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
portmapper: move userland-proxy lookup to daemon config #47000
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thaJeztah
added
status/2-code-review
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.
area/networking
impact/changelog
area/daemon
area/networking/portmapping
labels
Dec 29, 2023
thaJeztah
force-pushed
the
userland_proxy_validation
branch
from
December 29, 2023 13:36
a0250da
to
0358da0
Compare
thaJeztah
force-pushed
the
userland_proxy_validation
branch
2 times, most recently
from
December 29, 2023 14:00
363fab1
to
a6951f7
Compare
thaJeztah
commented
Dec 29, 2023
thaJeztah
force-pushed
the
userland_proxy_validation
branch
from
December 29, 2023 15:11
a6951f7
to
0ec5365
Compare
When mapping a port with the userland-proxy enabled, the daemon would perform an "exec.LookPath" for every mapped port (which, in case of a range of ports, would be for every port in the range). This was both inefficient (looking up the binary for each port), inconsistent (when running in rootless-mode, the binary was looked-up once), as well as inconvenient, because a missing binary, or a mis-configureed userland-proxy-path would not be detected daeemon startup, and not produce an error until starting the container; docker run -d -P nginx:alpine 4f7b6589a1680f883d98d03db12203973387f9061e7a963331776170e4414194 docker: Error response from daemon: driver failed programming external connectivity on endpoint romantic_wiles (7cfdc361821f75cbc665564cf49856cf216a5b09046d3c22d5b9988836ee088d): fork/exec docker-proxy: no such file or directory. However, the container would still be created (but invalid); docker ps -a CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 869f41d7e94f nginx:alpine "/docker-entrypoint.…" 10 seconds ago Created romantic_wiles This patch changes how the userland-proxy is configured; - The path of the userland-proxy is now looked up / configured at daemon startup; this is similar to how the proxy is configured in rootless-mode. - A warning is logged when failing to lookup the binary. - If the daemon is configured with "userland-proxy" enabled, an error is produced, and the daemon will refuse to start. - The "proxyPath" argument for newProxyCommand() (in libnetwork/portmapper) is now required to be set. It no longer looks up the executable, and produces an error if no path was provided. While this change was not required, it makes the daemon config the canonical source of truth, instead of logic spread accross multiplee locations. Some of this logic is a change of behavior, but these changes were made with the assumption that we don't want to support; - installing the userland proxy _after_ the daemon was started - moving the userland proxy (or installing a proxy with a higher preference in PATH) With this patch: Validating the config produces an error if the binary is not found: dockerd --validate WARN[2023-12-29T11:36:39.748699591Z] failed to lookup default userland-proxy binary error="exec: \"docker-proxy\": executable file not found in $PATH" userland-proxy is enabled, but userland-proxy-path is not set Disabling userland-proxy prints a warning, but validates as "OK": dockerd --userland-proxy=false --validate WARN[2023-12-29T11:38:30.752523879Z] ffailed to lookup default userland-proxy binary error="exec: \"docker-proxy\": executable file not found in $PATH" configuration OK Speficying a non-absolute path produces an error: dockerd --userland-proxy-path=docker-proxy --validate invalid userland-proxy-path: must be an absolute path: docker-proxy Befor this patch, we would not validate this path, which would allow the daemon to start, but fail to map a port; docker run -d -P nginx:alpine 4f7b6589a1680f883d98d03db12203973387f9061e7a963331776170e4414194 docker: Error response from daemon: driver failed programming external connectivity on endpoint romantic_wiles (7cfdc361821f75cbc665564cf49856cf216a5b09046d3c22d5b9988836ee088d): fork/exec docker-proxy: no such file or directory. Specifying an invalid userland-proxy-path produces an error as well: dockerd --userland-proxy-path=/usr/local/bin/no-such-binary --validate userland-proxy-path is invalid: stat /usr/local/bin/no-such-binary: no such file or directory mkdir -p /usr/local/bin/not-a-file dockerd --userland-proxy-path=/usr/local/bin/not-a-file --validate userland-proxy-path is invalid: exec: "/usr/local/bin/not-a-file": is a directory touch /usr/local/bin/not-an-executable dockerd --userland-proxy-path=/usr/local/bin/not-an-executable --validate userland-proxy-path is invalid: exec: "/usr/local/bin/not-an-executable": permission denied Same when using the daemon.json config-file; echo '{"userland-proxy-path":"no-such-binary"}' > /etc/docker/daemon.json dockerd --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid userland-proxy-path: must be an absolute path: no-such-binary dockerd --userland-proxy-path=hello --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: userland-proxy-path: (from flag: hello, from file: /usr/local/bin/docker-proxy) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
userland_proxy_validation
branch
from
December 29, 2023 15:23
0ec5365
to
4f9db65
Compare
AkihiroSuda
approved these changes
Dec 29, 2023
vvoland
approved these changes
Jan 2, 2024
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
This was referenced Jan 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/daemon
area/networking/portmapping
area/networking
impact/changelog
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.
status/2-code-review
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When mapping a port with the userland-proxy enabled, the daemon would perform an "exec.LookPath" for every mapped port (which, in case of a range of ports, would be for every port in the range).
This was both inefficient (looking up the binary for each port), inconsistent (when running in rootless-mode, the binary was looked-up once), as well as inconvenient, because a missing binary, or a mis-configureed userland-proxy-path would not be detected daeemon startup, and not produce an error until starting the container;
However, the container would still be created (but invalid);
This patch changes how the userland-proxy is configured;
Some of this logic is a change of behavior, but these changes were made with the assumption that we don't want to support;
With this patch:
Validating the config produces an error if the binary is not found:
Disabling userland-proxy prints a warning, but validates as "OK":
Speficying a non-absolute path produces an error:
Befor this patch, we would not validate this path, which would allow the daemon to start, but fail to map a port;
Specifying an invalid userland-proxy-path produces an error as well:
Same when using the daemon.json config-file;
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)