Browse code

client: remove NegotiateAPIVersion, NegotiateAPIVersionPing

Remove these methods in favor of adding negotiation options to the
Ping method.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2025/10/27 22:18:03
Showing 7 changed files
... ...
@@ -259,23 +259,13 @@ func (cli *Client) Close() error {
259 259
 // be negotiated when making the actual requests, and for which cases
260 260
 // we cannot do the negotiation lazily.
261 261
 func (cli *Client) checkVersion(ctx context.Context) error {
262
-	if !cli.manualOverride && cli.negotiateVersion && !cli.negotiated.Load() {
263
-		// Ensure exclusive write access to version and negotiated fields
264
-		cli.negotiateLock.Lock()
265
-		defer cli.negotiateLock.Unlock()
266
-
267
-		// May have been set during last execution of critical zone
268
-		if cli.negotiated.Load() {
269
-			return nil
270
-		}
271
-
272
-		ping, err := cli.Ping(ctx, PingOptions{})
273
-		if err != nil {
274
-			return err
275
-		}
276
-		return cli.negotiateAPIVersion(ping.APIVersion)
262
+	if cli.manualOverride || !cli.negotiateVersion || cli.negotiated.Load() {
263
+		return nil
277 264
 	}
278
-	return nil
265
+	_, err := cli.Ping(ctx, PingOptions{
266
+		NegotiateAPIVersion: true,
267
+	})
268
+	return err
279 269
 }
280 270
 
281 271
 // getAPIPath returns the versioned request path to call the API.
... ...
@@ -296,59 +286,6 @@ func (cli *Client) ClientVersion() string {
296 296
 	return cli.version
297 297
 }
298 298
 
299
-// NegotiateAPIVersion queries the API and updates the version to match the API
300
-// version. NegotiateAPIVersion downgrades the client's API version to match the
301
-// APIVersion if the ping version is lower than the default version. If the API
302
-// version reported by the server is higher than the maximum version supported
303
-// by the client, it uses the client's maximum version.
304
-//
305
-// If a manual override is in place, either through the "DOCKER_API_VERSION"
306
-// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
307
-// with a fixed version ([WithVersion]), no negotiation is performed.
308
-//
309
-// If the API server's ping response does not contain an API version, or if the
310
-// client did not get a successful ping response, it assumes it is connected with
311
-// an old daemon that does not support API version negotiation, in which case it
312
-// downgrades to the lowest supported API version.
313
-func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
314
-	if !cli.manualOverride {
315
-		// Avoid concurrent modification of version-related fields
316
-		cli.negotiateLock.Lock()
317
-		defer cli.negotiateLock.Unlock()
318
-
319
-		ping, err := cli.Ping(ctx, PingOptions{})
320
-		if err != nil {
321
-			// FIXME(thaJeztah): Ping returns an error when failing to connect to the API; we should not swallow the error here, and instead returning it.
322
-			return
323
-		}
324
-		// FIXME(thaJeztah): we should not swallow the error here, and instead returning it.
325
-		_ = cli.negotiateAPIVersion(ping.APIVersion)
326
-	}
327
-}
328
-
329
-// NegotiateAPIVersionPing downgrades the client's API version to match the
330
-// APIVersion in the ping response. If the API version in pingResponse is higher
331
-// than the maximum version supported by the client, it uses the client's maximum
332
-// version.
333
-//
334
-// If a manual override is in place, either through the "DOCKER_API_VERSION"
335
-// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
336
-// with a fixed version ([WithVersion]), no negotiation is performed.
337
-//
338
-// If the API server's ping response does not contain an API version, it falls
339
-// back to the oldest API version supported.
340
-func (cli *Client) NegotiateAPIVersionPing(pingResponse PingResult) {
341
-	// TODO(thaJeztah): should this take a "Ping" option? It only consumes the version. This method should be removed overall and not be exported.
342
-	if !cli.manualOverride {
343
-		// Avoid concurrent modification of version-related fields
344
-		cli.negotiateLock.Lock()
345
-		defer cli.negotiateLock.Unlock()
346
-
347
-		// FIXME(thaJeztah): we should not swallow the error here, and instead returning it.
348
-		_ = cli.negotiateAPIVersion(pingResponse.APIVersion)
349
-	}
350
-}
351
-
352 299
 // negotiateAPIVersion updates the version to match the API version from
353 300
 // the ping response. It falls back to the lowest version supported if the
354 301
 // API version is empty, or returns an error if the API version is lower than
... ...
@@ -32,8 +32,6 @@ type stableAPIClient interface {
32 32
 	ClientVersion() string
33 33
 	DaemonHost() string
34 34
 	ServerVersion(ctx context.Context) (types.Version, error)
35
-	NegotiateAPIVersion(ctx context.Context)
36
-	NegotiateAPIVersionPing(PingResult)
37 35
 	HijackDialer
38 36
 	Dialer() func(context.Context) (net.Conn, error)
39 37
 	Close() error
... ...
@@ -252,18 +252,27 @@ func TestNewClientWithOpsFromEnvSetsDefaultVersion(t *testing.T) {
252 252
 func TestNegotiateAPIVersionEmpty(t *testing.T) {
253 253
 	t.Setenv("DOCKER_API_VERSION", "")
254 254
 
255
-	client, err := NewClientWithOpts(FromEnv)
256
-	assert.NilError(t, err)
257
-
258
-	// set our version to something new
259
-	client.version = "1.51"
260
-
261 255
 	// if no version from server, expect the earliest
262 256
 	// version before APIVersion was implemented
263 257
 	const expected = fallbackAPIVersion
264 258
 
259
+	client, err := NewClientWithOpts(FromEnv,
260
+		WithAPIVersionNegotiation(),
261
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{expected}}, "OK")),
262
+	)
263
+	assert.NilError(t, err)
264
+
265
+	// set our version to something new.
266
+	// we're not using [WithVersion] here, as that marks the version
267
+	// as manually overridden.
268
+	client.version = "1.51"
269
+
265 270
 	// test downgrade
266
-	client.NegotiateAPIVersionPing(PingResult{})
271
+	ping, err := client.Ping(t.Context(), PingOptions{
272
+		NegotiateAPIVersion: true,
273
+	})
274
+	assert.NilError(t, err)
275
+	assert.Check(t, is.Equal(ping.APIVersion, expected))
267 276
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
268 277
 }
269 278
 
... ...
@@ -275,6 +284,7 @@ func TestNegotiateAPIVersion(t *testing.T) {
275 275
 		clientVersion   string
276 276
 		pingVersion     string
277 277
 		expectedVersion string
278
+		expectedErr     string
278 279
 	}{
279 280
 		{
280 281
 			// client should downgrade to the version reported by the daemon.
... ...
@@ -304,6 +314,7 @@ func TestNegotiateAPIVersion(t *testing.T) {
304 304
 			doc:             "no downgrade old",
305 305
 			pingVersion:     "1.19",
306 306
 			expectedVersion: MaxAPIVersion,
307
+			expectedErr:     "API version 1.19 is not supported by this client: the minimum supported API version is " + fallbackAPIVersion,
307 308
 		},
308 309
 		{
309 310
 			// client should not upgrade to a newer version if a version was set,
... ...
@@ -317,7 +328,12 @@ func TestNegotiateAPIVersion(t *testing.T) {
317 317
 
318 318
 	for _, tc := range tests {
319 319
 		t.Run(tc.doc, func(t *testing.T) {
320
-			opts := make([]Opt, 0)
320
+			opts := []Opt{
321
+				FromEnv,
322
+				WithAPIVersionNegotiation(),
323
+				WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{tc.pingVersion}}, "OK")),
324
+			}
325
+
321 326
 			if tc.clientVersion != "" {
322 327
 				// Note that this check is redundant, as WithVersion() considers
323 328
 				// an empty version equivalent to "not setting a version", but
... ...
@@ -326,7 +342,14 @@ func TestNegotiateAPIVersion(t *testing.T) {
326 326
 			}
327 327
 			client, err := NewClientWithOpts(opts...)
328 328
 			assert.NilError(t, err)
329
-			client.NegotiateAPIVersionPing(PingResult{APIVersion: tc.pingVersion})
329
+			_, err = client.Ping(t.Context(), PingOptions{
330
+				NegotiateAPIVersion: true,
331
+			})
332
+			if tc.expectedErr != "" {
333
+				assert.Check(t, is.ErrorContains(err, tc.expectedErr))
334
+			} else {
335
+				assert.NilError(t, err)
336
+			}
330 337
 			assert.Check(t, is.Equal(tc.expectedVersion, client.ClientVersion()))
331 338
 		})
332 339
 	}
... ...
@@ -338,11 +361,16 @@ func TestNegotiateAPIVersionOverride(t *testing.T) {
338 338
 	const expected = "9.99"
339 339
 	t.Setenv("DOCKER_API_VERSION", expected)
340 340
 
341
-	client, err := NewClientWithOpts(FromEnv)
341
+	client, err := NewClientWithOpts(
342
+		FromEnv,
343
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{"1.45"}}, "OK")),
344
+	)
342 345
 	assert.NilError(t, err)
343 346
 
344 347
 	// test that we honored the env var
345
-	client.NegotiateAPIVersionPing(PingResult{APIVersion: "1.24"})
348
+	_, err = client.Ping(t.Context(), PingOptions{
349
+		NegotiateAPIVersion: true,
350
+	})
346 351
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
347 352
 }
348 353
 
... ...
@@ -353,9 +381,10 @@ func TestNegotiateAPIVersionConnectionFailure(t *testing.T) {
353 353
 
354 354
 	client, err := NewClientWithOpts(WithHost("tcp://no-such-host.invalid"))
355 355
 	assert.NilError(t, err)
356
-
357 356
 	client.version = expected
358
-	client.NegotiateAPIVersion(context.Background())
357
+	_, err = client.Ping(t.Context(), PingOptions{
358
+		NegotiateAPIVersion: true,
359
+	})
359 360
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
360 361
 }
361 362
 
... ...
@@ -392,11 +421,16 @@ func TestNegotiateAPIVersionAutomatic(t *testing.T) {
392 392
 // TestNegotiateAPIVersionWithEmptyVersion asserts that initializing a client
393 393
 // with an empty version string does still allow API-version negotiation
394 394
 func TestNegotiateAPIVersionWithEmptyVersion(t *testing.T) {
395
-	client, err := NewClientWithOpts(WithVersion(""))
395
+	client, err := NewClientWithOpts(
396
+		WithVersion(""),
397
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{"1.50"}}, "OK")),
398
+	)
396 399
 	assert.NilError(t, err)
397 400
 
398 401
 	const expected = "1.50"
399
-	client.NegotiateAPIVersionPing(PingResult{APIVersion: expected})
402
+	_, err = client.Ping(t.Context(), PingOptions{
403
+		NegotiateAPIVersion: true,
404
+	})
400 405
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
401 406
 }
402 407
 
... ...
@@ -404,10 +438,17 @@ func TestNegotiateAPIVersionWithEmptyVersion(t *testing.T) {
404 404
 // with a fixed version disables API-version negotiation
405 405
 func TestNegotiateAPIVersionWithFixedVersion(t *testing.T) {
406 406
 	const customVersion = "1.50"
407
-	client, err := NewClientWithOpts(WithVersion(customVersion))
407
+	client, err := NewClientWithOpts(
408
+		WithVersion(customVersion),
409
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{"1.49"}}, "OK")),
410
+	)
408 411
 	assert.NilError(t, err)
409 412
 
410
-	client.NegotiateAPIVersionPing(PingResult{APIVersion: "1.49"})
413
+	_, err = client.Ping(t.Context(), PingOptions{
414
+		NegotiateAPIVersion: true,
415
+		ForceNegotiate:      true,
416
+	})
417
+	assert.NilError(t, err)
411 418
 	assert.Check(t, is.Equal(client.ClientVersion(), customVersion))
412 419
 }
413 420
 
... ...
@@ -12,7 +12,29 @@ import (
12 12
 
13 13
 // PingOptions holds options for [client.Ping].
14 14
 type PingOptions struct {
15
-	// Add future optional parameters here
15
+	// NegotiateAPIVersion queries the API and updates the version to match the API
16
+	// version. NegotiateAPIVersion downgrades the client's API version to match the
17
+	// APIVersion if the ping version is lower than the default version. If the API
18
+	// version reported by the server is higher than the maximum version supported
19
+	// by the client, it uses the client's maximum version.
20
+	//
21
+	// If a manual override is in place, either through the "DOCKER_API_VERSION"
22
+	// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
23
+	// with a fixed version ([WithVersion]), no negotiation is performed.
24
+	//
25
+	// If the API server's ping response does not contain an API version, or if the
26
+	// client did not get a successful ping response, it assumes it is connected with
27
+	// an old daemon that does not support API version negotiation, in which case it
28
+	// downgrades to the lowest supported API version.
29
+	NegotiateAPIVersion bool
30
+
31
+	// ForceNegotiate forces the client to re-negotiate the API version, even if
32
+	// API-version negotiation already happened. This option cannot be
33
+	// used if the client is configured with a fixed version using (using
34
+	// [WithVersion] or [WithVersionFromEnv]).
35
+	//
36
+	// This option has no effect if NegotiateAPIVersion is not set.
37
+	ForceNegotiate bool
16 38
 }
17 39
 
18 40
 // PingResult holds the result of a [Client.Ping] API call.
... ...
@@ -50,6 +72,30 @@ type SwarmStatus struct {
50 50
 // for other non-success status codes, failing to connect to the API, or failing
51 51
 // to parse the API response.
52 52
 func (cli *Client) Ping(ctx context.Context, options PingOptions) (PingResult, error) {
53
+	if cli.manualOverride {
54
+		return cli.ping(ctx)
55
+	}
56
+	if !options.NegotiateAPIVersion && !cli.negotiateVersion {
57
+		return cli.ping(ctx)
58
+	}
59
+
60
+	// Ensure exclusive write access to version and negotiated fields
61
+	cli.negotiateLock.Lock()
62
+	defer cli.negotiateLock.Unlock()
63
+
64
+	ping, err := cli.ping(ctx)
65
+	if err != nil {
66
+		return cli.ping(ctx)
67
+	}
68
+
69
+	if cli.negotiated.Load() && !options.ForceNegotiate {
70
+		return ping, nil
71
+	}
72
+
73
+	return ping, cli.negotiateAPIVersion(ping.APIVersion)
74
+}
75
+
76
+func (cli *Client) ping(ctx context.Context) (PingResult, error) {
53 77
 	// Using cli.buildRequest() + cli.doRequest() instead of cli.sendRequest()
54 78
 	// because ping requests are used during API version negotiation, so we want
55 79
 	// to hit the non-versioned /_ping endpoint, not /v1.xx/_ping
... ...
@@ -259,23 +259,13 @@ func (cli *Client) Close() error {
259 259
 // be negotiated when making the actual requests, and for which cases
260 260
 // we cannot do the negotiation lazily.
261 261
 func (cli *Client) checkVersion(ctx context.Context) error {
262
-	if !cli.manualOverride && cli.negotiateVersion && !cli.negotiated.Load() {
263
-		// Ensure exclusive write access to version and negotiated fields
264
-		cli.negotiateLock.Lock()
265
-		defer cli.negotiateLock.Unlock()
266
-
267
-		// May have been set during last execution of critical zone
268
-		if cli.negotiated.Load() {
269
-			return nil
270
-		}
271
-
272
-		ping, err := cli.Ping(ctx, PingOptions{})
273
-		if err != nil {
274
-			return err
275
-		}
276
-		return cli.negotiateAPIVersion(ping.APIVersion)
262
+	if cli.manualOverride || !cli.negotiateVersion || cli.negotiated.Load() {
263
+		return nil
277 264
 	}
278
-	return nil
265
+	_, err := cli.Ping(ctx, PingOptions{
266
+		NegotiateAPIVersion: true,
267
+	})
268
+	return err
279 269
 }
280 270
 
281 271
 // getAPIPath returns the versioned request path to call the API.
... ...
@@ -296,59 +286,6 @@ func (cli *Client) ClientVersion() string {
296 296
 	return cli.version
297 297
 }
298 298
 
299
-// NegotiateAPIVersion queries the API and updates the version to match the API
300
-// version. NegotiateAPIVersion downgrades the client's API version to match the
301
-// APIVersion if the ping version is lower than the default version. If the API
302
-// version reported by the server is higher than the maximum version supported
303
-// by the client, it uses the client's maximum version.
304
-//
305
-// If a manual override is in place, either through the "DOCKER_API_VERSION"
306
-// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
307
-// with a fixed version ([WithVersion]), no negotiation is performed.
308
-//
309
-// If the API server's ping response does not contain an API version, or if the
310
-// client did not get a successful ping response, it assumes it is connected with
311
-// an old daemon that does not support API version negotiation, in which case it
312
-// downgrades to the lowest supported API version.
313
-func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
314
-	if !cli.manualOverride {
315
-		// Avoid concurrent modification of version-related fields
316
-		cli.negotiateLock.Lock()
317
-		defer cli.negotiateLock.Unlock()
318
-
319
-		ping, err := cli.Ping(ctx, PingOptions{})
320
-		if err != nil {
321
-			// FIXME(thaJeztah): Ping returns an error when failing to connect to the API; we should not swallow the error here, and instead returning it.
322
-			return
323
-		}
324
-		// FIXME(thaJeztah): we should not swallow the error here, and instead returning it.
325
-		_ = cli.negotiateAPIVersion(ping.APIVersion)
326
-	}
327
-}
328
-
329
-// NegotiateAPIVersionPing downgrades the client's API version to match the
330
-// APIVersion in the ping response. If the API version in pingResponse is higher
331
-// than the maximum version supported by the client, it uses the client's maximum
332
-// version.
333
-//
334
-// If a manual override is in place, either through the "DOCKER_API_VERSION"
335
-// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
336
-// with a fixed version ([WithVersion]), no negotiation is performed.
337
-//
338
-// If the API server's ping response does not contain an API version, it falls
339
-// back to the oldest API version supported.
340
-func (cli *Client) NegotiateAPIVersionPing(pingResponse PingResult) {
341
-	// TODO(thaJeztah): should this take a "Ping" option? It only consumes the version. This method should be removed overall and not be exported.
342
-	if !cli.manualOverride {
343
-		// Avoid concurrent modification of version-related fields
344
-		cli.negotiateLock.Lock()
345
-		defer cli.negotiateLock.Unlock()
346
-
347
-		// FIXME(thaJeztah): we should not swallow the error here, and instead returning it.
348
-		_ = cli.negotiateAPIVersion(pingResponse.APIVersion)
349
-	}
350
-}
351
-
352 299
 // negotiateAPIVersion updates the version to match the API version from
353 300
 // the ping response. It falls back to the lowest version supported if the
354 301
 // API version is empty, or returns an error if the API version is lower than
... ...
@@ -32,8 +32,6 @@ type stableAPIClient interface {
32 32
 	ClientVersion() string
33 33
 	DaemonHost() string
34 34
 	ServerVersion(ctx context.Context) (types.Version, error)
35
-	NegotiateAPIVersion(ctx context.Context)
36
-	NegotiateAPIVersionPing(PingResult)
37 35
 	HijackDialer
38 36
 	Dialer() func(context.Context) (net.Conn, error)
39 37
 	Close() error
... ...
@@ -12,7 +12,29 @@ import (
12 12
 
13 13
 // PingOptions holds options for [client.Ping].
14 14
 type PingOptions struct {
15
-	// Add future optional parameters here
15
+	// NegotiateAPIVersion queries the API and updates the version to match the API
16
+	// version. NegotiateAPIVersion downgrades the client's API version to match the
17
+	// APIVersion if the ping version is lower than the default version. If the API
18
+	// version reported by the server is higher than the maximum version supported
19
+	// by the client, it uses the client's maximum version.
20
+	//
21
+	// If a manual override is in place, either through the "DOCKER_API_VERSION"
22
+	// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
23
+	// with a fixed version ([WithVersion]), no negotiation is performed.
24
+	//
25
+	// If the API server's ping response does not contain an API version, or if the
26
+	// client did not get a successful ping response, it assumes it is connected with
27
+	// an old daemon that does not support API version negotiation, in which case it
28
+	// downgrades to the lowest supported API version.
29
+	NegotiateAPIVersion bool
30
+
31
+	// ForceNegotiate forces the client to re-negotiate the API version, even if
32
+	// API-version negotiation already happened. This option cannot be
33
+	// used if the client is configured with a fixed version using (using
34
+	// [WithVersion] or [WithVersionFromEnv]).
35
+	//
36
+	// This option has no effect if NegotiateAPIVersion is not set.
37
+	ForceNegotiate bool
16 38
 }
17 39
 
18 40
 // PingResult holds the result of a [Client.Ping] API call.
... ...
@@ -50,6 +72,30 @@ type SwarmStatus struct {
50 50
 // for other non-success status codes, failing to connect to the API, or failing
51 51
 // to parse the API response.
52 52
 func (cli *Client) Ping(ctx context.Context, options PingOptions) (PingResult, error) {
53
+	if cli.manualOverride {
54
+		return cli.ping(ctx)
55
+	}
56
+	if !options.NegotiateAPIVersion && !cli.negotiateVersion {
57
+		return cli.ping(ctx)
58
+	}
59
+
60
+	// Ensure exclusive write access to version and negotiated fields
61
+	cli.negotiateLock.Lock()
62
+	defer cli.negotiateLock.Unlock()
63
+
64
+	ping, err := cli.ping(ctx)
65
+	if err != nil {
66
+		return cli.ping(ctx)
67
+	}
68
+
69
+	if cli.negotiated.Load() && !options.ForceNegotiate {
70
+		return ping, nil
71
+	}
72
+
73
+	return ping, cli.negotiateAPIVersion(ping.APIVersion)
74
+}
75
+
76
+func (cli *Client) ping(ctx context.Context) (PingResult, error) {
53 77
 	// Using cli.buildRequest() + cli.doRequest() instead of cli.sendRequest()
54 78
 	// because ping requests are used during API version negotiation, so we want
55 79
 	// to hit the non-versioned /_ping endpoint, not /v1.xx/_ping