update multiRESTMapper to properly union constituent RESTMappers

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) {
+		if IsNoResourceMatchError(err) {
-			return gvrs, 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) {
+		if IsNoResourceMatchError(err) {
-			return gvks, 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 {
+	resources, err := m.ResourcesFor(resource)
-		gvr, err := t.ResourceFor(resource)
+	if err != nil {
-		// ignore "no match" errors, but any other error percolates back up
+		return unversioned.GroupVersionResource{}, err
-		if !IsNoResourceMatchError(err) {
-			return gvr, 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 {
+	kinds, err := m.KindsFor(resource)
-		gvk, err := t.KindFor(resource)
+	if err != nil {
-		// ignore "no match" errors, but any other error percolates back up
+		return unversioned.GroupVersionKind{}, err
-		if !IsNoResourceMatchError(err) {
-			return gvk, 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...)
+		currMapping, err := t.RESTMapping(gk, versions...)
-		if err == nil {
+		// ignore "no match" errors, but any other error percolates back up
-			return
+		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 {
+		if currAliases, currOk := t.AliasesForResource(alias); currOk {
-			return
+			allAliases = append(allAliases, currAliases...)
+			handled = true
 		}
 	}
-	return nil, false
+	return allAliases, handled
 }

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 {
+		switch {
-			t.Errorf("%s: expected %v, got %v", tc.name, e, a)
+		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 {
+		switch {
-			t.Errorf("%s: expected %v, got %v", tc.name, e, a)
+		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 {
+		switch {
-			t.Errorf("%s: expected %v, got %v", tc.name, e, a)
+		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 {
+		switch {
-			t.Errorf("%s: expected %v, got %v", tc.name, e, a)
+		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)
 		}
 	}
 }

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
 	}
-
+	if len(kinds) == 1 {
-	// TODO for each group, choose the most preferred (first) version.  This keeps us consistent with code today.
+		return kinds[0], nil
-	// 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
 	}
 
 	return unversioned.GroupVersionKind{}, &AmbiguousResourceError{PartialResource: resource, MatchingKinds: kinds}