Browse code

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

Commit e6907243af215a90fe36b377d89a49e3a2eded0a 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>

Sebastiaan van Stijn authored on 2024/02/23 20:20:06
Showing 23 changed files
... ...
@@ -265,17 +265,22 @@ func (cli *Client) Close() error {
265 265
 // This allows for version-dependent code to use the same version as will
266 266
 // be negotiated when making the actual requests, and for which cases
267 267
 // we cannot do the negotiation lazily.
268
-func (cli *Client) checkVersion(ctx context.Context) {
269
-	if cli.negotiateVersion && !cli.negotiated {
270
-		cli.NegotiateAPIVersion(ctx)
268
+func (cli *Client) checkVersion(ctx context.Context) error {
269
+	if !cli.manualOverride && cli.negotiateVersion && !cli.negotiated {
270
+		ping, err := cli.Ping(ctx)
271
+		if err != nil {
272
+			return err
273
+		}
274
+		cli.negotiateAPIVersionPing(ping)
271 275
 	}
276
+	return nil
272 277
 }
273 278
 
274 279
 // getAPIPath returns the versioned request path to call the API.
275 280
 // It appends the query parameters to the path if they are not empty.
276 281
 func (cli *Client) getAPIPath(ctx context.Context, p string, query url.Values) string {
277 282
 	var apiPath string
278
-	cli.checkVersion(ctx)
283
+	_ = cli.checkVersion(ctx)
279 284
 	if cli.version != "" {
280 285
 		v := strings.TrimPrefix(cli.version, "v")
281 286
 		apiPath = path.Join(cli.basePath, "/v"+v, p)
... ...
@@ -359,7 +359,7 @@ func TestNegotiateAPVersionOverride(t *testing.T) {
359 359
 func TestNegotiateAPVersionConnectionFailure(t *testing.T) {
360 360
 	const expected = "9.99"
361 361
 
362
-	client, err := NewClientWithOpts(WithHost("unix:///no-such-socket"))
362
+	client, err := NewClientWithOpts(WithHost("tcp://no-such-host.invalid"))
363 363
 	assert.NilError(t, err)
364 364
 
365 365
 	client.version = expected
... ...
@@ -28,7 +28,9 @@ func (cli *Client) ContainerCreate(ctx context.Context, config *container.Config
28 28
 	//
29 29
 	// Normally, version-negotiation (if enabled) would not happen until
30 30
 	// the API request is made.
31
-	cli.checkVersion(ctx)
31
+	if err := cli.checkVersion(ctx); err != nil {
32
+		return response, err
33
+	}
32 34
 
33 35
 	if err := cli.NewVersionError(ctx, "1.25", "stop timeout"); config != nil && config.StopTimeout != nil && err != nil {
34 36
 		return response, err
... ...
@@ -113,3 +113,15 @@ func TestContainerCreateAutoRemove(t *testing.T) {
113 113
 		t.Fatal(err)
114 114
 	}
115 115
 }
116
+
117
+// TestContainerCreateConnection verifies that connection errors occurring
118
+// during API-version negotiation are not shadowed by API-version errors.
119
+//
120
+// Regression test for https://github.com/docker/cli/issues/4890
121
+func TestContainerCreateConnectionError(t *testing.T) {
122
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
123
+	assert.NilError(t, err)
124
+
125
+	_, err = client.ContainerCreate(context.Background(), nil, nil, nil, nil, "")
126
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
127
+}
... ...
@@ -18,7 +18,9 @@ func (cli *Client) ContainerExecCreate(ctx context.Context, container string, co
18 18
 	//
19 19
 	// Normally, version-negotiation (if enabled) would not happen until
20 20
 	// the API request is made.
21
-	cli.checkVersion(ctx)
21
+	if err := cli.checkVersion(ctx); err != nil {
22
+		return response, err
23
+	}
22 24
 
23 25
 	if err := cli.NewVersionError(ctx, "1.25", "env"); len(config.Env) != 0 && err != nil {
24 26
 		return response, err
... ...
@@ -24,6 +24,18 @@ func TestContainerExecCreateError(t *testing.T) {
24 24
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
25 25
 }
26 26
 
27
+// TestContainerExecCreateConnectionError verifies that connection errors occurring
28
+// during API-version negotiation are not shadowed by API-version errors.
29
+//
30
+// Regression test for https://github.com/docker/cli/issues/4890
31
+func TestContainerExecCreateConnectionError(t *testing.T) {
32
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
33
+	assert.NilError(t, err)
34
+
35
+	_, err = client.ContainerExecCreate(context.Background(), "", types.ExecConfig{})
36
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
37
+}
38
+
27 39
 func TestContainerExecCreate(t *testing.T) {
28 40
 	expectedURL := "/containers/container_id/exec"
29 41
 	client := &Client{
... ...
@@ -23,7 +23,9 @@ func (cli *Client) ContainerRestart(ctx context.Context, containerID string, opt
23 23
 		//
24 24
 		// Normally, version-negotiation (if enabled) would not happen until
25 25
 		// the API request is made.
26
-		cli.checkVersion(ctx)
26
+		if err := cli.checkVersion(ctx); err != nil {
27
+			return err
28
+		}
27 29
 		if versions.GreaterThanOrEqualTo(cli.version, "1.42") {
28 30
 			query.Set("signal", options.Signal)
29 31
 		}
... ...
@@ -23,6 +23,18 @@ func TestContainerRestartError(t *testing.T) {
23 23
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
24 24
 }
25 25
 
26
+// TestContainerRestartConnectionError verifies that connection errors occurring
27
+// during API-version negotiation are not shadowed by API-version errors.
28
+//
29
+// Regression test for https://github.com/docker/cli/issues/4890
30
+func TestContainerRestartConnectionError(t *testing.T) {
31
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
32
+	assert.NilError(t, err)
33
+
34
+	err = client.ContainerRestart(context.Background(), "nothing", container.StopOptions{})
35
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
36
+}
37
+
26 38
 func TestContainerRestart(t *testing.T) {
27 39
 	const expectedURL = "/v1.42/containers/container_id/restart"
28 40
 	client := &Client{
... ...
@@ -27,7 +27,9 @@ func (cli *Client) ContainerStop(ctx context.Context, containerID string, option
27 27
 		//
28 28
 		// Normally, version-negotiation (if enabled) would not happen until
29 29
 		// the API request is made.
30
-		cli.checkVersion(ctx)
30
+		if err := cli.checkVersion(ctx); err != nil {
31
+			return err
32
+		}
31 33
 		if versions.GreaterThanOrEqualTo(cli.version, "1.42") {
32 34
 			query.Set("signal", options.Signal)
33 35
 		}
... ...
@@ -23,6 +23,18 @@ func TestContainerStopError(t *testing.T) {
23 23
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
24 24
 }
25 25
 
26
+// TestContainerStopConnectionError verifies that connection errors occurring
27
+// during API-version negotiation are not shadowed by API-version errors.
28
+//
29
+// Regression test for https://github.com/docker/cli/issues/4890
30
+func TestContainerStopConnectionError(t *testing.T) {
31
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
32
+	assert.NilError(t, err)
33
+
34
+	err = client.ContainerStop(context.Background(), "nothing", container.StopOptions{})
35
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
36
+}
37
+
26 38
 func TestContainerStop(t *testing.T) {
27 39
 	const expectedURL = "/v1.42/containers/container_id/stop"
28 40
 	client := &Client{
... ...
@@ -30,19 +30,22 @@ const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
30 30
 // synchronize ContainerWait with other calls, such as specifying a
31 31
 // "next-exit" condition before issuing a ContainerStart request.
32 32
 func (cli *Client) ContainerWait(ctx context.Context, containerID string, condition container.WaitCondition) (<-chan container.WaitResponse, <-chan error) {
33
+	resultC := make(chan container.WaitResponse)
34
+	errC := make(chan error, 1)
35
+
33 36
 	// Make sure we negotiated (if the client is configured to do so),
34 37
 	// as code below contains API-version specific handling of options.
35 38
 	//
36 39
 	// Normally, version-negotiation (if enabled) would not happen until
37 40
 	// the API request is made.
38
-	cli.checkVersion(ctx)
41
+	if err := cli.checkVersion(ctx); err != nil {
42
+		errC <- err
43
+		return resultC, errC
44
+	}
39 45
 	if versions.LessThan(cli.ClientVersion(), "1.30") {
40 46
 		return cli.legacyContainerWait(ctx, containerID)
41 47
 	}
42 48
 
43
-	resultC := make(chan container.WaitResponse)
44
-	errC := make(chan error, 1)
45
-
46 49
 	query := url.Values{}
47 50
 	if condition != "" {
48 51
 		query.Set("condition", string(condition))
... ...
@@ -34,6 +34,23 @@ func TestContainerWaitError(t *testing.T) {
34 34
 	}
35 35
 }
36 36
 
37
+// TestContainerWaitConnectionError verifies that connection errors occurring
38
+// during API-version negotiation are not shadowed by API-version errors.
39
+//
40
+// Regression test for https://github.com/docker/cli/issues/4890
41
+func TestContainerWaitConnectionError(t *testing.T) {
42
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
43
+	assert.NilError(t, err)
44
+
45
+	resultC, errC := client.ContainerWait(context.Background(), "nothing", "")
46
+	select {
47
+	case result := <-resultC:
48
+		t.Fatalf("expected to not get a wait result, got %d", result.StatusCode)
49
+	case err := <-errC:
50
+		assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
51
+	}
52
+}
53
+
37 54
 func TestContainerWait(t *testing.T) {
38 55
 	expectedURL := "/containers/container_id/wait"
39 56
 	client := &Client{
... ...
@@ -67,7 +67,9 @@ func (cli *Client) NewVersionError(ctx context.Context, APIrequired, feature str
67 67
 	//
68 68
 	// Normally, version-negotiation (if enabled) would not happen until
69 69
 	// the API request is made.
70
-	cli.checkVersion(ctx)
70
+	if err := cli.checkVersion(ctx); err != nil {
71
+		return err
72
+	}
71 73
 	if cli.version != "" && versions.LessThan(cli.version, APIrequired) {
72 74
 		return fmt.Errorf("%q requires API version %s, but the Docker daemon API version is %s", feature, APIrequired, cli.version)
73 75
 	}
... ...
@@ -12,14 +12,17 @@ import (
12 12
 
13 13
 // ImageList returns a list of images in the docker host.
14 14
 func (cli *Client) ImageList(ctx context.Context, options image.ListOptions) ([]image.Summary, error) {
15
+	var images []image.Summary
16
+
15 17
 	// Make sure we negotiated (if the client is configured to do so),
16 18
 	// as code below contains API-version specific handling of options.
17 19
 	//
18 20
 	// Normally, version-negotiation (if enabled) would not happen until
19 21
 	// the API request is made.
20
-	cli.checkVersion(ctx)
22
+	if err := cli.checkVersion(ctx); err != nil {
23
+		return images, err
24
+	}
21 25
 
22
-	var images []image.Summary
23 26
 	query := url.Values{}
24 27
 
25 28
 	optionFilters := options.Filters
... ...
@@ -27,6 +27,18 @@ func TestImageListError(t *testing.T) {
27 27
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
28 28
 }
29 29
 
30
+// TestImageListConnectionError verifies that connection errors occurring
31
+// during API-version negotiation are not shadowed by API-version errors.
32
+//
33
+// Regression test for https://github.com/docker/cli/issues/4890
34
+func TestImageListConnectionError(t *testing.T) {
35
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
36
+	assert.NilError(t, err)
37
+
38
+	_, err = client.ImageList(context.Background(), image.ListOptions{})
39
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
40
+}
41
+
30 42
 func TestImageList(t *testing.T) {
31 43
 	const expectedURL = "/images/json"
32 44
 
... ...
@@ -10,12 +10,16 @@ import (
10 10
 
11 11
 // NetworkCreate creates a new network in the docker host.
12 12
 func (cli *Client) NetworkCreate(ctx context.Context, name string, options types.NetworkCreate) (types.NetworkCreateResponse, error) {
13
+	var response types.NetworkCreateResponse
14
+
13 15
 	// Make sure we negotiated (if the client is configured to do so),
14 16
 	// as code below contains API-version specific handling of options.
15 17
 	//
16 18
 	// Normally, version-negotiation (if enabled) would not happen until
17 19
 	// the API request is made.
18
-	cli.checkVersion(ctx)
20
+	if err := cli.checkVersion(ctx); err != nil {
21
+		return response, err
22
+	}
19 23
 
20 24
 	networkCreateRequest := types.NetworkCreateRequest{
21 25
 		NetworkCreate: options,
... ...
@@ -25,7 +29,6 @@ func (cli *Client) NetworkCreate(ctx context.Context, name string, options types
25 25
 		networkCreateRequest.CheckDuplicate = true //nolint:staticcheck // ignore SA1019: CheckDuplicate is deprecated since API v1.44.
26 26
 	}
27 27
 
28
-	var response types.NetworkCreateResponse
29 28
 	serverResp, err := cli.post(ctx, "/networks/create", nil, networkCreateRequest, nil)
30 29
 	defer ensureReaderClosed(serverResp)
31 30
 	if err != nil {
... ...
@@ -25,6 +25,18 @@ func TestNetworkCreateError(t *testing.T) {
25 25
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
26 26
 }
27 27
 
28
+// TestNetworkCreateConnectionError verifies that connection errors occurring
29
+// during API-version negotiation are not shadowed by API-version errors.
30
+//
31
+// Regression test for https://github.com/docker/cli/issues/4890
32
+func TestNetworkCreateConnectionError(t *testing.T) {
33
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
34
+	assert.NilError(t, err)
35
+
36
+	_, err = client.NetworkCreate(context.Background(), "mynetwork", types.NetworkCreate{})
37
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
38
+}
39
+
28 40
 func TestNetworkCreate(t *testing.T) {
29 41
 	expectedURL := "/networks/create"
30 42
 
... ...
@@ -25,7 +25,9 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec,
25 25
 	//
26 26
 	// Normally, version-negotiation (if enabled) would not happen until
27 27
 	// the API request is made.
28
-	cli.checkVersion(ctx)
28
+	if err := cli.checkVersion(ctx); err != nil {
29
+		return response, err
30
+	}
29 31
 
30 32
 	// Make sure containerSpec is not nil when no runtime is set or the runtime is set to container
31 33
 	if service.TaskTemplate.ContainerSpec == nil && (service.TaskTemplate.Runtime == "" || service.TaskTemplate.Runtime == swarm.RuntimeContainer) {
... ...
@@ -28,6 +28,18 @@ func TestServiceCreateError(t *testing.T) {
28 28
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
29 29
 }
30 30
 
31
+// TestServiceCreateConnectionError verifies that connection errors occurring
32
+// during API-version negotiation are not shadowed by API-version errors.
33
+//
34
+// Regression test for https://github.com/docker/cli/issues/4890
35
+func TestServiceCreateConnectionError(t *testing.T) {
36
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
37
+	assert.NilError(t, err)
38
+
39
+	_, err = client.ServiceCreate(context.Background(), swarm.ServiceSpec{}, types.ServiceCreateOptions{})
40
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
41
+}
42
+
31 43
 func TestServiceCreate(t *testing.T) {
32 44
 	expectedURL := "/services/create"
33 45
 	client := &Client{
... ...
@@ -16,18 +16,18 @@ import (
16 16
 // It should be the value as set *before* the update. You can find this value in the Meta field
17 17
 // of swarm.Service, which can be found using ServiceInspectWithRaw.
18 18
 func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (swarm.ServiceUpdateResponse, error) {
19
+	response := swarm.ServiceUpdateResponse{}
20
+
19 21
 	// Make sure we negotiated (if the client is configured to do so),
20 22
 	// as code below contains API-version specific handling of options.
21 23
 	//
22 24
 	// Normally, version-negotiation (if enabled) would not happen until
23 25
 	// the API request is made.
24
-	cli.checkVersion(ctx)
25
-
26
-	var (
27
-		query    = url.Values{}
28
-		response = swarm.ServiceUpdateResponse{}
29
-	)
26
+	if err := cli.checkVersion(ctx); err != nil {
27
+		return response, err
28
+	}
30 29
 
30
+	query := url.Values{}
31 31
 	if options.RegistryAuthFrom != "" {
32 32
 		query.Set("registryAuthFrom", options.RegistryAuthFrom)
33 33
 	}
... ...
@@ -25,6 +25,18 @@ func TestServiceUpdateError(t *testing.T) {
25 25
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
26 26
 }
27 27
 
28
+// TestServiceUpdateConnectionError verifies that connection errors occurring
29
+// during API-version negotiation are not shadowed by API-version errors.
30
+//
31
+// Regression test for https://github.com/docker/cli/issues/4890
32
+func TestServiceUpdateConnectionError(t *testing.T) {
33
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
34
+	assert.NilError(t, err)
35
+
36
+	_, err = client.ServiceUpdate(context.Background(), "service_id", swarm.Version{}, swarm.ServiceSpec{}, types.ServiceUpdateOptions{})
37
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
38
+}
39
+
28 40
 func TestServiceUpdate(t *testing.T) {
29 41
 	expectedURL := "/services/service_id/update"
30 42
 
... ...
@@ -16,7 +16,9 @@ func (cli *Client) VolumeRemove(ctx context.Context, volumeID string, force bool
16 16
 		//
17 17
 		// Normally, version-negotiation (if enabled) would not happen until
18 18
 		// the API request is made.
19
-		cli.checkVersion(ctx)
19
+		if err := cli.checkVersion(ctx); err != nil {
20
+			return err
21
+		}
20 22
 		if versions.GreaterThanOrEqualTo(cli.version, "1.25") {
21 23
 			query.Set("force", "1")
22 24
 		}
... ...
@@ -23,6 +23,18 @@ func TestVolumeRemoveError(t *testing.T) {
23 23
 	assert.Check(t, is.ErrorType(err, errdefs.IsSystem))
24 24
 }
25 25
 
26
+// TestVolumeRemoveConnectionError verifies that connection errors occurring
27
+// during API-version negotiation are not shadowed by API-version errors.
28
+//
29
+// Regression test for https://github.com/docker/cli/issues/4890
30
+func TestVolumeRemoveConnectionError(t *testing.T) {
31
+	client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid"))
32
+	assert.NilError(t, err)
33
+
34
+	err = client.VolumeRemove(context.Background(), "volume_id", false)
35
+	assert.Check(t, is.ErrorType(err, IsErrConnectionFailed))
36
+}
37
+
26 38
 func TestVolumeRemove(t *testing.T) {
27 39
 	expectedURL := "/volumes/volume_id"
28 40