Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

Merged
merged 1 commit into from
Jan 3, 2023
Merged

⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/envtest/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package envtest
import (
"context"
"path/filepath"
"strings"
"time"

. "github.com/onsi/ginkgo"
Expand Down Expand Up @@ -83,7 +84,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,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
Open in desktop
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
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Do we need to check the length of message or can we just always set resp.Result.Message? metav1.Status.Message is a string type, so leaving it "unset" is the same as setting it to an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we no longer need to check the length here. Is it worth changing or should we keep it as-is if there aren't other changes to be made?

resp.Result.Message = message
}
return resp
}
Expand Down
30 changes: 17 additions & 13 deletions pkg/webhook/admission/response_test.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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"
Expand Down Expand Up @@ -97,7 +98,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 @@ -120,7 +121,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 @@ -143,7 +144,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 @@ -199,7 +200,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