Merge pull request #2909 from smarterclayton/passversiontorestmapper

Allow --api-version to properly override various default behaviors in CLI

pkg/api/latest/latest_test.go

@@ -228,7 +228,7 @@ func TestRESTMapper(t *testing.T) {
 	}
 
 	for _, version := range Versions {
-		mapping, err := RESTMapper.RESTMapping(version, "ReplicationController")
+		mapping, err := RESTMapper.RESTMapping("ReplicationController", version)
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}

pkg/api/meta/interfaces.go

@@ -106,5 +106,5 @@ type RESTMapping struct {
 // consumers of Kubernetes compatible REST APIs as defined in docs/api-conventions.md.
 type RESTMapper interface {
 	VersionAndKindForResource(resource string) (defaultVersion, kind string, err error)
-	RESTMapping(version, kind string) (*RESTMapping, error)
+	RESTMapping(kind string, versions ...string) (*RESTMapping, error)
 }

pkg/api/meta/restmapper.go

@@ -122,21 +122,34 @@ func (m *DefaultRESTMapper) VersionAndKindForResource(resource string) (defaultV
 }
 
 // RESTMapping returns a struct representing the resource path and conversion interfaces a
-// RESTClient should use to operate on the provided version and kind. If a version is not
+// RESTClient should use to operate on the provided kind in order of versions. If a version search
-// provided, the search order provided to DefaultRESTMapper will be used to resolve which
+// order is not provided, the search order provided to DefaultRESTMapper will be used to resolve which
 // APIVersion should be used to access the named kind.
-func (m *DefaultRESTMapper) RESTMapping(version, kind string) (*RESTMapping, error) {
+func (m *DefaultRESTMapper) RESTMapping(kind string, versions ...string) (*RESTMapping, error) {
-	// Default to a version with this kind
+	// Pick an appropriate version
-	if len(version) == 0 {
+	var version string
+	hadVersion := false
+	for _, v := range versions {
+		if len(v) == 0 {
+			continue
+		}
+		hadVersion = true
+		if _, ok := m.reverse[typeMeta{APIVersion: v, Kind: kind}]; ok {
+			version = v
+			break
+		}
+	}
+	// Use the default preferred versions
+	if !hadVersion && len(version) == 0 {
 		for _, v := range m.versions {
 			if _, ok := m.reverse[typeMeta{APIVersion: v, Kind: kind}]; ok {
 				version = v
 				break
 			}
 		}
-		if len(version) == 0 {
+	}
-			return nil, fmt.Errorf("no object named %q is registered", kind)
+	if len(version) == 0 {
-		}
+		return nil, fmt.Errorf("no object named %q is registered", kind)
 	}
 
 	// Ensure we have a REST mapping

pkg/api/meta/restmapper_test.go

@@ -122,31 +122,39 @@ func TestKindToResource(t *testing.T) {
 
 func TestRESTMapperRESTMapping(t *testing.T) {
 	testCases := []struct {
-		Kind, APIVersion string
+		Kind            string
-		MixedCase        bool
+		APIVersions     []string
+		MixedCase       bool
+		DefaultVersions []string
 
 		Resource string
 		Version  string
 		Err      bool
 	}{
-		{Kind: "Unknown", APIVersion: "", Err: true},
+		{Kind: "Unknown", Err: true},
+		{Kind: "InternalObject", Err: true},
 
-		{Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"},
+		{DefaultVersions: []string{"test"}, Kind: "Unknown", Err: true},
-		{Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"},
-		{Kind: "InternalObject", APIVersion: "", Resource: "internalobjects", Version: "test"},
 
-		{Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"},
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
-		{Kind: "InternalObject", APIVersion: "test", MixedCase: true, Resource: "internalObjects"},
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
+
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
+
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{}, Resource: "internalobjects", Version: "test"},
+
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, MixedCase: true, Resource: "internalObjects"},
 
 		// TODO: add test for a resource that exists in one version but not another
 	}
 	for i, testCase := range testCases {
-		mapper := NewDefaultRESTMapper([]string{"test"}, fakeInterfaces)
+		mapper := NewDefaultRESTMapper(testCase.DefaultVersions, fakeInterfaces)
 		scheme := runtime.NewScheme()
 		scheme.AddKnownTypes("test", &InternalObject{})
 		mapper.Add(scheme, testCase.MixedCase, "test")
 
-		mapping, err := mapper.RESTMapping(testCase.APIVersion, testCase.Kind)
+		mapping, err := mapper.RESTMapping(testCase.Kind, testCase.APIVersions...)
 		hasErr := err != nil
 		if hasErr != testCase.Err {
 			t.Errorf("%d: unexpected error behavior %t: %v", i, testCase.Err, err)
@@ -159,7 +167,7 @@ func TestRESTMapperRESTMapping(t *testing.T) {
 		}
 		version := testCase.Version
 		if version == "" {
-			version = testCase.APIVersion
+			version = testCase.APIVersions[0]
 		}
 		if mapping.APIVersion != version {
 			t.Errorf("%d: unexpected version: %#v", i, mapping)
@@ -179,37 +187,51 @@ func TestRESTMapperRESTMappingSelectsVersion(t *testing.T) {
 	mapper.Add(scheme, false, "test1", "test2")
 
 	// pick default matching object kind based on search order
-	mapping, err := mapper.RESTMapping("", "OtherObject")
+	mapping, err := mapper.RESTMapping("OtherObject")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 	if mapping.Resource != "otherobjects" || mapping.APIVersion != "test2" {
 		t.Errorf("unexpected mapping: %#v", mapping)
 	}
 
-	mapping, err = mapper.RESTMapping("", "InternalObject")
+	mapping, err = mapper.RESTMapping("InternalObject")
 	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+		t.Fatalf("unexpected error: %v", err)
 	}
 	if mapping.Resource != "internalobjects" || mapping.APIVersion != "test1" {
 		t.Errorf("unexpected mapping: %#v", mapping)
 	}
 
 	// mismatch of version
-	mapping, err = mapper.RESTMapping("test2", "InternalObject")
+	mapping, err = mapper.RESTMapping("InternalObject", "test2")
 	if err == nil {
 		t.Errorf("unexpected non-error")
 	}
-	mapping, err = mapper.RESTMapping("test1", "OtherObject")
+	mapping, err = mapper.RESTMapping("OtherObject", "test1")
 	if err == nil {
 		t.Errorf("unexpected non-error")
 	}
 
 	// not in the search versions
-	mapping, err = mapper.RESTMapping("test3", "OtherObject")
+	mapping, err = mapper.RESTMapping("OtherObject", "test3")
 	if err == nil {
 		t.Errorf("unexpected non-error")
 	}
+
+	// explicit search order
+	mapping, err = mapper.RESTMapping("OtherObject", "test3", "test1")
+	if err == nil {
+		t.Errorf("unexpected non-error")
+	}
+
+	mapping, err = mapper.RESTMapping("OtherObject", "test3", "test2")
+	if err != nil {
+		t.Fatalf("unexpected non-error")
+	}
+	if mapping.Resource != "otherobjects" || mapping.APIVersion != "test2" {
+		t.Errorf("unexpected mapping: %#v", mapping)
+	}
 }
 
 func TestRESTMapperReportsErrorOnBadVersion(t *testing.T) {
@@ -218,7 +240,7 @@ func TestRESTMapperReportsErrorOnBadVersion(t *testing.T) {
 	scheme.AddKnownTypes("test1", &InternalObject{})
 	mapper.Add(scheme, false, "test1")
 
-	_, err := mapper.RESTMapping("test1", "InternalObject")
+	_, err := mapper.RESTMapping("InternalObject", "test1")
 	if err == nil {
 		t.Errorf("unexpected non-error")
 	}

pkg/client/clientcmd/client_builder.go

@@ -33,10 +33,15 @@ import (
 type Builder interface {
 	// BindFlags must bind and keep track of all the flags required to build a client config object
 	BindFlags(flags *pflag.FlagSet)
-	// Config uses the values of the bound flags and builds a complete client config
-	Config() (*client.Config, error)
 	// Client calls BuildConfig under the covers and uses that config to return a client
 	Client() (*client.Client, error)
+
+	// Config uses the values of the bound flags and builds a complete client config
+	Config() (*client.Config, error)
+	// Override invokes Config(), then passes that to the provided function, and returns a new
+	// builder that will use that config as its default. If Config() returns an error for the default
+	// values the function will not be invoked, and the error will be available when Client() is called.
+	Override(func(*client.Config)) Builder
 }
 
 // cmdAuthInfo is used to track whether flags have been set
@@ -58,6 +63,8 @@ type builder struct {
 	apiserver       string
 	apiVersion      string
 	matchApiVersion bool
+
+	config *client.Config
 }
 
 // NewBuilder returns a valid Builder that uses the passed authLoader.  If authLoader is nil, the NewDefaultAuthLoader is used.
@@ -124,6 +131,26 @@ func (builder *builder) Client() (*client.Client, error) {
 
 // Config implements Builder
 func (builder *builder) Config() (*client.Config, error) {
+	if builder.config != nil {
+		return builder.config, nil
+	}
+	return builder.newConfig()
+}
+
+// Override implements Builder
+func (builder *builder) Override(fn func(*client.Config)) Builder {
+	config, err := builder.newConfig()
+	if err != nil {
+		return builder
+	}
+	fn(config)
+	b := *builder
+	b.config = config
+	return &b
+}
+
+// newConfig creates a new config object for this builder
+func (builder *builder) newConfig() (*client.Config, error) {
 	clientConfig := client.Config{}
 	if len(builder.apiserver) > 0 {
 		clientConfig.Host = builder.apiserver
@@ -140,12 +167,11 @@ func (builder *builder) Config() (*client.Config, error) {
 		authInfoFileFound := true
 		authInfo, err := builder.authLoader.LoadAuth(builder.authPath)
 		if authInfo == nil && err != nil { // only consider failing if we don't have any auth info
-			if os.IsNotExist(err) { // if it's just a case of a missing file, simply flag the auth as not found and use the command line arguments
+			if !os.IsNotExist(err) { // if it's just a case of a missing file, simply flag the auth as not found and use the command line arguments
-				authInfoFileFound = false
-				authInfo = &clientauth.Info{}
-			} else {
 				return nil, err
 			}
+			authInfoFileFound = false
+			authInfo = &clientauth.Info{}
 		}
 
 		// If provided, the command line options override options from the auth file

pkg/client/clientcmd/client_builder_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/spf13/pflag"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/clientauth"
 )
 
@@ -251,6 +252,40 @@ func TestLoadClientAuthInfoOrPrompt(t *testing.T) {
 	}
 }
 
+func TestOverride(t *testing.T) {
+	b := NewBuilder(nil)
+	cfg, err := b.Config()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if cfg.Version != "" {
+		t.Errorf("unexpected default config version")
+	}
+
+	newCfg, err := b.Override(func(cfg *client.Config) {
+		if cfg.Version != "" {
+			t.Errorf("unexpected default config version")
+		}
+		cfg.Version = "test"
+	}).Config()
+
+	if newCfg.Version != "test" {
+		t.Errorf("unexpected override config version")
+	}
+
+	if cfg.Version != "" {
+		t.Errorf("original object should not change")
+	}
+
+	cfg, err = b.Config()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if cfg.Version != "" {
+		t.Errorf("override should not be persistent")
+	}
+}
+
 func matchStringArg(expected, got string, t *testing.T) {
 	if expected != got {
 		t.Errorf("Expected %v, got %v", expected, got)

pkg/config/config.go

@@ -45,7 +45,7 @@ func CreateObjects(typer runtime.ObjectTyper, mapper meta.RESTMapper, clientFor
 			continue
 		}
 
-		mapping, err := mapper.RESTMapping(version, kind)
+		mapping, err := mapper.RESTMapping(kind, version)
 		if err != nil {
 			allErrors = append(allErrors, fmt.Errorf("Config.item[%d] mapping: %v", i, err))
 			continue

pkg/kubectl/cmd/cmd.go

@@ -64,7 +64,9 @@ func NewFactory(clientBuilder clientcmd.Builder) *Factory {
 			}
 		},
 		Client: func(cmd *cobra.Command, mapping *meta.RESTMapping) (kubectl.RESTClient, error) {
-			return clientBuilder.Client()
+			return clientBuilder.Override(func(c *client.Config) {
+				c.Version = mapping.APIVersion
+			}).Client()
 		},
 		Describer: func(cmd *cobra.Command, mapping *meta.RESTMapping) (kubectl.Describer, error) {
 			client, err := clientBuilder.Client()

pkg/kubectl/cmd/create.go

@@ -45,7 +45,7 @@ Examples:
 			}
 			schema, err := f.Validator(cmd)
 			checkErr(err)
-			mapping, namespace, name, data := ResourceFromFile(filename, f.Typer, f.Mapper, schema)
+			mapping, namespace, name, data := ResourceFromFile(cmd, filename, f.Typer, f.Mapper, schema)
 			client, err := f.Client(cmd, mapping)
 			checkErr(err)
 

pkg/kubectl/cmd/createall.go

@@ -44,7 +44,7 @@ func DataToObjects(m meta.RESTMapper, t runtime.ObjectTyper, data []byte) (resul
 			continue
 		}
 
-		mapping, err := m.RESTMapping(version, kind)
+		mapping, err := m.RESTMapping(kind, version)
 		if err != nil {
 			errors = append(errors, fmt.Errorf("item[%d] mapping: %v", i, err))
 			continue

pkg/kubectl/cmd/describe_test.go

@@ -37,6 +37,7 @@ func TestDescribeUnknownSchemaObject(t *testing.T) {
 	buf := bytes.NewBuffer([]byte{})
 
 	cmd := f.NewCmdDescribe(buf)
+	cmd.Flags().String("api-version", "default", "")
 	cmd.Flags().String("namespace", "test", "")
 	cmd.Run(cmd, []string{"type", "foo"})
 

pkg/kubectl/cmd/get_test.go

@@ -37,6 +37,7 @@ func TestGetUnknownSchemaObject(t *testing.T) {
 	buf := bytes.NewBuffer([]byte{})
 
 	cmd := f.NewCmdGet(buf)
+	cmd.Flags().String("api-version", "default", "")
 	cmd.Flags().String("namespace", "test", "")
 	cmd.Run(cmd, []string{"type", "foo"})
 

pkg/kubectl/cmd/resource.go

@@ -201,13 +201,13 @@ func ResourceFromArgsOrFile(cmd *cobra.Command, args []string, filename string,
 			usageError(cmd, "Must specify filename or command line params")
 		}
 
-		version, kind, err := mapper.VersionAndKindForResource(resource)
+		defaultVersion, kind, err := mapper.VersionAndKindForResource(resource)
 		if err != nil {
 			// The error returned by mapper is "no resource defined", which is a usage error
 			usageError(cmd, err.Error())
 		}
-
+		version := GetFlagString(cmd, "api-version")
-		mapping, err = mapper.RESTMapping(version, kind)
+		mapping, err = mapper.RESTMapping(kind, version, defaultVersion)
 		checkErr(err)
 		return
 	}
@@ -216,7 +216,7 @@ func ResourceFromArgsOrFile(cmd *cobra.Command, args []string, filename string,
 		usageError(cmd, "Must specify filename or command line params")
 	}
 
-	mapping, namespace, name, _ = ResourceFromFile(filename, typer, mapper, schema)
+	mapping, namespace, name, _ = ResourceFromFile(cmd, filename, typer, mapper, schema)
 	if len(name) == 0 {
 		checkErr(fmt.Errorf("the resource in the provided file has no name (or ID) defined"))
 	}
@@ -242,7 +242,7 @@ func ResourceFromArgs(cmd *cobra.Command, args []string, mapper meta.RESTMapper)
 	version, kind, err := mapper.VersionAndKindForResource(resource)
 	checkErr(err)
 
-	mapping, err = mapper.RESTMapping(version, kind)
+	mapping, err = mapper.RESTMapping(kind, version)
 	checkErr(err)
 	return
 }
@@ -268,10 +268,11 @@ func ResourceOrTypeFromArgs(cmd *cobra.Command, args []string, mapper meta.RESTM
 		}
 	}
 
-	version, kind, err := mapper.VersionAndKindForResource(resource)
+	defaultVersion, kind, err := mapper.VersionAndKindForResource(resource)
 	checkErr(err)
 
-	mapping, err = mapper.RESTMapping(version, kind)
+	version := GetFlagString(cmd, "api-version")
+	mapping, err = mapper.RESTMapping(kind, version, defaultVersion)
 	checkErr(err)
 
 	return
@@ -280,23 +281,24 @@ func ResourceOrTypeFromArgs(cmd *cobra.Command, args []string, mapper meta.RESTM
 // ResourceFromFile retrieves the name and namespace from a valid file. If the file does not
 // resolve to a known type an error is returned. The returned mapping can be used to determine
 // the correct REST endpoint to modify this resource with.
-func ResourceFromFile(filename string, typer runtime.ObjectTyper, mapper meta.RESTMapper, schema validation.Schema) (mapping *meta.RESTMapping, namespace, name string, data []byte) {
+func ResourceFromFile(cmd *cobra.Command, filename string, typer runtime.ObjectTyper, mapper meta.RESTMapper, schema validation.Schema) (mapping *meta.RESTMapping, namespace, name string, data []byte) {
 	configData, err := ReadConfigData(filename)
 	checkErr(err)
 	data = configData
 
-	version, kind, err := typer.DataVersionAndKind(data)
+	objVersion, kind, err := typer.DataVersionAndKind(data)
 	checkErr(err)
 
 	// TODO: allow unversioned objects?
-	if len(version) == 0 {
+	if len(objVersion) == 0 {
 		checkErr(fmt.Errorf("the resource in the provided file has no apiVersion defined"))
 	}
 
 	err = schema.ValidateBytes(data)
 	checkErr(err)
 
-	mapping, err = mapper.RESTMapping(version, kind)
+	// decode using the version stored with the object (allows codec to vary across versions)
+	mapping, err = mapper.RESTMapping(kind, objVersion)
 	checkErr(err)
 
 	obj, err := mapping.Codec.Decode(data)
@@ -308,6 +310,13 @@ func ResourceFromFile(filename string, typer runtime.ObjectTyper, mapper meta.RE
 	name, err = meta.Name(obj)
 	checkErr(err)
 
+	// if the preferred API version differs, get a different mapper
+	version := GetFlagString(cmd, "api-version")
+	if version != objVersion {
+		mapping, err = mapper.RESTMapping(kind, version)
+		checkErr(err)
+	}
+
 	return
 }
 

pkg/kubectl/cmd/update.go

@@ -45,7 +45,7 @@ Examples:
 			}
 			schema, err := f.Validator(cmd)
 			checkErr(err)
-			mapping, namespace, name, data := ResourceFromFile(filename, f.Typer, f.Mapper, schema)
+			mapping, namespace, name, data := ResourceFromFile(cmd, filename, f.Typer, f.Mapper, schema)
 			client, err := f.Client(cmd, mapping)
 			checkErr(err)