From 2e64a0d10ca90cfd762299c16ec0f0b85bda5fc4 Mon Sep 17 00:00:00 2001 From: deads2k Date: Mon, 8 Feb 2016 13:14:40 -0500 Subject: [PATCH] update multiRESTMapper to properly union constituent RESTMappers --- pkg/api/meta/multirestmapper.go | 139 ++++++++++++++++++++------- pkg/api/meta/multirestmapper_test.go | 88 ++++++++++++++--- pkg/api/meta/restmapper.go | 20 +--- 3 files changed, 182 insertions(+), 65 deletions(-) diff --git a/pkg/api/meta/multirestmapper.go b/pkg/api/meta/multirestmapper.go index 4d284e62a99..3071d45072c 100644 --- a/pkg/api/meta/multirestmapper.go +++ b/pkg/api/meta/multirestmapper.go @@ -21,6 +21,7 @@ import ( "strings" "k8s.io/kubernetes/pkg/api/unversioned" + utilerrors "k8s.io/kubernetes/pkg/util/errors" ) // MultiRESTMapper is a wrapper for multiple RESTMappers. @@ -50,72 +51,144 @@ func (m MultiRESTMapper) ResourceSingularizer(resource string) (singular string, } func (m MultiRESTMapper) ResourcesFor(resource unversioned.GroupVersionResource) ([]unversioned.GroupVersionResource, error) { + allGVRs := []unversioned.GroupVersionResource{} for _, t := range m { gvrs, err := t.ResourcesFor(resource) // ignore "no match" errors, but any other error percolates back up - if !IsNoResourceMatchError(err) { - return gvrs, err + if IsNoResourceMatchError(err) { + continue + } + if err != nil { + return nil, err + } + + // walk the existing values to de-dup + for _, curr := range gvrs { + found := false + for _, existing := range allGVRs { + if curr == existing { + found = true + break + } + } + + if !found { + allGVRs = append(allGVRs, curr) + } } } - return nil, &NoResourceMatchError{PartialResource: resource} + + if len(allGVRs) == 0 { + return nil, &NoResourceMatchError{PartialResource: resource} + } + + return allGVRs, nil } -// KindsFor provides the Kind mappings for the REST resources. This implementation supports multiple REST schemas and returns -// the first match. func (m MultiRESTMapper) KindsFor(resource unversioned.GroupVersionResource) (gvk []unversioned.GroupVersionKind, err error) { + allGVKs := []unversioned.GroupVersionKind{} for _, t := range m { gvks, err := t.KindsFor(resource) // ignore "no match" errors, but any other error percolates back up - if !IsNoResourceMatchError(err) { - return gvks, err + if IsNoResourceMatchError(err) { + continue + } + if err != nil { + return nil, err + } + + // walk the existing values to de-dup + for _, curr := range gvks { + found := false + for _, existing := range allGVKs { + if curr == existing { + found = true + break + } + } + + if !found { + allGVKs = append(allGVKs, curr) + } } } - return nil, &NoResourceMatchError{PartialResource: resource} + + if len(allGVKs) == 0 { + return nil, &NoResourceMatchError{PartialResource: resource} + } + + return allGVKs, nil } func (m MultiRESTMapper) ResourceFor(resource unversioned.GroupVersionResource) (unversioned.GroupVersionResource, error) { - for _, t := range m { - gvr, err := t.ResourceFor(resource) - // ignore "no match" errors, but any other error percolates back up - if !IsNoResourceMatchError(err) { - return gvr, err - } + resources, err := m.ResourcesFor(resource) + if err != nil { + return unversioned.GroupVersionResource{}, err } - return unversioned.GroupVersionResource{}, &NoResourceMatchError{PartialResource: resource} + if len(resources) == 1 { + return resources[0], nil + } + + return unversioned.GroupVersionResource{}, &AmbiguousResourceError{PartialResource: resource, MatchingResources: resources} } -// KindsFor provides the Kind mapping for the REST resources. This implementation supports multiple REST schemas and returns -// the first match. func (m MultiRESTMapper) KindFor(resource unversioned.GroupVersionResource) (unversioned.GroupVersionKind, error) { - for _, t := range m { - gvk, err := t.KindFor(resource) - // ignore "no match" errors, but any other error percolates back up - if !IsNoResourceMatchError(err) { - return gvk, err - } + kinds, err := m.KindsFor(resource) + if err != nil { + return unversioned.GroupVersionKind{}, err } - return unversioned.GroupVersionKind{}, &NoResourceMatchError{PartialResource: resource} + if len(kinds) == 1 { + return kinds[0], nil + } + + return unversioned.GroupVersionKind{}, &AmbiguousResourceError{PartialResource: resource, MatchingKinds: kinds} } // RESTMapping provides the REST mapping for the resource based on the // kind and version. This implementation supports multiple REST schemas and // return the first match. -func (m MultiRESTMapper) RESTMapping(gk unversioned.GroupKind, versions ...string) (mapping *RESTMapping, err error) { +func (m MultiRESTMapper) RESTMapping(gk unversioned.GroupKind, versions ...string) (*RESTMapping, error) { + allMappings := []*RESTMapping{} + errors := []error{} + for _, t := range m { - mapping, err = t.RESTMapping(gk, versions...) - if err == nil { - return + currMapping, err := t.RESTMapping(gk, versions...) + // ignore "no match" errors, but any other error percolates back up + if IsNoResourceMatchError(err) { + continue } + if err != nil { + errors = append(errors, err) + continue + } + + allMappings = append(allMappings, currMapping) } - return + + // if we got exactly one mapping, then use it even if other requested failed + if len(allMappings) == 1 { + return allMappings[0], nil + } + if len(errors) > 0 { + return nil, utilerrors.NewAggregate(errors) + } + if len(allMappings) == 0 { + return nil, fmt.Errorf("no match found for %v in %v", gk, versions) + } + + return nil, fmt.Errorf("multiple matches found for %v in %v", gk, versions) } // AliasesForResource finds the first alias response for the provided mappers. -func (m MultiRESTMapper) AliasesForResource(alias string) (aliases []string, ok bool) { +func (m MultiRESTMapper) AliasesForResource(alias string) ([]string, bool) { + allAliases := []string{} + handled := false + for _, t := range m { - if aliases, ok = t.AliasesForResource(alias); ok { - return + if currAliases, currOk := t.AliasesForResource(alias); currOk { + allAliases = append(allAliases, currAliases...) + handled = true } } - return nil, false + return allAliases, handled } diff --git a/pkg/api/meta/multirestmapper_test.go b/pkg/api/meta/multirestmapper_test.go index c4d54311589..1b3685e850e 100644 --- a/pkg/api/meta/multirestmapper_test.go +++ b/pkg/api/meta/multirestmapper_test.go @@ -24,7 +24,7 @@ import ( "k8s.io/kubernetes/pkg/api/unversioned" ) -func TestMultiRESTMapperResourceForErrorHandling(t *testing.T) { +func TestMultiRESTMapperResourceFor(t *testing.T) { tcs := []struct { name string @@ -49,7 +49,7 @@ func TestMultiRESTMapperResourceForErrorHandling(t *testing.T) { }, { name: "accept first failure", - mapper: MultiRESTMapper{fixedRESTMapper{err: errors.New("fail on this")}, fixedRESTMapper{resourceFor: unversioned.GroupVersionResource{Resource: "unused"}}}, + mapper: MultiRESTMapper{fixedRESTMapper{err: errors.New("fail on this")}, fixedRESTMapper{resourcesFor: []unversioned.GroupVersionResource{{Resource: "unused"}}}}, input: unversioned.GroupVersionResource{Resource: "foo"}, result: unversioned.GroupVersionResource{}, err: errors.New("fail on this"), @@ -61,13 +61,19 @@ func TestMultiRESTMapperResourceForErrorHandling(t *testing.T) { if e, a := tc.result, actualResult; e != a { t.Errorf("%s: expected %v, got %v", tc.name, e, a) } - if e, a := tc.err.Error(), actualErr.Error(); e != a { - t.Errorf("%s: expected %v, got %v", tc.name, e, a) + switch { + case tc.err == nil && actualErr == nil: + case tc.err == nil: + t.Errorf("%s: unexpected error: %v", tc.name, actualErr) + case actualErr == nil: + t.Errorf("%s: expected error: %v got nil", tc.name, tc.err) + case tc.err.Error() != actualErr.Error(): + t.Errorf("%s: expected %v, got %v", tc.name, tc.err, actualErr) } } } -func TestMultiRESTMapperResourcesForErrorHandling(t *testing.T) { +func TestMultiRESTMapperResourcesFor(t *testing.T) { tcs := []struct { name string @@ -97,6 +103,24 @@ func TestMultiRESTMapperResourcesForErrorHandling(t *testing.T) { result: nil, err: errors.New("fail on this"), }, + { + name: "union and dedup", + mapper: MultiRESTMapper{ + fixedRESTMapper{resourcesFor: []unversioned.GroupVersionResource{{Resource: "dupe"}, {Resource: "first"}}}, + fixedRESTMapper{resourcesFor: []unversioned.GroupVersionResource{{Resource: "dupe"}, {Resource: "second"}}}, + }, + input: unversioned.GroupVersionResource{Resource: "foo"}, + result: []unversioned.GroupVersionResource{{Resource: "dupe"}, {Resource: "first"}, {Resource: "second"}}, + }, + { + name: "skip not and continue", + mapper: MultiRESTMapper{ + fixedRESTMapper{err: &NoResourceMatchError{PartialResource: unversioned.GroupVersionResource{Resource: "IGNORE_THIS"}}}, + fixedRESTMapper{resourcesFor: []unversioned.GroupVersionResource{{Resource: "first"}, {Resource: "second"}}}, + }, + input: unversioned.GroupVersionResource{Resource: "foo"}, + result: []unversioned.GroupVersionResource{{Resource: "first"}, {Resource: "second"}}, + }, } for _, tc := range tcs { @@ -104,13 +128,19 @@ func TestMultiRESTMapperResourcesForErrorHandling(t *testing.T) { if e, a := tc.result, actualResult; !reflect.DeepEqual(e, a) { t.Errorf("%s: expected %v, got %v", tc.name, e, a) } - if e, a := tc.err.Error(), actualErr.Error(); e != a { - t.Errorf("%s: expected %v, got %v", tc.name, e, a) + switch { + case tc.err == nil && actualErr == nil: + case tc.err == nil: + t.Errorf("%s: unexpected error: %v", tc.name, actualErr) + case actualErr == nil: + t.Errorf("%s: expected error: %v got nil", tc.name, tc.err) + case tc.err.Error() != actualErr.Error(): + t.Errorf("%s: expected %v, got %v", tc.name, tc.err, actualErr) } } } -func TestMultiRESTMapperKindsForErrorHandling(t *testing.T) { +func TestMultiRESTMapperKindsFor(t *testing.T) { tcs := []struct { name string @@ -140,6 +170,24 @@ func TestMultiRESTMapperKindsForErrorHandling(t *testing.T) { result: nil, err: errors.New("fail on this"), }, + { + name: "union and dedup", + mapper: MultiRESTMapper{ + fixedRESTMapper{kindsFor: []unversioned.GroupVersionKind{{Kind: "dupe"}, {Kind: "first"}}}, + fixedRESTMapper{kindsFor: []unversioned.GroupVersionKind{{Kind: "dupe"}, {Kind: "second"}}}, + }, + input: unversioned.GroupVersionResource{Resource: "foo"}, + result: []unversioned.GroupVersionKind{{Kind: "dupe"}, {Kind: "first"}, {Kind: "second"}}, + }, + { + name: "skip not and continue", + mapper: MultiRESTMapper{ + fixedRESTMapper{err: &NoResourceMatchError{PartialResource: unversioned.GroupVersionResource{Resource: "IGNORE_THIS"}}}, + fixedRESTMapper{kindsFor: []unversioned.GroupVersionKind{{Kind: "first"}, {Kind: "second"}}}, + }, + input: unversioned.GroupVersionResource{Resource: "foo"}, + result: []unversioned.GroupVersionKind{{Kind: "first"}, {Kind: "second"}}, + }, } for _, tc := range tcs { @@ -147,13 +195,19 @@ func TestMultiRESTMapperKindsForErrorHandling(t *testing.T) { if e, a := tc.result, actualResult; !reflect.DeepEqual(e, a) { t.Errorf("%s: expected %v, got %v", tc.name, e, a) } - if e, a := tc.err.Error(), actualErr.Error(); e != a { - t.Errorf("%s: expected %v, got %v", tc.name, e, a) + switch { + case tc.err == nil && actualErr == nil: + case tc.err == nil: + t.Errorf("%s: unexpected error: %v", tc.name, actualErr) + case actualErr == nil: + t.Errorf("%s: expected error: %v got nil", tc.name, tc.err) + case tc.err.Error() != actualErr.Error(): + t.Errorf("%s: expected %v, got %v", tc.name, tc.err, actualErr) } } } -func TestMultiRESTMapperKindForErrorHandling(t *testing.T) { +func TestMultiRESTMapperKindFor(t *testing.T) { tcs := []struct { name string @@ -178,7 +232,7 @@ func TestMultiRESTMapperKindForErrorHandling(t *testing.T) { }, { name: "accept first failure", - mapper: MultiRESTMapper{fixedRESTMapper{err: errors.New("fail on this")}, fixedRESTMapper{kindFor: unversioned.GroupVersionKind{Kind: "unused"}}}, + mapper: MultiRESTMapper{fixedRESTMapper{err: errors.New("fail on this")}, fixedRESTMapper{kindsFor: []unversioned.GroupVersionKind{{Kind: "unused"}}}}, input: unversioned.GroupVersionResource{Resource: "foo"}, result: unversioned.GroupVersionKind{}, err: errors.New("fail on this"), @@ -190,8 +244,14 @@ func TestMultiRESTMapperKindForErrorHandling(t *testing.T) { if e, a := tc.result, actualResult; e != a { t.Errorf("%s: expected %v, got %v", tc.name, e, a) } - if e, a := tc.err.Error(), actualErr.Error(); e != a { - t.Errorf("%s: expected %v, got %v", tc.name, e, a) + switch { + case tc.err == nil && actualErr == nil: + case tc.err == nil: + t.Errorf("%s: unexpected error: %v", tc.name, actualErr) + case actualErr == nil: + t.Errorf("%s: expected error: %v got nil", tc.name, tc.err) + case tc.err.Error() != actualErr.Error(): + t.Errorf("%s: expected %v, got %v", tc.name, tc.err, actualErr) } } } diff --git a/pkg/api/meta/restmapper.go b/pkg/api/meta/restmapper.go index 7b1856bc4b7..d56b18cea9a 100644 --- a/pkg/api/meta/restmapper.go +++ b/pkg/api/meta/restmapper.go @@ -24,7 +24,6 @@ import ( "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/runtime" - "k8s.io/kubernetes/pkg/util/sets" ) // Implements RESTScope interface @@ -330,23 +329,8 @@ func (m *DefaultRESTMapper) KindFor(resource unversioned.GroupVersionResource) ( if err != nil { return unversioned.GroupVersionKind{}, err } - - // TODO for each group, choose the most preferred (first) version. This keeps us consistent with code today. - // eventually, we'll need a RESTMapper that is aware of what's available server-side and deconflicts that with - // user preferences - oneKindPerGroup := []unversioned.GroupVersionKind{} - groupsAdded := sets.String{} - for _, kind := range kinds { - if groupsAdded.Has(kind.Group) { - continue - } - - oneKindPerGroup = append(oneKindPerGroup, kind) - groupsAdded.Insert(kind.Group) - } - - if len(oneKindPerGroup) == 1 { - return oneKindPerGroup[0], nil + if len(kinds) == 1 { + return kinds[0], nil } return unversioned.GroupVersionKind{}, &AmbiguousResourceError{PartialResource: resource, MatchingKinds: kinds}