From 3ff152dcfa04bd56350a246fb56e27d2b52471c7 Mon Sep 17 00:00:00 2001 From: Billie Cleek Date: Sun, 23 May 2021 11:27:43 -0700 Subject: [PATCH] pkg/webhook/admission: use Result.Message instead of Result.Reason Use Result.Message instead of Result.Reason for the user supplied portion. Reason is intended to be machine readable while Message is intended to be human readable. While each is documented as being informational only, the Is* family of functions in k8s.io/apimachinery/pkg/api/errors rely on the StatusReason. This change allows controllers to rely on "k8s.io/apimachinery/pkg/api/errors".IsForbidden to deal with errors consistently regardless of whether the operation failed due to an admission webhook implemented with controller-runtime, a standard kubernetes failure (e.g related to RBAC), or another controller not implemented with controller-runtime. See kubernetes/kubernetes#101926 for more information --- pkg/webhook/admission/http.go | 2 +- pkg/webhook/admission/http_test.go | 4 ++-- pkg/webhook/admission/response.go | 23 ++++++++++--------- pkg/webhook/admission/response_test.go | 30 ++++++++++++++----------- pkg/webhook/admission/validator_test.go | 9 ++++---- 5 files changed, 38 insertions(+), 30 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 052f803161..e1cc03f3e1 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -133,7 +133,7 @@ func (wh *Webhook) writeAdmissionResponse(w io.Writer, ar v1.AdmissionReview) { res := ar.Response if log := wh.log; log.V(1).Enabled() { if res.Result != nil { - log = log.WithValues("code", res.Result.Code, "reason", res.Result.Reason) + log = log.WithValues("code", res.Result.Code, "reason", res.Result.Reason, "message", res.Result.Message) } log.V(1).Info("wrote response", "UID", res.UID, "allowed", res.Allowed) } diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index 7dd2d5bcfc..c13987b7b2 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -154,7 +154,7 @@ var _ = Describe("Admission Webhooks", func() { log: logf.RuntimeLog.WithName("webhook"), } - expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"reason":%q,"code":200}}} + expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"message":%q,"code":200}}} `, gvkJSONv1, value) ctx, cancel := context.WithCancel(context.WithValue(context.Background(), key, value)) @@ -182,7 +182,7 @@ var _ = Describe("Admission Webhooks", func() { log: logf.RuntimeLog.WithName("webhook"), } - expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"reason":%q,"code":200}}} + expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"message":%q,"code":200}}} `, gvkJSONv1, "application/json") ctx, cancel := context.WithCancel(context.Background()) diff --git a/pkg/webhook/admission/response.go b/pkg/webhook/admission/response.go index 24ff1dee3c..ec1c88c989 100644 --- a/pkg/webhook/admission/response.go +++ b/pkg/webhook/admission/response.go @@ -26,21 +26,21 @@ import ( // Allowed constructs a response indicating that the given operation // is allowed (without any patches). -func Allowed(reason string) Response { - return ValidationResponse(true, reason) +func Allowed(message string) Response { + return ValidationResponse(true, message) } // Denied constructs a response indicating that the given operation // is not allowed. -func Denied(reason string) Response { - return ValidationResponse(false, reason) +func Denied(message string) Response { + return ValidationResponse(false, message) } // Patched constructs a response indicating that the given operation is // allowed, and that the target object should be modified by the given // JSONPatch operations. -func Patched(reason string, patches ...jsonpatch.JsonPatchOperation) Response { - resp := Allowed(reason) +func Patched(message string, patches ...jsonpatch.JsonPatchOperation) Response { + resp := Allowed(message) resp.Patches = patches return resp @@ -60,21 +60,24 @@ func Errored(code int32, err error) Response { } // ValidationResponse returns a response for admitting a request. -func ValidationResponse(allowed bool, reason string) Response { +func ValidationResponse(allowed bool, message string) Response { code := http.StatusForbidden + reason := metav1.StatusReasonForbidden if allowed { code = http.StatusOK + reason = "" } resp := Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: allowed, Result: &metav1.Status{ - Code: int32(code), + Code: int32(code), + Reason: reason, }, }, } - if len(reason) > 0 { - resp.Result.Reason = metav1.StatusReason(reason) + if len(message) > 0 { + resp.Result.Message = message } return resp } diff --git a/pkg/webhook/admission/response_test.go b/pkg/webhook/admission/response_test.go index e96b0e6ca7..91951edb15 100644 --- a/pkg/webhook/admission/response_test.go +++ b/pkg/webhook/admission/response_test.go @@ -49,8 +49,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ - Code: http.StatusOK, - Reason: "acceptable", + Code: http.StatusOK, + Message: "acceptable", }, }, }, @@ -65,7 +65,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ - Code: http.StatusForbidden, + Code: http.StatusForbidden, + Reason: metav1.StatusReasonForbidden, }, }, }, @@ -78,8 +79,9 @@ var _ = Describe("Admission Webhook Response Helpers", func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ - Code: http.StatusForbidden, - Reason: "UNACCEPTABLE!", + Code: http.StatusForbidden, + Reason: metav1.StatusReasonForbidden, + Message: "UNACCEPTABLE!", }, }, }, @@ -118,8 +120,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ - Code: http.StatusOK, - Reason: "some changes", + Code: http.StatusOK, + Message: "some changes", }, }, Patches: ops, @@ -146,15 +148,15 @@ var _ = Describe("Admission Webhook Response Helpers", func() { }) Describe("ValidationResponse", func() { - It("should populate a status with a reason when a reason is given", func() { + It("should populate a status with a message when a message is given", func() { By("checking that a message is populated for 'allowed' responses") Expect(ValidationResponse(true, "acceptable")).To(Equal( Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ - Code: http.StatusOK, - Reason: "acceptable", + Code: http.StatusOK, + Message: "acceptable", }, }, }, @@ -166,8 +168,9 @@ var _ = Describe("Admission Webhook Response Helpers", func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ - Code: http.StatusForbidden, - Reason: "UNACCEPTABLE!", + Code: http.StatusForbidden, + Reason: metav1.StatusReasonForbidden, + Message: "UNACCEPTABLE!", }, }, }, @@ -193,7 +196,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ - Code: http.StatusForbidden, + Code: http.StatusForbidden, + Reason: metav1.StatusReasonForbidden, }, }, }, diff --git a/pkg/webhook/admission/validator_test.go b/pkg/webhook/admission/validator_test.go index 8ee4f0b898..caf2feb110 100644 --- a/pkg/webhook/admission/validator_test.go +++ b/pkg/webhook/admission/validator_test.go @@ -169,7 +169,7 @@ var _ = Describe("validatingHandler", func() { }) Expect(response.Allowed).Should(BeFalse()) Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(string(response.Result.Reason)).Should(Equal(expectedError.Error())) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) }) @@ -190,7 +190,8 @@ var _ = Describe("validatingHandler", func() { }) Expect(response.Allowed).Should(BeFalse()) Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(string(response.Result.Reason)).Should(Equal(expectedError.Error())) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) }) @@ -207,8 +208,8 @@ var _ = Describe("validatingHandler", func() { }) Expect(response.Allowed).Should(BeFalse()) Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(string(response.Result.Reason)).Should(Equal(expectedError.Error())) - + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) }) })