From 94137f6df5d97edd6f3327f0f51bf299755a674f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 23 Feb 2024 12:20:06 +0100 Subject: [PATCH] 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 (cherry picked from commit 6aea26b431ea152a8b085e453da06ea403f89886) Conflicts: client/image_list.go client/image_list_test.go Signed-off-by: Bjorn Neergaard --- client/client.go | 13 +++++++++---- client/client_test.go | 2 +- client/container_create.go | 4 +++- client/container_create_test.go | 12 ++++++++++++ client/container_exec.go | 4 +++- client/container_exec_test.go | 12 ++++++++++++ client/container_restart.go | 4 +++- client/container_restart_test.go | 12 ++++++++++++ client/container_stop.go | 4 +++- client/container_stop_test.go | 12 ++++++++++++ client/container_wait.go | 11 +++++++---- client/container_wait_test.go | 17 +++++++++++++++++ client/errors.go | 4 +++- client/image_list.go | 7 +++++-- client/image_list_test.go | 12 ++++++++++++ client/network_create.go | 7 +++++-- client/network_create_test.go | 12 ++++++++++++ client/service_create.go | 4 +++- client/service_create_test.go | 12 ++++++++++++ client/service_update.go | 12 ++++++------ client/service_update_test.go | 12 ++++++++++++ client/volume_remove.go | 4 +++- client/volume_remove_test.go | 12 ++++++++++++ 23 files changed, 179 insertions(+), 26 deletions(-) diff --git a/client/client.go b/client/client.go index 36326c0f1a0c2..f2eeb6c5702ed 100644 --- 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 index b72c95cc97105..68d4724b440dc 100644 --- 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 index 409f5b492a6ed..5442d4267d09e 100644 --- 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 index cb593cd418979..284c9df2086a7 100644 --- 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 index 3fff0c8288974..526a3876a4a77 100644 --- 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 index fb5afc83e899c..cceba81eac9c4 100644 --- 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 index 825d3e4e9d9bd..02b5079bc463c 100644 --- 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 index 63e110209c73d..95f3fceb19bb6 100644 --- 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 index ac0cab69de94d..7c98a354b42e4 100644 --- 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 index 48ef64902b468..a0b38c10fd621 100644 --- 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 index b8d3bdef0db8e..8bb6be0a18b27 100644 --- 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 index 7cbfc72d20bfc..30854e46ae3de 100644 --- 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 index 5b1b9b324de68..0d01e243fe0ba 100644 --- 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 index f3f2280e32497..fa6aecfc6ed0d 100644 --- a/client/image_list.go +++ b/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 diff --git a/client/image_list_test.go b/client/image_list_test.go index bcd6db7d988f4..0f42ca41fdf2b 100644 --- a/client/image_list_test.go +++ b/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" diff --git a/client/network_create.go b/client/network_create.go index 668e87d653b2a..d510feb3db9b2 100644 --- 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 index 10abc16e5db8e..735634a16fd8b 100644 --- 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 index 2ebb5ee3a580d..b72cb420d49e3 100644 --- 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 index 0bbd0bc2830d9..d9f1482b3bca7 100644 --- 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 index e05eebf56657f..d2f03f02f07c1 100644 --- 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 index f310ef13e3b2e..683c03f81560b 100644 --- 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 index 31e08cb975972..b8bdc5ae85856 100644 --- 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 index d3ee614b07b0f..f242a3892db1c 100644 --- a/client/volume_remove_test.go +++ b/client/volume_remove_test.go @@ -23,6 +23,18 @@ func TestVolumeRemoveError(t *testing.T) { assert.Check(t, is.ErrorType(err, errdefs.IsSystem)) } +// TestVolumeRemoveConnectionError 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 TestVolumeRemoveConnectionError(t *testing.T) { + client, err := NewClientWithOpts(WithAPIVersionNegotiation(), WithHost("tcp://no-such-host.invalid")) + assert.NilError(t, err) + + err = client.VolumeRemove(context.Background(), "volume_id", false) + assert.Check(t, is.ErrorType(err, IsErrConnectionFailed)) +} + func TestVolumeRemove(t *testing.T) { expectedURL := "/volumes/volume_id"