Merge pull request #27644 from enj/dev/enj/issues/9307

Automatic merge from submit-queue

Use preferred group version when discovery fails due to 403

```
kubectl get pods --as bob
```
Returns:
```
error: failed to negotiate an api version; server supports: map[], client supports: map[autoscaling/v1:{} rbac.authorization.k8s.io/v1alpha1:{} federation/v1alpha1:{} batch/v1:{} v1:{} authentication.k8s.io/v1beta1:{} apps/v1alpha1:{} componentconfig/v1alpha1:{} authorization.k8s.io/v1beta1:{} batch/v2alpha1:{} extensions/v1beta1:{} policy/v1alpha1:{}]
```
It should return:
```
User "deads" cannot "impersonate" "users" with name "bob" in project ""
```

`serverVersions` is empty when discovery fails, thus we fallback to the `preferredGV`.

See openshift/origin#9307 and [openshift/origin/pull/9389](https://github.com/openshift/origin/pull/9389) for further details.

pkg/client/typed/discovery/client_test.go

@@ -95,6 +95,25 @@ 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)
+		}
+		groupVersions := unversioned.ExtractGroupVersions(apiGroupList)
+		if len(groupVersions) != 0 {
+			t.Errorf("expected empty list, got: %q", groupVersions)
+		}
+	}
+}
+
 func TestGetServerResourcesWithV1Server(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		var obj interface{}

pkg/client/unversioned/helper.go

@@ -193,6 +193,11 @@ func NegotiateVersion(client *Client, c *restclient.Config, requestedGV *unversi
 			return nil, fmt.Errorf("client does not support API version %q; client supported API versions: %v", preferredGV, clientVersions)
 
 		}
+		// If the server supports no versions, then we should just use the preferredGV
+		// This can happen because discovery fails due to 403 Forbidden errors
+		if len(serverVersions) == 0 {
+			return preferredGV, nil
+		}
 		if serverVersions.Has(preferredGV.String()) {
 			return preferredGV, nil
 		}

pkg/client/unversioned/helper_blackbox_test.go

@@ -52,6 +52,7 @@ func TestNegotiateVersion(t *testing.T) {
 		config          *restclient.Config
 		expectErr       func(err error) bool
 		sendErr         error
+		statusCode      int
 	}{
 		{
 			name:            "server supports client default",
@@ -60,6 +61,7 @@ func TestNegotiateVersion(t *testing.T) {
 			serverVersions:  []string{"version1", testapi.Default.GroupVersion().String()},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
 			expectedVersion: &uapi.GroupVersion{Version: "version1"},
+			statusCode:      http.StatusOK,
 		},
 		{
 			name:            "server falls back to client supported",
@@ -68,6 +70,7 @@ func TestNegotiateVersion(t *testing.T) {
 			serverVersions:  []string{"version1"},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
 			expectedVersion: &uapi.GroupVersion{Version: "version1"},
+			statusCode:      http.StatusOK,
 		},
 		{
 			name:            "explicit version supported",
@@ -75,6 +78,7 @@ func TestNegotiateVersion(t *testing.T) {
 			serverVersions:  []string{"/version1", testapi.Default.GroupVersion().String()},
 			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
 			expectedVersion: testapi.Default.GroupVersion(),
+			statusCode:      http.StatusOK,
 		},
 		{
 			name:           "explicit version not supported",
@@ -82,6 +86,7 @@ func TestNegotiateVersion(t *testing.T) {
 			serverVersions: []string{"version1"},
 			clientVersions: []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
 			expectErr:      func(err error) bool { return strings.Contains(err.Error(), `server does not support API version "v1"`) },
+			statusCode:     http.StatusOK,
 		},
 		{
 			name:           "connection refused error",
@@ -90,6 +95,29 @@ func TestNegotiateVersion(t *testing.T) {
 			clientVersions: []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
 			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 403 Forbidden errors and thus serverVersions is empty, use default GroupVersion",
+			config:          &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: testapi.Default.GroupVersion()}},
+			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
+			expectedVersion: testapi.Default.GroupVersion(),
+			statusCode:      http.StatusForbidden,
+		},
+		{
+			name:            "discovery fails due to 404 Not Found errors and thus serverVersions is empty, use requested GroupVersion",
+			version:         &uapi.GroupVersion{Version: "version1"},
+			config:          &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: testapi.Default.GroupVersion()}},
+			clientVersions:  []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
+			expectedVersion: &uapi.GroupVersion{Version: "version1"},
+			statusCode:      http.StatusNotFound,
+		},
+		{
+			name:           "discovery fails due to 403 Forbidden errors and thus serverVersions is empty, no fallback GroupVersion",
+			config:         &restclient.Config{},
+			clientVersions: []uapi.GroupVersion{{Version: "version1"}, *testapi.Default.GroupVersion()},
+			expectErr:      func(err error) bool { return strings.Contains(err.Error(), "failed to negotiate an api version;") },
+			statusCode:     http.StatusForbidden,
 		},
 	}
 	codec := testapi.Default.Codec()
@@ -98,7 +126,7 @@ func TestNegotiateVersion(t *testing.T) {
 		fakeClient := &fake.RESTClient{
 			Codec: codec,
 			Resp: &http.Response{
-				StatusCode: 200,
+				StatusCode: test.statusCode,
 				Body:       objBody(&uapi.APIVersions{Versions: test.serverVersions}),
 			},
 			Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
@@ -107,7 +135,7 @@ func TestNegotiateVersion(t *testing.T) {
 				}
 				header := http.Header{}
 				header.Set("Content-Type", runtime.ContentTypeJSON)
-				return &http.Response{StatusCode: 200, Header: header, Body: objBody(&uapi.APIVersions{Versions: test.serverVersions})}, nil
+				return &http.Response{StatusCode: test.statusCode, Header: header, Body: objBody(&uapi.APIVersions{Versions: test.serverVersions})}, nil
 			}),
 		}
 		c := unversioned.NewOrDie(test.config)