From cf4d5c32f3a7392e74d329bf7448c612984ac7bd Mon Sep 17 00:00:00 2001 From: deads2k Date: Fri, 12 May 2017 15:12:04 -0400 Subject: [PATCH] stop setting bad defaults that are indistinguishable from real values for clients Kubernetes-commit: edd2d973a3c777afd691d6abe7b1e6420ad43cba --- discovery/helper.go | 61 +++++-------------------------- discovery/helper_blackbox_test.go | 56 +++------------------------- tools/clientcmd/overrides.go | 2 - 3 files changed, 16 insertions(+), 103 deletions(-) diff --git a/discovery/helper.go b/discovery/helper.go index f184bc92..353d34b3 100644 --- a/discovery/helper.go +++ b/discovery/helper.go @@ -41,25 +41,13 @@ func MatchesServerVersion(clientVersion apimachineryversion.Info, client Discove return nil } -// NegotiateVersion queries the server's supported api versions to find -// 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 and the server does not support it, -// return an error. -// TODO negotiation should be reserved for cases where we need a version for a given group. In those cases, it should return an ordered list of -// server preferences. From that list, a separate function can match from an ordered list of client versions. -// This is not what the function has ever done before, but it makes more logical sense. -func NegotiateVersion(client DiscoveryInterface, requiredGV *schema.GroupVersion, clientRegisteredGVs []schema.GroupVersion) (*schema.GroupVersion, error) { - clientVersions := sets.String{} - for _, gv := range clientRegisteredGVs { - clientVersions.Insert(gv.String()) - } +// ServerSupportsVersion returns an error if the server doesn't have the required version +func ServerSupportsVersion(client DiscoveryInterface, requiredGV schema.GroupVersion) error { groups, err := client.ServerGroups() if err != nil { // This is almost always a connection error, and higher level code should treat this as a generic error, // not a negotiation specific error. - return nil, err + return err } versions := metav1.ExtractGroupVersions(groups) serverVersions := sets.String{} @@ -67,46 +55,17 @@ func NegotiateVersion(client DiscoveryInterface, requiredGV *schema.GroupVersion serverVersions.Insert(v) } - // 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 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 requiredGV, nil - } - if serverVersions.Has(requiredGV.String()) { - return requiredGV, nil - } - // If we are using an explicit config version the server does not support, fail. - return nil, fmt.Errorf("server does not support API version %q", requiredGV) + if serverVersions.Has(requiredGV.String()) { + return nil } - for _, clientGV := range clientRegisteredGVs { - if serverVersions.Has(clientGV.String()) { - // Version was not explicitly requested in command config (--api-version). - // Ok to fall back to a supported version with a warning. - // TODO: caesarxuchao: enable the warning message when we have - // proper fix. Please refer to issue #14895. - // if len(version) != 0 { - // glog.Warningf("Server does not support API version '%s'. Falling back to '%s'.", version, clientVersion) - // } - t := clientGV - return &t, nil - } + // If the server supports no versions, then we should pretend it has the version because of old servers. + // This can happen because discovery fails due to 403 Forbidden errors + if len(serverVersions) == 0 { + return 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 - } - - // fall back to an empty GroupVersion. Most client commands no longer respect a GroupVersion anyway - return &schema.GroupVersion{}, nil + return fmt.Errorf("server does not support API version %q", requiredGV) } // GroupVersionResources converts APIResourceLists to the GroupVersionResources. diff --git a/discovery/helper_blackbox_test.go b/discovery/helper_blackbox_test.go index ac4df8a9..7b7236bc 100644 --- a/discovery/helper_blackbox_test.go +++ b/discovery/helper_blackbox_test.go @@ -46,81 +46,40 @@ func objBody(object interface{}) io.ReadCloser { return ioutil.NopCloser(bytes.NewReader([]byte(output))) } -func TestNegotiateVersion(t *testing.T) { +func TestServerSupportsVersion(t *testing.T) { tests := []struct { name string - requiredVersion *schema.GroupVersion - expectedVersion *schema.GroupVersion + requiredVersion schema.GroupVersion serverVersions []string - clientVersions []schema.GroupVersion expectErr func(err error) bool sendErr error statusCode int }{ - { - name: "server supports client default", - serverVersions: []string{"version1", v1.SchemeGroupVersion.String()}, - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, - expectedVersion: &schema.GroupVersion{Version: "version1"}, - statusCode: http.StatusOK, - }, - { - name: "server falls back to client supported", - serverVersions: []string{"version1"}, - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, - expectedVersion: &schema.GroupVersion{Version: "version1"}, - statusCode: http.StatusOK, - }, { name: "explicit version supported", - requiredVersion: &schema.GroupVersion{Version: "v1"}, + requiredVersion: schema.GroupVersion{Version: "v1"}, serverVersions: []string{"/version1", v1.SchemeGroupVersion.String()}, - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, - expectedVersion: &schema.GroupVersion{Version: "v1"}, statusCode: http.StatusOK, }, { name: "explicit version not supported on server", - requiredVersion: &schema.GroupVersion{Version: "v1"}, + requiredVersion: schema.GroupVersion{Version: "v1"}, serverVersions: []string{"version1"}, - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, 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: &schema.GroupVersion{Version: "v1"}, - serverVersions: []string{"v1"}, - clientVersions: []schema.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", serverVersions: []string{"version1"}, - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, sendErr: errors.New("connection refused"), expectErr: func(err error) bool { return strings.Contains(err.Error(), "connection refused") }, statusCode: http.StatusOK, }, - { - name: "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, use default GroupVersion", - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, - expectedVersion: &schema.GroupVersion{Version: "version1"}, - statusCode: http.StatusForbidden, - }, { name: "discovery fails due to 404 Not Found errors and thus serverVersions is empty, use requested GroupVersion", - requiredVersion: &schema.GroupVersion{Version: "version1"}, - clientVersions: []schema.GroupVersion{{Version: "version1"}, v1.SchemeGroupVersion}, - expectedVersion: &schema.GroupVersion{Version: "version1"}, + requiredVersion: schema.GroupVersion{Version: "version1"}, statusCode: http.StatusNotFound, }, - { - name: "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, fallback to empty GroupVersion", - expectedVersion: &schema.GroupVersion{}, - statusCode: http.StatusForbidden, - }, } for _, test := range tests { @@ -141,7 +100,7 @@ func TestNegotiateVersion(t *testing.T) { } c := discovery.NewDiscoveryClientForConfigOrDie(&restclient.Config{}) c.RESTClient().(*restclient.RESTClient).Client = fakeClient.Client - response, err := discovery.NegotiateVersion(c, test.requiredVersion, test.clientVersions) + err := discovery.ServerSupportsVersion(c, test.requiredVersion) if err == nil && test.expectErr != nil { t.Errorf("expected error, got nil for [%s].", test.name) } @@ -151,9 +110,6 @@ func TestNegotiateVersion(t *testing.T) { } continue } - if *response != *test.expectedVersion { - t.Errorf("%s: expected version %s, got %s.", test.name, test.expectedVersion, response) - } } } diff --git a/tools/clientcmd/overrides.go b/tools/clientcmd/overrides.go index 12ff98a2..963ac8fa 100644 --- a/tools/clientcmd/overrides.go +++ b/tools/clientcmd/overrides.go @@ -135,7 +135,6 @@ const ( FlagContext = "context" FlagNamespace = "namespace" FlagAPIServer = "server" - FlagAPIVersion = "api-version" FlagInsecure = "insecure-skip-tls-verify" FlagCertFile = "client-certificate" FlagKeyFile = "client-key" @@ -178,7 +177,6 @@ func RecommendedAuthOverrideFlags(prefix string) AuthOverrideFlags { func RecommendedClusterOverrideFlags(prefix string) ClusterOverrideFlags { return ClusterOverrideFlags{ APIServer: FlagInfo{prefix + FlagAPIServer, "", "", "The address and port of the Kubernetes API server"}, - APIVersion: FlagInfo{prefix + FlagAPIVersion, "", "", "DEPRECATED: The API version to use when talking to the server"}, CertificateAuthority: FlagInfo{prefix + FlagCAFile, "", "", "Path to a cert file for the certificate authority"}, InsecureSkipTLSVerify: FlagInfo{prefix + FlagInsecure, "", "false", "If true, the server's certificate will not be checked for validity. This will make your HTTPS connections insecure"}, }