beenull/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

portmapper: move userland-proxy lookup to daemon config

Browse source 
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>
This commit is contained in:
Sebastiaan van Stijn 2023-12-29 13:52:45 +01:00
parent 488f2bd141
commit 4f9db655ed
No known key found for this signature in database
GPG key ID: 76698F39D527CE8C
4 changed files with 68 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
daemon/config/config_linux.go
View file

@ -1,12 +1,14 @@
package config // import "github.com/docker/docker/daemon/config"
import (
"context"
"fmt"
"net"
"os/exec"
"path/filepath"
"github.com/containerd/cgroups/v3"
"github.com/containerd/log"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/system"
"github.com/docker/docker/libnetwork/drivers/bridge"
@ -33,6 +35,10 @@ const (
// minAPIVersion represents Minimum REST API version supported
minAPIVersion = "1.12"
// userlandProxyBinary is the name of the userland-proxy binary.
// In rootless-mode, [rootless.RootlessKitDockerProxyBinary] is used instead.
userlandProxyBinary = "docker-proxy"
)
// BridgeConfig stores all the parameters for both the bridge driver and the default bridge network.
@ -175,6 +181,21 @@ func (conf *Config) ValidatePlatformConfig() error {
if conf.OOMScoreAdjust != 0 {
return errors.New(`DEPRECATED: The "oom-score-adjust" config parameter and the dockerd "--oom-score-adjust" options have been removed.`)
}
if conf.EnableUserlandProxy {
if conf.UserlandProxyPath == "" {
return errors.New("invalid userland-proxy-path: userland-proxy is enabled, but userland-proxy-path is not set")
}
if !filepath.IsAbs(conf.UserlandProxyPath) {
return errors.New("invalid userland-proxy-path: must be an absolute path: " + conf.UserlandProxyPath)
}
// Using exec.LookPath here, because it also produces an error if the
// given path is not a valid executable or a directory.
if _, err := exec.LookPath(conf.UserlandProxyPath); err != nil {
return errors.Wrap(err, "invalid userland-proxy-path")
}
}
if err := verifyDefaultIpcMode(conf.IpcMode); err != nil {
return err
}
@ -227,6 +248,17 @@ func setPlatformDefaults(cfg *Config) error {
cfg.ExecRoot = filepath.Join(runtimeDir, "docker")
cfg.Pidfile = filepath.Join(runtimeDir, "docker.pid")
} else {
var err error
cfg.BridgeConfig.UserlandProxyPath, err = exec.LookPath(userlandProxyBinary)
if err != nil {
// Log a warning, but don't error here. This allows running a daemon
// with userland-proxy disabled (which does not require the binary
// to be present).
//
// An error is still produced by [Config.ValidatePlatformConfig] if
// userland-proxy is enabled in the configuration.
log.G(context.TODO()).WithError(err).Warn("failed to lookup default userland-proxy binary")
}
cfg.Root = "/var/lib/docker"
cfg.ExecRoot = "/var/run/docker"
cfg.Pidfile = "/var/run/docker.pid"

20
integration-cli/docker_cli_daemon_test.go
View file

@ -2469,6 +2469,7 @@ func (s *DockerDaemonSuite) TestDaemonRestartSaveContainerExitCode(c *testing.T)
func (s *DockerDaemonSuite) TestDaemonWithUserlandProxyPath(c *testing.T) {
testRequires(c, testEnv.IsLocalDaemon, DaemonIsLinux)
ctx := context.TODO()
dockerProxyPath, err := exec.LookPath("docker-proxy")
assert.NilError(c, err)
@ -2490,11 +2491,20 @@ func (s *DockerDaemonSuite) TestDaemonWithUserlandProxyPath(c *testing.T) {
assert.NilError(c, err, out)
// not exist
s.d.Restart(c, "--userland-proxy-path", "/does/not/exist")
out, err = s.d.Cmd("run", "-p", "5000:5000", "busybox:latest", "true")
assert.ErrorContains(c, err, "", out)
assert.Assert(c, strings.Contains(out, "driver failed programming external connectivity on endpoint"))
assert.Assert(c, strings.Contains(out, "/does/not/exist: no such file or directory"))
s.d.Stop(c)
err = s.d.StartWithError("--userland-proxy-path", "/does/not/exist")
assert.ErrorContains(c, err, "", "daemon should fail to start")
expected := "invalid userland-proxy-path"
ok, _ := s.d.ScanLogsT(ctx, c, testdaemon.ScanLogsMatchString(expected))
assert.Assert(c, ok, "logs did not contain: %s", expected)
// not an absolute path
s.d.Stop(c)
err = s.d.StartWithError("--userland-proxy-path", "docker-proxy")
assert.ErrorContains(c, err, "", "daemon should fail to start")
expected = "invalid userland-proxy-path: must be an absolute path: docker-proxy"
ok, _ = s.d.ScanLogsT(ctx, c, testdaemon.ScanLogsMatchString(expected))
assert.Assert(c, ok, "logs did not contain: %s", expected)
}
// Test case for #22471

12
libnetwork/drivers/bridge/bridge_linux_test.go
View file

@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"net"
"os/exec"
"regexp"
"strconv"
"testing"
@ -644,9 +645,18 @@ func testQueryEndpointInfo(t *testing.T, ulPxyEnabled bool) {
d := newDriver()
d.portAllocator = portallocator.NewInstance()
var proxyBinary string
var err error
if ulPxyEnabled {
proxyBinary, err = exec.LookPath("docker-proxy")
if err != nil {
t.Fatalf("failed to lookup userland-proxy binary: %v", err)
}
}
config := &configuration{
EnableIPTables: true,
EnableUserlandProxy: ulPxyEnabled,
UserlandProxyPath: proxyBinary,
}
genericOption := make(map[string]interface{})
genericOption[netlabel.GenericData] = config
@ -663,7 +673,7 @@ func testQueryEndpointInfo(t *testing.T, ulPxyEnabled bool) {
genericOption[netlabel.GenericData] = netconfig
ipdList := getIPv4Data(t)
err := d.CreateNetwork("net1", genericOption, nil, ipdList, nil)
err = d.CreateNetwork("net1", genericOption, nil, ipdList, nil)
if err != nil {
t.Fatalf("Failed to create bridge: %v", err)
}

29
libnetwork/portmapper/proxy_linux.go
View file

@ -12,31 +12,22 @@ import (
"time"
)
const userlandProxyCommandName = "docker-proxy"
func newProxyCommand(proto string, hostIP net.IP, hostPort int, containerIP net.IP, containerPort int, proxyPath string) (userlandProxy, error) {
path := proxyPath
if proxyPath == "" {
cmd, err := exec.LookPath(userlandProxyCommandName)
if err != nil {
return nil, err
}
path = cmd
}
args := []string{
path,
"-proto", proto,
"-host-ip", hostIP.String(),
"-host-port", strconv.Itoa(hostPort),
"-container-ip", containerIP.String(),
"-container-port", strconv.Itoa(containerPort),
return nil, fmt.Errorf("no path provided for userland-proxy binary")
}
return &proxyCommand{
cmd: &exec.Cmd{
Path: path,
Args: args,
Path: proxyPath,
Args: []string{
proxyPath,
"-proto", proto,
"-host-ip", hostIP.String(),
"-host-port", strconv.Itoa(hostPort),
"-container-ip", containerIP.String(),
"-container-port", strconv.Itoa(containerPort),
},
SysProcAttr: &syscall.SysProcAttr{
Pdeathsig: syscall.SIGTERM, // send a sigterm to the proxy if the creating thread in the daemon process dies (https://go.dev/issue/27505)
},