Merge pull request #115865 from seans3/discovery-client-cleanup

Updates old 403 and 404 discovery response tolerations

Kubernetes-commit: e0ca10118e0aa0076a49a6f96ea96534ffe30dcd

discovery/discovery_client.go

@@ -230,13 +230,13 @@ func (d *DiscoveryClient) downloadLegacy() (*metav1.APIGroupList, map[schema.Gro
 		Do(context.TODO()).
 		ContentType(&responseContentType).
 		Raw()
-	// Special error handling for 403 or 404 to be compatible with older v1.0 servers.
+	if err != nil {
-	// Return empty group list to be merged with /apis.
+		// Tolerate 404, since aggregated api servers can return it.
-	if err != nil && !errors.IsNotFound(err) && !errors.IsForbidden(err) {
+		if errors.IsNotFound(err) {
+			return &metav1.APIGroupList{}, nil, nil
+		} else {
 			return nil, nil, err
 		}
-	if err != nil && (errors.IsNotFound(err) || errors.IsForbidden(err)) {
-		return &metav1.APIGroupList{}, nil, nil
 	}
 
 	apiGroupList := &metav1.APIGroupList{}
@@ -283,14 +283,9 @@ func (d *DiscoveryClient) downloadAPIs() (*metav1.APIGroupList, map[schema.Group
 		Do(context.TODO()).
 		ContentType(&responseContentType).
 		Raw()
-	// Special error handling for 403 or 404 to be compatible with older v1.0 servers.
+	if err != nil {
-	// Return empty group list to be merged with /api.
-	if err != nil && !errors.IsNotFound(err) && !errors.IsForbidden(err) {
 		return nil, nil, err
 	}
-	if err != nil && (errors.IsNotFound(err) || errors.IsForbidden(err)) {
-		return &metav1.APIGroupList{}, nil, nil
-	}
 
 	apiGroupList := &metav1.APIGroupList{}
 	var resourcesByGV map[schema.GroupVersion]*metav1.APIResourceList
@@ -341,8 +336,10 @@ func (d *DiscoveryClient) ServerResourcesForGroupVersion(groupVersion string) (r
 	}
 	err = d.restClient.Get().AbsPath(url.String()).Do(context.TODO()).Into(resources)
 	if err != nil {
-		// ignore 403 or 404 error to be compatible with an v1.0 server.
+		// Tolerate core/v1 not found response by returning empty resource list;
-		if groupVersion == "v1" && (errors.IsNotFound(err) || errors.IsForbidden(err)) {
+		// this probably should not happen. But we should verify all callers are
+		// not depending on this toleration before removal.
+		if groupVersion == "v1" && errors.IsNotFound(err) {
 			return resources, nil
 		}
 		return nil, err

discovery/discovery_client_test.go

@@ -110,7 +110,6 @@ func TestGetServerGroupsWithV1Server(t *testing.T) {
 	}))
 	defer server.Close()
 	client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
-	// ServerGroups should not return an error even if server returns error at /api and /apis
 	apiGroupList, err := client.ServerGroups()
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -121,32 +120,49 @@ func TestGetServerGroupsWithV1Server(t *testing.T) {
 	}
 }
 
-func TestGetServerGroupsWithBrokenServer(t *testing.T) {
+func TestDiscoveryToleratesMissingCoreGroup(t *testing.T) {
-	for _, statusCode := range []int{http.StatusNotFound, http.StatusForbidden} {
+	// Discovery tolerates 404 from /api. Aggregated api servers can do this.
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-			w.WriteHeader(statusCode)
+		var obj interface{}
+		switch req.URL.Path {
+		case "/api":
+			w.WriteHeader(http.StatusNotFound)
+		case "/apis":
+			obj = &metav1.APIGroupList{
+				Groups: []metav1.APIGroup{
+					{
+						Name: "extensions",
+						Versions: []metav1.GroupVersionForDiscovery{
+							{GroupVersion: "extensions/v1beta1"},
+						},
+					},
+				},
+			}
+		}
+		output, err := json.Marshal(obj)
+		if err != nil {
+			t.Fatalf("unexpected encoding error: %v", err)
+			return
+		}
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		w.Write(output)
 	}))
 	defer server.Close()
 	client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
-		// ServerGroups should not return an error even if server returns Not Found or Forbidden error at all end points
+	// ServerGroups should not return an error even if server returns 404 at /api.
 	apiGroupList, err := client.ServerGroups()
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	groupVersions := metav1.ExtractGroupVersions(apiGroupList)
-		if len(groupVersions) != 0 {
+	if !reflect.DeepEqual(groupVersions, []string{"extensions/v1beta1"}) {
-			t.Errorf("expected empty list, got: %q", groupVersions)
+		t.Errorf("expected: %q, got: %q", []string{"extensions/v1beta1"}, groupVersions)
-		}
 	}
 }
 
-func TestTimeoutIsSet(t *testing.T) {
+func TestDiscoveryFailsWhenNonCoreGroupsMissing(t *testing.T) {
-	cfg := &restclient.Config{}
+	// Discovery fails when /apis returns 404.
-	setDiscoveryDefaults(cfg)
-	assert.Equal(t, defaultTimeout, cfg.Timeout)
-}
-
-func TestGetServerResourcesWithV1Server(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		var obj interface{}
 		switch req.URL.Path {
@@ -156,13 +172,12 @@ func TestGetServerResourcesWithV1Server(t *testing.T) {
 					"v1",
 				},
 			}
-		default:
+		case "/apis":
 			w.WriteHeader(http.StatusNotFound)
-			return
 		}
 		output, err := json.Marshal(obj)
 		if err != nil {
-			t.Errorf("unexpected encoding error: %v", err)
+			t.Fatalf("unexpected encoding error: %v", err)
 			return
 		}
 		w.Header().Set("Content-Type", "application/json")
@@ -171,15 +186,32 @@ func TestGetServerResourcesWithV1Server(t *testing.T) {
 	}))
 	defer server.Close()
 	client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
-	// ServerResources should not return an error even if server returns error at /api/v1.
+	_, err := client.ServerGroups()
-	_, serverResources, err := client.ServerGroupsAndResources()
+	if err == nil {
-	if err != nil {
+		t.Fatal("expected error, received none")
-		t.Errorf("unexpected error: %v", err)
 	}
-	gvs := groupVersions(serverResources)
-	if !sets.NewString(gvs...).Has("v1") {
-		t.Errorf("missing v1 in resource list: %v", serverResources)
 }
+
+func TestGetServerGroupsWithBrokenServer(t *testing.T) {
+	// 404 Not Found errors because discovery at /apis returns an error.
+	// 403 Forbidden errors because discovery at both /api and /apis returns error.
+	for _, statusCode := range []int{http.StatusNotFound, http.StatusForbidden} {
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			w.WriteHeader(statusCode)
+		}))
+		defer server.Close()
+		client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
+		_, err := client.ServerGroups()
+		if err == nil {
+			t.Fatal("expected error, received none")
+		}
+	}
+}
+
+func TestTimeoutIsSet(t *testing.T) {
+	cfg := &restclient.Config{}
+	setDiscoveryDefaults(cfg)
+	assert.Equal(t, defaultTimeout, cfg.Timeout)
 }
 
 func TestGetServerResourcesForGroupVersion(t *testing.T) {
@@ -964,17 +996,33 @@ func TestServerPreferredNamespacedResources(t *testing.T) {
 		expected map[schema.GroupVersionResource]struct{}
 	}{
 		{
+			// Combines discovery for /api and /apis.
 			response: func(w http.ResponseWriter, req *http.Request) {
 				var list interface{}
 				switch req.URL.Path {
-				case "/api/v1":
-					list = &stable
 				case "/api":
 					list = &metav1.APIVersions{
 						Versions: []string{
 							"v1",
 						},
 					}
+				case "/api/v1":
+					list = &stable
+				case "/apis":
+					list = &metav1.APIGroupList{
+						Groups: []metav1.APIGroup{
+							{
+								Name: "batch",
+								Versions: []metav1.GroupVersionForDiscovery{
+									{GroupVersion: "batch/v1", Version: "v1"},
+								},
+								PreferredVersion: metav1.GroupVersionForDiscovery{GroupVersion: "batch/v1", Version: "v1"},
+							},
+						},
+					}
+				case "/apis/batch/v1":
+					list = &batchv1
+
 				default:
 					t.Logf("unexpected request: %s", req.URL.Path)
 					w.WriteHeader(http.StatusNotFound)
@@ -992,9 +1040,12 @@ func TestServerPreferredNamespacedResources(t *testing.T) {
 			expected: map[schema.GroupVersionResource]struct{}{
 				{Group: "", Version: "v1", Resource: "pods"}:      {},
 				{Group: "", Version: "v1", Resource: "services"}:  {},
+				{Group: "batch", Version: "v1", Resource: "jobs"}: {},
 			},
 		},
 		{
+			// Only return /apis (not legacy /api); does not error. 404 for legacy
+			// core/v1 at /api is tolerated.
 			response: func(w http.ResponseWriter, req *http.Request) {
 				var list interface{}
 				switch req.URL.Path {

discovery/helper_blackbox_test.go

@@ -65,6 +65,22 @@ func TestServerSupportsVersion(t *testing.T) {
 			expectErr:       func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) },
 			statusCode:      http.StatusOK,
 		},
+		{
+			name:            "Status 403 Forbidden for core/v1 group returns error and is unsupported",
+			requiredVersion: schema.GroupVersion{Version: "v1"},
+			serverVersions:  []string{"/version1", v1.SchemeGroupVersion.String()},
+			expectErr:       func(err error) bool { return strings.Contains(err.Error(), "unknown") },
+			statusCode:      http.StatusForbidden,
+		},
+		{
+			name:            "Status 404 Not Found for core/v1 group returns empty and is unsupported",
+			requiredVersion: schema.GroupVersion{Version: "v1"},
+			serverVersions:  []string{"/version1", v1.SchemeGroupVersion.String()},
+			expectErr: func(err error) bool {
+				return strings.Contains(err.Error(), "server could not find the requested resource")
+			},
+			statusCode: http.StatusNotFound,
+		},
 		{
 			name:           "connection refused error",
 			serverVersions: []string{"version1"},
@@ -72,11 +88,6 @@ func TestServerSupportsVersion(t *testing.T) {
 			expectErr:      func(err error) bool { return strings.Contains(err.Error(), "connection refused") },
 			statusCode:     http.StatusOK,
 		},
-		{
-			name:            "discovery fails due to 404 Not Found errors and thus serverVersions is empty, use requested GroupVersion",
-			requiredVersion: schema.GroupVersion{Version: "version1"},
-			statusCode:      http.StatusNotFound,
-		},
 	}
 
 	for _, test := range tests {

go.mod

@@ -24,8 +24,8 @@ require (
 	golang.org/x/term v0.5.0
 	golang.org/x/time v0.0.0-20220210224613-90d013bbcef8
 	google.golang.org/protobuf v1.28.1
-	k8s.io/api v0.0.0-20230302011010-42a6c324deb9
+	k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4
-	k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27
+	k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d
 	k8s.io/klog/v2 v2.90.1
 	k8s.io/kube-openapi v0.0.0-20230123231816-1cb3ae25d79a
 	k8s.io/utils v0.0.0-20230209194617-a36077c30491
@@ -59,6 +59,6 @@ require (
 )
 
 replace (
-	k8s.io/api => k8s.io/api v0.0.0-20230302011010-42a6c324deb9
+	k8s.io/api => k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4
-	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27
+	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d
 )

go.sum

@@ -473,10 +473,10 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
-k8s.io/api v0.0.0-20230302011010-42a6c324deb9 h1:cQzhf7hby/usJMsgKtdbYSTuIc+TzGqLDfddQHXIO30=
+k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4 h1:qm5Xz7wiOrtVHPKAZsBByEd6k2WST6Ts+e4RfpgVuvQ=
-k8s.io/api v0.0.0-20230302011010-42a6c324deb9/go.mod h1:xgkXVMREg0t0RxMFgpHj3ccDnk+xwBfDcIZC8xS/NrI=
+k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4/go.mod h1:cTD04/XhoraqP0GpFXtefYJYXBw6coqVSibz5Rzivkw=
-k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27 h1:w5AwK+Z2f7+RDs+AJMYmDz34cXM7KMUgK+VbDZ/IBI8=
+k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d h1:mridg1Zm6thnb5oTe+rOGnEUbPnjys9YHBFxlOf+GeA=
-k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27/go.mod h1:TO4higCGNMwebVSdb1XPJdXMU4kk+nmMY/cTMVCGa6M=
+k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d/go.mod h1:TO4higCGNMwebVSdb1XPJdXMU4kk+nmMY/cTMVCGa6M=
 k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw=
 k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
 k8s.io/kube-openapi v0.0.0-20230123231816-1cb3ae25d79a h1:s6zvHjyDQX1NtVT88pvw2tddqhqY0Bz0Gbnn+yctsFU=