Merge pull request #51533 from thaJeztah/client_refactor_negotiate

client: simplify logic for manual vs auto API versions

client/client.go

@@ -212,9 +212,9 @@ func New(ops ...Opt) (*Client, error) {
 	}
 
 	if cfg.envAPIVersion != "" {
-		cfg.version = cfg.envAPIVersion
+		c.setAPIVersion(cfg.envAPIVersion)
 	} else if cfg.manualAPIVersion != "" {
-		cfg.version = cfg.manualAPIVersion
+		c.setAPIVersion(cfg.manualAPIVersion)
 	}
 
 	if tr, ok := c.client.Transport.(*http.Transport); ok {
@@ -282,7 +282,7 @@ 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() {
+	if cli.negotiated.Load() || !cli.negotiateVersion {
 		return nil
 	}
 	_, err := cli.Ping(ctx, PingOptions{
@@ -311,15 +311,16 @@ func (cli *Client) ClientVersion() string {
 
 // 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
-// the lowest supported API version, in which case the version is not modified.
+// API version is empty.
+//
+// It returns an error if version is invalid, or lower than the minimum
+// supported API version in which case the client's API version is not
+// updated, and negotiation is not marked as completed.
 func (cli *Client) negotiateAPIVersion(pingVersion string) error {
 	pingVersion = strings.TrimPrefix(pingVersion, "v")
 	if pingVersion == "" {
 		// TODO(thaJeztah): consider returning an error on empty value or not falling back; see https://github.com/moby/moby/pull/51119#discussion_r2413148487
 		pingVersion = MinAPIVersion
-	} else if versions.LessThan(pingVersion, MinAPIVersion) {
-		return cerrdefs.ErrInvalidArgument.WithMessage(fmt.Sprintf("API version %s is not supported by this client: the minimum supported API version is %s", pingVersion, MinAPIVersion))
 	}
 
 	var err error
@@ -328,24 +329,35 @@ func (cli *Client) negotiateAPIVersion(pingVersion string) error {
 		return err
 	}
 
+	if versions.LessThan(pingVersion, MinAPIVersion) {
+		return cerrdefs.ErrInvalidArgument.WithMessage(fmt.Sprintf("API version %s is not supported by this client: the minimum supported API version is %s", pingVersion, MinAPIVersion))
+	}
+
 	// if the client is not initialized with a version, start with the latest supported version
-	if cli.version == "" {
-		cli.version = MaxAPIVersion
+	negotiatedVersion := cli.version
+	if negotiatedVersion == "" {
+		negotiatedVersion = MaxAPIVersion
 	}
 
 	// if server version is lower than the client version, downgrade
-	if versions.LessThan(pingVersion, cli.version) {
-		cli.version = pingVersion
+	if versions.LessThan(pingVersion, negotiatedVersion) {
+		negotiatedVersion = pingVersion
 	}
 
 	// Store the results, so that automatic API version negotiation (if enabled)
 	// won't be performed on the next request.
-	if cli.negotiateVersion {
-		cli.negotiated.Store(true)
-	}
+	cli.setAPIVersion(negotiatedVersion)
 	return nil
 }
 
+// setAPIVersion sets the client's API version and marks API version negotiation
+// as completed, so that automatic API version negotiation (if enabled) won't
+// be performed on the next request.
+func (cli *Client) setAPIVersion(version string) {
+	cli.version = version
+	cli.negotiated.Store(true)
+}
+
 // DaemonHost returns the host address used by the client
 func (cli *Client) DaemonHost() string {
 	return cli.host

client/client_options.go

@@ -38,8 +38,6 @@ type clientConfig struct {
 	userAgent *string
 	// custom HTTP headers configured by users.
 	customHTTPHeaders map[string]string
-	// manualOverride is set to true when the version was set by users.
-	manualOverride bool
 
 	// manualAPIVersion contains the API version set by users. This field
 	// will only be non-empty if a valid-formed version was set through
@@ -278,7 +276,6 @@ func WithAPIVersion(version string) Opt {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
 			c.manualAPIVersion = ver
-			c.manualOverride = true
 		}
 		return nil
 	}
@@ -311,7 +308,6 @@ func WithAPIVersionFromEnv() Opt {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
 			c.envAPIVersion = ver
-			c.manualOverride = true
 		}
 		return nil
 	}

client/client_options_test.go

@@ -141,7 +141,7 @@ func TestOptionWithAPIVersion(t *testing.T) {
 				assert.Check(t, client != nil)
 				assert.Check(t, is.Equal(client.ClientVersion(), tc.expected))
 				isNoOp := strings.TrimPrefix(strings.TrimSpace(tc.version), "v") == ""
-				assert.Check(t, is.Equal(client.manualOverride, !isNoOp))
+				assert.Check(t, is.Equal(client.negotiated.Load(), !isNoOp))
 			}
 		})
 	}
@@ -244,7 +244,7 @@ func TestOptionWithAPIVersionFromEnv(t *testing.T) {
 				assert.Check(t, client != nil)
 				assert.Check(t, is.Equal(client.ClientVersion(), tc.expected))
 				isNoOp := strings.TrimPrefix(strings.TrimSpace(tc.version), "v") == ""
-				assert.Check(t, is.Equal(client.manualOverride, !isNoOp))
+				assert.Check(t, is.Equal(client.negotiated.Load(), !isNoOp))
 			}
 		})
 	}
@@ -258,7 +258,7 @@ func TestOptionOverridePriority(t *testing.T) {
 		client, err := New(WithAPIVersionFromEnv(), WithAPIVersion("1.50"))
 		assert.NilError(t, err)
 		assert.Check(t, is.Equal(client.ClientVersion(), "1.50"))
-		assert.Check(t, is.Equal(client.manualOverride, true))
+		assert.Check(t, is.Equal(client.negotiated.Load(), true))
 	})
 
 	const expected = "1.51"
@@ -268,28 +268,28 @@ func TestOptionOverridePriority(t *testing.T) {
 		client, err := New(WithAPIVersionFromEnv(), WithAPIVersion("1.50"))
 		assert.NilError(t, err)
 		assert.Check(t, is.Equal(client.ClientVersion(), expected))
-		assert.Check(t, is.Equal(client.manualOverride, true))
+		assert.Check(t, is.Equal(client.negotiated.Load(), true))
 	})
 
 	t.Run("WithAPIVersionFromEnv last", func(t *testing.T) {
 		client, err := New(WithAPIVersion("1.50"), WithAPIVersionFromEnv())
 		assert.NilError(t, err)
 		assert.Check(t, is.Equal(client.ClientVersion(), expected))
-		assert.Check(t, is.Equal(client.manualOverride, true))
+		assert.Check(t, is.Equal(client.negotiated.Load(), true))
 	})
 
 	t.Run("FromEnv first", func(t *testing.T) {
 		client, err := New(FromEnv, WithAPIVersion("1.50"))
 		assert.NilError(t, err)
 		assert.Check(t, is.Equal(client.ClientVersion(), expected))
-		assert.Check(t, is.Equal(client.manualOverride, true))
+		assert.Check(t, is.Equal(client.negotiated.Load(), true))
 	})
 
 	t.Run("FromEnv last", func(t *testing.T) {
 		client, err := New(WithAPIVersion("1.50"), FromEnv)
 		assert.NilError(t, err)
 		assert.Check(t, is.Equal(client.ClientVersion(), expected))
-		assert.Check(t, is.Equal(client.manualOverride, true))
+		assert.Check(t, is.Equal(client.negotiated.Load(), true))
 	})
 }
 

client/ping.go

@@ -71,7 +71,7 @@ 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 || cli.negotiated.Load()) && !options.ForceNegotiate {
+	if cli.negotiated.Load() && !options.ForceNegotiate {
 		// API version was already negotiated or manually set.
 		return cli.ping(ctx)
 	}

vendor/github.com/moby/moby/client/client.go

@@ -212,9 +212,9 @@ func New(ops ...Opt) (*Client, error) {
 	}
 
 	if cfg.envAPIVersion != "" {
-		cfg.version = cfg.envAPIVersion
+		c.setAPIVersion(cfg.envAPIVersion)
 	} else if cfg.manualAPIVersion != "" {
-		cfg.version = cfg.manualAPIVersion
+		c.setAPIVersion(cfg.manualAPIVersion)
 	}
 
 	if tr, ok := c.client.Transport.(*http.Transport); ok {
@@ -282,7 +282,7 @@ 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() {
+	if cli.negotiated.Load() || !cli.negotiateVersion {
 		return nil
 	}
 	_, err := cli.Ping(ctx, PingOptions{
@@ -311,15 +311,16 @@ func (cli *Client) ClientVersion() string {
 
 // 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
-// the lowest supported API version, in which case the version is not modified.
+// API version is empty.
+//
+// It returns an error if version is invalid, or lower than the minimum
+// supported API version in which case the client's API version is not
+// updated, and negotiation is not marked as completed.
 func (cli *Client) negotiateAPIVersion(pingVersion string) error {
 	pingVersion = strings.TrimPrefix(pingVersion, "v")
 	if pingVersion == "" {
 		// TODO(thaJeztah): consider returning an error on empty value or not falling back; see https://github.com/moby/moby/pull/51119#discussion_r2413148487
 		pingVersion = MinAPIVersion
-	} else if versions.LessThan(pingVersion, MinAPIVersion) {
-		return cerrdefs.ErrInvalidArgument.WithMessage(fmt.Sprintf("API version %s is not supported by this client: the minimum supported API version is %s", pingVersion, MinAPIVersion))
 	}
 
 	var err error
@@ -328,24 +329,35 @@ func (cli *Client) negotiateAPIVersion(pingVersion string) error {
 		return err
 	}
 
+	if versions.LessThan(pingVersion, MinAPIVersion) {
+		return cerrdefs.ErrInvalidArgument.WithMessage(fmt.Sprintf("API version %s is not supported by this client: the minimum supported API version is %s", pingVersion, MinAPIVersion))
+	}
+
 	// if the client is not initialized with a version, start with the latest supported version
-	if cli.version == "" {
-		cli.version = MaxAPIVersion
+	negotiatedVersion := cli.version
+	if negotiatedVersion == "" {
+		negotiatedVersion = MaxAPIVersion
 	}
 
 	// if server version is lower than the client version, downgrade
-	if versions.LessThan(pingVersion, cli.version) {
-		cli.version = pingVersion
+	if versions.LessThan(pingVersion, negotiatedVersion) {
+		negotiatedVersion = pingVersion
 	}
 
 	// Store the results, so that automatic API version negotiation (if enabled)
 	// won't be performed on the next request.
-	if cli.negotiateVersion {
-		cli.negotiated.Store(true)
-	}
+	cli.setAPIVersion(negotiatedVersion)
 	return nil
 }
 
+// setAPIVersion sets the client's API version and marks API version negotiation
+// as completed, so that automatic API version negotiation (if enabled) won't
+// be performed on the next request.
+func (cli *Client) setAPIVersion(version string) {
+	cli.version = version
+	cli.negotiated.Store(true)
+}
+
 // DaemonHost returns the host address used by the client
 func (cli *Client) DaemonHost() string {
 	return cli.host

vendor/github.com/moby/moby/client/client_options.go

@@ -38,8 +38,6 @@ type clientConfig struct {
 	userAgent *string
 	// custom HTTP headers configured by users.
 	customHTTPHeaders map[string]string
-	// manualOverride is set to true when the version was set by users.
-	manualOverride bool
 
 	// manualAPIVersion contains the API version set by users. This field
 	// will only be non-empty if a valid-formed version was set through
@@ -278,7 +276,6 @@ func WithAPIVersion(version string) Opt {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
 			c.manualAPIVersion = ver
-			c.manualOverride = true
 		}
 		return nil
 	}
@@ -311,7 +308,6 @@ func WithAPIVersionFromEnv() Opt {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
 			c.envAPIVersion = ver
-			c.manualOverride = true
 		}
 		return nil
 	}

vendor/github.com/moby/moby/client/ping.go

@@ -71,7 +71,7 @@ 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 || cli.negotiated.Load()) && !options.ForceNegotiate {
+	if cli.negotiated.Load() && !options.ForceNegotiate {
 		// API version was already negotiated or manually set.
 		return cli.ping(ctx)
 	}