Merge pull request #32779 from rremer/auto-update-client

pull client API version negotiation out of the CLI and into the client

client/client.go

@@ -55,8 +55,11 @@ import (
 	"strings"
 
 	"github.com/docker/docker/api"
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/versions"
 	"github.com/docker/go-connections/sockets"
 	"github.com/docker/go-connections/tlsconfig"
+	"golang.org/x/net/context"
 )
 
 // ErrRedirect is the error returned by checkRedirect when the request is non-GET.
@@ -238,13 +241,29 @@ func (cli *Client) ClientVersion() string {
 	return cli.version
 }
 
-// UpdateClientVersion updates the version string associated with this
-// instance of the Client. This operation doesn't acquire a mutex.
-func (cli *Client) UpdateClientVersion(v string) {
-	if !cli.manualOverride {
-		cli.version = v
+// NegotiateAPIVersion updates the version string associated with this
+// instance of the Client to match the latest version the server supports
+func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
+	ping, _ := cli.Ping(ctx)
+	cli.NegotiateAPIVersionPing(ping)
+}
+
+// NegotiateAPIVersionPing updates the version string associated with this
+// instance of the Client to match the latest version the server supports
+func (cli *Client) NegotiateAPIVersionPing(p types.Ping) {
+	if cli.manualOverride {
+		return
 	}
 
+	// try the latest version before versioning headers existed
+	if p.APIVersion == "" {
+		p.APIVersion = "1.24"
+	}
+
+	// if server version is lower than the current cli, downgrade
+	if versions.LessThan(p.APIVersion, cli.ClientVersion()) {
+		cli.version = p.APIVersion
+	}
 }
 
 // DaemonHost returns the host associated with this instance of the Client.

client/client_test.go

@@ -2,8 +2,6 @@ package client
 
 import (
 	"bytes"
-	"encoding/json"
-	"io/ioutil"
 	"net/http"
 	"net/url"
 	"os"
@@ -14,7 +12,6 @@ import (
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
 	"github.com/stretchr/testify/assert"
-	"golang.org/x/net/context"
 )
 
 func TestNewEnvClient(t *testing.T) {
@@ -81,57 +78,27 @@ func TestNewEnvClient(t *testing.T) {
 			expectedVersion: "1.22",
 		},
 	}
+
+	env := envToMap()
+	defer mapToEnv(env)
 	for _, c := range cases {
-		recoverEnvs := setupEnvs(t, c.envs)
+		mapToEnv(env)
+		mapToEnv(c.envs)
 		apiclient, err := NewEnvClient()
 		if c.expectedError != "" {
-			if err == nil {
-				t.Errorf("expected an error for %v", c)
-			} else if err.Error() != c.expectedError {
-				t.Errorf("expected an error %s, got %s, for %v", c.expectedError, err.Error(), c)
-			}
+			assert.Error(t, err)
+			assert.Equal(t, c.expectedError, err.Error())
 		} else {
-			if err != nil {
-				t.Error(err)
-			}
+			assert.NoError(t, err)
 			version := apiclient.ClientVersion()
-			if version != c.expectedVersion {
-				t.Errorf("expected %s, got %s, for %v", c.expectedVersion, version, c)
-			}
+			assert.Equal(t, c.expectedVersion, version)
 		}
 
 		if c.envs["DOCKER_TLS_VERIFY"] != "" {
 			// pedantic checking that this is handled correctly
 			tr := apiclient.client.Transport.(*http.Transport)
-			if tr.TLSClientConfig == nil {
-				t.Error("no TLS config found when DOCKER_TLS_VERIFY enabled")
-			}
-
-			if tr.TLSClientConfig.InsecureSkipVerify {
-				t.Error("TLS verification should be enabled")
-			}
-		}
-
-		recoverEnvs(t)
-	}
-}
-
-func setupEnvs(t *testing.T, envs map[string]string) func(*testing.T) {
-	oldEnvs := map[string]string{}
-	for key, value := range envs {
-		oldEnv := os.Getenv(key)
-		oldEnvs[key] = oldEnv
-		err := os.Setenv(key, value)
-		if err != nil {
-			t.Error(err)
-		}
-	}
-	return func(t *testing.T) {
-		for key, value := range oldEnvs {
-			err := os.Setenv(key, value)
-			if err != nil {
-				t.Error(err)
-			}
+			assert.NotNil(t, tr.TLSClientConfig)
+			assert.Equal(t, tr.TLSClientConfig.InsecureSkipVerify, false)
 		}
 	}
 }
@@ -161,14 +128,10 @@ func TestGetAPIPath(t *testing.T) {
 			t.Fatal(err)
 		}
 		g := c.getAPIPath(cs.p, cs.q)
-		if g != cs.e {
-			t.Fatalf("Expected %s, got %s", cs.e, g)
-		}
+		assert.Equal(t, g, cs.e)
 
 		err = c.Close()
-		if nil != err {
-			t.Fatalf("close client failed, error message: %s", err)
-		}
+		assert.NoError(t, err)
 	}
 }
 
@@ -189,84 +152,33 @@ func TestParseHost(t *testing.T) {
 
 	for _, cs := range cases {
 		p, a, b, e := ParseHost(cs.host)
-		if cs.err && e == nil {
-			t.Fatalf("expected error, got nil")
-		}
-		if !cs.err && e != nil {
-			t.Fatal(e)
-		}
-		if cs.proto != p {
-			t.Fatalf("expected proto %s, got %s", cs.proto, p)
-		}
-		if cs.addr != a {
-			t.Fatalf("expected addr %s, got %s", cs.addr, a)
-		}
-		if cs.base != b {
-			t.Fatalf("expected base %s, got %s", cs.base, b)
-		}
-	}
-}
-
-func TestUpdateClientVersion(t *testing.T) {
-	client := &Client{
-		client: newMockClient(func(req *http.Request) (*http.Response, error) {
-			splitQuery := strings.Split(req.URL.Path, "/")
-			queryVersion := splitQuery[1]
-			b, err := json.Marshal(types.Version{
-				APIVersion: queryVersion,
-			})
-			if err != nil {
-				return nil, err
-			}
-			return &http.Response{
-				StatusCode: http.StatusOK,
-				Body:       ioutil.NopCloser(bytes.NewReader(b)),
-			}, nil
-		}),
-	}
-
-	cases := []struct {
-		v string
-	}{
-		{"1.20"},
-		{"v1.21"},
-		{"1.22"},
-		{"v1.22"},
-	}
-
-	for _, cs := range cases {
-		client.UpdateClientVersion(cs.v)
-		r, err := client.ServerVersion(context.Background())
-		if err != nil {
-			t.Fatal(err)
-		}
-		if strings.TrimPrefix(r.APIVersion, "v") != strings.TrimPrefix(cs.v, "v") {
-			t.Fatalf("Expected %s, got %s", cs.v, r.APIVersion)
+		// if we expected an error to be returned...
+		if cs.err {
+			assert.Error(t, e)
 		}
+		assert.Equal(t, cs.proto, p)
+		assert.Equal(t, cs.addr, a)
+		assert.Equal(t, cs.base, b)
 	}
 }
 
 func TestNewEnvClientSetsDefaultVersion(t *testing.T) {
-	// Unset environment variables
-	envVarKeys := []string{
-		"DOCKER_HOST",
-		"DOCKER_API_VERSION",
-		"DOCKER_TLS_VERIFY",
-		"DOCKER_CERT_PATH",
-	}
-	envVarValues := make(map[string]string)
-	for _, key := range envVarKeys {
-		envVarValues[key] = os.Getenv(key)
-		os.Setenv(key, "")
+	env := envToMap()
+	defer mapToEnv(env)
+
+	envMap := map[string]string{
+		"DOCKER_HOST":        "",
+		"DOCKER_API_VERSION": "",
+		"DOCKER_TLS_VERIFY":  "",
+		"DOCKER_CERT_PATH":   "",
 	}
+	mapToEnv(envMap)
 
 	client, err := NewEnvClient()
 	if err != nil {
 		t.Fatal(err)
 	}
-	if client.version != api.DefaultVersion {
-		t.Fatalf("Expected %s, got %s", api.DefaultVersion, client.version)
-	}
+	assert.Equal(t, client.version, api.DefaultVersion)
 
 	expected := "1.22"
 	os.Setenv("DOCKER_API_VERSION", expected)
@@ -274,14 +186,112 @@ func TestNewEnvClientSetsDefaultVersion(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if client.version != expected {
-		t.Fatalf("Expected %s, got %s", expected, client.version)
+	assert.Equal(t, expected, client.version)
+}
+
+// TestNegotiateAPIVersionEmpty asserts that client.Client can
+// negotiate a compatible APIVersion when omitted
+func TestNegotiateAPIVersionEmpty(t *testing.T) {
+	env := envToMap()
+	defer mapToEnv(env)
+
+	envMap := map[string]string{
+		"DOCKER_API_VERSION": "",
+	}
+	mapToEnv(envMap)
+
+	client, err := NewEnvClient()
+	if err != nil {
+		t.Fatal(err)
 	}
 
-	// Restore environment variables
-	for _, key := range envVarKeys {
-		os.Setenv(key, envVarValues[key])
+	ping := types.Ping{
+		APIVersion:   "",
+		OSType:       "linux",
+		Experimental: false,
 	}
+
+	// set our version to something new
+	client.version = "1.25"
+
+	// if no version from server, expect the earliest
+	// version before APIVersion was implemented
+	expected := "1.24"
+
+	// test downgrade
+	client.NegotiateAPIVersionPing(ping)
+	assert.Equal(t, expected, client.version)
+}
+
+// TestNegotiateAPIVersion asserts that client.Client can
+// negotiate a compatible APIVersion with the server
+func TestNegotiateAPIVersion(t *testing.T) {
+	client, err := NewEnvClient()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expected := "1.21"
+
+	ping := types.Ping{
+		APIVersion:   expected,
+		OSType:       "linux",
+		Experimental: false,
+	}
+
+	// set our version to something new
+	client.version = "1.22"
+
+	// test downgrade
+	client.NegotiateAPIVersionPing(ping)
+	assert.Equal(t, expected, client.version)
+}
+
+// TestNegotiateAPIVersionOverride asserts that we honor
+// the environment variable DOCKER_API_VERSION when negotianing versions
+func TestNegotiateAPVersionOverride(t *testing.T) {
+	env := envToMap()
+	defer mapToEnv(env)
+
+	envMap := map[string]string{
+		"DOCKER_API_VERSION": "9.99",
+	}
+	mapToEnv(envMap)
+
+	client, err := NewEnvClient()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	ping := types.Ping{
+		APIVersion:   "1.24",
+		OSType:       "linux",
+		Experimental: false,
+	}
+
+	expected := envMap["DOCKER_API_VERSION"]
+
+	// test that we honored the env var
+	client.NegotiateAPIVersionPing(ping)
+	assert.Equal(t, expected, client.version)
+}
+
+// mapToEnv takes a map of environment variables and sets them
+func mapToEnv(env map[string]string) {
+	for k, v := range env {
+		os.Setenv(k, v)
+	}
+}
+
+// envToMap returns a map of environment variables
+func envToMap() map[string]string {
+	env := make(map[string]string)
+	for _, e := range os.Environ() {
+		kv := strings.SplitAfterN(e, "=", 2)
+		env[kv[0]] = kv[1]
+	}
+
+	return env
 }
 
 type roundTripFunc func(*http.Request) (*http.Response, error)

client/interface.go

@@ -33,7 +33,8 @@ type CommonAPIClient interface {
 	ClientVersion() string
 	DaemonHost() string
 	ServerVersion(ctx context.Context) (types.Version, error)
-	UpdateClientVersion(v string)
+	NegotiateAPIVersion(ctx context.Context)
+	NegotiateAPIVersionPing(types.Ping)
 }
 
 // ContainerAPIClient defines API client methods for the containers