clean up client version negotiation to handle no legacy API

pkg/client/typed/discovery/helper.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
-	"k8s.io/kubernetes/pkg/client/restclient"
 	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/version"
 	// Import solely to initialize client auth plugins.
@@ -30,14 +29,7 @@ import (
 // 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(client DiscoveryInterface, c *restclient.Config) error {
-	var err error
-	if client == nil {
-		client, err = NewDiscoveryClientForConfig(c)
-		if err != nil {
-			return err
-		}
-	}
+func MatchesServerVersion(client DiscoveryInterface) error {
 	cVer := version.Get()
 	sVer, err := client.ServerVersion()
 	if err != nil {
@@ -55,19 +47,9 @@ func MatchesServerVersion(client DiscoveryInterface, c *restclient.Config) error
 // a version that both client and server support.
 // - 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,
+// - If version is provided and the server does not support it,
 //   return an error.
-func NegotiateVersion(client DiscoveryInterface, c *restclient.Config, requestedGV *unversioned.GroupVersion, clientRegisteredGVs []unversioned.GroupVersion) (*unversioned.GroupVersion, error) {
-	var err error
-	if client == nil {
-		client, err = NewDiscoveryClientForConfig(c)
-		if err != nil {
-			return nil, err
-		}
-	}
+func NegotiateVersion(client DiscoveryInterface, requiredGV *unversioned.GroupVersion, clientRegisteredGVs []unversioned.GroupVersion) (*unversioned.GroupVersion, error) {
 	clientVersions := sets.String{}
 	for _, gv := range clientRegisteredGVs {
 		clientVersions.Insert(gv.String())
@@ -84,37 +66,23 @@ func NegotiateVersion(client DiscoveryInterface, c *restclient.Config, requested
 		serverVersions.Insert(v)
 	}
 
-	// If no version requested, use config version (may also be empty).
-	// make a copy of the original so we don't risk mutating input here or in the returned value
-	var preferredGV *unversioned.GroupVersion
-	switch {
-	case requestedGV != nil:
-		t := *requestedGV
-		preferredGV = &t
-	case c.GroupVersion != nil:
-		t := *c.GroupVersion
-		preferredGV = &t
-	}
-
 	// If version explicitly requested verify that both client and server support it.
 	// If server does not support warn, but try to negotiate a lower version.
-	if preferredGV != nil {
-		if !clientVersions.Has(preferredGV.String()) {
-			return nil, fmt.Errorf("client does not support API version %q; client supported API versions: %v", preferredGV, clientVersions)
+	if requiredGV != nil {
+		if !clientVersions.Has(requiredGV.String()) {
+			return nil, fmt.Errorf("client does not support API version %q; client supported API versions: %v", requiredGV, clientVersions)
 
 		}
 		// If the server supports no versions, then we should just use the preferredGV
 		// This can happen because discovery fails due to 403 Forbidden errors
 		if len(serverVersions) == 0 {
-			return preferredGV, nil
+			return requiredGV, nil
 		}
-		if serverVersions.Has(preferredGV.String()) {
-			return preferredGV, nil
+		if serverVersions.Has(requiredGV.String()) {
+			return requiredGV, nil
 		}
 		// If we are using an explicit config version the server does not support, fail.
-		if (c.GroupVersion != nil) && (*preferredGV == *c.GroupVersion) {
-			return nil, fmt.Errorf("server does not support API version %q", preferredGV)
-		}
+		return nil, fmt.Errorf("server does not support API version %q", requiredGV)
 	}
 
 	for _, clientGV := range clientRegisteredGVs {
@@ -130,6 +98,12 @@ func NegotiateVersion(client DiscoveryInterface, c *restclient.Config, requested
 			return &t, nil
 		}
 	}
+
+	// if we have no server versions and we have no required version, choose the first clientRegisteredVersion
+	if len(serverVersions) == 0 && len(clientRegisteredGVs) > 0 {
+		return &clientRegisteredGVs[0], nil
+	}
+
 	return nil, fmt.Errorf("failed to negotiate an api version; server supports: %v, client supports: %v",
 		serverVersions, clientVersions)
 }

pkg/client/typed/discovery/helper_blackbox_test.go

@@ -47,19 +47,16 @@ func objBody(object interface{}) io.ReadCloser {
 func TestNegotiateVersion(t *testing.T) {
 	tests := []struct {
 		name            string
-		version         *uapi.GroupVersion
+		requiredVersion *uapi.GroupVersion
 		expectedVersion *uapi.GroupVersion
 		serverVersions  []string
 		clientVersions  []uapi.GroupVersion
-		config          *restclient.Config
 		expectErr       func(err error) bool
 		sendErr         error
 		statusCode      int
 	}{
 		{
 			name:            "server supports client default",
-			version:         &uapi.GroupVersion{Version: "version1"},
-			config:          &restclient.Config{},
 			serverVersions:  []string{"version1", registered.GroupOrDie(api.GroupName).GroupVersion.String()},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
 			expectedVersion: &uapi.GroupVersion{Version: "version1"},
@@ -67,8 +64,6 @@ func TestNegotiateVersion(t *testing.T) {
 		},
 		{
 			name:            "server falls back to client supported",
-			version:         &registered.GroupOrDie(api.GroupName).GroupVersion,
-			config:          &restclient.Config{},
 			serverVersions:  []string{"version1"},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
 			expectedVersion: &uapi.GroupVersion{Version: "version1"},
@@ -76,23 +71,30 @@ func TestNegotiateVersion(t *testing.T) {
 		},
 		{
 			name:            "explicit version supported",
-			config:          &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
+			requiredVersion: &uapi.GroupVersion{Version: "v1"},
 			serverVersions:  []string{"/version1", registered.GroupOrDie(api.GroupName).GroupVersion.String()},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
-			expectedVersion: &registered.GroupOrDie(api.GroupName).GroupVersion,
+			expectedVersion: &uapi.GroupVersion{Version: "v1"},
 			statusCode:      http.StatusOK,
 		},
 		{
-			name:           "explicit version not supported",
-			config:         &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
-			serverVersions: []string{"version1"},
-			clientVersions: []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
-			expectErr:      func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) },
-			statusCode:     http.StatusOK,
+			name:            "explicit version not supported on server",
+			requiredVersion: &uapi.GroupVersion{Version: "v1"},
+			serverVersions:  []string{"version1"},
+			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
+			expectErr:       func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) },
+			statusCode:      http.StatusOK,
+		},
+		{
+			name:            "explicit version not supported on client",
+			requiredVersion: &uapi.GroupVersion{Version: "v1"},
+			serverVersions:  []string{"v1"},
+			clientVersions:  []uapi.GroupVersion{{Version: "version1"}},
+			expectErr:       func(err error) bool { return strings.Contains(err.Error(), `client does not support API version "v1"`) },
+			statusCode:      http.StatusOK,
 		},
 		{
 			name:           "connection refused error",
-			config:         &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
 			serverVersions: []string{"version1"},
 			clientVersions: []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
 			sendErr:        errors.New("connection refused"),
@@ -101,25 +103,21 @@ func TestNegotiateVersion(t *testing.T) {
 		},
 		{
 			name:            "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, use default GroupVersion",
-			config:          &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
-			expectedVersion: &registered.GroupOrDie(api.GroupName).GroupVersion,
+			expectedVersion: &uapi.GroupVersion{Version: "version1"},
 			statusCode:      http.StatusForbidden,
 		},
 		{
 			name:            "discovery fails due to 404 Not Found errors and thus serverVersions is empty, use requested GroupVersion",
-			version:         &uapi.GroupVersion{Version: "version1"},
-			config:          &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
+			requiredVersion: &uapi.GroupVersion{Version: "version1"},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
 			expectedVersion: &uapi.GroupVersion{Version: "version1"},
 			statusCode:      http.StatusNotFound,
 		},
 		{
-			name:           "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, no fallback GroupVersion",
-			config:         &restclient.Config{},
-			clientVersions: []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
-			expectErr:      func(err error) bool { return strings.Contains(err.Error(), "failed to negotiate an api version;") },
-			statusCode:     http.StatusForbidden,
+			name:       "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, no fallback GroupVersion",
+			expectErr:  func(err error) bool { return strings.Contains(err.Error(), "failed to negotiate an api version;") },
+			statusCode: http.StatusForbidden,
 		},
 	}
 
@@ -139,9 +137,9 @@ func TestNegotiateVersion(t *testing.T) {
 				return &http.Response{StatusCode: test.statusCode, Header: header, Body: objBody(&uapi.APIVersions{Versions: test.serverVersions})}, nil
 			}),
 		}
-		c := discovery.NewDiscoveryClientForConfigOrDie(test.config)
+		c := discovery.NewDiscoveryClientForConfigOrDie(&restclient.Config{})
 		c.RESTClient().(*restclient.RESTClient).Client = fakeClient.Client
-		response, err := discovery.NegotiateVersion(c, test.config, test.version, test.clientVersions)
+		response, err := discovery.NegotiateVersion(c, test.requiredVersion, test.clientVersions)
 		if err == nil && test.expectErr != nil {
 			t.Errorf("expected error, got nil for [%s].", test.name)
 		}

pkg/kubectl/cmd/util/clientcache.go

@@ -17,6 +17,8 @@ limitations under the License.
 package util
 
 import (
+	"sync"
+
 	fed_clientset "k8s.io/kubernetes/federation/client/clientset_generated/federation_internalclientset"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apimachinery/registered"
@@ -43,51 +45,75 @@ type ClientCache struct {
 	clientsets    map[unversioned.GroupVersion]*internalclientset.Clientset
 	fedClientSets map[unversioned.GroupVersion]fed_clientset.Interface
 	configs       map[unversioned.GroupVersion]*restclient.Config
-	defaultConfig *restclient.Config
-	// TODO this is only ever read.  It should be moved to initialization at some point.
-	discoveryClient discovery.DiscoveryInterface
-	matchVersion    bool
+
+	matchVersion bool
+
+	defaultConfigLock sync.Mutex
+	defaultConfig     *restclient.Config
+	discoveryClient   discovery.DiscoveryInterface
+}
+
+// also looks up the discovery client.  We can't do this during init because the flags won't have been set
+// because this is constructed pre-command execution before the command tree is even set up
+func (c *ClientCache) getDefaultConfig() (restclient.Config, discovery.DiscoveryInterface, error) {
+	c.defaultConfigLock.Lock()
+	defer c.defaultConfigLock.Unlock()
+
+	if c.defaultConfig != nil && c.discoveryClient != nil {
+		return *c.defaultConfig, c.discoveryClient, nil
+	}
+
+	config, err := c.loader.ClientConfig()
+	if err != nil {
+		return restclient.Config{}, nil, err
+	}
+	discoveryClient, err := discovery.NewDiscoveryClientForConfig(config)
+	if err != nil {
+		return restclient.Config{}, nil, err
+	}
+	if c.matchVersion {
+		if err := discovery.MatchesServerVersion(discoveryClient); err != nil {
+			return restclient.Config{}, nil, err
+		}
+	}
+
+	c.defaultConfig = config
+	c.discoveryClient = discoveryClient
+	return *c.defaultConfig, c.discoveryClient, nil
 }
 
 // ClientConfigForVersion returns the correct config for a server
-func (c *ClientCache) ClientConfigForVersion(version *unversioned.GroupVersion) (*restclient.Config, error) {
-	if c.defaultConfig == nil {
-		config, err := c.loader.ClientConfig()
-		if err != nil {
-			return nil, err
-		}
-		c.defaultConfig = config
-		if c.matchVersion {
-			if err := discovery.MatchesServerVersion(c.discoveryClient, config); err != nil {
-				return nil, err
-			}
-		}
+func (c *ClientCache) ClientConfigForVersion(requiredVersion *unversioned.GroupVersion) (*restclient.Config, error) {
+	// TODO: have a better config copy method
+	config, discoveryClient, err := c.getDefaultConfig()
+	if err != nil {
+		return nil, err
 	}
-	if version != nil {
-		if config, ok := c.configs[*version]; ok {
+	if requiredVersion == nil && config.GroupVersion != nil {
+		// if someone has set the values via flags, our config will have the groupVersion set
+		// that means it is required.
+		requiredVersion = config.GroupVersion
+	}
+
+	// required version may still be nil, since config.GroupVersion may have been nil.  Do the check
+	// before looking up from the cache
+	if requiredVersion != nil {
+		if config, ok := c.configs[*requiredVersion]; ok {
 			return config, nil
 		}
 	}
 
-	// TODO: have a better config copy method
-	config := *c.defaultConfig
-
-	// TODO these fall out when we finish the refactor
-	var preferredGV *unversioned.GroupVersion
-	if version != nil {
-		versionCopy := *version
-		preferredGV = &versionCopy
-	}
-
-	oldclient.SetKubernetesDefaults(&config)
-	negotiatedVersion, err := discovery.NegotiateVersion(c.discoveryClient, &config, preferredGV, registered.EnabledVersions())
+	negotiatedVersion, err := discovery.NegotiateVersion(discoveryClient, requiredVersion, registered.EnabledVersions())
 	if err != nil {
 		return nil, err
 	}
 	config.GroupVersion = negotiatedVersion
 
-	if version != nil {
-		c.configs[*version] = &config
+	// TODO this isn't what we want.  Each clientset should be setting defaults as it sees fit.
+	oldclient.SetKubernetesDefaults(&config)
+
+	if requiredVersion != nil {
+		c.configs[*requiredVersion] = &config
 	}
 
 	// `version` does not necessarily equal `config.Version`.  However, we know that we call this method again with
@@ -100,13 +126,13 @@ func (c *ClientCache) ClientConfigForVersion(version *unversioned.GroupVersion)
 
 // ClientSetForVersion initializes or reuses a clientset for the specified version, or returns an
 // error if that is not possible
-func (c *ClientCache) ClientSetForVersion(version *unversioned.GroupVersion) (*internalclientset.Clientset, error) {
-	if version != nil {
-		if clientset, ok := c.clientsets[*version]; ok {
+func (c *ClientCache) ClientSetForVersion(requiredVersion *unversioned.GroupVersion) (*internalclientset.Clientset, error) {
+	if requiredVersion != nil {
+		if clientset, ok := c.clientsets[*requiredVersion]; ok {
 			return clientset, nil
 		}
 	}
-	config, err := c.ClientConfigForVersion(version)
+	config, err := c.ClientConfigForVersion(requiredVersion)
 	if err != nil {
 		return nil, err
 	}
@@ -120,13 +146,13 @@ func (c *ClientCache) ClientSetForVersion(version *unversioned.GroupVersion) (*i
 	// `version` does not necessarily equal `config.Version`.  However, we know that if we call this method again with
 	// `version`, we should get a client based on the same config we just found.  There's no guarantee that a client
 	// is copiable, so create a new client and save it in the cache.
-	if version != nil {
+	if requiredVersion != nil {
 		configCopy := *config
 		clientset, err := internalclientset.NewForConfig(&configCopy)
 		if err != nil {
 			return nil, err
 		}
-		c.clientsets[*version] = clientset
+		c.clientsets[*requiredVersion] = clientset
 	}
 
 	return clientset, nil