Browse code

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 <github@gone.nl>

Sebastiaan van Stijn authored on 2024/02/23 19:59:31
Showing 3 changed files
... ...
@@ -307,7 +307,11 @@ func (cli *Client) ClientVersion() string {
307 307
 // added (1.24).
308 308
 func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
309 309
 	if !cli.manualOverride {
310
-		ping, _ := cli.Ping(ctx)
310
+		ping, err := cli.Ping(ctx)
311
+		if err != nil {
312
+			// FIXME(thaJeztah): Ping returns an error when failing to connect to the API; we should not swallow the error here, and instead returning it.
313
+			return
314
+		}
311 315
 		cli.negotiateAPIVersionPing(ping)
312 316
 	}
313 317
 }
... ...
@@ -354,6 +354,19 @@ func TestNegotiateAPVersionOverride(t *testing.T) {
354 354
 	assert.Equal(t, client.ClientVersion(), expected)
355 355
 }
356 356
 
357
+// TestNegotiateAPVersionConnectionFailure asserts that we do not modify the
358
+// API version when failing to connect.
359
+func TestNegotiateAPVersionConnectionFailure(t *testing.T) {
360
+	const expected = "9.99"
361
+
362
+	client, err := NewClientWithOpts(WithHost("unix:///no-such-socket"))
363
+	assert.NilError(t, err)
364
+
365
+	client.version = expected
366
+	client.NegotiateAPIVersion(context.Background())
367
+	assert.Equal(t, client.ClientVersion(), expected)
368
+}
369
+
357 370
 func TestNegotiateAPIVersionAutomatic(t *testing.T) {
358 371
 	var pingVersion string
359 372
 	httpClient := newMockClient(func(req *http.Request) (*http.Response, error) {
... ...
@@ -14,7 +14,10 @@ import (
14 14
 // Ping pings the server and returns the value of the "Docker-Experimental",
15 15
 // "Builder-Version", "OS-Type" & "API-Version" headers. It attempts to use
16 16
 // a HEAD request on the endpoint, but falls back to GET if HEAD is not supported
17
-// by the daemon.
17
+// by the daemon. It ignores internal server errors returned by the API, which
18
+// may be returned if the daemon is in an unhealthy state, but returns errors
19
+// for other non-success status codes, failing to connect to the API, or failing
20
+// to parse the API response.
18 21
 func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
19 22
 	var ping types.Ping
20 23