fix: add Docker API version negotiation to prevent client/daemon mismatch#761
fix: add Docker API version negotiation to prevent client/daemon mismatch#761AlexKantor87 merged 3 commits intomainfrom
Conversation
The Docker SDK bump from v28.0.4 to v28.3.2 changed the default API version from 1.48 to 1.51. This breaks any environment where the Docker daemon is older than 28.3 (e.g. GitHub Actions ubuntu-24.04 runners which ship Docker Engine 28.0.4, max API 1.48). Adding client.WithAPIVersionNegotiation() makes the client auto-downgrade to match whatever API version the daemon supports, as recommended by Docker's own documentation. In internal/docker/docker.go, a newDockerClient() helper centralises client creation so the negotiation option cannot be accidentally omitted in future call sites. Includes a regression test that verifies the client can Ping the daemon and that the negotiated API version does not exceed the server maximum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @AlexKantor87's task in 2m 4s —— View job PR Review: Docker API version negotiation
Static AnalysisAll checks pass: SummaryThe fix is correct and well-motivated. Issues1. Centralisation is incomplete (main concern) The This means a future contributor could add a new call site in 2. Unrelated whitespace change ( A trailing-space removal on a comment line. Harmless but adds noise to the diff. See inline comment. 3. No regression test The PR description mentions What's good
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // remote docker registry | ||
| func RemoteDockerImageSha256(imageName, imageTag, registryEndPoint, registryToken string, logger *logger.Logger) (string, error) { | ||
| // Some docker images have Manifest list, aka “fat manifest” which combines | ||
| // Some docker images have Manifest list, aka "fat manifest" which combines |
There was a problem hiding this comment.
Nit: this line only has a trailing-whitespace change (removed a trailing space from the comment). Not harmful, but it's unrelated to the fix — consider dropping it to keep the diff focused on the version-negotiation change.
| func CreateDockerArtifactsData() ([]*server.ServerData, error) { | ||
| result := []*server.ServerData{} | ||
| cli, err := client.NewClientWithOpts(client.FromEnv) | ||
| cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) |
There was a problem hiding this comment.
This call site and DockerImageSha256 in internal/digest/digest.go still use client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) directly instead of going through the new newDockerClient() helper.
The PR description says the helper was introduced to "centralise the option so it cannot be accidentally omitted in future call sites," but these two call sites don't use it. Consider one of:
- Make
newDockerClient()exported (NewDockerClient()) and call it from here anddigest.go. - Or accept the duplication and document the pattern in a comment.
Option 1 would fully deliver on the stated goal of centralisation.
| cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) | |
| cli, err := docker.NewDockerClient() |
internal/docker/docker_test.go
Outdated
| // After negotiation the client version must not exceed the server's max. | ||
| suite.Assert().NotEmpty(ping.APIVersion, | ||
| "server should report its API version in the ping response") | ||
| suite.Assert().LessOrEqual(cli.ClientVersion(), ping.APIVersion, |
There was a problem hiding this comment.
Minor: LessOrEqual does a lexicographic comparison on strings like "1.48" vs "1.51". This works for the current version scheme (single-digit major, two-digit minor) but would break if versions ever reach "1.100" (lexicographically "1.100" < "1.48").
Not urgent — Docker API versions have been incrementing slowly — but worth a comment or a numeric comparison for robustness.
internal/docker/docker_test.go
Outdated
| require.NoError(suite.T(), err) | ||
| require.NotEmpty(suite.T(), id) | ||
| err = RemoveDockerContainer(id) | ||
| func (suite *DockerTestSuite) TestRunAndRemoveDockerContainer() { |
There was a problem hiding this comment.
The old test also verified the error case (RunDockerContainer("not-existing-image")), which has been dropped. Consider adding it back as a table-driven subtest to maintain coverage of the error path.
Restores the original docker_test.go and only adds the new TestNewDockerClientNegotiatesAPIVersion regression test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| func CreateDockerArtifactsData() ([]*server.ServerData, error) { | ||
| result := []*server.ServerData{} | ||
| cli, err := client.NewClientWithOpts(client.FromEnv) | ||
| cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) |
There was a problem hiding this comment.
This call site and DockerImageSha256 in internal/digest/digest.go still construct the client inline rather than going through the newDockerClient() helper introduced in internal/docker/docker.go.
The PR description says the helper was introduced to "centralise the option so it cannot be accidentally omitted in future call sites," but two out of eight call sites don't use it. Consider exporting the helper as docker.NewDockerClient() and calling it from here and digest.go to fully deliver on that centralisation goal.
| cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) | |
| cli, err := docker.NewDockerClient() |
internal/docker/docker_test.go
Outdated
| // After negotiation the client version must not exceed the server's max. | ||
| suite.Assert().NotEmpty(ping.APIVersion, | ||
| "server should report its API version in the ping response") | ||
| suite.Assert().LessOrEqual(cli.ClientVersion(), ping.APIVersion, |
There was a problem hiding this comment.
Nit: LessOrEqual on strings does a lexicographic comparison. This works for current Docker API versions (e.g. "1.48" vs "1.51") but would give a wrong result if versions ever reach "1.100" ("1.100" < "1.48" lexicographically). Not urgent since Docker API versions increment slowly, but worth a brief comment noting the assumption.
| // remote docker registry | ||
| func RemoteDockerImageSha256(imageName, imageTag, registryEndPoint, registryToken string, logger *logger.Logger) (string, error) { | ||
| // Some docker images have Manifest list, aka “fat manifest” which combines | ||
| // Some docker images have Manifest list, aka "fat manifest" which combines |
There was a problem hiding this comment.
Nit: this line is a whitespace-only change (trailing space removed from the comment). Not harmful, but unrelated to the fix — consider dropping it to keep the diff focused.
The DOCKER_API_VERSION env var was masking version mismatch issues in CI by forcing all Docker clients to use API 1.45 regardless of the SDK default. Removing it means the existing Docker integration tests will naturally fail if any call site is missing WithAPIVersionNegotiation(), making the custom negotiation test redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| func CreateDockerArtifactsData() ([]*server.ServerData, error) { | ||
| result := []*server.ServerData{} | ||
| cli, err := client.NewClientWithOpts(client.FromEnv) | ||
| cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) |
There was a problem hiding this comment.
This call site still constructs the Docker client inline rather than going through the newDockerClient() helper introduced in internal/docker/docker.go.
The PR description says the helper was introduced to "centralise the option so it cannot be accidentally omitted in future call sites," but this site (and DockerImageSha256 in digest.go) don't use it. Consider exporting the helper as docker.NewDockerClient() and calling it from both locations:
| cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) | |
| cli, err := docker.NewDockerClient() |
This would require adding the import and renaming/exporting the helper, but it fully delivers on the centralisation goal and prevents future drift.
| // remote docker registry | ||
| func RemoteDockerImageSha256(imageName, imageTag, registryEndPoint, registryToken string, logger *logger.Logger) (string, error) { | ||
| // Some docker images have Manifest list, aka “fat manifest” which combines | ||
| // Some docker images have Manifest list, aka "fat manifest" which combines |
There was a problem hiding this comment.
Nit: this line is a whitespace-only change (trailing space removed from the comment). Not harmful, but unrelated to the fix — consider dropping it to keep the diff focused.
…761) The Docker SDK bump (v28.0.4 → v28.3.2) changed the default API version from 1.48 to 1.51, breaking Docker operations on hosts with older daemons. - Add WithAPIVersionNegotiation() to all 8 Docker client creation sites - Remove DOCKER_API_VERSION pin from test.yml so CI naturally catches missing negotiation in future Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
client.WithAPIVersionNegotiation()to all 8 Docker client creation sites across 3 filesinternal/docker/docker.go, extract anewDockerClient()helper to centralise the option so it cannot be accidentally omitted in future call sitesTestNewDockerClientNegotiatesAPIVersionthat verifies the client can Ping the daemon and the negotiated API version does not exceed the server maximumRoot cause
The Dependabot bump of
github.com/docker/dockerfrom v28.0.4 → v28.3.2 changed the SDK's default API version from 1.48 to 1.51. GitHub Actionsubuntu-24.04runners ship Docker Engine 28.0.4 (max API 1.48), so everykosli attestwith--artifact-type dockerfails with:This breaks the server master pipeline — all attestation steps fail, the trail is non-compliant, and deployments are blocked.
Impact
Blocked:
kosli-dev/servermaster branch (run #23892447496) — 5 attestations MISSING (snyk-scan, lint-src, lint-tests, validate-openapi, system-test). Both v2.14.0 and v2.15.0 are affected.Fix
WithAPIVersionNegotiation()makes the Docker client auto-downgrade its API version to match whatever the daemon supports, as recommended by Docker's documentation. This is a one-line change per call site, and makes the CLI resilient against any future SDK-to-Engine version gap.Files changed
internal/docker/docker.gonewDockerClient()helper with negotiation; replace 6 inlineNewClientWithOptscallsinternal/digest/digest.goWithAPIVersionNegotiation()toDockerImageSha256()cmd/kosli/snapshotDocker.goWithAPIVersionNegotiation()toCreateDockerArtifactsData()internal/docker/docker_test.goTestNewDockerClientNegotiatesAPIVersionregression testTest plan
TestNewDockerClientNegotiatesAPIVersionpasses — verifies Ping succeeds and negotiated version ≤ server maxkosli-dev/serverto use the new CLI version and confirm the pipeline goes green🤖 Generated with Claude Code