client: simplify logic for manual vs auto API versions

When manually setting the API version to use, automatic API version
negotiation should no longer be performed. Instead of keeping track
of these options individually, we can mark negotiation to have happend
if either the version was set manually, or if API version negotiation
took place.

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

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)
 	}