From b530cef3b1b95c2d815b107f7237d7a2aa8d59aa Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Sat, 13 Mar 2021 20:25:23 -0500 Subject: [PATCH] impersonator: encode proper API status on failure Signed-off-by: Monis Khan --- .../concierge/impersonator/impersonator.go | 27 ++++++++++++--- .../impersonator/impersonator_test.go | 33 ++++++++++++++----- .../concierge_impersonation_proxy_test.go | 21 +++++++++--- 3 files changed, 64 insertions(+), 17 deletions(-) diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index 87f2d1b4..aae77bee 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/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) +} diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 66985781..d08cbbca 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/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", - wantHTTPStatus: http.StatusUnprocessableEntity, + 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.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 { diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 6132ffaf..8d9cfa73 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/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) - // The server checks that we have a UID in the request and rejects it with a 422 Unprocessable Entity. - // The API machinery turns 422's into this error text... - require.Contains(t, err.Error(), "the server rejected our request due to an error in our request") + require.EqualError(t, err, "Internal error occurred: unimplemented functionality - unable to act as current user") + require.True(t, k8serrors.IsInternalError(err), err) + require.Equal(t, &k8serrors.StatusError{ + 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) {