diff --git a/pkg/client/typed/discovery/helper.go b/pkg/client/typed/discovery/helper.go index 81efc6171d7..1ac4a25a410 100644 --- a/pkg/client/typed/discovery/helper.go +++ b/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) } diff --git a/pkg/client/typed/discovery/helper_blackbox_test.go b/pkg/client/typed/discovery/helper_blackbox_test.go index aa658d153dd..cfc8b1934d1 100644 --- a/pkg/client/typed/discovery/helper_blackbox_test.go +++ b/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: ®istered.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: ®istered.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: ®istered.GroupOrDie(api.GroupName).GroupVersion, + expectedVersion: &uapi.GroupVersion{Version: "v1"}, statusCode: http.StatusOK, }, { - name: "explicit version not supported", - config: &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: ®istered.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: ®istered.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: ®istered.GroupOrDie(api.GroupName).GroupVersion}}, clientVersions: []uapi.GroupVersion{{Version: "version1"}, registered.GroupOrDie(api.GroupName).GroupVersion}, - expectedVersion: ®istered.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: ®istered.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) } diff --git a/pkg/kubectl/cmd/util/clientcache.go b/pkg/kubectl/cmd/util/clientcache.go index fd580dbb2eb..db8897cf3b2 100644 --- a/pkg/kubectl/cmd/util/clientcache.go +++ b/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