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

portmapper: move userland-proxy lookup to daemon config #47000

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

thaJeztah
Copy link
Member

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)

- What I did

- How I did it

- How to verify it

- Description for the changelog

Improve validation of the "userland-proxy-path" daemon configuration. Validation now happens during daemon startup, instead of producing an error when starting a container with port-mapping.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah 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 thaJeztah added this to the 25.0.0 milestone Dec 29, 2023
@thaJeztah thaJeztah force-pushed the userland_proxy_validation branch from a0250da to 0358da0 Compare December 29, 2023 13:36
@thaJeztah thaJeztah self-assigned this Dec 29, 2023
@thaJeztah thaJeztah force-pushed the userland_proxy_validation branch 2 times, most recently from 363fab1 to a6951f7 Compare December 29, 2023 14:00
@thaJeztah thaJeztah force-pushed the userland_proxy_validation branch from a6951f7 to 0ec5365 Compare December 29, 2023 15:11
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 thaJeztah force-pushed the userland_proxy_validation branch from 0ec5365 to 4f9db65 Compare December 29, 2023 15:23
Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit abdaf7c into moby:master Jan 2, 2024
105 checks passed
@thaJeztah thaJeztah deleted the userland_proxy_validation branch January 2, 2024 09:55
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants