Skip to content

Commit

Permalink
Removes old discovery hack ignoring 403 and 404
Browse files Browse the repository at this point in the history 
Kubernetes-commit: 6968f56567c90ea4329448a9185445bb1f114295
  • Loading branch information
@seans3 @k8s-publishing-bot
seans3 authored and k8s-publishing-bot committed Feb 17, 2023
1 parent f39ba12 commit fc13749
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 46 deletions.
9 changes: 4 additions & 5 deletions discovery/discovery_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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
Expand Down
123 changes: 87 additions & 36 deletions discovery/discovery_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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")
Expand All @@ -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",
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
21 changes: 16 additions & 5 deletions discovery/helper_blackbox_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,29 @@ 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"},
sendErr: errors.New("connection refused"),
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 {
Expand Down

0 comments on commit fc13749

Please sign in to comment.