RESTMapper should take into account multiple versions

When a CLI command `kubectl get rc --api-version=v1beta3` is called, the API resource name should match v1beta3, not whatever the default RESTMapper version is. This allows the correct resource name to be returned ("replicationcontrollers", instead of "replicationControllers").
duda · Dec 22, 2014 · 8bef68d · 8bef68d
1 parent 9b6aec5
commit 8bef68d
Show file tree

Hide file tree

Showing 9 changed files with 76 additions and 38 deletions.
diff --git a/pkg/api/latest/latest_test.go b/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)
 		}

diff --git a/pkg/api/meta/interfaces.go b/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)
 }
diff --git a/pkg/api/meta/restmapper.go b/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
-// provided, the search order provided to DefaultRESTMapper will be used to resolve which
+// RESTClient should use to operate on the provided kind in order of versions. If a version search
+// 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) {
-	// Default to a version with this kind
-	if len(version) == 0 {
+func (m *DefaultRESTMapper) RESTMapping(kind string, versions ...string) (*RESTMapping, error) {
+	// Pick an appropriate version
+	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

diff --git a/pkg/api/meta/restmapper_test.go b/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
-		MixedCase        bool
+		Kind            string
+		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},
+
+		{DefaultVersions: []string{"test"}, Kind: "Unknown", Err: true},
+
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
 
-		{Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"},
-		{Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"},
-		{Kind: "InternalObject", APIVersion: "", Resource: "internalobjects", Version: "test"},
+		{DefaultVersions: []string{"test"}, Kind: "InternalObject", APIVersions: []string{"test"}, Resource: "internalobjects"},
 
-		{Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"},
-		{Kind: "InternalObject", APIVersion: "test", MixedCase: true, 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")
 	}

diff --git a/pkg/config/config.go b/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

diff --git a/pkg/kubectl/cmd/createall.go b/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

diff --git a/pkg/kubectl/cmd/describe_test.go b/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"})
 

diff --git a/pkg/kubectl/cmd/get_test.go b/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"})
 

diff --git a/pkg/kubectl/cmd/resource.go b/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())
 		}
-
-		mapping, err = mapper.RESTMapping(version, kind)
+		version := GetFlagString(cmd, "api-version")
+		mapping, err = mapper.RESTMapping(kind, version, defaultVersion)
 		checkErr(err)
 		return
 	}
@@ -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
@@ -296,7 +297,7 @@ func ResourceFromFile(filename string, typer runtime.ObjectTyper, mapper meta.RE
 	err = schema.ValidateBytes(data)
 	checkErr(err)
 
-	mapping, err = mapper.RESTMapping(version, kind)
+	mapping, err = mapper.RESTMapping(kind, version)
 	checkErr(err)
 
 	obj, err := mapping.Codec.Decode(data)