Improve API version handling by paperbenni · Pull Request #1187 · fsouza/go-dockerclient

paperbenni · 2026-06-05T19:14:05Z

The client requests the lowest API version containing all the features used in a request. This used to just provide support for older docker versions if newer features simply weren't used. It causes the client to be rejeceted by new docker versions because old API versions are explicitly no longer supported. This changes API version negotiation to be compatible with newer docker versions

This makes versioning negotiation somewhat more complex, requiring the other changes to make thread safe.
The race conditions were technically preexisting but rarely triggered because checking the server version was skipped most of the time

Closes #1030.

PullImageOptions and BuildImageOptions already expose this field.

The ver struct tags describe the minimum Docker API version required by an option. They should not decide the request URL version. Reusing those capability minimums as URL versions can make modern daemons reject otherwise valid requests. A field introduced in an old API version may still be valid when sent to a daemon whose minimum supported request version has moved past that old value. Keep explicit API versions authoritative, validate them against the option minimum, and use the negotiated/effective server version when version checks are enabled. When version checks are skipped and no explicit version was requested, keep the path unversioned instead of inventing /v<minimum>/ from a tag. Add focused pathVersionCheck coverage for explicit, negotiated, skipped, insufficient, and nil-required-version cases, plus image/build regressions for the behavior that originally exposed the bug.

- Store serverAPIVersion and expectedAPIVersion in atomic.Value and guard updates with a mutex - Add helpers: getExpectedVersion, getServerVersion, ensureAPIVersion, ensureServerVersion, setExpectedVersion, ensureServerVersionForCompatibility - Replace direct version checks with the new helpers in request and stream paths - Use ensureServerVersionForCompatibility to bypass checks when requested - Update tests to align with atomic version storage and negotiated paths

fsouza

Thanks for working on this — the approach is right and it does fix #1030 (the ver-tag minimum versions no longer leak into request URLs, and unversioned clients negotiate the server's version instead). A few issues to sort out before this can merge, though. The first three inline comments are the substantive ones (a behavior regression in ExportImages, a silent-failure regression for default NewClient users, and a hang risk in the new probe locking); the fourth is an API-compatibility question; the rest are smaller cleanups.

fsouza · 2026-08-22T15:25:26Z

+	} else if c.requestedAPIVersion != nil {
+		effectiveVersion = c.requestedAPIVersion
+	}
+	if effectiveVersion != nil && effectiveVersion.GreaterThanOrEqualTo(apiVersion125) {


This breaks multi-image export for negotiating clients. The comma-join format now applies whenever the negotiated server version is >= 1.25, but the daemon's /images/get handler reads r.Form["names"] without splitting on commas — the joined string goes straight to reference.ParseAnyReference, which rejects commas. So ExportImages with two names against a modern daemon, which worked before (repeated names= params), now errors. The "1.25 uses comma-separated" note traces back to a Swagger collectionFormat artifact, not actual daemon behavior. I'd drop the comma-join branch entirely and always send repeated names= params (note TestExportImagesNegotiatedVersionFirstCall currently locks in the broken wire format). Also, if the join does stay for some reason, strings.Join(opts.Names, ",") would replace the manual loop.

fsouza · 2026-08-22T15:25:26Z

-	return fmt.Sprintf("%s%s?%s", urlStr, basepath, queryStr), nil
+	// Return only a request path. The caller applies the endpoint and selected
+	// API version later through getURL.
+	return fmt.Sprintf("%s?%s", basepath, queryStr), nil


For SkipServerVersionCheck clients — which is the default for NewClient/NewTLSClient — this now neither pins the URL to requiredAPIVersion nor validates anything: ensureAPIVersion is a no-op under skip, expectedAPIVersion stays nil, and the request goes out unversioned. Before, PullImage with Platform (ver:"1.32") against an old daemon was forced to /v1.32/... and failed loudly; now the old daemon silently ignores the unknown platform param and "successfully" pulls the wrong-architecture image. When skip is on and there's no expected version, I think the required version still needs to be either pinned into the URL (old behavior) or validated somehow, rather than dropped.

fsouza · 2026-08-22T15:25:26Z

+	}
+	// Serialize only the first /version request; version reads stay lock-free
+	// once the server version has been cached.
+	c.versionMu.Lock()


versionMu is held across the /version HTTP round-trip, which runs on context.Background() (and defaultClient() sets no http.Client.Timeout). A daemon that accepts connections but stalls on /version hangs the first caller forever and parks every other do()/stream()/hijack() call — plus the unconditional probes in StartContainer/CreateExec/CopyFromContainer — on an uncancellable mutex. And since failures are never cached, a persistently failing /version turns N concurrent calls into N serialized probes. Consider doing the HTTP call outside the lock (e.g. singleflight-style, or accept a rare duplicate probe), threading the caller's context into the probe, and/or caching probe failure.

fsouza · 2026-08-22T15:25:26Z

-	expectedAPIVersion  APIVersion
+	// serverAPIVersion and expectedAPIVersion are read while requests are built,
+	// so keep access lock-free after the first version probe completes.
+	serverAPIVersion   atomicAPIVersion


Adding a sync.Mutex and atomic.Pointer-based fields makes the exported Client struct non-copyable: c2 := *client was vet-clean before and now trips copylocks (this PR itself had to change newTestClient from returning Client to *Client). Downstream code that copies Client by value will start failing vet-gated CI, and a runtime copy forks the version cache or carries a locked mutex. Maybe unavoidable for the thread-safety goal, but worth an explicit decision — and a mention in the changelog if we accept it.

fsouza · 2026-08-22T15:25:26Z

 		return err
 	}
-	return c.streamURL(http.MethodGet, exporturl, streamOptions{
+	return c.stream(http.MethodGet, exportpath, streamOptions{


opts.Context never reaches the HTTP request: it's copied into the inner options struct (where it has no effect on the query string) but not set as context: in these streamOptions, so stream substitutes context.Background() and a cancelled/deadline context can't abort a hung export. Pre-existing omission, but since this function is being rewritten anyway, worth adding context: opts.Context here.

fsouza · 2026-08-22T15:25:26Z

 	if c.endpointURL.Scheme == unixProtocol || c.endpointURL.Scheme == namedPipeProtocol {
 		urlStr = ""
 	}
+	if expected := c.expectedAPIVersion.Load(); expected != nil {


The effective-version precedence rule (expectedAPIVersion, else requestedAPIVersion) is now hand-copied in getURL, getFakeNativeURL, and ExportImages, while pathVersionCheck implements a fourth variant that checks requestedAPIVersion first. They agree today only because setExpectedVersion mirrors requestedAPIVersion verbatim — the moment negotiation gets smarter (e.g. clamping a requested version to the server's max, as moby does), all four sites must change in lockstep or validation and URL-building diverge. A single c.effectiveAPIVersion() accessor used everywhere would remove that risk.

fsouza · 2026-08-22T15:25:26Z

-	if path != "/version" && !c.SkipServerVersionCheck && c.expectedAPIVersion == nil {
-		err := c.checkAPIVersion()
-		if err != nil {
+	if path != "/version" {


This literal path != "/version" guard (repeated in do, stream, and hijack) is now the only thing preventing the probe's own do("/version", ...) from re-entering probeServerVersion while the non-reentrant versionMu is held — i.e. it's a deadlock guard, but nothing documents that. Any future change that makes the probe path not byte-equal to "/version" (switching to /_ping, adding a query param) silently self-deadlocks the first call on a fresh client. An explicit doOptions flag (e.g. skipVersionNegotiation) set by getServerAPIVersionString would encode the invariant where it belongs.

fsouza · 2026-08-22T15:25:26Z

-		c.checkAPIVersion()
-	}
-	if len(opts.Env) > 0 && c.serverAPIVersion.LessThan(apiVersion125) {
+	c.probeServerVersion()


Best-effort probing is scattered as bare c.probeServerVersion() calls with ignored errors here and in container_copy.go, container_start.go, and image.go (versionedAuthConfigs), and each site has a different nil-version policy: exec treats nil as "too old, hard error", container_copy proceeds, container_start picks the new wire format. Also note probeServerVersion itself never checks SkipServerVersionCheck, so default NewClient users now pay a real /version round-trip on every one of these calls while the endpoint is failing (nothing is negatively cached), serialized under versionMu. A single helper expressing the best-effort contract — including the skip check and ideally failure caching — would fix both.

fsouza · 2026-08-22T15:25:26Z


 	// if the caller elected to skip checking the server's version, assume it's the latest
-	if c.SkipServerVersionCheck || c.expectedAPIVersion.GreaterThanOrEqualTo(apiVersion112) {
+	if c.SkipServerVersionCheck {


The SkipServerVersionCheck branch and the >= 1.12 branch below have identical bodies (decode resp.Body into image); the original expressed this once behind a combined condition. Something like v := c.expectedAPIVersion.Load(); if c.SkipServerVersionCheck || (v != nil && v.GreaterThanOrEqualTo(apiVersion112)) { ...decode... } else { ...legacy re-request... } keeps the semantics without the duplication.

fsouza · 2026-08-22T15:25:26Z

+
+// setExpectedVersion records the version used in request URLs. It may differ
+// from the server version when the caller requested an explicit API version.
+func (c *Client) setExpectedVersion(serverVersion APIVersion) {


Minor structural point: setExpectedVersion re-checks the exact guard ensureAPIVersion just evaluated and is called from three separate points inside probeServerVersion, so a reader has to trace all three to learn which call actually stores. Simpler shape: probeServerVersion only caches serverAPIVersion, and ensureAPIVersion performs the single expected-version store after a successful probe. (Similarly, both success branches of pathVersionCheck end in the identical Sprintf — that can collapse to one shared return after the error checks.)

benjamin and others added 13 commits May 18, 2026 10:58

ImportImageOptions: add Platform field for cross-arch image import

0485ec9

PullImageOptions and BuildImageOptions already expose this field.

Merge branch 'main' of https://github.com/fsouza/go-dockerclient

aab0813

Merge branch 'main' of ufr:~/workspace/go-dockerclient

2b0c9db

Merge branch 'main' of ufr:~/workspace/go-dockerclient

972c603

refactor: restore type safety

81d8842

Merge branch 'main' of https://github.com/fsouza/go-dockerclient

ac42fe7

Merge branch 'main' of ufr:~/workspace/go-dockerclient

100826c

Use atomicAPIVersion for API version management

ac46126

Merge branch 'main' of ufr:~/workspace/go-dockerclient

89fdcb4

refactor: remove redundant function

6446e79

refactor: rename functions for clarity

fc7c323

paperbenni marked this pull request as ready for review June 15, 2026 13:19

paperbenni mentioned this pull request Jun 17, 2026

With the Latest Docker Engine v25.x the Version Selection Magic Based on the Call Option 'ver` Tags Results in Failing Calls #1030

Open

fsouza reviewed Aug 22, 2026

View reviewed changes

paperbenni added 6 commits September 7, 2026 10:43

Merge branch 'main' of https://github.com/fsouza/go-dockerclient

a139045

fix: comma splitting logic

0653373

fix: centralize explicit best effort version detection

465e13e

fix: fail loudly on old versions

0a777d0

fix: singleflight probeServerVersion and context cancellation

148f76f

fix: pass context from ExportImagesOptions to streamOptions

2dcc5cb

Navigation Menu

Improve API version handling - #1187

Improve API version handling#1187
paperbenni wants to merge 19 commits into
fsouza:mainfrom
paperbenni:main

paperbenni commented Jun 5, 2026 •

edited by fsouza

Loading

Uh oh!

fsouza left a comment

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

fsouza Aug 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

paperbenni commented Jun 5, 2026 • edited by fsouza Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsouza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

paperbenni commented Jun 5, 2026 •

edited by fsouza

Loading