From 6aea26b431ea152a8b085e453da06ea403f89886 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 --- 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 36326c0f1a..f2eeb6c570 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 b72c95cc97..68d4724b44 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 409f5b492a..5442d4267d 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 cb593cd418..284c9df208 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 3fff0c8288..526a3876a4 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 fb5afc83e8..cceba81eac 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 825d3e4e9d..02b5079bc4 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 63e110209c..95f3fceb19 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 ac0cab69de..7c98a354b4 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 48ef64902b..a0b38c10fd 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 b8d3bdef0d..8bb6be0a18 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 7cbfc72d20..30854e46ae 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 5b1b9b324d..0d01e243fe 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 b4df6ff86a..a9cc1e21e5 100644 --- 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 index 0320b14258..0b332228e3 100644 --- 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 index 668e87d653..d510feb3db 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 10abc16e5d..735634a16f 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 2ebb5ee3a5..b72cb420d4 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 0bbd0bc283..d9f1482b3b 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 e05eebf566..d2f03f02f0 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 f310ef13e3..683c03f815 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 31e08cb975..b8bdc5ae85 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 d3ee614b07..f242a3892d 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"