Merge pull request #32182 from smarterclayton/aggregate_discovery_errors

Automatic merge from submit-queue

Use a structured error rather than an Aggregate error in discovery

Should provide more information for debugging the root cause of
discovery failures.

Exposes the true cause of #32009

@lavalamp @derekwaynecarr

pkg/client/typed/discovery/discovery_client.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/url"
+	"sort"
 	"strings"
 
 	"github.com/emicklei/go-restful/swagger"
@@ -31,7 +32,6 @@ import (
 	"k8s.io/kubernetes/pkg/client/restclient"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/runtime/serializer"
-	utilerrors "k8s.io/kubernetes/pkg/util/errors"
 	"k8s.io/kubernetes/pkg/version"
 )
 
@@ -149,9 +149,8 @@ func (d *DiscoveryClient) ServerResourcesForGroupVersion(groupVersion string) (r
 		// ignore 403 or 404 error to be compatible with an v1.0 server.
 		if groupVersion == "v1" && (errors.IsNotFound(err) || errors.IsForbidden(err)) {
 			return resources, nil
-		} else {
-			return nil, err
 		}
+		return nil, err
 	}
 	return resources, nil
 }
@@ -174,6 +173,29 @@ func (d *DiscoveryClient) ServerResources() (map[string]*unversioned.APIResource
 	return result, nil
 }
 
+// ErrGroupDiscoveryFailed is returned if one or more API groups fail to load.
+type ErrGroupDiscoveryFailed struct {
+	// Groups is a list of the groups that failed to load and the error cause
+	Groups map[unversioned.GroupVersion]error
+}
+
+// Error implements the error interface
+func (e *ErrGroupDiscoveryFailed) Error() string {
+	var groups []string
+	for k, v := range e.Groups {
+		groups = append(groups, fmt.Sprintf("%s: %v", k, v))
+	}
+	sort.Strings(groups)
+	return fmt.Sprintf("unable to retrieve the complete list of server APIs: %s", strings.Join(groups, ", "))
+}
+
+// IsGroupDiscoveryFailedError returns true if the provided error indicates the server was unable to discover
+// a complete list of APIs for the client to use.
+func IsGroupDiscoveryFailedError(err error) bool {
+	_, ok := err.(*ErrGroupDiscoveryFailed)
+	return err != nil && ok
+}
+
 // serverPreferredResources returns the supported resources with the version preferred by the
 // server. If namespaced is true, only namespaced resources will be returned.
 func (d *DiscoveryClient) serverPreferredResources(namespaced bool) ([]unversioned.GroupVersionResource, error) {
@@ -183,15 +205,18 @@ func (d *DiscoveryClient) serverPreferredResources(namespaced bool) ([]unversion
 		return results, err
 	}
 
-	allErrs := []error{}
+	var failedGroups map[unversioned.GroupVersion]error
 	for _, apiGroup := range serverGroupList.Groups {
 		preferredVersion := apiGroup.PreferredVersion
+		groupVersion := unversioned.GroupVersion{Group: apiGroup.Name, Version: preferredVersion.Version}
 		apiResourceList, err := d.ServerResourcesForGroupVersion(preferredVersion.GroupVersion)
 		if err != nil {
-			allErrs = append(allErrs, err)
+			if failedGroups == nil {
+				failedGroups = make(map[unversioned.GroupVersion]error)
+			}
+			failedGroups[groupVersion] = err
 			continue
 		}
-		groupVersion := unversioned.GroupVersion{Group: apiGroup.Name, Version: preferredVersion.Version}
 		for _, apiResource := range apiResourceList.APIResources {
 			// ignore the root scoped resources if "namespaced" is true.
 			if namespaced && !apiResource.Namespaced {
@@ -203,7 +228,10 @@ func (d *DiscoveryClient) serverPreferredResources(namespaced bool) ([]unversion
 			results = append(results, groupVersion.WithResource(apiResource.Name))
 		}
 	}
-	return results, utilerrors.NewAggregate(allErrs)
+	if len(failedGroups) > 0 {
+		return results, &ErrGroupDiscoveryFailed{Groups: failedGroups}
+	}
+	return results, nil
 }
 
 // ServerPreferredResources returns the supported resources with the version preferred by the

pkg/client/typed/discovery/discovery_client_test.go

@@ -320,3 +320,137 @@ func TestGetSwaggerSchemaFail(t *testing.T) {
 		t.Errorf("expected an error, got %v", err)
 	}
 }
+
+func TestGetServerPreferredResources(t *testing.T) {
+	stable := unversioned.APIResourceList{
+		GroupVersion: "v1",
+		APIResources: []unversioned.APIResource{
+			{Name: "pods", Namespaced: true, Kind: "Pod"},
+			{Name: "services", Namespaced: true, Kind: "Service"},
+			{Name: "namespaces", Namespaced: false, Kind: "Namespace"},
+		},
+	}
+	/*beta := unversioned.APIResourceList{
+		GroupVersion: "extensions/v1",
+		APIResources: []unversioned.APIResource{
+			{Name: "deployments", Namespaced: true, Kind: "Deployment"},
+			{Name: "ingresses", Namespaced: true, Kind: "Ingress"},
+			{Name: "jobs", Namespaced: true, Kind: "Job"},
+		},
+	}*/
+	tests := []struct {
+		resourcesList *unversioned.APIResourceList
+		response      func(w http.ResponseWriter, req *http.Request)
+		expectErr     func(err error) bool
+	}{
+		{
+			resourcesList: &stable,
+			expectErr:     IsGroupDiscoveryFailedError,
+			response: func(w http.ResponseWriter, req *http.Request) {
+				var list interface{}
+				switch req.URL.Path {
+				case "/apis/extensions/v1beta1":
+					w.WriteHeader(http.StatusInternalServerError)
+					return
+				case "/api/v1":
+					list = &stable
+				case "/api":
+					list = &unversioned.APIVersions{
+						Versions: []string{
+							"v1",
+						},
+					}
+				case "/apis":
+					list = &unversioned.APIGroupList{
+						Groups: []unversioned.APIGroup{
+							{
+								Versions: []unversioned.GroupVersionForDiscovery{
+									{GroupVersion: "extensions/v1beta1"},
+								},
+							},
+						},
+					}
+				default:
+					t.Logf("unexpected request: %s", req.URL.Path)
+					w.WriteHeader(http.StatusNotFound)
+					return
+				}
+				output, err := json.Marshal(list)
+				if err != nil {
+					t.Errorf("unexpected encoding error: %v", err)
+					return
+				}
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				w.Write(output)
+			},
+		},
+		{
+			resourcesList: nil,
+			expectErr:     IsGroupDiscoveryFailedError,
+			response: func(w http.ResponseWriter, req *http.Request) {
+				var list interface{}
+				switch req.URL.Path {
+				case "/apis/extensions/v1beta1":
+					w.WriteHeader(http.StatusInternalServerError)
+					return
+				case "/api/v1":
+					w.WriteHeader(http.StatusInternalServerError)
+				case "/api":
+					list = &unversioned.APIVersions{
+						Versions: []string{
+							"v1",
+						},
+					}
+				case "/apis":
+					list = &unversioned.APIGroupList{
+						Groups: []unversioned.APIGroup{
+							{
+								Versions: []unversioned.GroupVersionForDiscovery{
+									{GroupVersion: "extensions/v1beta1"},
+								},
+							},
+						},
+					}
+				default:
+					t.Logf("unexpected request: %s", req.URL.Path)
+					w.WriteHeader(http.StatusNotFound)
+					return
+				}
+				output, err := json.Marshal(list)
+				if err != nil {
+					t.Errorf("unexpected encoding error: %v", err)
+					return
+				}
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				w.Write(output)
+			},
+		},
+		/*{
+			resourcesList: &stable,
+		},*/
+	}
+	for _, test := range tests {
+		server := httptest.NewServer(http.HandlerFunc(test.response))
+		defer server.Close()
+
+		client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
+		got, err := client.ServerPreferredResources()
+		if test.expectErr != nil {
+			if err == nil {
+				t.Error("unexpected non-error")
+			}
+
+			continue
+		}
+		if err != nil {
+			t.Errorf("unexpected error: %v", err)
+			continue
+		}
+		if !reflect.DeepEqual(got, test.resourcesList) {
+			t.Errorf("expected:\n%v\ngot:\n%v\n", test.resourcesList, got)
+		}
+		server.Close()
+	}
+}