From ff42ff9f066b81ddf7c5f3e81116a4f69d5346b2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 15 Aug 2025 23:39:39 +0200 Subject: [PATCH 1/2] cli/flags: use a regular StringArray for the `--host` / `-H` flag The ClientOptions struct and related flags were inherited from the Moby repository, where originally the CLI and Daemon used the same implementation and had a "Common" options struct. When the CLI moved to a separate repository, those structs were duplicated, but some daemon-specific logic remained. For example, the daemon can be configured to listen on multiple ports and sockets ([moby@dede158]), but the CLI [can only connect to a single host][1]. The daemon config also had to account for flags conflicting with `daemon.json`, and use special flag-vars for this ([moby@677a6b3]). Unfortunately, the `ClientConfig` struct became part of the public API and is used as argument in various places, but we can remove the use of the special flag var. This patch replaces the use of `NewNamedListOptsRef` for a regular `StringArray`. Unfortunately this changes the flag's type description from `list` to `stringArray`, but we can look at changing that separately. [moby@dede158]: https://github.com/moby/moby/commit/dede1585ee00f957e153691c464aab293c2dc469 [1]: https://github.com/moby/moby/blob/0af135e9065562e14a77439e13a29b4f1eb627a0/docker/docker.go#L191-L193 [moby@677a6b3]: https://github.com/moby/moby/commit/677a6b3506107468ed8c00331991afd9176fa0b9 Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 5ee2906e784da604d0df340f9ba41a921a01f2f1) Signed-off-by: Sebastiaan van Stijn --- cli/flags/options.go | 7 +++---- docs/reference/commandline/docker.md | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/cli/flags/options.go b/cli/flags/options.go index 601c9c8eb7c6..f24992f5bc9d 100644 --- a/cli/flags/options.go +++ b/cli/flags/options.go @@ -6,7 +6,6 @@ import ( "path/filepath" "github.com/docker/cli/cli/config" - "github.com/docker/cli/opts" "github.com/docker/docker/client" "github.com/docker/go-connections/tlsconfig" "github.com/sirupsen/logrus" @@ -94,9 +93,9 @@ func (o *ClientOptions) InstallFlags(flags *pflag.FlagSet) { flags.Var("edString{&tlsOptions.CertFile}, "tlscert", "Path to TLS certificate file") flags.Var("edString{&tlsOptions.KeyFile}, "tlskey", "Path to TLS key file") - // opts.ValidateHost is not used here, so as to allow connection helpers - hostOpt := opts.NewNamedListOptsRef("hosts", &o.Hosts, nil) - flags.VarP(hostOpt, "host", "H", "Daemon socket to connect to") + // TODO(thaJeztah): show the default host. + // TODO(thaJeztah): this should be a string, not an "array" as we only allow a single host. + flags.StringArrayVarP(&o.Hosts, "host", "H", nil, "Daemon socket to connect to") flags.StringVarP(&o.Context, "context", "c", "", `Name of the context to use to connect to the daemon (overrides `+client.EnvOverrideHost+` env var and default context set with "docker context use")`) } diff --git a/docs/reference/commandline/docker.md b/docs/reference/commandline/docker.md index 69b1c91303be..00ab5cce62ab 100644 --- a/docs/reference/commandline/docker.md +++ b/docs/reference/commandline/docker.md @@ -69,18 +69,18 @@ The base command for the Docker CLI. ### Options -| Name | Type | Default | Description | -|:---------------------------------|:---------|:-------------------------|:--------------------------------------------------------------------------------------------------------------------------------------| -| `--config` | `string` | `/root/.docker` | Location of client config files | -| `-c`, `--context` | `string` | | Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with `docker context use`) | -| `-D`, `--debug` | `bool` | | Enable debug mode | -| [`-H`](#host), [`--host`](#host) | `list` | | Daemon socket to connect to | -| `-l`, `--log-level` | `string` | `info` | Set the logging level (`debug`, `info`, `warn`, `error`, `fatal`) | -| `--tls` | `bool` | | Use TLS; implied by --tlsverify | -| `--tlscacert` | `string` | `/root/.docker/ca.pem` | Trust certs signed only by this CA | -| `--tlscert` | `string` | `/root/.docker/cert.pem` | Path to TLS certificate file | -| `--tlskey` | `string` | `/root/.docker/key.pem` | Path to TLS key file | -| `--tlsverify` | `bool` | | Use TLS and verify the remote | +| Name | Type | Default | Description | +|:---------------------------------|:--------------|:-------------------------|:--------------------------------------------------------------------------------------------------------------------------------------| +| `--config` | `string` | `/root/.docker` | Location of client config files | +| `-c`, `--context` | `string` | | Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with `docker context use`) | +| `-D`, `--debug` | `bool` | | Enable debug mode | +| [`-H`](#host), [`--host`](#host) | `stringArray` | | Daemon socket to connect to | +| `-l`, `--log-level` | `string` | `info` | Set the logging level (`debug`, `info`, `warn`, `error`, `fatal`) | +| `--tls` | `bool` | | Use TLS; implied by --tlsverify | +| `--tlscacert` | `string` | `/root/.docker/ca.pem` | Trust certs signed only by this CA | +| `--tlscert` | `string` | `/root/.docker/cert.pem` | Path to TLS certificate file | +| `--tlskey` | `string` | `/root/.docker/key.pem` | Path to TLS key file | +| `--tlsverify` | `bool` | | Use TLS and verify the remote | From 7091e8bea4172cefb27e863c484d98205d34cedc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 16 Aug 2025 19:10:58 +0200 Subject: [PATCH 2/2] cli/flags: add "hostVar" to handle --host / -H as a single string hostVar is used for the '--host' / '-H' flag to set [ClientOptions.Hosts]. The [ClientOptions.Hosts] field is a slice because it was originally shared with the daemon config. However, the CLI only allows for a single host to be specified. hostVar presents itself as a "string", but stores the value in a string slice. It produces an error when trying to set multiple values, matching the check in [getServerHost]. [getServerHost]: https://github.com/docker/cli/blob/7eab668982645def1cd46fe1b60894cba6fd17a4/cli/command/cli.go#L542-L551 Signed-off-by: Sebastiaan van Stijn (cherry picked from commit f14eeeb361acbdf8c30cd3c57c927b5a7a724c99) Signed-off-by: Sebastiaan van Stijn --- cli/flags/options.go | 36 +++++++++++++++++++++++++++- docs/reference/commandline/docker.md | 24 +++++++++---------- e2e/cli-plugins/flags_test.go | 5 ++-- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/cli/flags/options.go b/cli/flags/options.go index f24992f5bc9d..309cb4616fa3 100644 --- a/cli/flags/options.go +++ b/cli/flags/options.go @@ -1,6 +1,7 @@ package flags import ( + "errors" "fmt" "os" "path/filepath" @@ -53,6 +54,39 @@ var ( dockerTLS = os.Getenv(EnvEnableTLS) != "" ) +// hostVar is used for the '--host' / '-H' flag to set [ClientOptions.Hosts]. +// The [ClientOptions.Hosts] field is a slice because it was originally shared +// with the daemon config. However, the CLI only allows for a single host to +// be specified. +// +// hostVar presents itself as a "string", but stores the value in a string +// slice. It produces an error when trying to set multiple values, matching +// the check in [getServerHost]. +// +// [getServerHost]: https://github.com/docker/cli/blob/7eab668982645def1cd46fe1b60894cba6fd17a4/cli/command/cli.go#L542-L551 +type hostVar struct { + dst *[]string + set bool +} + +func (h *hostVar) String() string { + if h.dst == nil || len(*h.dst) == 0 { + return "" + } + return (*h.dst)[0] +} + +func (h *hostVar) Set(s string) error { + if h.set { + return errors.New("specify only one -H") + } + *h.dst = []string{s} + h.set = true + return nil +} + +func (*hostVar) Type() string { return "string" } + // ClientOptions are the options used to configure the client cli. type ClientOptions struct { Debug bool @@ -95,7 +129,7 @@ func (o *ClientOptions) InstallFlags(flags *pflag.FlagSet) { // TODO(thaJeztah): show the default host. // TODO(thaJeztah): this should be a string, not an "array" as we only allow a single host. - flags.StringArrayVarP(&o.Hosts, "host", "H", nil, "Daemon socket to connect to") + flags.VarP(&hostVar{dst: &o.Hosts}, "host", "H", "Daemon socket to connect to") flags.StringVarP(&o.Context, "context", "c", "", `Name of the context to use to connect to the daemon (overrides `+client.EnvOverrideHost+` env var and default context set with "docker context use")`) } diff --git a/docs/reference/commandline/docker.md b/docs/reference/commandline/docker.md index 00ab5cce62ab..03dbddc8cced 100644 --- a/docs/reference/commandline/docker.md +++ b/docs/reference/commandline/docker.md @@ -69,18 +69,18 @@ The base command for the Docker CLI. ### Options -| Name | Type | Default | Description | -|:---------------------------------|:--------------|:-------------------------|:--------------------------------------------------------------------------------------------------------------------------------------| -| `--config` | `string` | `/root/.docker` | Location of client config files | -| `-c`, `--context` | `string` | | Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with `docker context use`) | -| `-D`, `--debug` | `bool` | | Enable debug mode | -| [`-H`](#host), [`--host`](#host) | `stringArray` | | Daemon socket to connect to | -| `-l`, `--log-level` | `string` | `info` | Set the logging level (`debug`, `info`, `warn`, `error`, `fatal`) | -| `--tls` | `bool` | | Use TLS; implied by --tlsverify | -| `--tlscacert` | `string` | `/root/.docker/ca.pem` | Trust certs signed only by this CA | -| `--tlscert` | `string` | `/root/.docker/cert.pem` | Path to TLS certificate file | -| `--tlskey` | `string` | `/root/.docker/key.pem` | Path to TLS key file | -| `--tlsverify` | `bool` | | Use TLS and verify the remote | +| Name | Type | Default | Description | +|:---------------------------------|:---------|:-------------------------|:--------------------------------------------------------------------------------------------------------------------------------------| +| `--config` | `string` | `/root/.docker` | Location of client config files | +| `-c`, `--context` | `string` | | Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with `docker context use`) | +| `-D`, `--debug` | `bool` | | Enable debug mode | +| [`-H`](#host), [`--host`](#host) | `string` | | Daemon socket to connect to | +| `-l`, `--log-level` | `string` | `info` | Set the logging level (`debug`, `info`, `warn`, `error`, `fatal`) | +| `--tls` | `bool` | | Use TLS; implied by --tlsverify | +| `--tlscacert` | `string` | `/root/.docker/ca.pem` | Trust certs signed only by this CA | +| `--tlscert` | `string` | `/root/.docker/cert.pem` | Path to TLS certificate file | +| `--tlskey` | `string` | `/root/.docker/key.pem` | Path to TLS key file | +| `--tlsverify` | `bool` | | Use TLS and verify the remote | diff --git a/e2e/cli-plugins/flags_test.go b/e2e/cli-plugins/flags_test.go index 69fe6d6ad3b4..0cb24ba2488e 100644 --- a/e2e/cli-plugins/flags_test.go +++ b/e2e/cli-plugins/flags_test.go @@ -1,6 +1,7 @@ package cliplugins import ( + "fmt" "os" "testing" @@ -91,9 +92,9 @@ func TestGlobalArgsOnlyParsedOnce(t *testing.T) { // This is checking the precondition wrt -H mentioned in the function comment name: "fails-if-H-used-twice", args: []string{"-H", dh, "-H", dh, "version", "-f", "{{.Client.Version}}"}, - expectedExitCode: 1, + expectedExitCode: 125, expectedOut: icmd.None, - expectedErr: "Specify only one -H", + expectedErr: fmt.Sprintf(`invalid argument %q for "-H, --host" flag: specify only one -H`, dh), }, { name: "builtin",