From 32afe3f11114bd9eedf01544bc08482bb8d04002 Mon Sep 17 00:00:00 2001 From: Rafael Franzke Date: Tue, 25 May 2021 09:15:46 +0200 Subject: [PATCH] Adapt response writing in webhook handlers --- pkg/webhook/admission/http.go | 38 ++++++++-------------- pkg/webhook/admission/http_test.go | 19 ++++++----- pkg/webhook/authentication/http.go | 42 +++++++++++-------------- pkg/webhook/authentication/http_test.go | 23 +++++++++----- pkg/webhook/authorization/http.go | 42 ++++++++++++------------- pkg/webhook/authorization/http_test.go | 16 ++++++---- 6 files changed, 89 insertions(+), 91 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index c332833cc7..db9ad67fb8 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -55,7 +55,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = errors.New("request body is empty") wh.log.Error(err, "bad request") reviewResponse = Errored(http.StatusBadRequest, err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -63,7 +63,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if body, err = ioutil.ReadAll(r.Body); err != nil { wh.log.Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(http.StatusBadRequest, err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -73,7 +73,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = fmt.Errorf("contentType=%s, expected application/json", contentType) wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = Errored(http.StatusBadRequest, err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -92,50 +92,38 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err != nil { wh.log.Error(err, "unable to decode the request") reviewResponse = Errored(http.StatusBadRequest, err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, actualAdmRevGVK, reviewResponse) return } wh.log.V(1).Info("received request", "UID", req.UID, "kind", req.Kind, "resource", req.Resource) reviewResponse = wh.Handle(ctx, req) - wh.writeResponseTyped(w, reviewResponse, actualAdmRevGVK) + wh.writeResponse(w, actualAdmRevGVK, reviewResponse) } -// writeResponse writes response to w generically, i.e. without encoding GVK information. -func (wh *Webhook) writeResponse(w io.Writer, response Response) { - wh.writeAdmissionResponse(w, v1.AdmissionReview{Response: &response.AdmissionResponse}) -} - -// writeResponseTyped writes response to w with GVK set to admRevGVK, which is necessary -// if multiple AdmissionReview versions are permitted by the webhook. -func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, admRevGVK *schema.GroupVersionKind) { +func (wh *Webhook) writeResponse(w io.Writer, gvk *schema.GroupVersionKind, response Response) { ar := v1.AdmissionReview{ Response: &response.AdmissionResponse, } // Default to a v1 AdmissionReview, otherwise the API server may not recognize the request // if multiple AdmissionReview versions are permitted by the webhook config. // TODO(estroz): this should be configurable since older API servers won't know about v1. - if admRevGVK == nil || *admRevGVK == (schema.GroupVersionKind{}) { + if gvk == nil || *gvk == (schema.GroupVersionKind{}) { ar.SetGroupVersionKind(v1.SchemeGroupVersion.WithKind("AdmissionReview")) } else { - ar.SetGroupVersionKind(*admRevGVK) + ar.SetGroupVersionKind(*gvk) } - wh.writeAdmissionResponse(w, ar) -} -// writeAdmissionResponse writes ar to w. -func (wh *Webhook) writeAdmissionResponse(w io.Writer, ar v1.AdmissionReview) { - err := json.NewEncoder(w).Encode(ar) - if err != nil { + if err := json.NewEncoder(w).Encode(ar); err != nil { wh.log.Error(err, "unable to encode the response") - wh.writeResponse(w, Errored(http.StatusInternalServerError, err)) + wh.writeResponse(w, gvk, Errored(http.StatusInternalServerError, err)) } - log := wh.log.V(1) + logger := wh.log.V(1) if result := ar.Response.Result; result != nil { - log = log.WithValues("code", result.Code, "reason", result.Reason) + logger = logger.WithValues("code", result.Code, "reason", result.Reason) } - log.V(1).Info("wrote response", "UID", ar.Response.UID, "allowed", ar.Response.Allowed) + logger.V(1).Info("wrote response", "UID", ar.Response.UID, "allowed", ar.Response.Allowed) } // unversionedAdmissionReview is used to decode both v1 and v1beta1 AdmissionReview types. diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index 7dd2d5bcfc..f14e0c9072 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -56,8 +56,9 @@ var _ = Describe("Admission Webhooks", func() { It("should return bad-request when given an empty body", func() { req := &http.Request{Body: nil} - expected := `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}} -` + expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -68,9 +69,10 @@ var _ = Describe("Admission Webhooks", func() { Body: nopCloser{Reader: bytes.NewBuffer(nil)}, } - expected := - `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expected application/json","code":400}}} -` + expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":false,"status":{"metadata":{},`+ + `"message":"contentType=application/foo, expected application/json","code":400}}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -81,9 +83,10 @@ var _ = Describe("Admission Webhooks", func() { Body: nopCloser{Reader: bytes.NewBufferString("{")}, } - expected := - `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"couldn't get version/kind; json parse error: unexpected end of JSON input","code":400}}} -` + expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":false,"status":{"metadata":{},`+ + `"message":"couldn't get version/kind; json parse error: unexpected end of JSON input","code":400}}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) diff --git a/pkg/webhook/authentication/http.go b/pkg/webhook/authentication/http.go index 77ad520190..21db9d2e43 100644 --- a/pkg/webhook/authentication/http.go +++ b/pkg/webhook/authentication/http.go @@ -55,7 +55,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = errors.New("request body is empty") wh.log.Error(err, "bad request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -63,7 +63,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if body, err = ioutil.ReadAll(r.Body); err != nil { wh.log.Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -73,7 +73,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = fmt.Errorf("contentType=%s, expected application/json", contentType) wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -93,7 +93,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err != nil { wh.log.Error(err, "unable to decode the request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, actualTokRevGVK, reviewResponse) return } wh.log.V(1).Info("received request", "UID", req.UID, "kind", req.Kind) @@ -102,42 +102,38 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = errors.New("token is empty") wh.log.Error(err, "bad request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, actualTokRevGVK, reviewResponse) return } reviewResponse = wh.Handle(ctx, req) - wh.writeResponseTyped(w, reviewResponse, actualTokRevGVK) + wh.writeResponse(w, actualTokRevGVK, reviewResponse) } -// writeResponse writes response to w generically, i.e. without encoding GVK information. -func (wh *Webhook) writeResponse(w io.Writer, response Response) { - wh.writeTokenResponse(w, response.TokenReview) -} - -// writeResponseTyped writes response to w with GVK set to tokRevGVK, which is necessary -// if multiple TokenReview versions are permitted by the webhook. -func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, tokRevGVK *schema.GroupVersionKind) { +func (wh *Webhook) writeResponse(w io.Writer, gvk *schema.GroupVersionKind, response Response) { ar := response.TokenReview // Default to a v1 TokenReview, otherwise the API server may not recognize the request // if multiple TokenReview versions are permitted by the webhook config. - if tokRevGVK == nil || *tokRevGVK == (schema.GroupVersionKind{}) { + if gvk == nil || *gvk == (schema.GroupVersionKind{}) { ar.SetGroupVersionKind(authenticationv1.SchemeGroupVersion.WithKind("TokenReview")) } else { - ar.SetGroupVersionKind(*tokRevGVK) + ar.SetGroupVersionKind(*gvk) } - wh.writeTokenResponse(w, ar) -} -// writeTokenResponse writes ar to w. -func (wh *Webhook) writeTokenResponse(w io.Writer, ar authenticationv1.TokenReview) { if err := json.NewEncoder(w).Encode(ar); err != nil { wh.log.Error(err, "unable to encode the response") - wh.writeResponse(w, Errored(err)) + wh.writeResponse(w, gvk, Errored(err)) } - log.V(1).Info("wrote response", "UID", ar.UID, "authenticated", ar.Status.Authenticated) - return + + wh.log. + V(1). + WithValues( + "uid", ar.UID, + "authenticated", ar.Status.Authenticated, + "error", ar.Status.Error, + ). + Info("wrote response") } // unversionedTokenReview is used to decode both v1 and v1beta1 TokenReview types. diff --git a/pkg/webhook/authentication/http_test.go b/pkg/webhook/authentication/http_test.go index 3007ddda78..34b5e9cc08 100644 --- a/pkg/webhook/authentication/http_test.go +++ b/pkg/webhook/authentication/http_test.go @@ -55,8 +55,10 @@ var _ = Describe("Authentication Webhooks", func() { It("should return bad-request when given an empty body", func() { req := &http.Request{Body: nil} - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"request body is empty"}} -` + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},`+ + `"error":"request body is empty"}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -68,8 +70,10 @@ var _ = Describe("Authentication Webhooks", func() { Body: nopCloser{Reader: bytes.NewBuffer(nil)}, } - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"contentType=application/foo, expected application/json"}} -` + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},`+ + `"error":"contentType=application/foo, expected application/json"}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -81,8 +85,10 @@ var _ = Describe("Authentication Webhooks", func() { Body: nopCloser{Reader: bytes.NewBufferString("{")}, } - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"couldn't get version/kind; json parse error: unexpected end of JSON input"}} -` + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},`+ + `"error":"couldn't get version/kind; json parse error: unexpected end of JSON input"}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -94,8 +100,9 @@ var _ = Describe("Authentication Webhooks", func() { Body: nopCloser{Reader: bytes.NewBufferString(`{"spec":{"token":""}}`)}, } - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"token is empty"}} -` + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"token is empty"}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) diff --git a/pkg/webhook/authorization/http.go b/pkg/webhook/authorization/http.go index 12cf0e4f1f..be2a92940f 100644 --- a/pkg/webhook/authorization/http.go +++ b/pkg/webhook/authorization/http.go @@ -55,7 +55,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = errors.New("request body is empty") wh.log.Error(err, "bad request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -63,7 +63,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if body, err = ioutil.ReadAll(r.Body); err != nil { wh.log.Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -73,7 +73,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = fmt.Errorf("contentType=%s, expected application/json", contentType) wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, nil, reviewResponse) return } @@ -82,42 +82,42 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err != nil { wh.log.Error(err, "unable to decode the request") reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, actualTokRevGVK, reviewResponse) return } wh.log.V(1).Info("received request", "UID", sar.UID, "kind", sar.Kind) reviewResponse = wh.Handle(ctx, Request{sar.SubjectAccessReview}) - wh.writeResponseTyped(w, reviewResponse, actualTokRevGVK) + wh.writeResponse(w, actualTokRevGVK, reviewResponse) } -// writeResponse writes response to w generically, i.e. without encoding GVK information. -func (wh *Webhook) writeResponse(w io.Writer, response Response) { - wh.writeSubjectAccessReviewResponse(w, response.SubjectAccessReview) -} - -// writeResponseTyped writes response to w with GVK set to subjRevGVK, which is necessary -// if multiple SubjectAccessReview versions are permitted by the webhook. -func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, subjRevGVK *schema.GroupVersionKind) { +func (wh *Webhook) writeResponse(w io.Writer, gvk *schema.GroupVersionKind, response Response) { ar := response.SubjectAccessReview // Default to a v1 SubjectAccessReview, otherwise the API server may not recognize the request // if multiple SubjectAccessReview versions are permitted by the webhook config. - if subjRevGVK == nil || *subjRevGVK == (schema.GroupVersionKind{}) { + if gvk == nil || *gvk == (schema.GroupVersionKind{}) { ar.SetGroupVersionKind(authorizationv1.SchemeGroupVersion.WithKind("SubjectAccessReview")) } else { - ar.SetGroupVersionKind(*subjRevGVK) + ar.SetGroupVersionKind(*gvk) } - wh.writeSubjectAccessReviewResponse(w, ar) -} -// writeSubjectAccessReviewResponse writes ar to w. -func (wh *Webhook) writeSubjectAccessReviewResponse(w io.Writer, ar authorizationv1.SubjectAccessReview) { if err := json.NewEncoder(w).Encode(ar); err != nil { wh.log.Error(err, "unable to encode the response") - wh.writeResponse(w, Errored(err)) + wh.writeResponse(w, gvk, Errored(err)) + return } - log.V(1).Info("wrote response", "UID", ar.UID, "authorized", ar.Status.Allowed) + + wh.log. + V(1). + WithValues( + "uid", ar.UID, + "allowed", ar.Status.Allowed, + "denied", ar.Status.Denied, + "reason", ar.Status.Reason, + "error", ar.Status.EvaluationError, + ). + Info("wrote response") } func (wh *Webhook) decodeRequestBody(body []byte) (unversionedSubjectAccessReview, *schema.GroupVersionKind, error) { diff --git a/pkg/webhook/authorization/http_test.go b/pkg/webhook/authorization/http_test.go index 914c361edd..21aa71e8d0 100644 --- a/pkg/webhook/authorization/http_test.go +++ b/pkg/webhook/authorization/http_test.go @@ -55,8 +55,9 @@ var _ = Describe("Authentication Webhooks", func() { It("should return bad-request when given an empty body", func() { req := &http.Request{Body: nil} - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"evaluationError":"request body is empty"}} -` + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"evaluationError":"request body is empty"}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -68,8 +69,10 @@ var _ = Describe("Authentication Webhooks", func() { Body: nopCloser{Reader: bytes.NewBuffer(nil)}, } - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"evaluationError":"contentType=application/foo, expected application/json"}} -` + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,`+ + `"evaluationError":"contentType=application/foo, expected application/json"}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) }) @@ -81,9 +84,10 @@ var _ = Describe("Authentication Webhooks", func() { Body: nopCloser{Reader: bytes.NewBufferString("{")}, } - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,` + + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,`+ `"evaluationError":"couldn't get version/kind; json parse error: unexpected end of JSON input"}} -` +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) })