Merge pull request #35799 from deads2k/client-17-negotiation

Automatic merge from submit-queue

clean up client version negotiation to handle no legacy API

Version negotiation fails if the legacy API endpoint isn't available.

This tightens up the negotiation interface based to more clearly express what each stage is doing and what the constraints on negotiation are.  This is needed to speak to generic API servers.

@kubernetes/kubectl

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 {
+func MatchesServerVersion(client DiscoveryInterface) error {
-	var err error
-	if client == nil {
-		client, err = NewDiscoveryClientForConfig(c)
-		if err != nil {
-			return err
-		}
-	}
 	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
+// - If version is provided and the server does not support it,
-//   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(client DiscoveryInterface, c *restclient.Config, requestedGV *unversioned.GroupVersion, clientRegisteredGVs []unversioned.GroupVersion) (*unversioned.GroupVersion, error) {
+func NegotiateVersion(client DiscoveryInterface, requiredGV *unversioned.GroupVersion, clientRegisteredGVs []unversioned.GroupVersion) (*unversioned.GroupVersion, error) {
-	var err error
-	if client == nil {
-		client, err = NewDiscoveryClientForConfig(c)
-		if err != nil {
-			return nil, err
-		}
-	}
 	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 requiredGV != nil {
-		if !clientVersions.Has(preferredGV.String()) {
+		if !clientVersions.Has(requiredGV.String()) {
-			return nil, fmt.Errorf("client does not support API version %q; client supported API versions: %v", preferredGV, clientVersions)
+			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()) {
+		if serverVersions.Has(requiredGV.String()) {
-			return preferredGV, nil
+			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", requiredGV)
-			return nil, fmt.Errorf("server does not support API version %q", preferredGV)
-		}
 	}
 
 	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",
+			name:            "explicit version not supported on server",
-			config:         &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
+			requiredVersion: &uapi.GroupVersion{Version: "v1"},
-			serverVersions: []string{"version1"},
+			serverVersions:  []string{"version1"},
-			clientVersions: []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
+			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"`) },
+			expectErr:       func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) },
-			statusCode:     http.StatusOK,
+			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"},
+			requiredVersion: &uapi.GroupVersion{Version: "version1"},
-			config:          &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &registered.GroupOrDie(api.GroupName).GroupVersion}},
 			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",
+			name:       "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, no fallback GroupVersion",
-			config:         &restclient.Config{},
+			expectErr:  func(err error) bool { return strings.Contains(err.Error(), "failed to negotiate an api version;") },
-			clientVersions: []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion},
+			statusCode: http.StatusForbidden,
-			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.
+	matchVersion bool
-	discoveryClient discovery.DiscoveryInterface
+
-	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) {
+func (c *ClientCache) ClientConfigForVersion(requiredVersion *unversioned.GroupVersion) (*restclient.Config, error) {
-	if c.defaultConfig == nil {
+	// TODO: have a better config copy method
-		config, err := c.loader.ClientConfig()
+	config, discoveryClient, err := c.getDefaultConfig()
-		if err != nil {
+	if err != nil {
-			return nil, err
+		return nil, err
-		}
-		c.defaultConfig = config
-		if c.matchVersion {
-			if err := discovery.MatchesServerVersion(c.discoveryClient, config); err != nil {
-				return nil, err
-			}
-		}
 	}
-	if version != nil {
+	if requiredVersion == nil && config.GroupVersion != nil {
-		if config, ok := c.configs[*version]; ok {
+		// 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
+	negotiatedVersion, err := discovery.NegotiateVersion(discoveryClient, requiredVersion, registered.EnabledVersions())
-	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())
 	if err != nil {
 		return nil, err
 	}
 	config.GroupVersion = negotiatedVersion
 
-	if version != nil {
+	// TODO this isn't what we want.  Each clientset should be setting defaults as it sees fit.
-		c.configs[*version] = &config
+	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) {
+func (c *ClientCache) ClientSetForVersion(requiredVersion *unversioned.GroupVersion) (*internalclientset.Clientset, error) {
-	if version != nil {
+	if requiredVersion != nil {
-		if clientset, ok := c.clientsets[*version]; ok {
+		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