client: WithVersion: strip v-prefix when setting API version#49352
Merged
thaJeztah merged 1 commit intomoby:masterfrom Jan 30, 2025
Merged
client: WithVersion: strip v-prefix when setting API version#49352thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah merged 1 commit intomoby:masterfrom
Conversation
When overriding the API version through DOCKER_API_VERSION, no validation
happens on the given version. However, some code-paths in the client do
some minor normalizing, and strip the "v" prefix (if present) as part of
[`Client.getAPIPath()`][1].
This resulted in some inconsistent handling of the version that's set. For
example, [`Client.checkResponseErr()`][2] decides whether or not the API
response is expected to support errors in JSON format (`types.ErrorResponse`),
which would fail because `versions.GreaterThan()` does not strip the prefix,
therefore making the first element "zero" (ranking lower than any valid version).
Net result was "mixed" because of this; for example in the following, half
the output is handled correctly ("downgraded from 1.47"), but the response
is handled as < 1.23 (so printed as-is);
DOCKER_API_VERSION=v1.23 docker version
Client: Docker Engine - Community
Version: 27.5.1
API version: v1.23 (downgraded from 1.47)
Go version: go1.22.11
Git commit: 9f9e405
Built: Wed Jan 22 13:41:13 2025
OS/Arch: linux/amd64
Context: default
Error response from daemon: {"message":"client version 1.23 is too old. Minimum supported API version is 1.24, please upgrade your client to a newer version"}
Passing the version without v-prefix corrects this problem;
DOCKER_API_VERSION=1.23 docker version
Client: Docker Engine - Community
Version: 27.5.1
API version: 1.99 (downgraded from 1.47)
Go version: go1.22.11
Git commit: 9f9e405
Built: Wed Jan 22 13:41:13 2025
OS/Arch: linux/amd64
Context: default
Error response from daemon: client version 1.99 is too new. Maximum supported API version is 1.47
DOCKER_API_VERSION=v1.99 docker version
Client: Docker Engine - Community
Version: 27.5.1
API version: v1.99 (downgraded from 1.47)
Go version: go1.22.11
Git commit: 9f9e405
Built: Wed Jan 22 13:41:13 2025
OS/Arch: linux/amd64
Context: default
Error response from daemon: {"message":"client version 1.99 is too new. Maximum supported API version is 1.47"}
This patch strips the prefix when setting a custom version, so that
normalization happens consistently. The existing code to strip the
prefix in [`Client.getAPIPath()`][1] is kept for now, in case values
are set through other ways.
[1]: https://github.com/moby/moby/blob/47dc8d5dd8c9226ad53e22cdad2011a7ab48278a/client/client.go#L303-L309
[2]: https://github.com/moby/moby/blob/47dc8d5dd8c9226ad53e22cdad2011a7ab48278a/client/request.go#L231-L241
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
area/api
thaJeztah
commented
Jan 29, 2025
| @@ -194,8 +195,8 @@ func WithTLSClientConfigFromEnv() Opt { | |||
| // (see [WithAPIVersionNegotiation]). | |||
| func WithVersion(version string) Opt { | |||
| return func(c *Client) error { | |||
Member
Author
There was a problem hiding this comment.
FWIW, I think we should start looking at some of these to return an error; I know the DOCKER_API_VERSION at the time was introduced as a "power-user" feature and by design didn't do any validation, but I think it's becoming commonly used, and became a much more public option; one that may warrant an actual error when invalid.
I left that for now to not change the behavior, but something we should consider.
vvoland
approved these changes
Jan 30, 2025
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When overriding the API version through DOCKER_API_VERSION, no validation happens on the given version. However, some code-paths in the client do some minor normalizing, and strip the "v" prefix (if present) as part of
Client.getAPIPath().This resulted in some inconsistent handling of the version that's set. For example,
Client.checkResponseErr()decides whether or not the API response is expected to support errors in JSON format (types.ErrorResponse), which would fail becauseversions.GreaterThan()does not strip the prefix, therefore making the first element "zero" (ranking lower than any valid version).Net result was "mixed" because of this; for example in the following, half the output is handled correctly ("downgraded from 1.47"), but the response is handled as < 1.23 (so printed as-is);
Passing the version without v-prefix corrects this problem;
This patch strips the prefix when setting a custom version, so that normalization happens consistently. The existing code to strip the prefix in
Client.getAPIPath()is kept for now, in case values are set through other ways.- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)