impersonator: encode proper API status on failure

Signed-off-by: Monis Khan <mok@vmware.com>

internal/concierge/impersonator/impersonator.go

@@ -12,14 +12,18 @@ import (
 	"strings"
 	"time"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
+	"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
 	"k8s.io/apiserver/pkg/endpoints/request"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 	"k8s.io/apiserver/pkg/server/filters"
@@ -227,7 +231,7 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 					"url", r.URL.String(),
 					"method", r.Method,
 				)
-				http.Error(w, "invalid authorization header", http.StatusInternalServerError)
+				newInternalErrResponse(w, r, c.Serializer, "invalid authorization header")
 				return
 			}
 
@@ -237,7 +241,7 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 					"url", r.URL.String(),
 					"method", r.Method,
 				)
-				http.Error(w, "invalid impersonation", http.StatusInternalServerError)
+				newInternalErrResponse(w, r, c.Serializer, "invalid impersonation")
 				return
 			}
 
@@ -247,7 +251,7 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 					"url", r.URL.String(),
 					"method", r.Method,
 				)
-				http.Error(w, "invalid user", http.StatusInternalServerError)
+				newInternalErrResponse(w, r, c.Serializer, "invalid user")
 				return
 			}
 
@@ -257,7 +261,7 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 					"url", r.URL.String(),
 					"method", r.Method,
 				)
-				http.Error(w, "unable to act as user", http.StatusUnprocessableEntity)
+				newInternalErrResponse(w, r, c.Serializer, "unimplemented functionality - unable to act as current user")
 				return
 			}
 
@@ -309,3 +313,18 @@ func getTransportForUser(userInfo user.Info, delegate http.RoundTripper) (http.R
 	// 8. the above would be safe even if in the future Kube started supporting UIDs asserted by client certs
 	return nil, constable.Error("unexpected uid")
 }
+
+func newInternalErrResponse(w http.ResponseWriter, r *http.Request, s runtime.NegotiatedSerializer, msg string) {
+	newStatusErrResponse(w, r, s, apierrors.NewInternalError(constable.Error(msg)))
+}
+
+func newStatusErrResponse(w http.ResponseWriter, r *http.Request, s runtime.NegotiatedSerializer, err *apierrors.StatusError) {
+	requestInfo, ok := genericapirequest.RequestInfoFrom(r.Context())
+	if !ok {
+		responsewriters.InternalError(w, r, constable.Error("no RequestInfo found in the context"))
+		return
+	}
+
+	gv := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+	responsewriters.ErrorNegotiated(err, s, gv, w, r)
+}

internal/concierge/impersonator/impersonator_test.go

@@ -16,9 +16,12 @@ import (
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/features"
+	genericapiserver "k8s.io/apiserver/pkg/server"
 	genericoptions "k8s.io/apiserver/pkg/server/options"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/rest"
@@ -284,6 +287,12 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 		r, err := http.NewRequestWithContext(ctx, http.MethodGet, validURL.String(), nil)
 		require.NoError(t, err)
 		r.Header = h
+		reqInfo := &request.RequestInfo{
+			IsResourceRequest: false,
+			Path:              validURL.Path,
+			Verb:              "get",
+		}
+		r = r.WithContext(request.WithRequestInfo(ctx, reqInfo))
 		return r
 	}
 
@@ -324,44 +333,44 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 		{
 			name:           "Impersonate-User header already in request",
 			request:        newRequest(map[string][]string{"Impersonate-User": {"some-user"}}, nil),
-			wantHTTPBody:   "invalid impersonation\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: invalid impersonation","reason":"InternalError","details":{"causes":[{"message":"invalid impersonation"}]},"code":500}` + "\n",
 			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		{
 			name:           "Impersonate-Group header already in request",
 			request:        newRequest(map[string][]string{"Impersonate-Group": {"some-group"}}, nil),
-			wantHTTPBody:   "invalid impersonation\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: invalid impersonation","reason":"InternalError","details":{"causes":[{"message":"invalid impersonation"}]},"code":500}` + "\n",
 			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		{
 			name:           "Impersonate-Extra header already in request",
 			request:        newRequest(map[string][]string{"Impersonate-Extra-something": {"something"}}, nil),
-			wantHTTPBody:   "invalid impersonation\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: invalid impersonation","reason":"InternalError","details":{"causes":[{"message":"invalid impersonation"}]},"code":500}` + "\n",
 			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		{
 			name:           "Impersonate-* header already in request",
 			request:        newRequest(map[string][]string{"Impersonate-Something": {"some-newfangled-impersonate-header"}}, nil),
-			wantHTTPBody:   "invalid impersonation\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: invalid impersonation","reason":"InternalError","details":{"causes":[{"message":"invalid impersonation"}]},"code":500}` + "\n",
 			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		{
 			name:           "unexpected authorization header",
 			request:        newRequest(map[string][]string{"Authorization": {"panda"}}, nil),
-			wantHTTPBody:   "invalid authorization header\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: invalid authorization header","reason":"InternalError","details":{"causes":[{"message":"invalid authorization header"}]},"code":500}` + "\n",
 			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		{
 			name:           "missing user",
 			request:        newRequest(map[string][]string{}, nil),
-			wantHTTPBody:   "invalid user\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: invalid user","reason":"InternalError","details":{"causes":[{"message":"invalid user"}]},"code":500}` + "\n",
 			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		{
 			name:           "unexpected UID",
 			request:        newRequest(map[string][]string{}, &user.DefaultInfo{UID: "007"}),
-			wantHTTPBody:   "unable to act as user\n",
+			wantHTTPBody:   `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: unimplemented functionality - unable to act as current user","reason":"InternalError","details":{"causes":[{"message":"unimplemented functionality - unable to act as current user"}]},"code":500}` + "\n",
-			wantHTTPStatus: http.StatusUnprocessableEntity,
+			wantHTTPStatus: http.StatusInternalServerError,
 		},
 		// happy path
 		{
@@ -458,9 +467,15 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 			require.NoError(t, err)
 			require.NotNil(t, impersonatorHTTPHandlerFunc)
 
+			// this is not a valid way to get a server config, but it is good enough for a unit test
+			scheme := runtime.NewScheme()
+			metav1.AddToGroupVersion(scheme, metav1.Unversioned)
+			codecs := serializer.NewCodecFactory(scheme)
+			serverConfig := genericapiserver.NewRecommendedConfig(codecs)
+
 			w := httptest.NewRecorder()
 			requestBeforeServe := tt.request.Clone(tt.request.Context())
-			impersonatorHTTPHandlerFunc(nil).ServeHTTP(w, tt.request)
+			impersonatorHTTPHandlerFunc(&serverConfig.Config).ServeHTTP(w, tt.request)
 
 			require.Equal(t, requestBeforeServe, tt.request, "ServeHTTP() mutated the request, and it should not per http.Handler docs")
 			if tt.wantHTTPStatus != 0 {

test/integration/concierge_impersonation_proxy_test.go

@@ -475,10 +475,23 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			impersonationProxyURL, impersonationProxyCACertPEM, "").PinnipedConcierge
 		_, err = impersonationProxyServiceAccountPinnipedConciergeClient.IdentityV1alpha1().WhoAmIRequests().
 			Create(ctx, &identityv1alpha1.WhoAmIRequest{}, metav1.CreateOptions{})
-		require.Error(t, err)
+		require.EqualError(t, err, "Internal error occurred: unimplemented functionality - unable to act as current user")
-		// The server checks that we have a UID in the request and rejects it with a 422 Unprocessable Entity.
+		require.True(t, k8serrors.IsInternalError(err), err)
-		// The API machinery turns 422's into this error text...
+		require.Equal(t, &k8serrors.StatusError{
-		require.Contains(t, err.Error(), "the server rejected our request due to an error in our request")
+			ErrStatus: metav1.Status{
+				Status: metav1.StatusFailure,
+				Code:   http.StatusInternalServerError,
+				Reason: metav1.StatusReasonInternalError,
+				Details: &metav1.StatusDetails{
+					Causes: []metav1.StatusCause{
+						{
+							Message: "unimplemented functionality - unable to act as current user",
+						},
+					},
+				},
+				Message: "Internal error occurred: unimplemented functionality - unable to act as current user",
+			},
+		}, err)
 	})
 
 	t.Run("kubectl as a client", func(t *testing.T) {