diff --git a/discovery/discovery_client.go b/discovery/discovery_client.go index 1eeb5f6795..641568008b 100644 --- a/discovery/discovery_client.go +++ b/discovery/discovery_client.go @@ -308,9 +308,6 @@ func (d *DiscoveryClient) downloadAPIs() ( if err != nil { return nil, nil, nil, err } - if err != nil && (errors.IsNotFound(err) || errors.IsForbidden(err)) { - return &metav1.APIGroupList{}, nil, nil - } apiGroupList := &metav1.APIGroupList{} failedGVs := map[schema.GroupVersion]error{} @@ -362,8 +359,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. - if groupVersion == "v1" && (errors.IsNotFound(err) || errors.IsForbidden(err)) { + // Tolerate core/v1 not found response by returning empty resource list; + // 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 diff --git a/discovery/discovery_client_test.go b/discovery/discovery_client_test.go index 594a1324c8..f41a051bca 100644 --- a/discovery/discovery_client_test.go +++ b/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) { - 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}) - // ServerGroups should not return an error even if server returns Not Found or Forbidden error at all end points - apiGroupList, err := client.ServerGroups() - if err != nil { - t.Fatalf("unexpected error: %v", err) +func TestDiscoveryToleratesMissingCoreGroup(t *testing.T) { + // Discovery tolerates 404 from /api. Aggregated api servers can do this. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + 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"}, + }, + }, + }, + } } - groupVersions := metav1.ExtractGroupVersions(apiGroupList) - if len(groupVersions) != 0 { - t.Errorf("expected empty list, got: %q", groupVersions) + 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 404 at /api. + apiGroupList, err := client.ServerGroups() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + groupVersions := metav1.ExtractGroupVersions(apiGroupList) + if !reflect.DeepEqual(groupVersions, []string{"extensions/v1beta1"}) { + t.Errorf("expected: %q, got: %q", []string{"extensions/v1beta1"}, groupVersions) } } -func TestTimeoutIsSet(t *testing.T) { - cfg := &restclient.Config{} - setDiscoveryDefaults(cfg) - assert.Equal(t, defaultTimeout, cfg.Timeout) -} - -func TestGetServerResourcesWithV1Server(t *testing.T) { +func TestDiscoveryFailsWhenNonCoreGroupsMissing(t *testing.T) { + // Discovery fails when /apis returns 404. 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,17 +186,34 @@ 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. - _, serverResources, err := client.ServerGroupsAndResources() - if err != nil { - t.Errorf("unexpected error: %v", err) + _, err := client.ServerGroups() + if err == nil { + t.Fatal("expected error, received none") } - 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) { stable := metav1.APIResourceList{ GroupVersion: "v1", @@ -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) @@ -990,11 +1038,14 @@ func TestServerPreferredNamespacedResources(t *testing.T) { w.Write(output) }, expected: map[schema.GroupVersionResource]struct{}{ - {Group: "", Version: "v1", Resource: "pods"}: {}, - {Group: "", Version: "v1", Resource: "services"}: {}, + {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 { diff --git a/discovery/helper_blackbox_test.go b/discovery/helper_blackbox_test.go index d9481ec224..1273d5fbbe 100644 --- a/discovery/helper_blackbox_test.go +++ b/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 {