diff --git a/discovery/discovery_client.go b/discovery/discovery_client.go index 43906190..52390bf9 100644 --- a/discovery/discovery_client.go +++ b/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. - // Return empty group list to be merged with /apis. - 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 + if err != nil { + // Tolerate 404, since aggregated api servers can return it. + if errors.IsNotFound(err) { + return &metav1.APIGroupList{}, nil, nil + } else { + return nil, nil, err + } } 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. - // Return empty group list to be merged with /api. - if err != nil && !errors.IsNotFound(err) && !errors.IsForbidden(err) { + if err != nil { 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. - 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 4746fac5..fbf1ddff 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() +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"}, + }, + }, + }, + } + } + output, err := json.Marshal(obj) if err != nil { - t.Fatalf("unexpected error: %v", err) - } - groupVersions := metav1.ExtractGroupVersions(apiGroupList) - if len(groupVersions) != 0 { - t.Errorf("expected empty list, got: %q", groupVersions) + 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 d9481ec2..1273d5fb 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 { diff --git a/go.mod b/go.mod index 66773791..0ca78352 100644 --- a/go.mod +++ b/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/apimachinery v0.0.0-20230302010315-590a2612ff27 + k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4 + 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/apimachinery => k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27 + k8s.io/api => k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4 + k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d ) diff --git a/go.sum b/go.sum index b8526b77..3129595c 100644 --- a/go.sum +++ b/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-20230302011010-42a6c324deb9/go.mod h1:xgkXVMREg0t0RxMFgpHj3ccDnk+xwBfDcIZC8xS/NrI= -k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27 h1:w5AwK+Z2f7+RDs+AJMYmDz34cXM7KMUgK+VbDZ/IBI8= -k8s.io/apimachinery v0.0.0-20230302010315-590a2612ff27/go.mod h1:TO4higCGNMwebVSdb1XPJdXMU4kk+nmMY/cTMVCGa6M= +k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4 h1:qm5Xz7wiOrtVHPKAZsBByEd6k2WST6Ts+e4RfpgVuvQ= +k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4/go.mod h1:cTD04/XhoraqP0GpFXtefYJYXBw6coqVSibz5Rzivkw= +k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d h1:mridg1Zm6thnb5oTe+rOGnEUbPnjys9YHBFxlOf+GeA= +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=