diff --git a/pkg/crt/docker/dockerclient/client.go b/pkg/crt/docker/dockerclient/client.go index 635fe4e2e..9de82e03f 100644 --- a/pkg/crt/docker/dockerclient/client.go +++ b/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,15 +386,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 == "" && @@ -306,7 +398,7 @@ func New(config *config.DockerClient) (*docker.Client, error) { 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: diff --git a/pkg/crt/docker/dockerclient/client_api_version_internal_test.go b/pkg/crt/docker/dockerclient/client_api_version_internal_test.go new file mode 100644 index 000000000..a5e2ff608 --- /dev/null +++ b/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") + } +} diff --git a/pkg/crt/docker/dockerclient/client_api_version_test.go b/pkg/crt/docker/dockerclient/client_api_version_test.go new file mode 100644 index 000000000..a6680528c --- /dev/null +++ b/pkg/crt/docker/dockerclient/client_api_version_test.go @@ -0,0 +1,118 @@ +package dockerclient + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/mintoolkit/mint/pkg/app/master/config" +) + +// daemonAnsweringVersion is a stand-in daemon that answers the unversioned +// "/version" probe and records the path of the next request it receives. +// +// The match on "/version" is exact on purpose: once the client knows an API +// version it prefixes every path, so a lenient match would take a later +// "/v1.44/version" for the probe and hide the very thing under test. +func daemonAnsweringVersion(t *testing.T, seen *string) *httptest.Server { + t.Helper() + + return 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" { + _, _ = w.Write([]byte(`{"ApiVersion":"1.44"}`)) + return + } + + *seen = r.URL.Path + _, _ = w.Write([]byte(`{}`)) + })) +} + +// TestNewClientVersionsRequestsWithoutExplicitAPIVersion covers #95 on the +// plain host path. +// +// When the caller leaves DOCKER_API_VERSION / config.APIVersion empty - the +// default for a plain "mint build" - go-dockerclient never parses an API +// version into the client's requestedAPIVersion, and getURL() consults that +// field alone when it builds a request path. Every request the client sends +// therefore omits the "/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". +// +// The client's own /version negotiation does not help: its result is stored +// in expectedAPIVersion and never copied into requestedAPIVersion. So the +// request worth asserting on is the next one - here Info()'s GET /info - +// which is the one a proxying daemon actually rejects. +func TestNewClientVersionsRequestsWithoutExplicitAPIVersion(t *testing.T) { + // New() writes DOCKER_HOST (and DOCKER_API_VERSION) into the process + // environment. t.Setenv restores whatever was there when the test ends, + // so a closed test server's URL cannot leak into later tests. + t.Setenv(EnvDockerHost, "") + t.Setenv(EnvDockerAPIVer, "") + + var infoPath string + srv := daemonAnsweringVersion(t, &infoPath) + defer srv.Close() + + cfg := &config.DockerClient{ + Host: srv.URL, + UseTLS: false, + // Left empty on purpose: this is the path every report hit. + APIVersion: "", + } + + client, err := New(cfg) + if err != nil { + t.Fatalf("New() with an empty APIVersion must succeed: %v", err) + } + + if _, err := client.Info(); err != nil { + t.Fatalf("Info() must succeed against a daemon that answers /version: %v", err) + } + + if infoPath == "" { + t.Fatal("expected the fake daemon to receive the /info request, got none") + } + + if !strings.HasPrefix(infoPath, "/v1.44/") { + t.Fatalf("the /info request path %q carries no API version segment although the daemon "+ + "answered the version probe; a daemon behind a proxy rejects such a request as coming "+ + "from a client that is too old (#95)", infoPath) + } +} + +// TestNewClientFromEnvVersionsRequests covers the same defect on the +// DOCKER_HOST path, which reaches the daemon through +// docker.NewClientFromEnv() instead of an explicit host. +func TestNewClientFromEnvVersionsRequests(t *testing.T) { + var infoPath string + srv := daemonAnsweringVersion(t, &infoPath) + defer srv.Close() + + t.Setenv(EnvDockerHost, srv.URL) + t.Setenv(EnvDockerAPIVer, "") + t.Setenv(EnvDockerTLSVerify, "") + + cfg := &config.DockerClient{ + Env: map[string]string{EnvDockerHost: srv.URL}, + } + + client, err := New(cfg) + if err != nil { + t.Fatalf("New() from the environment must succeed: %v", err) + } + + if _, err := client.Info(); err != nil { + t.Fatalf("Info() must succeed against a daemon that answers /version: %v", err) + } + + if !strings.HasPrefix(infoPath, "/v1.44/") { + t.Fatalf("the /info request path %q carries no API version segment on the DOCKER_HOST "+ + "path (#95)", infoPath) + } +}