client: remove support for negotiating API version < v1.44 (docker 25.0) #51119
Conversation
d8b7fb3 to
f5a5826
Compare
|
This was taken from a larger branch I had; double check if I didn't make mistakes in the logic; there's still some areas in the |
| // 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 types.Ping) { |
There was a problem hiding this comment.
I'd like to get rid of this function; it's used in the CLI currently to use a "Ping" result that was triggered in some code, that uses the Ping for feature detection, then (likely only to avoid pinging "twice") reuses the ping result as optimization, but we probably shouldn't, and maybe either optimise the "ping" to only happen once for the lifecycle of the client, or have a method to query features.
| // FIXME(thaJeztah): we should not swallow the error here, and instead returning it. | ||
| _ = cli.negotiateAPIVersion(pingResponse.APIVersion) |
There was a problem hiding this comment.
If we do keep it, we should probably change the signature to return the error.
| doc: "no downgrade old", | ||
| pingVersion: "1.19", | ||
| expectedVersion: "1.19", | ||
| expectedVersion: MaxAPIVersion, |
There was a problem hiding this comment.
If the ping response has no version, the client will use fallbackVersion - but if it has a too-low version the client now uses its current version ... maybe it should be fallbackVersion in both cases?
There was a problem hiding this comment.
If the ping response has no version, the client will use
fallbackVersion
Yes, this is something we should also look at; that behavior was based on "versions before 1.24 didn't have API negotiation, so would return an empty string", in which case we assumed "1.23". But perhaps we should also stop doing so; consider negotiation failed, and don't do anything; continue with "current" API version.
but if it has a too-low version the client now uses its current version ... maybe it should be fallbackVersion
I thought about that for a minute, but then considered that falling back to a lower API version wouldn't help if that fallback version is still too high for the daemon. In this case, the daemon DID respond with an API version that it supports, and the client found that it doesn't support it.
If we don't fall back in that case, the daemon will reply with an error message, making it clear to the user "sorry, can't do";
curl --unix-socket /var/run/docker.sock 'http://localhost/v1.53/info'
{"message":"client version 1.53 is too new. Maximum supported API version is 1.52"}Well... except for Docker Desktop, which still has a bug because the proxy ignores the error, and returns a modified response in some cases, based on an empty struct;
curl --unix-socket /var/run/docker.sock 'http://localhost/v1.53/info'
{"ID":"","Containers":0,"ContainersRunning":0,"ContainersPaused":0,"ContainersStopped":0,"Images":0,"Driver":"","DriverStatus":null,"Plugins":{"Volume":null,"Network":null,"Authorization":null,"Log":null},"MemoryLimit":false,"SwapLimit":false,"CpuCfsPeriod":false,"CpuCfsQuota":false,"CPUShares":false,"CPUSet":false,"PidsLimit":false,"IPv4Forwarding":false,"Debug":false,"NFd":0,"OomKillDisable":false,"NGoroutines":0,"SystemTime":"","LoggingDriver":"","CgroupDriver":"","NEventsListener":0,"KernelVersion":"","OperatingSystem":"","OSVersion":"","OSType":"","Architecture":"","IndexServerAddress":"","RegistryConfig":null,"NCPU":0,"MemTotal":0,"GenericResources":null,"DockerRootDir":"","HttpProxy":"","HttpsProxy":"","NoProxy":"","Name":"","Labels":["com.docker.desktop.address=unix:///Users/thajeztah/Library/Containers/com.docker.docker/Data/docker-cli.sock"],"ExperimentalBuild":false,"ServerVersion":"","Runtimes":null,"DefaultRuntime":"","Swarm":{"NodeID":"","NodeAddr":"","LocalNodeState":"","ControlAvailable":false,"Error":"","RemoteManagers":null},"LiveRestoreEnabled":false,"Isolation":"","InitBinary":"","ContainerdCommit":{"ID":""},"RuncCommit":{"ID":""},"InitCommit":{"ID":""},"SecurityOptions":null,"CDISpecDirs":null,"Warnings":null}⏎Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2831a1f to
8bb04bd
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes support for negotiating API versions below v1.44 (Docker 25.0) to align with Docker's EOL policy. Docker versions below 25.0 are no longer supported, with 25.0 being maintained as an LTS version by Mirantis.
- Raises the fallback API version from v1.24 to v1.44 for version negotiation
- Updates error handling to reject unsupported API versions during negotiation
- Modernizes test cases to use supported API versions and adds validation for version override behavior
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client/client.go | Updates fallback API version to v1.44 and adds validation to reject unsupported versions during negotiation |
| client/client_test.go | Updates test cases with supported API versions and adds test for manual version override |
| client/request.go | Updates comment to remove reference to legacy API version behavior |
| client/request_test.go | Updates comment to clarify plain text error handling without version-specific references |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "sync/atomic" | ||
| "time" | ||
|
|
||
| cerrdefs "github.com/containerd/errdefs" |
There was a problem hiding this comment.
Missing import of 'fmt' package which is used in line 362 for fmt.Sprintf(). This will cause a compilation error.
There was a problem hiding this comment.
I know your employer decides to hide that part of the diff, Copilot, but if you scroll up a bit, you'll find that it's there ... really!
https://github.com/thaJeztah/docker/blob/8bb04bdc3854c182dd83027ef7f372b0d206eb0a/client/client.go#L47
Docker versions below 25.0 have reached EOL; 25.0 is currently maintained as an LTS version by Mirantis, and we want to allow current versions of the CLI to be able to connect to such setups. This patch raises the fallback API version to API v1.44; when negotiating an API version with a daemon, this will be the lowest version negotiated. Currently, it still allows manually overriding the version to versions that are not supported (`WithVersion`, `WithVersionFromEnv`), and no code has been removed yet that adjusts the client for old API versions, but this can be done in a follow-up. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
8bb04bd to
96b29f5
Compare
|
Thanks! I'll bring this one in, so that we can work on the follow-ups |
|
This change may be a bit harsh on users:
I don't have a strong opinion on this. |
|
@vincentbernat , thanks for the feedback. If you are inclined, perhaps worth a thread over on #51468 (I just feel sometimes comments on closed PRs can get lost) The following is just my personal opinions not to be taken as the viewpoint of the moby project as a whole. I agree the decision to no longer support older APIs affected end users the most. To the best of my knowledge this is only the second time the engine has not supported a previous API version (The first time coming with the v26 release ref: https://docs.docker.com/engine/deprecated/#deprecate-legacy-api-versions)
Interesting point here. As an LTS user, I might see the deprecations as jarring; although, with that mindset I'm also unlikely to update to the new client/engine anytime soon. Certainly a trade-off of drift when deciding to stay LTS.
While I certainly empathize with that community (I use to do similar work at Amazon Linux), when Bookworm launched 20.10 was 6 months away from EOL. I do not know the context so not going to speculate further why that version was chosen but would be interested in learning more from downstream distributions regarding their support models. To be frank, I do not really expect to be able to provide 5 years of LTS support for a given branch. Maintainer burnout is real enough as is before the AI era. |
|
Debian is lagging a lot behind because packaging Go applications is complicated by the requirement to package each dependency independently and each new dependency requires a random-long review by an external team. This should not really be the problem of Docker, but at the end, you have users with older versions because they prefer to use what is in the distribution rather than adding a third-party repository (maybe they don't know about it or maybe they value stability as upgrading Docker can break things, like Traefik recently). I see that Ubuntu updates Docker with more recent versions in their LTS. I think this is smarter, but in Debian, this is really a hard exception to get (currently browsers get it, but there was a tentative for Virtualbox that was not granted and now Virtualbox is not part of Debian anymore). @tianon may chime him (even if he did not package Docker lately). But, again, I totally understand you cannot support everything. I don't intend to change your mind. My goal is just to raise awareness around these users.
As the discussion started here, I don't know if this is worth it. I could post a link. |
|
You've nailed it all, Vincent ❤️ It's not that Debian "chose" that version so much as "there are a lot more constraints on what goes into Debian than any one person can necessarily just choose" and the biggest constraint of all is that it's entirely based on typically unpaid volunteer labor, and packaging Go applications is especially painful: every version bump of every Go application brings a fresh new hell of dependency updates (many brand new, so review delay too especially for license compliance) that have to be packaged first, all while tracking and dealing with breakage elsewhere caused by the bumps (because Docker doesn't exist in a vacuum). To put that more succinctly: every single change to |
|
For the (public) record: I think these API cutoffs are a tad aggressive too, as we've discussed internally during the maintainer calls. |
|
Related to the above, the new embedded Go modules are also a hassle for downstream packaging, as the repository now represents several different source packages each with different versions and different consumers (we've also discussed this point internally, but probably less widely). |
client: remove support for negotiating API version < v1.44 (docker 25.0)
Docker versions below 25.0 have reached EOL; 25.0 is currently maintained
as an LTS version by Mirantis, and we want to allow current versions of the
CLI to be able to connect to such setups.
This patch raises the fallback API version to API v1.44; when negotiating an API
version with a daemon, this will be the lowest version negotiated.
Currently, it still allows manually overriding the version to versions that
are not supported (
WithVersion,WithVersionFromEnv), and no code hasbeen removed yet that adjusts the client for old API versions, but this
can be done in a follow-up.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)