Skip to content

Commit

Permalink
Merge pull request #1539 from bhcleek/message-human-readable
Browse files Browse the repository at this point in the history 
⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jan 3, 2023
2 parents 3c4deba + 2c95a03 commit 056869c
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 35 deletions.
3 changes: 2 additions & 1 deletion pkg/envtest/webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"crypto/tls"
"path/filepath"
"strings"
"time"

. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -87,7 +88,7 @@ var _ = Describe("Test", func() {

Eventually(func() bool {
err = c.Create(context.TODO(), obj)
return apierrors.ReasonForError(err) == metav1.StatusReason("Always denied")
return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden
}, 1*time.Second).Should(BeTrue())

cancel()
Expand Down
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 @@ -138,7 +138,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 @@ -155,7 +155,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 @@ -183,7 +183,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 @@ -185,7 +185,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 @@ -206,7 +206,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 @@ -223,8 +224,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
9 changes: 5 additions & 4 deletions pkg/webhook/webhook_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"net/http"
"path/filepath"
"strconv"
"strings"
"time"

. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -99,7 +100,7 @@ var _ = Describe("Webhook", func() {

Eventually(func() bool {
err = c.Create(context.TODO(), obj)
return apierrors.ReasonForError(err) == metav1.StatusReason("Always denied")
return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden
}, 1*time.Second).Should(BeTrue())

cancel()
Expand All @@ -123,7 +124,7 @@ var _ = Describe("Webhook", func() {

Eventually(func() bool {
err = c.Create(context.TODO(), obj)
return apierrors.ReasonForError(err) == metav1.StatusReason("Always denied")
return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden
}, 1*time.Second).Should(BeTrue())

cancel()
Expand All @@ -146,7 +147,7 @@ var _ = Describe("Webhook", func() {

Eventually(func() bool {
err := c.Create(context.TODO(), obj)
return apierrors.ReasonForError(err) == metav1.StatusReason("Always denied")
return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden
}, 1*time.Second).Should(BeTrue())

cancel()
Expand Down Expand Up @@ -202,7 +203,7 @@ var _ = Describe("Webhook", func() {

Eventually(func() bool {
err = c.Create(context.TODO(), obj)
return apierrors.ReasonForError(err) == metav1.StatusReason("Always denied")
return err != nil && strings.HasSuffix(err.Error(), "Always denied") && apierrors.ReasonForError(err) == metav1.StatusReasonForbidden
}, 1*time.Second).Should(BeTrue())

cancel()
Expand Down

0 comments on commit 056869c

Please sign in to comment.