Unittest for #9762

Started looking at refactoring of the kubectl Factory, which
caused me to experience all 5 stages of grief. There are some options
for improving, but it probably warrants a proposal with some discussion/
feedback. In the meantime this tests `NegotiateVersion` in isolation.

pkg/client/helper.go

@@ -144,12 +144,14 @@ func New(c *Config) (*Client, error) {
 // MatchesServerVersion queries the server to compares the build version
 // (git hash) of the client with the server's build version. It returns an error
 // if it failed to contact the server or if the versions are not an exact match.
-func MatchesServerVersion(c *Config) error {
-	client, err := New(c)
-	if err != nil {
-		return err
+func MatchesServerVersion(client *Client, c *Config) error {
+	var err error
+	if client == nil {
+		client, err = New(c)
+		if err != nil {
+			return err
+		}
 	}
-
 	clientVersion := version.Get()
 	serverVersion, err := client.ServerVersion()
 	if err != nil {
@@ -164,17 +166,20 @@ func MatchesServerVersion(c *Config) error {
 
 // NegotiateVersion queries the server's supported api versions to find
 // a version that both client and server support.
-// - If no version is provided, try the client's registered versions in order of
+// - If no version is provided, try registered client versions in order of
 //   preference.
 // - If version is provided, but not default config (explicitly requested via
 //   commandline flag), and is unsupported by the server, print a warning to
 //   stderr and try client's registered versions in order of preference.
 // - If version is config default, and the server does not support it,
 //   return an error.
-func NegotiateVersion(c *Config, version string) (string, error) {
-	client, err := New(c)
-	if err != nil {
-		return "", err
+func NegotiateVersion(client *Client, c *Config, version string) (string, error) {
+	var err error
+	if client == nil {
+		client, err = New(c)
+		if err != nil {
+			return "", err
+		}
 	}
 	clientVersions := util.StringSet{}
 	for _, v := range registered.RegisteredVersions {

pkg/client/helper_test.go

@@ -17,12 +17,18 @@ limitations under the License.
 package client
 
 import (
+	"bytes"
+	"encoding/json"
+	"io"
+	"io/ioutil"
 	"net/http"
 	"reflect"
 	"strings"
 	"testing"
 
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi"
 )
 
 const (
@@ -314,3 +320,79 @@ func TestSetKubernetesDefaultsUserAgent(t *testing.T) {
 		t.Errorf("no user agent set: %#v", config)
 	}
 }
+
+func objBody(object interface{}) io.ReadCloser {
+	output, err := json.MarshalIndent(object, "", "")
+	if err != nil {
+		panic(err)
+	}
+	return ioutil.NopCloser(bytes.NewReader([]byte(output)))
+}
+
+func TestNegotiateVersion(t *testing.T) {
+	tests := []struct {
+		name, version, expectedVersion string
+		serverVersions                 []string
+		config                         *Config
+		expectErr                      bool
+	}{
+		{
+			name:            "server supports client default",
+			version:         "v1",
+			expectedVersion: "v1",
+			config:          &Config{},
+			serverVersions:  []string{"v1beta3", "v1"},
+			expectErr:       false,
+		},
+		{
+			name:            "server falls back to client supported",
+			version:         "v1",
+			expectedVersion: "v1beta3",
+			config:          &Config{},
+			serverVersions:  []string{"v1beta3"},
+			expectErr:       false,
+		},
+		{
+			name:            "explicit version supported",
+			version:         "",
+			expectedVersion: "v1",
+			config:          &Config{Version: "v1"},
+			serverVersions:  []string{"v1beta3", "v1"},
+			expectErr:       false,
+		},
+		{
+			name:            "explicit version not supported",
+			version:         "",
+			expectedVersion: "",
+			config:          &Config{Version: "v1"},
+			serverVersions:  []string{"v1beta3"},
+			expectErr:       true,
+		},
+	}
+	codec := testapi.Codec()
+
+	for _, test := range tests {
+		fakeClient := &FakeRESTClient{
+			Codec: codec,
+			Resp: &http.Response{
+				StatusCode: 200,
+				Body:       objBody(&api.APIVersions{Versions: test.serverVersions}),
+			},
+			Client: HTTPClientFunc(func(req *http.Request) (*http.Response, error) {
+				return &http.Response{StatusCode: 200, Body: objBody(&api.APIVersions{Versions: test.serverVersions})}, nil
+			}),
+		}
+		c := NewOrDie(test.config)
+		c.Client = fakeClient.Client
+		response, err := NegotiateVersion(c, test.config, test.version)
+		if err == nil && test.expectErr {
+			t.Errorf("expected error, got nil for [%s].", test.name)
+		}
+		if err != nil && !test.expectErr {
+			t.Errorf("unexpected error for [%s]: %v.", test.name, err)
+		}
+		if response != test.expectedVersion {
+			t.Errorf("expected version %s, got %s.", test.expectedVersion, response)
+		}
+	}
+}

pkg/kubectl/cmd/util/clientcache.go

@@ -36,6 +36,7 @@ type clientCache struct {
 	clients       map[string]*client.Client
 	configs       map[string]*client.Config
 	defaultConfig *client.Config
+	defaultClient *client.Client
 	matchVersion  bool
 }
 
@@ -48,7 +49,7 @@ func (c *clientCache) ClientConfigForVersion(version string) (*client.Config, er
 		}
 		c.defaultConfig = config
 		if c.matchVersion {
-			if err := client.MatchesServerVersion(config); err != nil {
+			if err := client.MatchesServerVersion(c.defaultClient, config); err != nil {
 				return nil, err
 			}
 		}
@@ -58,7 +59,7 @@ func (c *clientCache) ClientConfigForVersion(version string) (*client.Config, er
 	}
 	// TODO: have a better config copy method
 	config := *c.defaultConfig
-	negotiatedVersion, err := client.NegotiateVersion(&config, version)
+	negotiatedVersion, err := client.NegotiateVersion(c.defaultClient, &config, version)
 	if err != nil {
 		return nil, err
 	}