diff --git a/pkg/docker/dockerclient/client.go b/pkg/docker/dockerclient/client.go index d62a9f0dbf..ec2094ceee 100644 --- a/pkg/docker/dockerclient/client.go +++ b/pkg/docker/dockerclient/client.go @@ -130,6 +130,65 @@ func GetUnixSocketAddr() (*SocketInfo, error) { return nil, fmt.Errorf("docker socket not found") } +// newVersionedClient builds a *docker.Client for host, honoring an explicit +// apiVersion when the caller set one. When apiVersion is empty (a plain +// "slim build"/"mint build" with no DOCKER_API_VERSION set - the common +// case in every slimtoolkit/slim#646 and mintoolkit/mint#95 report), +// go-dockerclient leaves the client's internal requestedAPIVersion nil for +// its whole life (it only parses config.APIVersion into that field when the +// string is non-empty), so every request the client sends omits the +// "/vX.Y/" path segment. A daemon reached through a proxy - dind +// (Docker-in-Docker: a CI runner's own container running a Docker daemon, +// the topology behind every #646/#95 report) - reads an unversioned +// request as coming from the oldest client it supports and rejects it +// ("client version ... is too old"). Probing the daemon's real API version +// with an initial Version() call and rebuilding the client with that +// version populates requestedAPIVersion exactly as if the caller had set +// DOCKER_API_VERSION by hand, which is the only workaround either issue +// thread ever found. +func newVersionedClient(host, apiVersion string) (*docker.Client, error) { + client, err := docker.NewVersionedClient(host, apiVersion) + if err != nil { + return nil, err + } + + if apiVersion != "" { + client.SkipServerVersionCheck = true + return client, nil + } + + env, err := client.Version() + if err != nil { + // Daemon unreachable or otherwise misbehaving: hand back the + // original unversioned client so callers see the same connection + // error they always would have, instead of masking it here. + return client, nil + } + + discovered := env.Get("ApiVersion") + if discovered == "" { + return client, nil + } + + versioned, err := docker.NewVersionedClient(host, discovered) + if err != nil { + return client, nil + } + + // The just-discovered version must be trusted as-is: go-dockerclient's + // own internal version bootstrap (triggered lazily by the first request + // when SkipServerVersionCheck is false) builds its own "/version" probe + // through getURL(), which - now that requestedAPIVersion is set - + // prefixes even that bootstrap call with "/vX.Y/", so it would ask a + // real daemon for "/v1.44/version" instead of "/version" and fail to + // parse the (missing) result. Skipping that redundant self-check is + // exactly what happens today when a caller sets DOCKER_API_VERSION by + // hand (see the config.APIVersion != "" branches above). + versioned.SkipServerVersionCheck = true + + return versioned, nil +} + // New creates a new Docker client instance func New(config *config.DockerClient) (*docker.Client, error) { var client *docker.Client @@ -184,15 +243,11 @@ func New(config *config.DockerClient) (*docker.Client, error) { case config.Host != "" && !config.UseTLS: - client, err = docker.NewVersionedClient(config.Host, config.APIVersion) + client, err = newVersionedClient(config.Host, config.APIVersion) if err != nil { return nil, err } - if config.APIVersion != "" { - client.SkipServerVersionCheck = true - } - log.Debug("dockerclient.New: new Docker client [3]") case config.Host == "" && @@ -230,15 +285,11 @@ func New(config *config.DockerClient) (*docker.Client, error) { } config.Host = socketInfo.Address - client, err = docker.NewVersionedClient(config.Host, config.APIVersion) + client, err = newVersionedClient(config.Host, config.APIVersion) if err != nil { return nil, err } - if config.APIVersion != "" { - client.SkipServerVersionCheck = true - } - log.Debug("dockerclient.New: new Docker client (default) [6]") default: diff --git a/pkg/docker/dockerclient/client_issue646_repro_test.go b/pkg/docker/dockerclient/client_issue646_repro_test.go new file mode 100644 index 0000000000..fb68c60fc2 --- /dev/null +++ b/pkg/docker/dockerclient/client_issue646_repro_test.go @@ -0,0 +1,93 @@ +package dockerclient + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/slimtoolkit/slim/pkg/app/master/config" +) + +// TestNewClientDefaultAPIVersion_Issue646 reproduces slimtoolkit/slim#646 +// (the same bug as mintoolkit/mint#95 — ported one-to-one from +// w157-mint/pkg/crt/docker/dockerclient/client_issue95_repro_test.go, +// prep run #157/#158; slim's dockerclient.New() (pkg/docker/dockerclient/ +// client.go) uses the exact same docker.NewVersionedClient(config.Host, +// config.APIVersion) + "SkipServerVersionCheck = true only when +// config.APIVersion != \"\"" logic as mint's pre-crt-refactor New(), +// confirmed by direct line-by-line comparison in prep run #154). +// +// When the caller does not set DOCKER_API_VERSION / config.APIVersion +// explicitly (the common case — a plain "slim build" from CI, every +// occurrence in the issue), dockerclient.New() builds a *docker.Client +// whose internal requestedAPIVersion stays nil for the whole life of the +// client. Because requestedAPIVersion is nil, every request that client +// sends is built WITHOUT a "/vX.Y/" path segment +// (vendor/github.com/fsouza/go-dockerclient/client.go:865-897). A Docker +// daemon that sits behind a proxy — dind (Docker-in-Docker: a container +// that itself runs a Docker daemon, the standard way Bitbucket/GitLab +// self-hosted CI runners give a pipeline Docker access without mounting +// the host socket) is the topology in the report — treats an unversioned +// request as coming from the oldest client it supports and rejects it +// with "client version ... is too old", the exact error text in #646. +// +// This test stands in a fake Docker daemon with httptest.Server (an +// in-process HTTP test server — no real Docker daemon needed) and records +// the path of the request dockerclient.New()'s client actually sends. It +// asserts the desired, fixed behavior: that the request path carries a +// version segment even when config.APIVersion was left empty by the +// caller. That is exactly what today's code does NOT do, so this test +// fails (red) against the unpatched client.New(). +func TestNewClientDefaultAPIVersion_Issue646(t *testing.T) { + var infoPath string + sawInfoRequest := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if r.URL.Path == "/version" { + // Answer the internal negotiation truthfully so checkAPIVersion() + // succeeds and the client proceeds to the real /info call. + _, _ = w.Write([]byte(`{"ApiVersion":"1.44"}`)) + return + } + infoPath = r.URL.Path + sawInfoRequest = true + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + cfg := &config.DockerClient{ + Host: srv.URL, + UseTLS: false, + // APIVersion intentionally left empty: this is the default, + // undocumented-requirement path the #646 reporter hit. + APIVersion: "", + } + + client, err := New(cfg) + if err != nil { + t.Fatalf("dockerclient.New() with empty APIVersion must succeed (it does for the real reporter): %v", err) + } + + // Any request is enough to observe the path the client actually sends; + // Info() is the simplest no-argument call go-dockerclient exposes. + if _, err := client.Info(); err != nil { + t.Fatalf("client.Info() must succeed against a daemon that answers /version correctly: %v", err) + } + + if !sawInfoRequest { + t.Fatalf("expected the fake daemon to receive the actual /info request after version negotiation, got none") + } + t.Logf("DEBUG infoPath = %q", infoPath) + + if !strings.Contains(infoPath, "/v") { + t.Fatalf("slim#646: the /info request path %q carries no API version segment even though "+ + "config.APIVersion was left empty by the caller and the daemon answered the internal "+ + "/version negotiation just fine; a dind/CI daemon behind a proxy reads an unversioned "+ + "request as coming from the oldest supported client and rejects it with \"client version "+ + "... is too old\" (the exact error text reported in #646) — go-dockerclient negotiates the "+ + "server's version but never feeds the result back into requestedAPIVersion, which is the "+ + "only field getURL() consults", infoPath) + } +}