Merge pull request #45743 from deads2k/cli-12-apiversion

Automatic merge from submit-queue

stop secretly setting defaults under the pretense of negotiation

Turns out that the negotiate code in kubectl doesn't negotiate and that the clients to respect it anyway.   The `--api-version` flag has been deprecated since 1.2 and the comment says to remove it in 1.3, so I finally did. 

After chasing the repercussions backwards, I agree with @soltysh that the negotiate was just giving back an effectively random groupversion when the argument wasn't being set.

I tried leaving it as nil: panic.  I tried setting it to empty: can't encode parameters.  This isn't pretty, but it at least makes the fact that we're setting `""/v1` explicit and I think it resolves the describer problem.  It also simplifies the client-cache code, which is a thing we needed.

pkg/client/unversioned/helper.go

@@ -17,6 +17,7 @@ limitations under the License.
 package unversioned
 
 import (
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/kubernetes/pkg/api"
 	// Import solely to initialize client auth plugins.
@@ -35,13 +36,9 @@ func SetKubernetesDefaults(config *restclient.Config) error {
 	if config.APIPath == "" {
 		config.APIPath = legacyAPIPath
 	}
-	if config.GroupVersion == nil || config.GroupVersion.Group != api.GroupName {
-		g, err := api.Registry.Group(api.GroupName)
-		if err != nil {
-			return err
-		}
-		copyGroupVersion := g.GroupVersion
-		config.GroupVersion = &copyGroupVersion
+	// TODO chase down uses and tolerate nil
+	if config.GroupVersion == nil {
+		config.GroupVersion = &schema.GroupVersion{}
 	}
 	if config.NegotiatedSerializer == nil {
 		config.NegotiatedSerializer = api.Codecs

pkg/client/unversioned/helper_test.go

@@ -42,7 +42,7 @@ func TestSetKubernetesDefaults(t *testing.T) {
 			restclient.Config{
 				APIPath: "/api",
 				ContentConfig: restclient.ContentConfig{
-					GroupVersion:         &api.Registry.GroupOrDie(api.GroupName).GroupVersion,
+					GroupVersion:         &schema.GroupVersion{},
 					NegotiatedSerializer: testapi.Default.NegotiatedSerializer(),
 				},
 			},

pkg/kubectl/cmd/config/create_cluster.go

@@ -37,7 +37,6 @@ type createClusterOptions struct {
 	configAccess          clientcmd.ConfigAccess
 	name                  string
 	server                flag.StringFlag
-	apiVersion            flag.StringFlag
 	insecureSkipTLSVerify flag.Tristate
 	certificateAuthority  flag.StringFlag
 	embedCAData           flag.Tristate
@@ -78,7 +77,6 @@ func NewCmdConfigSetCluster(out io.Writer, configAccess clientcmd.ConfigAccess)
 	options.insecureSkipTLSVerify.Default(false)
 
 	cmd.Flags().Var(&options.server, clientcmd.FlagAPIServer, clientcmd.FlagAPIServer+" for the cluster entry in kubeconfig")
-	cmd.Flags().Var(&options.apiVersion, clientcmd.FlagAPIVersion, clientcmd.FlagAPIVersion+" for the cluster entry in kubeconfig")
 	f := cmd.Flags().VarPF(&options.insecureSkipTLSVerify, clientcmd.FlagInsecure, "", clientcmd.FlagInsecure+" for the cluster entry in kubeconfig")
 	f.NoOptDefVal = "true"
 	cmd.Flags().Var(&options.certificateAuthority, clientcmd.FlagCAFile, "path to "+clientcmd.FlagCAFile+" file for the cluster entry in kubeconfig")
@@ -118,9 +116,6 @@ func (o createClusterOptions) run() error {
 func (o *createClusterOptions) modifyCluster(existingCluster clientcmdapi.Cluster) clientcmdapi.Cluster {
 	modifiedCluster := existingCluster
 
-	if o.apiVersion.Provided() {
-		modifiedCluster.APIVersion = o.apiVersion.Value()
-	}
 	if o.server.Provided() {
 		modifiedCluster.Server = o.server.Value()
 	}

pkg/kubectl/cmd/explain.go

@@ -57,6 +57,7 @@ func NewCmdExplain(f cmdutil.Factory, out, cmdErr io.Writer) *cobra.Command {
 		},
 	}
 	cmd.Flags().Bool("recursive", false, "Print the fields of fields (Currently only 1 level deep)")
+	cmd.Flags().String("api-version", "", "The API version to use when talking to the server")
 	cmdutil.AddInclude3rdPartyFlags(cmd)
 	return cmd
 }

pkg/kubectl/cmd/util/clientcache.go

@@ -24,7 +24,6 @@ import (
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	fedclientset "k8s.io/kubernetes/federation/client/clientset_generated/federation_internalclientset"
-	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	oldclient "k8s.io/kubernetes/pkg/client/unversioned"
 	"k8s.io/kubernetes/pkg/version"
@@ -98,19 +97,7 @@ func (c *ClientCache) ClientConfigForVersion(requiredVersion *schema.GroupVersio
 
 // clientConfigForVersion returns the correct config for a server
 func (c *ClientCache) clientConfigForVersion(requiredVersion *schema.GroupVersion) (*restclient.Config, error) {
-	// TODO: have a better config copy method
-	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
-	}
-
-	// required version may still be nil, since config.GroupVersion may have been nil.  Do the check
-	// before looking up from the cache
+	// only lookup in the cache if the requiredVersion is set
 	if requiredVersion != nil {
 		if config, ok := c.configs[*requiredVersion]; ok {
 			return copyConfig(config), nil
@@ -119,11 +106,21 @@ func (c *ClientCache) clientConfigForVersion(requiredVersion *schema.GroupVersio
 		return copyConfig(c.noVersionConfig), nil
 	}
 
-	negotiatedVersion, err := discovery.NegotiateVersion(discoveryClient, requiredVersion, api.Registry.EnabledVersions())
+	// this returns a shallow copy to work with
+	config, discoveryClient, err := c.getDefaultConfig()
 	if err != nil {
 		return nil, err
 	}
-	config.GroupVersion = negotiatedVersion
+
+	if requiredVersion != nil {
+		if err := discovery.ServerSupportsVersion(discoveryClient, *requiredVersion); err != nil {
+			return nil, err
+		}
+		config.GroupVersion = requiredVersion
+	} else {
+		// TODO remove this hack.  This is allowing the GetOptions to be serialized.
+		config.GroupVersion = &schema.GroupVersion{Group: "", Version: "v1"}
+	}
 
 	// TODO this isn't what we want.  Each clientset should be setting defaults as it sees fit.
 	oldclient.SetKubernetesDefaults(&config)

staging/src/k8s.io/client-go/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.

staging/src/k8s.io/client-go/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)
-		}
 	}
 }
 

staging/src/k8s.io/client-go/tools/clientcmd/api/types.go

@@ -30,11 +30,8 @@ type Config struct {
 	// TODO(jlowdermilk): remove this after eliminating downstream dependencies.
 	// +optional
 	Kind string `json:"kind,omitempty"`
-	// DEPRECATED: APIVersion is the preferred api version for communicating with the kubernetes cluster (v1, v2, etc).
-	// Because a cluster can run multiple API groups and potentially multiple versions of each, it no longer makes sense to specify
-	// a single value for the cluster version.
-	// This field isn't really needed anyway, so we are deprecating it without replacement.
-	// It will be ignored if it is present.
+	// Legacy field from pkg/api/types.go TypeMeta.
+	// TODO(jlowdermilk): remove this after eliminating downstream dependencies.
 	// +optional
 	APIVersion string `json:"apiVersion,omitempty"`
 	// Preferences holds general information to be use for cli interactions
@@ -67,9 +64,6 @@ type Cluster struct {
 	LocationOfOrigin string
 	// Server is the address of the kubernetes cluster (https://hostname:port).
 	Server string `json:"server"`
-	// APIVersion is the preferred api version for communicating with the kubernetes cluster (v1, v2, etc).
-	// +optional
-	APIVersion string `json:"api-version,omitempty"`
 	// InsecureSkipTLSVerify skips the validity check for the server's certificate. This will make your HTTPS connections insecure.
 	// +optional
 	InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify,omitempty"`

staging/src/k8s.io/client-go/tools/clientcmd/api/v1/types.go

@@ -29,11 +29,8 @@ type Config struct {
 	// TODO(jlowdermilk): remove this after eliminating downstream dependencies.
 	// +optional
 	Kind string `json:"kind,omitempty"`
-	// DEPRECATED: APIVersion is the preferred api version for communicating with the kubernetes cluster (v1, v2, etc).
-	// Because a cluster can run multiple API groups and potentially multiple versions of each, it no longer makes sense to specify
-	// a single value for the cluster version.
-	// This field isn't really needed anyway, so we are deprecating it without replacement.
-	// It will be ignored if it is present.
+	// Legacy field from pkg/api/types.go TypeMeta.
+	// TODO(jlowdermilk): remove this after eliminating downstream dependencies.
 	// +optional
 	APIVersion string `json:"apiVersion,omitempty"`
 	// Preferences holds general information to be use for cli interactions
@@ -63,9 +60,6 @@ type Preferences struct {
 type Cluster struct {
 	// Server is the address of the kubernetes cluster (https://hostname:port).
 	Server string `json:"server"`
-	// APIVersion is the preferred api version for communicating with the kubernetes cluster (v1, v2, etc).
-	// +optional
-	APIVersion string `json:"api-version,omitempty"`
 	// InsecureSkipTLSVerify skips the validity check for the server's certificate. This will make your HTTPS connections insecure.
 	// +optional
 	InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify,omitempty"`

staging/src/k8s.io/client-go/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"},
 	}
@@ -216,9 +214,6 @@ func BindAuthInfoFlags(authInfo *clientcmdapi.AuthInfo, flags *pflag.FlagSet, fl
 // BindClusterFlags is a convenience method to bind the specified flags to their associated variables
 func BindClusterFlags(clusterInfo *clientcmdapi.Cluster, flags *pflag.FlagSet, flagNames ClusterOverrideFlags) {
 	flagNames.APIServer.BindStringFlag(flags, &clusterInfo.Server)
-	// TODO: remove --api-version flag in 1.3.
-	flagNames.APIVersion.BindStringFlag(flags, &clusterInfo.APIVersion)
-	flags.MarkDeprecated(FlagAPIVersion, "flag is no longer respected and will be deleted in the next release")
 	flagNames.CertificateAuthority.BindStringFlag(flags, &clusterInfo.CertificateAuthority)
 	flagNames.InsecureSkipTLSVerify.BindBoolFlag(flags, &clusterInfo.InsecureSkipTLSVerify)
 }

test/integration/examples/apiserver_test.go

@@ -378,9 +378,6 @@ func createKubeConfig(clientCfg *rest.Config) *clientcmdapi.Config {
 		cluster.CertificateAuthorityData = clientCfg.CAData
 	}
 	cluster.InsecureSkipTLSVerify = clientCfg.Insecure
-	if clientCfg.GroupVersion != nil {
-		cluster.APIVersion = clientCfg.GroupVersion.String()
-	}
 	config.Clusters[clusterNick] = cluster
 
 	context := clientcmdapi.NewContext()