From 5f8e7745171a9585a24142f4cd33faa1a7f25f07 Mon Sep 17 00:00:00 2001 From: p0lyn0mial Date: Fri, 2 Dec 2016 18:41:26 +0100 Subject: [PATCH] extended RESTMappings method by a version parameter. RESTMapping method can now rely on RESTMappings by passing versions parameter and taking the first match found by RESTMappings method. In addition a UT that test the new method has been added. The only change in logic to what was before is when calling RESTMapping we search all defaultGroupVersion as opposed to just one when no mapping was found for provided versions. --- pkg/api/meta/interfaces.go | 6 +- pkg/api/meta/multirestmapper.go | 4 +- pkg/api/meta/multirestmapper_test.go | 2 +- pkg/api/meta/priority.go | 4 +- pkg/api/meta/restmapper.go | 93 ++++++------------- pkg/api/meta/restmapper_test.go | 86 +++++++++++++++++ pkg/client/typed/discovery/restmapper.go | 6 +- pkg/kubectl/cmd/util/shortcut_restmapper.go | 4 +- .../thirdpartyresourcedata/codec.go | 4 +- 9 files changed, 131 insertions(+), 78 deletions(-) diff --git a/pkg/api/meta/interfaces.go b/pkg/api/meta/interfaces.go index 0ba53cbb247..275f023594a 100644 --- a/pkg/api/meta/interfaces.go +++ b/pkg/api/meta/interfaces.go @@ -177,8 +177,10 @@ type RESTMapper interface { // RESTMapping identifies a preferred resource mapping for the provided group kind. RESTMapping(gk schema.GroupKind, versions ...string) (*RESTMapping, error) - // RESTMappings returns all resource mappings for the provided group kind. - RESTMappings(gk schema.GroupKind) ([]*RESTMapping, error) + // RESTMappings returns all resource mappings for the provided group kind if no + // version search is provided. Otherwise identifies a preferred resource mapping for + // the provided version(s). + RESTMappings(gk schema.GroupKind, versions ...string) ([]*RESTMapping, error) AliasesForResource(resource string) ([]string, bool) ResourceSingularizer(resource string) (singular string, err error) diff --git a/pkg/api/meta/multirestmapper.go b/pkg/api/meta/multirestmapper.go index b47a58cc239..816eb321b65 100644 --- a/pkg/api/meta/multirestmapper.go +++ b/pkg/api/meta/multirestmapper.go @@ -185,12 +185,12 @@ func (m MultiRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (* // RESTMappings returns all possible RESTMappings for the provided group kind, or an error // if the type is not recognized. -func (m MultiRESTMapper) RESTMappings(gk schema.GroupKind) ([]*RESTMapping, error) { +func (m MultiRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*RESTMapping, error) { var allMappings []*RESTMapping var errors []error for _, t := range m { - currMappings, err := t.RESTMappings(gk) + currMappings, err := t.RESTMappings(gk, versions...) // ignore "no match" errors, but any other error percolates back up if IsNoMatchError(err) { continue diff --git a/pkg/api/meta/multirestmapper_test.go b/pkg/api/meta/multirestmapper_test.go index a6cfb1507b0..6d306c3760c 100644 --- a/pkg/api/meta/multirestmapper_test.go +++ b/pkg/api/meta/multirestmapper_test.go @@ -346,7 +346,7 @@ func (m fixedRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (m return nil, m.err } -func (m fixedRESTMapper) RESTMappings(gk schema.GroupKind) (mappings []*RESTMapping, err error) { +func (m fixedRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) (mappings []*RESTMapping, err error) { return m.mappings, m.err } diff --git a/pkg/api/meta/priority.go b/pkg/api/meta/priority.go index 9dc603a1a1c..f99273b6499 100644 --- a/pkg/api/meta/priority.go +++ b/pkg/api/meta/priority.go @@ -205,8 +205,8 @@ func (m PriorityRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) return nil, &AmbiguousKindError{PartialKind: gk.WithVersion(""), MatchingKinds: kinds} } -func (m PriorityRESTMapper) RESTMappings(gk schema.GroupKind) ([]*RESTMapping, error) { - return m.Delegate.RESTMappings(gk) +func (m PriorityRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*RESTMapping, error) { + return m.Delegate.RESTMappings(gk, versions...) } func (m PriorityRESTMapper) AliasesForResource(alias string) (aliases []string, ok bool) { diff --git a/pkg/api/meta/restmapper.go b/pkg/api/meta/restmapper.go index c31224afa67..eb2f6382b06 100644 --- a/pkg/api/meta/restmapper.go +++ b/pkg/api/meta/restmapper.go @@ -468,91 +468,56 @@ func (o resourceByPreferredGroupVersion) Less(i, j int) bool { // RESTClient should use to operate on the provided group/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 // version should be used to access the named group/kind. -// TODO: consider refactoring to use RESTMappings in a way that preserves version ordering and preference func (m *DefaultRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*RESTMapping, error) { - // Pick an appropriate version - var gvk *schema.GroupVersionKind + mappings, err := m.RESTMappings(gk, versions...) + if err != nil { + return nil, err + } + if len(mappings) == 0 { + return nil, &NoKindMatchError{PartialKind: gk.WithVersion("")} + } + // since we rely on RESTMappings method + // take the first match and return to the caller + // as this was the existing behavior. + return mappings[0], nil +} + +// RESTMappings returns the RESTMappings for the provided group kind. If a version search order +// is not provided, the search order provided to DefaultRESTMapper will be used. +func (m *DefaultRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*RESTMapping, error) { + mappings := make([]*RESTMapping, 0) + potentialGVK := make([]schema.GroupVersionKind, 0) hadVersion := false + + // Pick an appropriate version for _, version := range versions { if len(version) == 0 || version == runtime.APIVersionInternal { continue } - currGVK := gk.WithVersion(version) hadVersion = true if _, ok := m.kindToPluralResource[currGVK]; ok { - gvk = &currGVK + potentialGVK = append(potentialGVK, currGVK) break } } // Use the default preferred versions - if !hadVersion && (gvk == nil) { + if !hadVersion && len(potentialGVK) == 0 { for _, gv := range m.defaultGroupVersions { if gv.Group != gk.Group { continue } - - currGVK := gk.WithVersion(gv.Version) - if _, ok := m.kindToPluralResource[currGVK]; ok { - gvk = &currGVK - break - } + potentialGVK = append(potentialGVK, gk.WithVersion(gv.Version)) } } - if gvk == nil { + + if len(potentialGVK) == 0 { return nil, &NoKindMatchError{PartialKind: gk.WithVersion("")} } - // Ensure we have a REST mapping - resource, ok := m.kindToPluralResource[*gvk] - if !ok { - found := []schema.GroupVersion{} - for _, gv := range m.defaultGroupVersions { - if _, ok := m.kindToPluralResource[*gvk]; ok { - found = append(found, gv) - } - } - if len(found) > 0 { - return nil, fmt.Errorf("object with kind %q exists in versions %v, not %v", gvk.Kind, found, gvk.GroupVersion().String()) - } - return nil, fmt.Errorf("the provided version %q and kind %q cannot be mapped to a supported object", gvk.GroupVersion().String(), gvk.Kind) - } - - // Ensure we have a REST scope - scope, ok := m.kindToScope[*gvk] - if !ok { - return nil, fmt.Errorf("the provided version %q and kind %q cannot be mapped to a supported scope", gvk.GroupVersion().String(), gvk.Kind) - } - - interfaces, err := m.interfacesFunc(gvk.GroupVersion()) - if err != nil { - return nil, fmt.Errorf("the provided version %q has no relevant versions: %v", gvk.GroupVersion().String(), err) - } - - retVal := &RESTMapping{ - Resource: resource.Resource, - GroupVersionKind: *gvk, - Scope: scope, - - ObjectConvertor: interfaces.ObjectConvertor, - MetadataAccessor: interfaces.MetadataAccessor, - } - - return retVal, nil -} - -// RESTMappings returns the RESTMappings for the provided group kind in a rough internal preferred order. If no -// kind is found it will return a NoResourceMatchError. -func (m *DefaultRESTMapper) RESTMappings(gk schema.GroupKind) ([]*RESTMapping, error) { - // Use the default preferred versions - var mappings []*RESTMapping - for _, gv := range m.defaultGroupVersions { - if gv.Group != gk.Group { - continue - } - - gvk := gk.WithVersion(gv.Version) - gvr, ok := m.kindToPluralResource[gvk] + for _, gvk := range potentialGVK { + //Ensure we have a REST mapping + res, ok := m.kindToPluralResource[gvk] if !ok { continue } @@ -569,7 +534,7 @@ func (m *DefaultRESTMapper) RESTMappings(gk schema.GroupKind) ([]*RESTMapping, e } mappings = append(mappings, &RESTMapping{ - Resource: gvr.Resource, + Resource: res.Resource, GroupVersionKind: gvk, Scope: scope, diff --git a/pkg/api/meta/restmapper_test.go b/pkg/api/meta/restmapper_test.go index c278c7eec72..6ab9fb642f2 100644 --- a/pkg/api/meta/restmapper_test.go +++ b/pkg/api/meta/restmapper_test.go @@ -651,6 +651,92 @@ func TestRESTMapperRESTMappingSelectsVersion(t *testing.T) { } } +func TestRESTMapperRESTMappings(t *testing.T) { + testGroup := "tgroup" + testGroupVersion := schema.GroupVersion{Group: testGroup, Version: "v1"} + + testCases := []struct { + Kind string + APIGroupVersions []schema.GroupVersion + DefaultVersions []schema.GroupVersion + AddGroupVersionKind []schema.GroupVersionKind + + ExpectedRESTMappings []*RESTMapping + Err bool + }{ + {Kind: "Unknown", Err: true}, + {Kind: "InternalObject", Err: true}, + + {DefaultVersions: []schema.GroupVersion{testGroupVersion}, Kind: "Unknown", Err: true}, + + // ask for specific version - not available - thus error + {DefaultVersions: []schema.GroupVersion{testGroupVersion}, Kind: "InternalObject", APIGroupVersions: []schema.GroupVersion{{Group: testGroup, Version: "v2"}}, Err: true}, + + // ask for specific version - available - check ExpectedRESTMappings + { + DefaultVersions: []schema.GroupVersion{testGroupVersion}, + Kind: "InternalObject", + APIGroupVersions: []schema.GroupVersion{{Group: testGroup, Version: "v2"}}, + AddGroupVersionKind: []schema.GroupVersionKind{schema.GroupVersion{Group: testGroup, Version: "v2"}.WithKind("InternalObject")}, + ExpectedRESTMappings: []*RESTMapping{{Resource: "internalobjects", GroupVersionKind: schema.GroupVersionKind{Group: testGroup, Version: "v2", Kind: "InternalObject"}}}, + }, + + // ask for specific versions - only one available - check ExpectedRESTMappings + { + DefaultVersions: []schema.GroupVersion{testGroupVersion}, + Kind: "InternalObject", + APIGroupVersions: []schema.GroupVersion{{Group: testGroup, Version: "v3"}, {Group: testGroup, Version: "v2"}}, + AddGroupVersionKind: []schema.GroupVersionKind{schema.GroupVersion{Group: testGroup, Version: "v2"}.WithKind("InternalObject")}, + ExpectedRESTMappings: []*RESTMapping{{Resource: "internalobjects", GroupVersionKind: schema.GroupVersionKind{Group: testGroup, Version: "v2", Kind: "InternalObject"}}}, + }, + + // do not ask for specific version - search through default versions - check ExpectedRESTMappings + { + DefaultVersions: []schema.GroupVersion{testGroupVersion, {Group: testGroup, Version: "v2"}}, + Kind: "InternalObject", + AddGroupVersionKind: []schema.GroupVersionKind{schema.GroupVersion{Group: testGroup, Version: "v1"}.WithKind("InternalObject"), schema.GroupVersion{Group: testGroup, Version: "v2"}.WithKind("InternalObject")}, + ExpectedRESTMappings: []*RESTMapping{{Resource: "internalobjects", GroupVersionKind: schema.GroupVersionKind{Group: testGroup, Version: "v1", Kind: "InternalObject"}}, {Resource: "internalobjects", GroupVersionKind: schema.GroupVersionKind{Group: testGroup, Version: "v2", Kind: "InternalObject"}}}, + }, + } + + for i, testCase := range testCases { + mapper := NewDefaultRESTMapper(testCase.DefaultVersions, fakeInterfaces) + for _, gvk := range testCase.AddGroupVersionKind { + mapper.Add(gvk, RESTScopeNamespace) + } + + preferredVersions := []string{} + for _, gv := range testCase.APIGroupVersions { + preferredVersions = append(preferredVersions, gv.Version) + } + gk := schema.GroupKind{Group: testGroup, Kind: testCase.Kind} + + mappings, err := mapper.RESTMappings(gk, preferredVersions...) + hasErr := err != nil + if hasErr != testCase.Err { + t.Errorf("%d: unexpected error behavior %t: %v", i, testCase.Err, err) + } + if hasErr { + continue + } + if len(mappings) != len(testCase.ExpectedRESTMappings) { + t.Errorf("%d: unexpected number = %d of rest mappings was returned, expected = %d", i, len(mappings), len(testCase.ExpectedRESTMappings)) + } + for j, mapping := range mappings { + exp := testCase.ExpectedRESTMappings[j] + if mapping.Resource != exp.Resource { + t.Errorf("%d - %d: unexpected resource: %#v", i, j, mapping) + } + if mapping.MetadataAccessor == nil || mapping.ObjectConvertor == nil { + t.Errorf("%d - %d: missing codec and accessor: %#v", i, j, mapping) + } + if mapping.GroupVersionKind != exp.GroupVersionKind { + t.Errorf("%d - %d: unexpected GroupVersionKind: %#v", i, j, mapping) + } + } + } +} + func TestRESTMapperReportsErrorOnBadVersion(t *testing.T) { expectedGroupVersion1 := schema.GroupVersion{Group: "tgroup", Version: "test1"} expectedGroupVersion2 := schema.GroupVersion{Group: "tgroup", Version: "test2"} diff --git a/pkg/client/typed/discovery/restmapper.go b/pkg/client/typed/discovery/restmapper.go index 53866584f1c..a1889f6aeab 100644 --- a/pkg/client/typed/discovery/restmapper.go +++ b/pkg/client/typed/discovery/restmapper.go @@ -265,15 +265,15 @@ func (d *DeferredDiscoveryRESTMapper) RESTMapping(gk schema.GroupKind, versions // RESTMappings returns the RESTMappings for the provided group kind // in a rough internal preferred order. If no kind is found, it will // return a NoResourceMatchError. -func (d *DeferredDiscoveryRESTMapper) RESTMappings(gk schema.GroupKind) (ms []*meta.RESTMapping, err error) { +func (d *DeferredDiscoveryRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) (ms []*meta.RESTMapping, err error) { del, err := d.getDelegate() if err != nil { return nil, err } - ms, err = del.RESTMappings(gk) + ms, err = del.RESTMappings(gk, versions...) if len(ms) == 0 && !d.cl.Fresh() { d.Reset() - ms, err = d.RESTMappings(gk) + ms, err = d.RESTMappings(gk, versions...) } return } diff --git a/pkg/kubectl/cmd/util/shortcut_restmapper.go b/pkg/kubectl/cmd/util/shortcut_restmapper.go index c85ac277340..357f69862fc 100644 --- a/pkg/kubectl/cmd/util/shortcut_restmapper.go +++ b/pkg/kubectl/cmd/util/shortcut_restmapper.go @@ -101,8 +101,8 @@ func (e ShortcutExpander) RESTMapping(gk schema.GroupKind, versions ...string) ( return e.RESTMapper.RESTMapping(gk, versions...) } -func (e ShortcutExpander) RESTMappings(gk schema.GroupKind) ([]*meta.RESTMapping, error) { - return e.RESTMapper.RESTMappings(gk) +func (e ShortcutExpander) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) { + return e.RESTMapper.RESTMappings(gk, versions...) } // userResources are the resource names that apply to the primary, user facing resources used by diff --git a/pkg/registry/extensions/thirdpartyresourcedata/codec.go b/pkg/registry/extensions/thirdpartyresourcedata/codec.go index 936c45818ff..9dabd225b47 100644 --- a/pkg/registry/extensions/thirdpartyresourcedata/codec.go +++ b/pkg/registry/extensions/thirdpartyresourcedata/codec.go @@ -152,7 +152,7 @@ func (t *thirdPartyResourceDataMapper) RESTMapping(gk schema.GroupKind, versions return mapping, nil } -func (t *thirdPartyResourceDataMapper) RESTMappings(gk schema.GroupKind) ([]*meta.RESTMapping, error) { +func (t *thirdPartyResourceDataMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) { if gk.Group != t.group { return nil, fmt.Errorf("unknown group %q expected %s", gk.Group, t.group) } @@ -163,7 +163,7 @@ func (t *thirdPartyResourceDataMapper) RESTMappings(gk schema.GroupKind) ([]*met // TODO figure out why we're doing this rewriting extensionGK := schema.GroupKind{Group: extensions.GroupName, Kind: "ThirdPartyResourceData"} - mappings, err := t.mapper.RESTMappings(extensionGK) + mappings, err := t.mapper.RESTMappings(extensionGK, versions...) if err != nil { return nil, err }