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

pkg/crt/docker/dockerclient/client.go

@@ -174,6 +174,88 @@ func GetUnixSocketAddr() (*SocketInfo, error) {
 	return nil, fmt.Errorf("docker socket not found")
 }
 
+// discoverAPIVersion asks the daemon behind client which API version it
+// speaks. An unreachable or otherwise misbehaving daemon yields "", which
+// leaves the caller with the client it already had, so a connection problem
+// still surfaces as the same error it always did.
+func discoverAPIVersion(client *docker.Client) string {
+	env, err := client.Version()
+	if err != nil {
+		return ""
+	}
+
+	return env.Get("ApiVersion")
+}
+
+// withDiscoveredAPIVersion returns a client whose requests carry the "/vX.Y/"
+// path segment even when the caller configured no API version.
+//
+// go-dockerclient parses an API version into the client's internal
+// requestedAPIVersion only when the string it is given is non-empty, and
+// getURL() consults that field alone when it builds a request path. With no
+// version configured - a plain "mint build"/"slim build" with no
+// DOCKER_API_VERSION set, which is the case in every mintoolkit/mint#95 and
+// slimtoolkit/slim#646 report - every request the client sends omits the
+// version segment. A daemon reached through a proxy, dind (Docker-in-Docker:
+// a CI runner's own container running a Docker daemon, the topology behind
+// those reports), reads an unversioned request as coming from the oldest
+// client it supports and rejects it ("client version ... is too old"). The
+// client's own /version negotiation does not help here: it stores its result
+// in expectedAPIVersion and never copies it into requestedAPIVersion.
+//
+// rebuild constructs the replacement client for the discovered version. Each
+// construction path passes its own constructor, so the rebuilt client keeps
+// that path's transport, credentials and endpoint; this is what lets the TLS
+// and environment paths share this logic with the plain one.
+func withDiscoveredAPIVersion(
+	client *docker.Client,
+	apiVersion string,
+	rebuild func(apiVersion string) (*docker.Client, error),
+) *docker.Client {
+	if client == nil {
+		return client
+	}
+
+	if apiVersion != "" {
+		// An explicitly configured version already populates
+		// requestedAPIVersion, so the lazy self-check buys nothing. This is
+		// what the call sites did by hand before.
+		client.SkipServerVersionCheck = true
+		return client
+	}
+
+	discovered := discoverAPIVersion(client)
+	if discovered == "" {
+		return client
+	}
+
+	versioned, err := rebuild(discovered)
+	if err != nil || versioned == nil {
+		return client
+	}
+
+	// The version was just read from this very daemon, so the lazy self-check
+	// that the first request would otherwise trigger only repeats the probe
+	// that has already happened. Skipping it is exactly what happens today
+	// when a caller sets DOCKER_API_VERSION by hand.
+	versioned.SkipServerVersionCheck = true
+
+	return versioned
+}
+
+// newVersionedClient builds a plain (non-TLS) client for host and gives it the
+// daemon's API version when the caller configured none.
+func newVersionedClient(host, apiVersion string) (*docker.Client, error) {
+	client, err := docker.NewVersionedClient(host, apiVersion)
+	if err != nil {
+		return nil, err
+	}
+
+	return withDiscoveredAPIVersion(client, apiVersion, func(apiVersion string) (*docker.Client, error) {
+		return docker.NewVersionedClient(host, apiVersion)
+	}), nil
+}
+
 // New creates a new Docker client instance
 func New(config *config.DockerClient) (*docker.Client, error) {
 	var client *docker.Client
@@ -205,6 +287,20 @@ func New(config *config.DockerClient) (*docker.Client, error) {
 		return docker.NewVersionedTLSClientFromBytes(host, cert, key, ca, apiVersion)
 	}
 
+	// newVersionedTLSClient is newTLSClient plus the version discovery the
+	// plain path gets: same certificates, same transport, and a "/vX.Y/"
+	// segment on every request when the caller configured no version.
+	newVersionedTLSClient := func(host string, certPath string, verify bool, apiVersion string) (*docker.Client, error) {
+		client, err := newTLSClient(host, certPath, verify, apiVersion)
+		if err != nil {
+			return nil, err
+		}
+
+		return withDiscoveredAPIVersion(client, apiVersion, func(apiVersion string) (*docker.Client, error) {
+			return newTLSClient(host, certPath, verify, apiVersion)
+		}), nil
+	}
+
 	//NOTE:
 	//go-dockerclient doesn't support DOCKER_CONTEXT natively
 	//so we need to lookup the context first to extract its connection info
@@ -270,7 +366,7 @@ func New(config *config.DockerClient) (*docker.Client, error) {
 		config.UseTLS &&
 		config.VerifyTLS &&
 		config.TLSCertPath != "":
-		client, err = newTLSClient(config.Host, config.TLSCertPath, true, config.APIVersion)
+		client, err = newVersionedTLSClient(config.Host, config.TLSCertPath, true, config.APIVersion)
 		if err != nil {
 			return nil, err
 		}
@@ -281,7 +377,7 @@ func New(config *config.DockerClient) (*docker.Client, error) {
 		config.UseTLS &&
 		!config.VerifyTLS &&
 		config.TLSCertPath != "":
-		client, err = newTLSClient(config.Host, config.TLSCertPath, false, config.APIVersion)
+		client, err = newVersionedTLSClient(config.Host, config.TLSCertPath, false, config.APIVersion)
 		if err != nil {
 			return nil, err
 		}
@@ -290,23 +386,19 @@ 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 == "" &&
 		!config.VerifyTLS &&
 		config.Env[EnvDockerTLSVerify] == "1" &&
 		config.Env[EnvDockerCertPath] != "" &&
 		config.Env[EnvDockerHost] != "":
-		client, err = newTLSClient(config.Env[EnvDockerHost], config.Env[EnvDockerCertPath], false, config.APIVersion)
+		client, err = newVersionedTLSClient(config.Env[EnvDockerHost], config.Env[EnvDockerCertPath], false, config.APIVersion)
 		if err != nil {
 			return nil, err
 		}
@@ -319,6 +411,12 @@ func New(config *config.DockerClient) (*docker.Client, error) {
 			return nil, err
 		}
 
+		// NewClientFromEnv reads DOCKER_API_VERSION itself, so the same string
+		// decides here whether a version was configured at all.
+		client = withDiscoveredAPIVersion(client, os.Getenv(EnvDockerAPIVer), func(apiVersion string) (*docker.Client, error) {
+			return docker.NewVersionedClientFromEnv(apiVersion)
+		})
+
 		log.Debug("dockerclient.New: new Docker client (env) [5]")
 
 	case config.Host != "" && (strings.HasPrefix(config.Host, "/") || strings.HasPrefix(config.Host, "unix://")):
@@ -342,15 +440,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
-		}
-
 	case config.Host == "" && config.Env[EnvDockerHost] == "" && contextHost != "":
 		log.Debugf("dockerclient.New: new Docker client - from context ('%s') contextVerifyTLS=%v", contextHost, contextVerifyTLS)
 		if strings.HasPrefix(contextHost, "/") ||
@@ -374,15 +468,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.Debugf("dockerclient.New: new Docker client - from context ('%s') - [7]", contextHost)
 		} else {
 			log.Debugf("dockerclient.New: new Docker client - from context - non-unix socket host (%s) [todo]", contextHost)
@@ -403,15 +493,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/crt/docker/dockerclient/client_api_version_internal_test.go

@@ -0,0 +1,121 @@
+package dockerclient
+
+import (
+	"errors"
+	"testing"
+
+	docker "github.com/fsouza/go-dockerclient"
+)
+
+// TestWithDiscoveredAPIVersionKeepsAnExplicitVersion asserts the branch every
+// caller relied on before: a configured version is left alone, and the
+// redundant self-check is skipped.
+func TestWithDiscoveredAPIVersionKeepsAnExplicitVersion(t *testing.T) {
+	client, err := docker.NewVersionedClient("http://127.0.0.1:1", "1.41")
+	if err != nil {
+		t.Fatalf("building the fixture client: %v", err)
+	}
+
+	rebuilt := false
+	got := withDiscoveredAPIVersion(client, "1.41", func(string) (*docker.Client, error) {
+		rebuilt = true
+		return nil, nil
+	})
+
+	if rebuilt {
+		t.Fatal("a configured API version must not trigger a probe or a rebuild")
+	}
+
+	if got != client {
+		t.Fatal("a configured API version must leave the client as it is")
+	}
+
+	if !got.SkipServerVersionCheck {
+		t.Fatal("a configured API version must skip the redundant server version check")
+	}
+}
+
+// TestWithDiscoveredAPIVersionKeepsTheClientWhenTheDaemonIsUnreachable makes
+// sure the probe never turns a connection problem into a different error: the
+// caller gets the client it would have had, and fails where it always did.
+func TestWithDiscoveredAPIVersionKeepsTheClientWhenTheDaemonIsUnreachable(t *testing.T) {
+	// Port 1 refuses connections, which is what an absent daemon looks like.
+	client, err := docker.NewVersionedClient("http://127.0.0.1:1", "")
+	if err != nil {
+		t.Fatalf("building the fixture client: %v", err)
+	}
+
+	rebuilt := false
+	got := withDiscoveredAPIVersion(client, "", func(string) (*docker.Client, error) {
+		rebuilt = true
+		return nil, nil
+	})
+
+	if rebuilt {
+		t.Fatal("an unreachable daemon must not produce a rebuilt client")
+	}
+
+	if got != client {
+		t.Fatal("an unreachable daemon must leave the original client in place")
+	}
+}
+
+// TestWithDiscoveredAPIVersionRebuildsThroughTheCallersConstructor is the
+// TLS and environment paths' coverage: they differ from the plain path only
+// in the constructor they hand over, so this asserts that the discovered
+// version reaches that constructor and that its client is the one returned.
+func TestWithDiscoveredAPIVersionRebuildsThroughTheCallersConstructor(t *testing.T) {
+	var probePath string
+	srv := daemonAnsweringVersion(t, &probePath)
+	defer srv.Close()
+
+	client, err := docker.NewVersionedClient(srv.URL, "")
+	if err != nil {
+		t.Fatalf("building the fixture client: %v", err)
+	}
+
+	replacement, err := docker.NewVersionedClient(srv.URL, "1.44")
+	if err != nil {
+		t.Fatalf("building the replacement client: %v", err)
+	}
+
+	var handed string
+	got := withDiscoveredAPIVersion(client, "", func(apiVersion string) (*docker.Client, error) {
+		handed = apiVersion
+		return replacement, nil
+	})
+
+	if handed != "1.44" {
+		t.Fatalf("the constructor was handed %q, expected the version the daemon reported", handed)
+	}
+
+	if got != replacement {
+		t.Fatal("the client built by the caller's own constructor must be the one returned")
+	}
+
+	if !got.SkipServerVersionCheck {
+		t.Fatal("the rebuilt client must skip the self-check that repeats the probe just made")
+	}
+}
+
+// TestWithDiscoveredAPIVersionKeepsTheClientWhenTheRebuildFails covers the
+// remaining branch: a constructor that fails leaves the working client in
+// place rather than dropping the caller into a nil.
+func TestWithDiscoveredAPIVersionKeepsTheClientWhenTheRebuildFails(t *testing.T) {
+	var probePath string
+	srv := daemonAnsweringVersion(t, &probePath)
+	defer srv.Close()
+
+	client, err := docker.NewVersionedClient(srv.URL, "")
+	if err != nil {
+		t.Fatalf("building the fixture client: %v", err)
+	}
+
+	got := withDiscoveredAPIVersion(client, "", func(string) (*docker.Client, error) {
+		return nil, errors.New("no certificates here")
+	})
+
+	if got != client {
+		t.Fatal("a failed rebuild must leave the original client in place")
+	}
+}