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"