From 4e2e2cde7e2a4259c54a12112c68a3a4d58847ac Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 14 Nov 2025 15:30:47 +0100 Subject: [PATCH] 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 --- client/client.go | 40 ++++++++++++------- client/client_options.go | 4 -- client/client_options_test.go | 14 +++---- client/ping.go | 2 +- vendor/github.com/moby/moby/client/client.go | 40 ++++++++++++------- .../moby/moby/client/client_options.go | 4 -- vendor/github.com/moby/moby/client/ping.go | 2 +- 7 files changed, 61 insertions(+), 45 deletions(-) diff --git a/client/client.go b/client/client.go index 5306e5b091..dd5f060b0a 100644 --- a/client/client.go +++ b/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 diff --git a/client/client_options.go b/client/client_options.go index 81b1e9797d..7236f88638 100644 --- a/client/client_options.go +++ b/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 } diff --git a/client/client_options_test.go b/client/client_options_test.go index 319e65c207..7271926578 100644 --- a/client/client_options_test.go +++ b/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)) }) } diff --git a/client/ping.go b/client/ping.go index 3238f29567..3f001ecf73 100644 --- a/client/ping.go +++ b/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) } diff --git a/vendor/github.com/moby/moby/client/client.go b/vendor/github.com/moby/moby/client/client.go index 5306e5b091..dd5f060b0a 100644 --- a/vendor/github.com/moby/moby/client/client.go +++ b/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 diff --git a/vendor/github.com/moby/moby/client/client_options.go b/vendor/github.com/moby/moby/client/client_options.go index 81b1e9797d..7236f88638 100644 --- a/vendor/github.com/moby/moby/client/client_options.go +++ b/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 } diff --git a/vendor/github.com/moby/moby/client/ping.go b/vendor/github.com/moby/moby/client/ping.go index 3238f29567..3f001ecf73 100644 --- a/vendor/github.com/moby/moby/client/ping.go +++ b/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) }