Conversation
…gured go-dockerclient only parses an API version into the client`s internal requestedAPIVersion when the caller passes one, and getURL() consults that field alone when building a request path. With DOCKER_API_VERSION unset - the default for a plain "mint build" - every request therefore goes out without a "/vX.Y/" segment. A daemon reached through a proxy, which is what dind gives a CI pipeline, reads an unversioned request as coming from the oldest client it supports and rejects it with "client version ... is too old". Setting DOCKER_API_VERSION by hand is the only workaround the issue thread found, and that is precisely because it is the only way the field gets populated. Probe the daemon once and rebuild the client with the version it reports, so requestedAPIVersion ends up set exactly as if the caller had configured it. If the probe fails, hand back the original client so the caller still sees the real connection error. Fixes mintoolkit#95 Signed-off-by: Eljees <3.14hell@gmail.com>
|
A note on the red checks, so they are not read as coming from this change: What I could run in isolation was green: Happy to rebase once the base build is sorted out, if that helps. |
|
looking |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several client construction paths remain unversioned, and the regression test needs isolation and stricter path matching.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Fixes unversioned Docker API requests when no API version is configured, particularly for proxy-backed daemons.
Changes:
- Probes the daemon API version and rebuilds clients with it.
- Updates four host/socket construction paths.
- Adds regression coverage for versioned requests.
| File | Summary |
|---|---|
pkg/crt/docker/dockerclient/client.go |
Moderate issues: TLS and DOCKER_HOST construction paths still bypass version discovery, leaving requests unversioned. Votes: 2 and 1. |
pkg/crt/docker/dockerclient/client_api_version_test.go |
Moderate issues: the test should restore DOCKER_HOST and match the exact unversioned /version path. Votes: 2 and 1. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The first commit covered only the plain-host and socket paths. The TLS
branches build their clients through NewVersionedTLSClientFromBytes and the
DOCKER_HOST branch through NewClientFromEnv; both leave requestedAPIVersion
unset when no version is configured, so a TLS or environment-configured
daemon behind a proxy hits the same unversioned-request rejection.
Version discovery now lives in withDiscoveredAPIVersion, which takes the
constructor to rebuild with. Each path hands over its own, so the rebuilt
client keeps that path's transport, certificates and endpoint:
- plain host and socket paths: docker.NewVersionedClient
- TLS paths: the file's own newTLSClient closure
- DOCKER_HOST path: docker.NewVersionedClientFromEnv
Tests:
- the regression test no longer leaks DOCKER_HOST into the process
environment (t.Setenv puts the original back) and matches the
unversioned "/version" probe exactly, so a later "/v1.44/version"
cannot be taken for it
- a second end-to-end test covers the DOCKER_HOST path
- four unit tests cover the shared discovery logic, including that the
caller's own constructor receives the discovered version, which is
what the TLS paths rely on
go test ./pkg/crt/docker/dockerclient/... gives 6 green tests with this
change. With master's client.go in place, both end-to-end tests fail on
`the /info request path "/info" carries no API version segment`.
Signed-off-by: Eljees <57435526+Eljees@users.noreply.github.com>
|
Both points taken; the first one changed the shape of the fix. Pushed as a follow-up commit rather than a force push, so the first round stays readable. TLS and
The test's environment. Coverage. A second end-to-end test drives the With One correction to my own comment from the first round: it claimed the lazy self-check had to be skipped because the bootstrap probe would ask a daemon for The three red jobs are still the ones failing on |
Fixes #95
The defect
dockerclient.New()builds its client withdocker.NewVersionedClient(host, config.APIVersion). go-dockerclient only parses that string into the client's internalrequestedAPIVersionwhen it is non-empty, andgetURL()consults that field alone when it builds a request path. So withDOCKER_API_VERSIONunset — the default for a plainmint build— every request the client sends goes out without a/vX.Y/segment.That is invisible against a daemon on a local socket. It is not invisible against a daemon reached through a proxy, which is what dind gives a CI pipeline: an unversioned request is read as coming from the oldest client the daemon supports, and the run dies with
client version ... is too old. Minimum supported API version is ....The client's own version negotiation does not rescue this. Its result is kept in
expectedAPIVersionand never copied back intorequestedAPIVersion, so the path stays unversioned. SettingDOCKER_API_VERSIONby hand is the workaround the thread arrived at precisely because it is the only thing that populates the field.The change
newVersionedClientwraps the construction: when the caller supplied a version, behaviour is exactly as before. When the caller did not, the daemon is asked once for its API version and the client is rebuilt with it, sorequestedAPIVersionends up set as if the caller had configured it by hand.Two things it deliberately does not do:
SkipServerVersionCheck = true. Without it, go-dockerclient's lazy self-check would issue its own/versionprobe — and that probe now goes throughgetURL()too, so it would ask for/v1.44/versioninstead of/version. This is the same flag the existingconfig.APIVersion != ""branches already set.Applied at the four call sites that construct a client from a host or a socket.
What I ran
Go 1.26.6,
go test ./pkg/crt/docker/dockerclient/...:client.goas it is onmaster: FAIL —the /info request path "/info" carries no API version segmentThe test stands in a fake daemon with
httptest.Server, answers the version probe truthfully, and asserts that the next request —Info()'sGET /info, the one a proxying daemon actually rejects — carries a version segment. No real Docker daemon needed.go vet ./pkg/crt/...over the whole tree could not complete in my container:containers/storageneedsgpgmeand the btrfs headers, which are not installed there. That is an environment limit rather than anything about this change; CI covers it.