Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clean up client version negotiation to handle no legacy API #35799

Merged
merged 1 commit into from
Nov 3, 2016
Merged

clean up client version negotiation to handle no legacy API #35799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 16 additions & 42 deletions pkg/client/typed/discovery/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 {
Expand All @@ -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())
Expand All @@ -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 {
Expand All @@ -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)
}
50 changes: 24 additions & 26 deletions pkg/client/typed/discovery/helper_blackbox_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,52 +47,54 @@ 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"},
statusCode: http.StatusOK,
},
{
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"},
statusCode: http.StatusOK,
},
{
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"),
Expand All @@ -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,
},
}

Expand All @@ -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)
}
Expand Down
98 changes: 62 additions & 36 deletions pkg/kubectl/cmd/util/clientcache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this up since it guards defaultConfig and discoveryClient.

matchVersion shouldn't be in this block since it isn't written under the lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

defaultConfig *restclient.Config
discoveryClient discovery.DiscoveryInterface
}

// 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
}
}
// 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
}
if version != nil {
if config, ok := c.configs[*version]; ok {
return config, 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(requiredVersion *unversioned.GroupVersion) (*restclient.Config, error) {
// TODO: have a better config copy method
config := *c.defaultConfig
config, discoveryClient, err := c.getDefaultConfig()
if err != nil {
return nil, err
}
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
}

// TODO these fall out when we finish the refactor
var preferredGV *unversioned.GroupVersion
if version != nil {
versionCopy := *version
preferredGV = &versionCopy
// 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
}
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hoist this up and out of this code? It's no longer correct in the original sense of what it was used for (there are different defaulters). It's really SetDefaultsForVersion(negotiatedVersion) and I'm not sure that it should be called the same way as it used to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now uses the restclient.SetDefaults


if requiredVersion != nil {
c.configs[*requiredVersion] = &config
}

// `version` does not necessarily equal `config.Version`. However, we know that we call this method again with
Expand All @@ -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
}
Expand All @@ -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
Expand Down