[25.0 backport] client: fix connection-errors being shadowed by API version errors

client/client.go

@@ -265,17 +265,22 @@ func (cli *Client) Close() error {
 // This allows for version-dependent code to use the same version as will
 // be negotiated when making the actual requests, and for which cases
 // we cannot do the negotiation lazily.
-func (cli *Client) checkVersion(ctx context.Context) {
-	if cli.negotiateVersion && !cli.negotiated {
-		cli.NegotiateAPIVersion(ctx)
+func (cli *Client) checkVersion(ctx context.Context) error {
+	if !cli.manualOverride && cli.negotiateVersion && !cli.negotiated {
+		ping, err := cli.Ping(ctx)
+		if err != nil {
+			return err
+		}
+		cli.negotiateAPIVersionPing(ping)
 	}
+	return nil
 }
 
 // getAPIPath returns the versioned request path to call the API.
 // It appends the query parameters to the path if they are not empty.
 func (cli *Client) getAPIPath(ctx context.Context, p string, query url.Values) string {
 	var apiPath string
-	cli.checkVersion(ctx)
+	_ = cli.checkVersion(ctx)
 	if cli.version != "" {
 		v := strings.TrimPrefix(cli.version, "v")
 		apiPath = path.Join(cli.basePath, "/v"+v, p)
@@ -307,7 +312,11 @@ func (cli *Client) ClientVersion() string {
 // added (1.24).
 func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
 	if !cli.manualOverride {
-		ping, _ := cli.Ping(ctx)
+		ping, err := cli.Ping(ctx)
+		if err != nil {
+			// FIXME(thaJeztah): Ping returns an error when failing to connect to the API; we should not swallow the error here, and instead returning it.
+			return
+		}
 		cli.negotiateAPIVersionPing(ping)
 	}
 }

client/client_test.go

@@ -354,6 +354,19 @@ func TestNegotiateAPVersionOverride(t *testing.T) {
 	assert.Equal(t, client.ClientVersion(), expected)
 }
 
+// TestNegotiateAPVersionConnectionFailure asserts that we do not modify the
+// API version when failing to connect.
+func TestNegotiateAPVersionConnectionFailure(t *testing.T) {
+	const expected = "9.99"
+
+	client, err := NewClientWithOpts(WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	client.version = expected
+	client.NegotiateAPIVersion(context.Background())
+	assert.Equal(t, client.ClientVersion(), expected)
+}
+
 func TestNegotiateAPIVersionAutomatic(t *testing.T) {
 	var pingVersion string
 	httpClient := newMockClient(func(req *http.Request) (*http.Response, error) {

client/container_create.go

@@ -28,7 +28,9 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config
 	//
 	// Normally, version-negotiation (if enabled) would not happen until
 	// the API request is made.
-	cli.checkVersion(ctx)
+	if err := cli.checkVersion(ctx); err != nil {
+		return response, err
+	}
 
 	if err := cli.NewVersionError(ctx, "1.25", "stop timeout"); config != nil && config.StopTimeout != nil && err != nil {
 		return response, err

client/container_create_test.go

@@ -113,3 +113,15 @@ func TestContainerCreateAutoRemove(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// TestContainerCreateConnection verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestContainerCreateConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	_, err = client.ContainerCreate(context.Background(), nil, nil, nil, nil, "")
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}

client/container_exec.go

@@ -18,7 +18,9 @@ func (cli *Client) ContainerExecCreate(ctx context.Context, container string, co
 	//
 	// Normally, version-negotiation (if enabled) would not happen until
 	// the API request is made.
-	cli.checkVersion(ctx)
+	if err := cli.checkVersion(ctx); err != nil {
+		return response, err
+	}
 
 	if err := cli.NewVersionError(ctx, "1.25", "env"); len(config.Env) != 0 && err != nil {
 		return response, err

client/container_exec_test.go

@@ -24,6 +24,18 @@ func TestContainerExecCreateError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestContainerExecCreateConnectionError verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestContainerExecCreateConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	_, err = client.ContainerExecCreate(context.Background(), "", types.ExecConfig{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestContainerExecCreate(t *testing.T) {
 	expectedURL := "/containers/container_id/exec"
 	client := &Client{

client/container_restart.go

@@ -23,7 +23,9 @@ func (cli *Client) ContainerRestart(ctx context.Context, containerID string, opt
 		//
 		// Normally, version-negotiation (if enabled) would not happen until
 		// the API request is made.
-		cli.checkVersion(ctx)
+		if err := cli.checkVersion(ctx); err != nil {
+			return err
+		}
 		if versions.GreaterThanOrEqualTo(cli.version, "1.42") {
 			query.Set("signal", options.Signal)
 		}

client/container_restart_test.go

@@ -23,6 +23,18 @@ func TestContainerRestartError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestContainerRestartConnectionError verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestContainerRestartConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	err = client.ContainerRestart(context.Background(), "nothing", container.StopOptions{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestContainerRestart(t *testing.T) {
 	const expectedURL = "/v1.42/containers/container_id/restart"
 	client := &Client{

client/container_stop.go

@@ -27,7 +27,9 @@ func (cli *Client) ContainerStop(ctx context.Context, containerID string, option
 		//
 		// Normally, version-negotiation (if enabled) would not happen until
 		// the API request is made.
-		cli.checkVersion(ctx)
+		if err := cli.checkVersion(ctx); err != nil {
+			return err
+		}
 		if versions.GreaterThanOrEqualTo(cli.version, "1.42") {
 			query.Set("signal", options.Signal)
 		}

client/container_stop_test.go

@@ -23,6 +23,18 @@ func TestContainerStopError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestContainerStopConnectionError verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestContainerStopConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	err = client.ContainerStop(context.Background(), "nothing", container.StopOptions{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestContainerStop(t *testing.T) {
 	const expectedURL = "/v1.42/containers/container_id/stop"
 	client := &Client{

client/container_wait.go

@@ -30,19 +30,22 @@ const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
 // synchronize ContainerWait with other calls, such as specifying a
 // "next-exit" condition before issuing a ContainerStart request.
 func (cli *Client) ContainerWait(ctx context.Context, containerID string, condition container.WaitCondition) (<-chan container.WaitResponse, <-chan error) {
+	resultC := make(chan container.WaitResponse)
+	errC := make(chan error, 1)
+
 	// Make sure we negotiated (if the client is configured to do so),
 	// as code below contains API-version specific handling of options.
 	//
 	// Normally, version-negotiation (if enabled) would not happen until
 	// the API request is made.
-	cli.checkVersion(ctx)
+	if err := cli.checkVersion(ctx); err != nil {
+		errC <- err
+		return resultC, errC
+	}
 	if versions.LessThan(cli.ClientVersion(), "1.30") {
 		return cli.legacyContainerWait(ctx, containerID)
 	}
 
-	resultC := make(chan container.WaitResponse)
-	errC := make(chan error, 1)
-
 	query := url.Values{}
 	if condition != "" {
 		query.Set("condition", string(condition))

client/container_wait_test.go

@@ -34,6 +34,23 @@ func TestContainerWaitError(t *testing.T) {
 	}
 }
 
+// TestContainerWaitConnectionError verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestContainerWaitConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	resultC, errC := client.ContainerWait(context.Background(), "nothing", "")
+	select {
+	case result := <-resultC:
+		t.Fatalf("expected to not get a wait result, got %d", result.StatusCode)
+	case err := <-errC:
+		assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+	}
+}
+
 func TestContainerWait(t *testing.T) {
 	expectedURL := "/containers/container_id/wait"
 	client := &Client{

client/errors.go

@@ -11,15 +11,16 @@ import (
 
 // errConnectionFailed implements an error returned when connection failed.
 type errConnectionFailed struct {
-	host string
+	error
 }
 
 // Error returns a string representation of an errConnectionFailed
-func (err errConnectionFailed) Error() string {
-	if err.host == "" {
-		return "Cannot connect to the Docker daemon. Is the docker daemon running on this host?"
-	}
-	return fmt.Sprintf("Cannot connect to the Docker daemon at %s. Is the docker daemon running?", err.host)
+func (e errConnectionFailed) Error() string {
+	return e.error.Error()
+}
+
+func (e errConnectionFailed) Unwrap() error {
+	return e.error
 }
 
 // IsErrConnectionFailed returns true if the error is caused by connection failed.
@@ -29,7 +30,13 @@ func IsErrConnectionFailed(err error) bool {
 
 // ErrorConnectionFailed returns an error with host in the error message when connection to docker daemon failed.
 func ErrorConnectionFailed(host string) error {
-	return errConnectionFailed{host: host}
+	var err error
+	if host == "" {
+		err = fmt.Errorf("Cannot connect to the Docker daemon. Is the docker daemon running on this host?")
+	} else {
+		err = fmt.Errorf("Cannot connect to the Docker daemon at %s. Is the docker daemon running?", host)
+	}
+	return errConnectionFailed{error: err}
 }
 
 // IsErrNotFound returns true if the error is a NotFound error, which is returned
@@ -60,7 +67,9 @@ func (cli *Client) NewVersionError(ctx context.Context, APIrequired, feature str
 	//
 	// Normally, version-negotiation (if enabled) would not happen until
 	// the API request is made.
-	cli.checkVersion(ctx)
+	if err := cli.checkVersion(ctx); err != nil {
+		return err
+	}
 	if cli.version != "" && versions.LessThan(cli.version, APIrequired) {
 		return fmt.Errorf("%q requires API version %s, but the Docker daemon API version is %s", feature, APIrequired, cli.version)
 	}

client/image_list.go

@@ -13,14 +13,17 @@ import (
 
 // ImageList returns a list of images in the docker host.
 func (cli *Client) ImageList(ctx context.Context, options types.ImageListOptions) ([]image.Summary, error) {
+	var images []image.Summary
+
 	// Make sure we negotiated (if the client is configured to do so),
 	// as code below contains API-version specific handling of options.
 	//
 	// Normally, version-negotiation (if enabled) would not happen until
 	// the API request is made.
-	cli.checkVersion(ctx)
+	if err := cli.checkVersion(ctx); err != nil {
+		return images, err
+	}
 
-	var images []image.Summary
 	query := url.Values{}
 
 	optionFilters := options.Filters

client/image_list_test.go

@@ -28,6 +28,18 @@ func TestImageListError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestImageListConnectionError verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestImageListConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	_, err = client.ImageList(context.Background(), types.ImageListOptions{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestImageList(t *testing.T) {
 	const expectedURL = "/images/json"
 

client/network_create.go

@@ -10,12 +10,16 @@ import (
 
 // NetworkCreate creates a new network in the docker host.
 func (cli *Client) NetworkCreate(ctx context.Context, name string, options types.NetworkCreate) (types.NetworkCreateResponse, error) {
+	var response types.NetworkCreateResponse
+
 	// Make sure we negotiated (if the client is configured to do so),
 	// as code below contains API-version specific handling of options.
 	//
 	// Normally, version-negotiation (if enabled) would not happen until
 	// the API request is made.
-	cli.checkVersion(ctx)
+	if err := cli.checkVersion(ctx); err != nil {
+		return response, err
+	}
 
 	networkCreateRequest := types.NetworkCreateRequest{
 		NetworkCreate: options,
@@ -25,7 +29,6 @@ func (cli *Client) NetworkCreate(ctx context.Context, name string, options types
 		networkCreateRequest.CheckDuplicate = true //nolint:staticcheck // ignore SA1019: CheckDuplicate is deprecated since API v1.44.
 	}
 
-	var response types.NetworkCreateResponse
 	serverResp, err := cli.post(ctx, "/networks/create", nil, networkCreateRequest, nil)
 	defer ensureReaderClosed(serverResp)
 	if err != nil {

client/network_create_test.go

@@ -25,6 +25,18 @@ func TestNetworkCreateError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestNetworkCreateConnectionError verifies that connection errors occurring
+// during API-version negotiation are not shadowed by API-version errors.
+//
+// Regression test for https://github.com/docker/cli/issues/4890
+func TestNetworkCreateConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	_, err = client.NetworkCreate(context.Background(), "mynetwork", types.NetworkCreate{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestNetworkCreate(t *testing.T) {
 	expectedURL := "/networks/create"
 

client/ping.go

@@ -14,7 +14,10 @@ import (
 // Ping pings the server and returns the value of the "Docker-Experimental",
 // "Builder-Version", "OS-Type" & "API-Version" headers. It attempts to use
 // a HEAD request on the endpoint, but falls back to GET if HEAD is not supported
-// by the daemon.
+// by the daemon. It ignores internal server errors returned by the API, which
+// may be returned if the daemon is in an unhealthy state, but returns errors
+// for other non-success status codes, failing to connect to the API, or failing
+// to parse the API response.
 func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
 	var ping types.Ping
 

client/ping_test.go

@@ -53,18 +53,12 @@ func TestPingFail(t *testing.T) {
 func TestPingWithError(t *testing.T) {
 	client := &Client{
 		client: newMockClient(func(req *http.Request) (*http.Response, error) {
-			resp := &http.Response{StatusCode: http.StatusInternalServerError}
-			resp.Header = http.Header{}
-			resp.Header.Set("API-Version", "awesome")
-			resp.Header.Set("Docker-Experimental", "true")
-			resp.Header.Set("Swarm", "active/manager")
-			resp.Body = io.NopCloser(strings.NewReader("some error with the server"))
-			return resp, errors.New("some error")
+			return nil, errors.New("some connection error")
 		}),
 	}
 
 	ping, err := client.Ping(context.Background())
-	assert.Check(t, is.ErrorContains(err, "some error"))
+	assert.Check(t, is.ErrorContains(err, "some connection error"))
 	assert.Check(t, is.Equal(false, ping.Experimental))
 	assert.Check(t, is.Equal("", ping.APIVersion))
 	var si *swarm.Status