From 901b90593d6f845ccd403a36345a70a2a8a3b279 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 23 Feb 2024 11:59:31 +0100 Subject: [PATCH] client: NegotiateAPIVersion: do not ignore (connection) errors from Ping NegotiateAPIVersion was ignoring errors returned by Ping. The intent here was to handle API responses from a daemon that may be in an unhealthy state, however this case is already handled by Ping itself. Ping only returns an error when either failing to connect to the API (daemon not running or permissions errors), or when failing to parse the API response. Neither of those should be ignored in this code, or considered a successful "ping", so update the code to return Signed-off-by: Sebastiaan van Stijn --- client/client.go | 6 +++++- client/client_test.go | 13 +++++++++++++ client/ping.go | 5 ++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/client/client.go b/client/client.go index 0b496b0fa6..36326c0f1a 100644 --- a/client/client.go +++ b/client/client.go @@ -307,7 +307,11 @@ func (cli *Client) ClientVersion() string { // added (1.24). func (cli *Client) NegotiateAPIVersion(ctx context.Context) { if !cli.manualOverride { - ping, _ := cli.Ping(ctx) + ping, err := cli.Ping(ctx) + if err != nil { + // FIXME(thaJeztah): Ping returns an error when failing to connect to the API; we should not swallow the error here, and instead returning it. + return + } cli.negotiateAPIVersionPing(ping) } } diff --git a/client/client_test.go b/client/client_test.go index 5e76f03d86..b72c95cc97 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -354,6 +354,19 @@ func TestNegotiateAPVersionOverride(t *testing.T) { assert.Equal(t, client.ClientVersion(), expected) } +// TestNegotiateAPVersionConnectionFailure asserts that we do not modify the +// API version when failing to connect. +func TestNegotiateAPVersionConnectionFailure(t *testing.T) { + const expected = "9.99" + + client, err := NewClientWithOpts(WithHost("unix:///no-such-socket")) + assert.NilError(t, err) + + client.version = expected + client.NegotiateAPIVersion(context.Background()) + assert.Equal(t, client.ClientVersion(), expected) +} + func TestNegotiateAPIVersionAutomatic(t *testing.T) { var pingVersion string httpClient := newMockClient(func(req *http.Request) (*http.Response, error) { diff --git a/client/ping.go b/client/ping.go index dfd1042fab..bf3e9b1cd6 100644 --- a/client/ping.go +++ b/client/ping.go @@ -14,7 +14,10 @@ import ( // Ping pings the server and returns the value of the "Docker-Experimental", // "Builder-Version", "OS-Type" & "API-Version" headers. It attempts to use // a HEAD request on the endpoint, but falls back to GET if HEAD is not supported -// by the daemon. +// by the daemon. It ignores internal server errors returned by the API, which +// may be returned if the daemon is in an unhealthy state, but returns errors +// for other non-success status codes, failing to connect to the API, or failing +// to parse the API response. func (cli *Client) Ping(ctx context.Context) (types.Ping, error) { var ping types.Ping