From 8bef68d475d2260b4a3bac747ed64f8807a499e5 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 12 Dec 2014 14:05:27 -0500 Subject: [PATCH] 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"). --- pkg/api/latest/latest_test.go | 2 +- pkg/api/meta/interfaces.go | 2 +- pkg/api/meta/restmapper.go | 29 ++++++++++----- pkg/api/meta/restmapper_test.go | 60 ++++++++++++++++++++++---------- pkg/config/config.go | 2 +- pkg/kubectl/cmd/createall.go | 2 +- pkg/kubectl/cmd/describe_test.go | 1 + pkg/kubectl/cmd/get_test.go | 1 + pkg/kubectl/cmd/resource.go | 15 ++++---- 9 files changed, 76 insertions(+), 38 deletions(-) diff --git a/pkg/api/latest/latest_test.go b/pkg/api/latest/latest_test.go index d954ded8ef2..bedb8a1fcae 100644 --- 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 index dd1dba33d0f..9ae4a001bb2 100644 --- 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 index df83f6cc638..9ce592e4396 100644 --- 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 index 7941332ab3f..02eb507777f 100644 --- 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}, - {Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"}, - {Kind: "InternalObject", APIVersion: "test", Resource: "internalobjects"}, - {Kind: "InternalObject", APIVersion: "", Resource: "internalobjects", Version: "test"}, + {DefaultVersions: []string{"test"}, Kind: "Unknown", Err: true}, - {Kind: "InternalObject", APIVersion: "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{"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") } diff --git a/pkg/config/config.go b/pkg/config/config.go index 36f77e1b75f..3a66498f02b 100644 --- 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 index af9995fab3c..b1ea7bb2d9a 100644 --- 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 index 81baed2ee05..914aa2097dd 100644 --- 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 index 13676ef723f..0f5b58200f7 100644 --- 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 index aa72fa22f1d..83455bf7522 100644 --- 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)