Skip to content

Commit

Permalink
pkg/webhook/admission: use Result.Message instead of Result.Reason
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bhcleek
bhcleek committed May 23, 2021
1 parent 316aea4 commit 3ff152d
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 30 deletions.
2 changes: 1 addition & 1 deletion pkg/webhook/admission/http.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/webhook/admission/http_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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())
Expand Down
23 changes: 13 additions & 10 deletions pkg/webhook/admission/response.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
30 changes: 17 additions & 13 deletions pkg/webhook/admission/response_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
},
Expand All @@ -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,
},
},
},
Expand All @@ -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!",
},
},
},
Expand Down Expand Up @@ -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,
Expand All @@ -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",
},
},
},
Expand All @@ -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!",
},
},
},
Expand All @@ -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,
},
},
},
Expand Down
9 changes: 5 additions & 4 deletions pkg/webhook/admission/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()))

})

Expand All @@ -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()))

})

Expand All @@ -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()))
})

})
Expand Down

0 comments on commit 3ff152d

Please sign in to comment.