cli/command: use longer init-ping timeout for ssh connections

cli/command/cli.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"runtime"
 	"strconv"
+	"strings"
 	"sync"
 	"time"
 
@@ -29,7 +30,36 @@ import (
 	"github.com/spf13/cobra"
 )
 
-const defaultInitTimeout = 2 * time.Second
+const (
+	// defaultInitTimeout is the default timeout for the initial ping to the
+	// daemon, which is used for API-version negotiation and to detect whether
+	// the daemon is running. It must be short, as we don't want the CLI to
+	// hang when connecting to a local daemon that is not running.
+	//
+	// See: https://github.com/docker/cli/issues/3652
+	defaultInitTimeout = 2 * time.Second
+
+	// sshInitTimeout is the timeout for the initial ping when connecting
+	// to a remote daemon over ssh ("ssh://" endpoint). Establishing an ssh
+	// connection requires a TCP connection, host key verification, and
+	// authentication before the connection to the daemon is established,
+	// which can take longer than defaultInitTimeout, in particular when
+	// connecting to a host for the first time.
+	//
+	// The ssh connection helper sets a default "ConnectTimeout=30" for the
+	// ssh process (see cli/connhelper), so we allow for the connection to
+	// be established within that time, plus some additional time for the
+	// handshake and the initial ping.
+	//
+	// Using the default (short) timeout for ssh connections could make the
+	// initial ping fail on slow connections, in which case API-version
+	// negotiation would be skipped, and the client would use its default
+	// (maximum) API version, potentially resulting in errors when making
+	// requests to a daemon with a lower API version.
+	//
+	// See: https://github.com/docker/cli/issues/6125
+	sshInitTimeout = 32 * time.Second
+)
 
 // Streams is an interface which exposes the standard input and output streams
 type Streams interface {
@@ -374,6 +404,16 @@ func (cli *DockerCli) getInitTimeout() time.Duration {
 	if cli.initTimeout != 0 {
 		return cli.initTimeout
 	}
+	// Establishing a connection to a remote daemon through the ssh
+	// connection helper can take longer than the default timeout, as it
+	// requires a TCP connection, host key verification, and authentication
+	// before the connection to the daemon is established. Use a longer
+	// timeout for the initial ping, so that API-version negotiation is
+	// not skipped on slow connections.
+	// See: https://github.com/docker/cli/issues/6125
+	if strings.HasPrefix(cli.dockerEndpoint.Host, "ssh://") {
+		return sshInitTimeout
+	}
 	return defaultInitTimeout
 }
 

cli/command/cli_test.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/cli/config/configfile"
+	"github.com/docker/cli/cli/context/docker"
 	"github.com/docker/cli/cli/context/store"
 	"github.com/docker/cli/cli/flags"
 	"github.com/moby/moby/client"
@@ -284,6 +285,133 @@ func TestInitializeFromClientHangs(t *testing.T) {
 	}
 }
 
+func TestGetInitTimeout(t *testing.T) {
+	testcases := []struct {
+		doc         string
+		host        string
+		initTimeout time.Duration
+		expected    time.Duration
+	}{
+		{
+			doc:      "default timeout for local socket",
+			host:     "unix:///var/run/docker.sock",
+			expected: defaultInitTimeout,
+		},
+		{
+			doc:      "default timeout for tcp",
+			host:     "tcp://127.0.0.1:2375",
+			expected: defaultInitTimeout,
+		},
+		{
+			doc:      "longer timeout for ssh",
+			host:     "ssh://user@remote",
+			expected: sshInitTimeout,
+		},
+		{
+			doc:         "custom timeout takes precedence for local socket",
+			host:        "unix:///var/run/docker.sock",
+			initTimeout: 5 * time.Second,
+			expected:    5 * time.Second,
+		},
+		{
+			doc:         "custom timeout takes precedence for ssh",
+			host:        "ssh://user@remote",
+			initTimeout: 5 * time.Second,
+			expected:    5 * time.Second,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.doc, func(t *testing.T) {
+			cli := &DockerCli{
+				dockerEndpoint: docker.Endpoint{
+					EndpointMeta: docker.EndpointMeta{Host: tc.host},
+				},
+				initTimeout: tc.initTimeout,
+			}
+			assert.Equal(t, cli.getInitTimeout(), tc.expected)
+		})
+	}
+}
+
+// TestInitializeFromClientSlowConnection checks that API version negotiation
+// is not skipped when establishing a connection to the daemon takes longer
+// than the default init timeout. Connecting through the ssh connection helper
+// requires a TCP connection, host key verification, and authentication before
+// the connection to the daemon is established, which can take longer than the
+// default timeout, in particular when connecting to a host for the first time.
+//
+// See https://github.com/docker/cli/issues/6125
+func TestInitializeFromClientSlowConnection(t *testing.T) {
+	// dialDelay must be longer than defaultInitTimeout to simulate a slow
+	// connection (see the ssh sub-test), but is otherwise arbitrary.
+	const dialDelay = defaultInitTimeout + 500*time.Millisecond
+
+	// Simulate a daemon that responds to the initial ping, but is slow
+	// to connect to.
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Api-Version", "1.44")
+		w.Header().Set("Ostype", "linux")
+		w.Header().Set("Docker-Experimental", "true")
+		w.WriteHeader(http.StatusOK)
+	}))
+	t.Cleanup(ts.Close)
+	host := strings.Replace(ts.URL, "http://", "tcp://", 1)
+
+	newSlowAPIClient := func(t *testing.T) client.APIClient {
+		t.Helper()
+		apiClient, err := client.New(
+			client.WithHost(host),
+			client.WithHTTPClient(&http.Client{
+				Transport: &http.Transport{
+					// Simulate the delay of establishing a connection through
+					// the ssh connection helper before the connection to the
+					// daemon is ready to be used.
+					DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
+						select {
+						case <-time.After(dialDelay):
+						case <-ctx.Done():
+							return nil, ctx.Err()
+						}
+						return (&net.Dialer{}).DialContext(ctx, network, addr)
+					},
+				},
+				CheckRedirect: client.CheckRedirect,
+			}),
+		)
+		assert.NilError(t, err)
+		return apiClient
+	}
+
+	t.Run("ssh connection gets a longer init timeout", func(t *testing.T) {
+		apiClient := newSlowAPIClient(t)
+		cli := &DockerCli{client: apiClient}
+		err := cli.Initialize(&flags.ClientOptions{Hosts: []string{"ssh://user@remote"}})
+		assert.NilError(t, err)
+
+		// Trigger the initial ping (it is performed lazily on first use
+		// of the client). The initial ping completed within the (longer)
+		// init timeout for ssh connections, so the API version was
+		// negotiated (downgraded to the daemon's API version) and the
+		// server info is set.
+		assert.Equal(t, cli.CurrentVersion(), "1.44")
+		assert.DeepEqual(t, cli.ServerInfo(), ServerInfo{HasExperimental: true, OSType: "linux"})
+	})
+
+	t.Run("non-ssh connections keep the default init timeout", func(t *testing.T) {
+		apiClient := newSlowAPIClient(t)
+		cli := &DockerCli{client: apiClient}
+		err := cli.Initialize(&flags.ClientOptions{Hosts: []string{host}})
+		assert.NilError(t, err)
+
+		// The initial ping was aborted (the connection took longer than
+		// the default init timeout), so the API version was not
+		// negotiated, and the server info falls back to its defaults.
+		assert.Equal(t, cli.CurrentVersion(), client.MaxAPIVersion)
+		assert.DeepEqual(t, cli.ServerInfo(), ServerInfo{HasExperimental: true})
+	})
+}
+
 func TestNewDockerCliAndOperators(t *testing.T) {
 	outbuf := bytes.NewBuffer(nil)
 	errbuf := bytes.NewBuffer(nil)