Skip to content

Commit 2c95a03

Browse files
bhcleekbhcleek
committed
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
1 parent 8b55f85 commit 2c95a03

File tree

7 files changed

+45
-35
lines changed

7 files changed

+45
-35
lines changed

‎pkg/envtest/webhook_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package envtest
1919
import (
2020
"context"
2121
"path/filepath"
22+
"strings"
2223
"time"
2324

2425
. "github.com/onsi/ginkgo"
@@ -83,7 +84,7 @@ var _ = Describe("Test", func() {
8384

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

8990
cancel()

‎pkg/webhook/admission/http.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func (wh *Webhook) writeAdmissionResponse(w io.Writer, ar v1.AdmissionReview) {
131131
res := ar.Response
132132
if log := wh.log; log.V(1).Enabled() {
133133
if res.Result != nil {
134-
log = log.WithValues("code", res.Result.Code, "reason", res.Result.Reason)
134+
log = log.WithValues("code", res.Result.Code, "reason", res.Result.Reason, "message", res.Result.Message)
135135
}
136136
log.V(1).Info("wrote response", "UID", res.UID, "allowed", res.Allowed)
137137
}

‎pkg/webhook/admission/http_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ var _ = Describe("Admission Webhooks", func() {
154154
log: logf.RuntimeLog.WithName("webhook"),
155155
}
156156

157-
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"reason":%q,"code":200}}}
157+
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"message":%q,"code":200}}}
158158
`, gvkJSONv1, value)
159159

160160
ctx, cancel := context.WithCancel(context.WithValue(context.Background(), key, value))
@@ -182,7 +182,7 @@ var _ = Describe("Admission Webhooks", func() {
182182
log: logf.RuntimeLog.WithName("webhook"),
183183
}
184184

185-
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"reason":%q,"code":200}}}
185+
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"message":%q,"code":200}}}
186186
`, gvkJSONv1, "application/json")
187187

188188
ctx, cancel := context.WithCancel(context.Background())

‎pkg/webhook/admission/response.go

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,21 +26,21 @@ import (
2626

2727
// Allowed constructs a response indicating that the given operation
2828
// is allowed (without any patches).
29-
func Allowed(reason string) Response {
30-
return ValidationResponse(true, reason)
29+
func Allowed(message string) Response {
30+
return ValidationResponse(true, message)
3131
}
3232

3333
// Denied constructs a response indicating that the given operation
3434
// is not allowed.
35-
func Denied(reason string) Response {
36-
return ValidationResponse(false, reason)
35+
func Denied(message string) Response {
36+
return ValidationResponse(false, message)
3737
}
3838

3939
// Patched constructs a response indicating that the given operation is
4040
// allowed, and that the target object should be modified by the given
4141
// JSONPatch operations.
42-
func Patched(reason string, patches ...jsonpatch.JsonPatchOperation) Response {
43-
resp := Allowed(reason)
42+
func Patched(message string, patches ...jsonpatch.JsonPatchOperation) Response {
43+
resp := Allowed(message)
4444
resp.Patches = patches
4545

4646
return resp
@@ -60,21 +60,24 @@ func Errored(code int32, err error) Response {
6060
}
6161

6262
// ValidationResponse returns a response for admitting a request.
63-
func ValidationResponse(allowed bool, reason string) Response {
63+
func ValidationResponse(allowed bool, message string) Response {
6464
code := http.StatusForbidden
65+
reason := metav1.StatusReasonForbidden
6566
if allowed {
6667
code = http.StatusOK
68+
reason = ""
6769
}
6870
resp := Response{
6971
AdmissionResponse: admissionv1.AdmissionResponse{
7072
Allowed: allowed,
7173
Result: &metav1.Status{
72-
Code: int32(code),
74+
Code: int32(code),
75+
Reason: reason,
7376
},
7477
},
7578
}
76-
if len(reason) > 0 {
77-
resp.Result.Reason = metav1.StatusReason(reason)
79+
if len(message) > 0 {
80+
resp.Result.Message = message
7881
}
7982
return resp
8083
}

‎pkg/webhook/admission/response_test.go

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
4949
AdmissionResponse: admissionv1.AdmissionResponse{
5050
Allowed: true,
5151
Result: &metav1.Status{
52-
Code: http.StatusOK,
53-
Reason: "acceptable",
52+
Code: http.StatusOK,
53+
Message: "acceptable",
5454
},
5555
},
5656
},
@@ -65,7 +65,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
6565
AdmissionResponse: admissionv1.AdmissionResponse{
6666
Allowed: false,
6767
Result: &metav1.Status{
68-
Code: http.StatusForbidden,
68+
Code: http.StatusForbidden,
69+
Reason: metav1.StatusReasonForbidden,
6970
},
7071
},
7172
},
@@ -78,8 +79,9 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
7879
AdmissionResponse: admissionv1.AdmissionResponse{
7980
Allowed: false,
8081
Result: &metav1.Status{
81-
Code: http.StatusForbidden,
82-
Reason: "UNACCEPTABLE!",
82+
Code: http.StatusForbidden,
83+
Reason: metav1.StatusReasonForbidden,
84+
Message: "UNACCEPTABLE!",
8385
},
8486
},
8587
},
@@ -118,8 +120,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
118120
AdmissionResponse: admissionv1.AdmissionResponse{
119121
Allowed: true,
120122
Result: &metav1.Status{
121-
Code: http.StatusOK,
122-
Reason: "some changes",
123+
Code: http.StatusOK,
124+
Message: "some changes",
123125
},
124126
},
125127
Patches: ops,
@@ -146,15 +148,15 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
146148
})
147149

148150
Describe("ValidationResponse", func() {
149-
It("should populate a status with a reason when a reason is given", func() {
151+
It("should populate a status with a message when a message is given", func() {
150152
By("checking that a message is populated for 'allowed' responses")
151153
Expect(ValidationResponse(true, "acceptable")).To(Equal(
152154
Response{
153155
AdmissionResponse: admissionv1.AdmissionResponse{
154156
Allowed: true,
155157
Result: &metav1.Status{
156-
Code: http.StatusOK,
157-
Reason: "acceptable",
158+
Code: http.StatusOK,
159+
Message: "acceptable",
158160
},
159161
},
160162
},
@@ -166,8 +168,9 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
166168
AdmissionResponse: admissionv1.AdmissionResponse{
167169
Allowed: false,
168170
Result: &metav1.Status{
169-
Code: http.StatusForbidden,
170-
Reason: "UNACCEPTABLE!",
171+
Code: http.StatusForbidden,
172+
Reason: metav1.StatusReasonForbidden,
173+
Message: "UNACCEPTABLE!",
171174
},
172175
},
173176
},
@@ -193,7 +196,8 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
193196
AdmissionResponse: admissionv1.AdmissionResponse{
194197
Allowed: false,
195198
Result: &metav1.Status{
196-
Code: http.StatusForbidden,
199+
Code: http.StatusForbidden,
200+
Reason: metav1.StatusReasonForbidden,
197201
},
198202
},
199203
},

‎pkg/webhook/admission/validator_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ var _ = Describe("validatingHandler", func() {
185185
})
186186
Expect(response.Allowed).Should(BeFalse())
187187
Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden)))
188-
Expect(string(response.Result.Reason)).Should(Equal(expectedError.Error()))
188+
Expect(response.Result.Message).Should(Equal(expectedError.Error()))
189189

190190
})
191191

@@ -206,7 +206,8 @@ var _ = Describe("validatingHandler", func() {
206206
})
207207
Expect(response.Allowed).Should(BeFalse())
208208
Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden)))
209-
Expect(string(response.Result.Reason)).Should(Equal(expectedError.Error()))
209+
Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden))
210+
Expect(response.Result.Message).Should(Equal(expectedError.Error()))
210211

211212
})
212213

@@ -223,8 +224,8 @@ var _ = Describe("validatingHandler", func() {
223224
})
224225
Expect(response.Allowed).Should(BeFalse())
225226
Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden)))
226-
Expect(string(response.Result.Reason)).Should(Equal(expectedError.Error()))
227-
227+
Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden))
228+
Expect(response.Result.Message).Should(Equal(expectedError.Error()))
228229
})
229230

230231
})

‎pkg/webhook/webhook_integration_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"net/http"
2525
"path/filepath"
2626
"strconv"
27+
"strings"
2728
"time"
2829

2930
. "github.com/onsi/ginkgo"
@@ -97,7 +98,7 @@ var _ = Describe("Webhook", func() {
9798

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

103104
cancel()
@@ -120,7 +121,7 @@ var _ = Describe("Webhook", func() {
120121

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

126127
cancel()
@@ -143,7 +144,7 @@ var _ = Describe("Webhook", func() {
143144

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

149150
cancel()
@@ -199,7 +200,7 @@ var _ = Describe("Webhook", func() {
199200

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

205206
cancel()

0 commit comments

Comments
 (0)