Merge pull request moby#51307 from thaJeztah/ping_once

client: remove NegotiateAPIVersion, NegotiateAPIVersionPing

Author: austinvazquez

client/client.go

@@ -259,23 +259,13 @@ func (cli *Client) Close() error {
 // be negotiated when making the actual requests, and for which cases
 // we cannot do the negotiation lazily.
 func (cli *Client) checkVersion(ctx context.Context) error {
-	if !cli.manualOverride && cli.negotiateVersion && !cli.negotiated.Load() {
-		// Ensure exclusive write access to version and negotiated fields
-		cli.negotiateLock.Lock()
-		defer cli.negotiateLock.Unlock()
-
-		// May have been set during last execution of critical zone
-		if cli.negotiated.Load() {
-			return nil
-		}
-
-		ping, err := cli.Ping(ctx, PingOptions{})
-		if err != nil {
-			return err
-		}
-		return cli.negotiateAPIVersion(ping.APIVersion)
+	if cli.manualOverride || !cli.negotiateVersion || cli.negotiated.Load() {
+		return nil
 	}
-	return nil
+	_, err := cli.Ping(ctx, PingOptions{
+		NegotiateAPIVersion: true,
+	})
+	return err
 }
 
 // getAPIPath returns the versioned request path to call the API.
@@ -296,59 +286,6 @@ func (cli *Client) ClientVersion() string {
 	return cli.version
 }
 
-// NegotiateAPIVersion queries the API and updates the version to match the API
-// version. NegotiateAPIVersion downgrades the client's API version to match the
-// APIVersion if the ping version is lower than the default version. If the API
-// version reported by the server is higher than the maximum version supported
-// by the client, it uses the client's maximum version.
-//
-// If a manual override is in place, either through the "DOCKER_API_VERSION"
-// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
-// with a fixed version ([WithVersion]), no negotiation is performed.
-//
-// If the API server's ping response does not contain an API version, or if the
-// client did not get a successful ping response, it assumes it is connected with
-// an old daemon that does not support API version negotiation, in which case it
-// downgrades to the lowest supported API version.
-func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
-	if !cli.manualOverride {
-		// Avoid concurrent modification of version-related fields
-		cli.negotiateLock.Lock()
-		defer cli.negotiateLock.Unlock()
-
-		ping, err := cli.Ping(ctx, PingOptions{})
-		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
-		}
-		// FIXME(thaJeztah): we should not swallow the error here, and instead returning it.
-		_ = cli.negotiateAPIVersion(ping.APIVersion)
-	}
-}
-
-// NegotiateAPIVersionPing downgrades the client's API version to match the
-// APIVersion in the ping response. If the API version in pingResponse is higher
-// than the maximum version supported by the client, it uses the client's maximum
-// version.
-//
-// If a manual override is in place, either through the "DOCKER_API_VERSION"
-// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
-// with a fixed version ([WithVersion]), no negotiation is performed.
-//
-// If the API server's ping response does not contain an API version, it falls
-// back to the oldest API version supported.
-func (cli *Client) NegotiateAPIVersionPing(pingResponse PingResult) {
-	// TODO(thaJeztah): should this take a "Ping" option? It only consumes the version. This method should be removed overall and not be exported.
-	if !cli.manualOverride {
-		// Avoid concurrent modification of version-related fields
-		cli.negotiateLock.Lock()
-		defer cli.negotiateLock.Unlock()
-
-		// FIXME(thaJeztah): we should not swallow the error here, and instead returning it.
-		_ = cli.negotiateAPIVersion(pingResponse.APIVersion)
-	}
-}
-
 // negotiateAPIVersion updates the version to match the API version from
 // the ping response. It falls back to the lowest version supported if the
 // API version is empty, or returns an error if the API version is lower than

client/client_interfaces.go

@@ -32,8 +32,6 @@ type stableAPIClient interface {
 	ClientVersion() string
 	DaemonHost() string
 	ServerVersion(ctx context.Context) (types.Version, error)
-	NegotiateAPIVersion(ctx context.Context)
-	NegotiateAPIVersionPing(PingResult)
 	HijackDialer
 	Dialer() func(context.Context) (net.Conn, error)
 	Close() error

client/client_test.go

@@ -252,18 +252,27 @@ func TestNewClientWithOpsFromEnvSetsDefaultVersion(t *testing.T) {
 func TestNegotiateAPIVersionEmpty(t *testing.T) {
 	t.Setenv("DOCKER_API_VERSION", "")
 
-	client, err := NewClientWithOpts(FromEnv)
-	assert.NilError(t, err)
-
-	// set our version to something new
-	client.version = "1.51"
-
 	// if no version from server, expect the earliest
 	// version before APIVersion was implemented
 	const expected = fallbackAPIVersion
 
+	client, err := NewClientWithOpts(FromEnv,
+		WithAPIVersionNegotiation(),
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{expected}}, "OK")),
+	)
+	assert.NilError(t, err)
+
+	// set our version to something new.
+	// we're not using [WithVersion] here, as that marks the version
+	// as manually overridden.
+	client.version = "1.51"
+
 	// test downgrade
-	client.NegotiateAPIVersionPing(PingResult{})
+	ping, err := client.Ping(t.Context(), PingOptions{
+		NegotiateAPIVersion: true,
+	})
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(ping.APIVersion, expected))
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
 }
 
@@ -275,6 +284,7 @@ func TestNegotiateAPIVersion(t *testing.T) {
 		clientVersion   string
 		pingVersion     string
 		expectedVersion string
+		expectedErr     string
 	}{
 		{
 			// client should downgrade to the version reported by the daemon.
@@ -304,6 +314,7 @@ func TestNegotiateAPIVersion(t *testing.T) {
 			doc:             "no downgrade old",
 			pingVersion:     "1.19",
 			expectedVersion: MaxAPIVersion,
+			expectedErr:     "API version 1.19 is not supported by this client: the minimum supported API version is " + fallbackAPIVersion,
 		},
 		{
 			// client should not upgrade to a newer version if a version was set,
@@ -317,7 +328,12 @@ func TestNegotiateAPIVersion(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.doc, func(t *testing.T) {
-			opts := make([]Opt, 0)
+			opts := []Opt{
+				FromEnv,
+				WithAPIVersionNegotiation(),
+				WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{tc.pingVersion}}, "OK")),
+			}
+
 			if tc.clientVersion != "" {
 				// Note that this check is redundant, as WithVersion() considers
 				// an empty version equivalent to "not setting a version", but
@@ -326,7 +342,14 @@ func TestNegotiateAPIVersion(t *testing.T) {
 			}
 			client, err := NewClientWithOpts(opts...)
 			assert.NilError(t, err)
-			client.NegotiateAPIVersionPing(PingResult{APIVersion: tc.pingVersion})
+			_, err = client.Ping(t.Context(), PingOptions{
+				NegotiateAPIVersion: true,
+			})
+			if tc.expectedErr != "" {
+				assert.Check(t, is.ErrorContains(err, tc.expectedErr))
+			} else {
+				assert.NilError(t, err)
+			}
 			assert.Check(t, is.Equal(tc.expectedVersion, client.ClientVersion()))
 		})
 	}
@@ -338,11 +361,16 @@ func TestNegotiateAPIVersionOverride(t *testing.T) {
 	const expected = "9.99"
 	t.Setenv("DOCKER_API_VERSION", expected)
 
-	client, err := NewClientWithOpts(FromEnv)
+	client, err := NewClientWithOpts(
+		FromEnv,
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{"1.45"}}, "OK")),
+	)
 	assert.NilError(t, err)
 
 	// test that we honored the env var
-	client.NegotiateAPIVersionPing(PingResult{APIVersion: "1.24"})
+	_, err = client.Ping(t.Context(), PingOptions{
+		NegotiateAPIVersion: true,
+	})
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
 }
 
@@ -353,9 +381,10 @@ func TestNegotiateAPIVersionConnectionFailure(t *testing.T) {
 
 	client, err := NewClientWithOpts(WithHost("tcp://no-such-host.invalid"))
 	assert.NilError(t, err)
-
 	client.version = expected
-	client.NegotiateAPIVersion(context.Background())
+	_, err = client.Ping(t.Context(), PingOptions{
+		NegotiateAPIVersion: true,
+	})
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
 }
 
@@ -392,22 +421,34 @@ func TestNegotiateAPIVersionAutomatic(t *testing.T) {
 // TestNegotiateAPIVersionWithEmptyVersion asserts that initializing a client
 // with an empty version string does still allow API-version negotiation
 func TestNegotiateAPIVersionWithEmptyVersion(t *testing.T) {
-	client, err := NewClientWithOpts(WithVersion(""))
+	client, err := NewClientWithOpts(
+		WithVersion(""),
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{"1.50"}}, "OK")),
+	)
 	assert.NilError(t, err)
 
 	const expected = "1.50"
-	client.NegotiateAPIVersionPing(PingResult{APIVersion: expected})
+	_, err = client.Ping(t.Context(), PingOptions{
+		NegotiateAPIVersion: true,
+	})
 	assert.Check(t, is.Equal(client.ClientVersion(), expected))
 }
 
 // TestNegotiateAPIVersionWithFixedVersion asserts that initializing a client
 // with a fixed version disables API-version negotiation
 func TestNegotiateAPIVersionWithFixedVersion(t *testing.T) {
 	const customVersion = "1.50"
-	client, err := NewClientWithOpts(WithVersion(customVersion))
+	client, err := NewClientWithOpts(
+		WithVersion(customVersion),
+		WithMockClient(mockResponse(http.StatusOK, http.Header{"Api-Version": []string{"1.49"}}, "OK")),
+	)
 	assert.NilError(t, err)
 
-	client.NegotiateAPIVersionPing(PingResult{APIVersion: "1.49"})
+	_, err = client.Ping(t.Context(), PingOptions{
+		NegotiateAPIVersion: true,
+		ForceNegotiate:      true,
+	})
+	assert.NilError(t, err)
 	assert.Check(t, is.Equal(client.ClientVersion(), customVersion))
 }
 

client/ping.go

@@ -12,7 +12,29 @@ import (
 
 // PingOptions holds options for [client.Ping].
 type PingOptions struct {
-	// Add future optional parameters here
+	// NegotiateAPIVersion queries the API and updates the version to match the API
+	// version. NegotiateAPIVersion downgrades the client's API version to match the
+	// APIVersion if the ping version is lower than the default version. If the API
+	// version reported by the server is higher than the maximum version supported
+	// by the client, it uses the client's maximum version.
+	//
+	// If a manual override is in place, either through the "DOCKER_API_VERSION"
+	// ([EnvOverrideAPIVersion]) environment variable, or if the client is initialized
+	// with a fixed version ([WithVersion]), no negotiation is performed.
+	//
+	// If the API server's ping response does not contain an API version, or if the
+	// client did not get a successful ping response, it assumes it is connected with
+	// an old daemon that does not support API version negotiation, in which case it
+	// downgrades to the lowest supported API version.
+	NegotiateAPIVersion bool
+
+	// ForceNegotiate forces the client to re-negotiate the API version, even if
+	// API-version negotiation already happened. This option cannot be
+	// used if the client is configured with a fixed version using (using
+	// [WithVersion] or [WithVersionFromEnv]).
+	//
+	// This option has no effect if NegotiateAPIVersion is not set.
+	ForceNegotiate bool
 }
 
 // PingResult holds the result of a [Client.Ping] API call.
@@ -50,6 +72,30 @@ type SwarmStatus struct {
 // 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, options PingOptions) (PingResult, error) {
+	if cli.manualOverride {
+		return cli.ping(ctx)
+	}
+	if !options.NegotiateAPIVersion && !cli.negotiateVersion {
+		return cli.ping(ctx)
+	}
+
+	// Ensure exclusive write access to version and negotiated fields
+	cli.negotiateLock.Lock()
+	defer cli.negotiateLock.Unlock()
+
+	ping, err := cli.ping(ctx)
+	if err != nil {
+		return cli.ping(ctx)
+	}
+
+	if cli.negotiated.Load() && !options.ForceNegotiate {
+		return ping, nil
+	}
+
+	return ping, cli.negotiateAPIVersion(ping.APIVersion)
+}
+
+func (cli *Client) ping(ctx context.Context) (PingResult, error) {
 	// Using cli.buildRequest() + cli.doRequest() instead of cli.sendRequest()
 	// because ping requests are used during API version negotiation, so we want
 	// to hit the non-versioned /_ping endpoint, not /v1.xx/_ping