client: fix connection-errors being shadowed by API version mismatch errors

thaJeztah · thaJeztah · commit 6aea26b431ea · 2024-02-23T15:17:10.000+01:00
Commit e690724 applied a fix for situations where the client was configured with API-version negotiation, but did not yet negotiate a version. However, the checkVersion() function that was implemented copied the semantics of cli.NegotiateAPIVersion, which ignored connection failures with the assumption that connection errors would still surface further down. However, when using the result of a failed negotiation for NewVersionError, an API version mismatch error would be produced, masking the actual connection error. This patch changes the signature of checkVersion to return unexpected errors, including failures to connect to the API. Before this patch: docker -H unix:///no/such/socket.sock secret ls "secret list" requires API version 1.25, but the Docker daemon API version is 1.24 With this patch applied: docker -H unix:///no/such/socket.sock secret ls Cannot connect to the Docker daemon at unix:///no/such/socket.sock. Is the docker daemon running? Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
diff --git a/client/client.go b/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)
diff --git a/client/client_test.go b/client/client_test.go
@@ -359,7 +359,7 @@ func TestNegotiateAPVersionOverride(t *testing.T) {
 func TestNegotiateAPVersionConnectionFailure(t *testing.T) {
 	const expected = "9.99"
 
-	client, err := NewClientWithOpts(WithHost("unix:///no-such-socket"))
+	client, err := NewClientWithOpts(WithHost("tcp://no-such-host.invalid"))
 	assert.NilError(t, err)
 
 	client.version = expected
diff --git a/client/container_create.go b/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
diff --git a/client/container_create_test.go b/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))
+}
diff --git a/client/container_exec.go b/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
diff --git a/client/container_exec_test.go b/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{
diff --git a/client/container_restart.go b/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)
 		}
diff --git a/client/container_restart_test.go b/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{
diff --git a/client/container_stop.go b/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)
 		}
diff --git a/client/container_stop_test.go b/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{
diff --git a/client/container_wait.go b/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))
diff --git a/client/container_wait_test.go b/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{
diff --git a/client/errors.go b/client/errors.go
@@ -67,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)
 	}
diff --git a/client/image_list.go b/client/image_list.go
@@ -12,14 +12,17 @@ import (
 
 // ImageList returns a list of images in the docker host.
 func (cli *Client) ImageList(ctx context.Context, options image.ListOptions) ([]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
diff --git a/client/image_list_test.go b/client/image_list_test.go
@@ -27,6 +27,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(), image.ListOptions{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestImageList(t *testing.T) {
 	const expectedURL = "/images/json"
 
diff --git a/client/network_create.go b/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 {
diff --git a/client/network_create_test.go b/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"
 
diff --git a/client/service_create.go b/client/service_create.go
@@ -25,7 +25,9 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec,
 	//
 	// 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
+	}
 
 	// Make sure containerSpec is not nil when no runtime is set or the runtime is set to container
 	if service.TaskTemplate.ContainerSpec == nil && (service.TaskTemplate.Runtime == "" || service.TaskTemplate.Runtime == swarm.RuntimeContainer) {
diff --git a/client/service_create_test.go b/client/service_create_test.go
@@ -28,6 +28,18 @@ func TestServiceCreateError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestServiceCreateConnectionError 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 TestServiceCreateConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	_, err = client.ServiceCreate(context.Background(), swarm.ServiceSpec{}, types.ServiceCreateOptions{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestServiceCreate(t *testing.T) {
 	expectedURL := "/services/create"
 	client := &Client{
diff --git a/client/service_update.go b/client/service_update.go
@@ -16,18 +16,18 @@ import (
 // It should be the value as set *before* the update. You can find this value in the Meta field
 // of swarm.Service, which can be found using ServiceInspectWithRaw.
 func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (swarm.ServiceUpdateResponse, error) {
+	response := swarm.ServiceUpdateResponse{}
+
 	// 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)
-
-	var (
-		query    = url.Values{}
-		response = swarm.ServiceUpdateResponse{}
-	)
+	if err := cli.checkVersion(ctx); err != nil {
+		return response, err
+	}
 
+	query := url.Values{}
 	if options.RegistryAuthFrom != "" {
 		query.Set("registryAuthFrom", options.RegistryAuthFrom)
 	}
diff --git a/client/service_update_test.go b/client/service_update_test.go
@@ -25,6 +25,18 @@ func TestServiceUpdateError(t *testing.T) {
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
 }
 
+// TestServiceUpdateConnectionError 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 TestServiceUpdateConnectionError(t *testing.T) {
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
+	assert.NilError(t, err)
+
+	_, err = client.ServiceUpdate(context.Background(), "service_id", swarm.Version{}, swarm.ServiceSpec{}, types.ServiceUpdateOptions{})
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
+}
+
 func TestServiceUpdate(t *testing.T) {
 	expectedURL := "/services/service_id/update"
 
diff --git a/client/volume_remove.go b/client/volume_remove.go
@@ -16,7 +16,9 @@ func (cli *Client) VolumeRemove(ctx context.Context, volumeID string, force bool
 		//
 		// 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.25") {
 			query.Set("force", "1")
 		}
diff --git a/client/volume_remove_test.go b/client/volume_remove_test.go
