fix(dockerclient): version API requests when no API version was configured

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:

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)
+	}
+}