Skip to content

Commit f7a3dc6

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1293 from estroz/bugfix/webhook-admissionreview-version
🐛 pkg/webhook/admission: set GVK on AdmissionReview responses
2 parents 3410e75 + 27ef229 commit f7a3dc6

File tree

2 files changed

+84
-23
lines changed

2 files changed

+84
-23
lines changed

‎pkg/webhook/admission/http.go‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
v1 "k8s.io/api/admission/v1"
2828
"k8s.io/api/admission/v1beta1"
2929
"k8s.io/apimachinery/pkg/runtime"
30+
"k8s.io/apimachinery/pkg/runtime/schema"
3031
"k8s.io/apimachinery/pkg/runtime/serializer"
3132
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
3233
)
@@ -73,14 +74,17 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
7374

7475
// Both v1 and v1beta1 AdmissionReview types are exactly the same, so the v1beta1 type can
7576
// be decoded into the v1 type. However the runtime codec's decoder guesses which type to
76-
// decode into by type name if an Object's TypeMeta isn't set. By setting TypeMeta of an`
77+
// decode into by type name if an Object's TypeMeta isn't set. By setting TypeMeta of an
7778
// unregistered type to the v1 GVK, the decoder will coerce a v1beta1 AdmissionReview to v1.
79+
// The actual AdmissionReview GVK will be used to write a typed response in case the
80+
// webhook config permits multiple versions, otherwise this response will fail.
7881
req := Request{}
7982
ar := unversionedAdmissionReview{}
8083
// avoid an extra copy
8184
ar.Request = &req.AdmissionRequest
8285
ar.SetGroupVersionKind(v1.SchemeGroupVersion.WithKind("AdmissionReview"))
83-
if _, _, err := admissionCodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil {
86+
_, actualAdmRevGVK, err := admissionCodecs.UniversalDeserializer().Decode(body, nil, &ar)
87+
if err != nil {
8488
wh.log.Error(err, "unable to decode the request")
8589
reviewResponse = Errored(http.StatusBadRequest, err)
8690
wh.writeResponse(w, reviewResponse)
@@ -90,20 +94,39 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
9094

9195
// TODO: add panic-recovery for Handle
9296
reviewResponse = wh.Handle(r.Context(), req)
93-
wh.writeResponse(w, reviewResponse)
97+
wh.writeResponseTyped(w, reviewResponse, actualAdmRevGVK)
9498
}
9599

100+
// writeResponse writes response to w generically, i.e. without encoding GVK information.
96101
func (wh *Webhook) writeResponse(w io.Writer, response Response) {
97-
encoder := json.NewEncoder(w)
98-
responseAdmissionReview := v1.AdmissionReview{
102+
wh.writeAdmissionResponse(w, v1.AdmissionReview{Response: &response.AdmissionResponse})
103+
}
104+
105+
// writeResponseTyped writes response to w with GVK set to admRevGVK, which is necessary
106+
// if multiple AdmissionReview versions are permitted by the webhook.
107+
func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, admRevGVK *schema.GroupVersionKind) {
108+
ar := v1.AdmissionReview{
99109
Response: &response.AdmissionResponse,
100110
}
101-
err := encoder.Encode(responseAdmissionReview)
111+
// Default to a v1 AdmissionReview, otherwise the API server may not recognize the request
112+
// if multiple AdmissionReview versions are permitted by the webhook config.
113+
// TODO(estroz): this should be configurable since older API servers won't know about v1.
114+
if admRevGVK == nil || *admRevGVK == (schema.GroupVersionKind{}) {
115+
ar.SetGroupVersionKind(v1.SchemeGroupVersion.WithKind("AdmissionReview"))
116+
} else {
117+
ar.SetGroupVersionKind(*admRevGVK)
118+
}
119+
wh.writeAdmissionResponse(w, ar)
120+
}
121+
122+
// writeAdmissionResponse writes ar to w.
123+
func (wh *Webhook) writeAdmissionResponse(w io.Writer, ar v1.AdmissionReview) {
124+
err := json.NewEncoder(w).Encode(ar)
102125
if err != nil {
103126
wh.log.Error(err, "unable to encode the response")
104127
wh.writeResponse(w, Errored(http.StatusInternalServerError, err))
105128
} else {
106-
res := responseAdmissionReview.Response
129+
res := ar.Response
107130
if log := wh.log; log.V(1).Enabled() {
108131
if res.Result != nil {
109132
log = log.WithValues("code", res.Result.Code, "reason", res.Result.Reason)

‎pkg/webhook/admission/http_test.go‎

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ import (
3535

3636
var _ = Describe("Admission Webhooks", func() {
3737

38+
const (
39+
gvkJSONv1 = `"kind":"AdmissionReview","apiVersion":"admission.k8s.io/v1"`
40+
gvkJSONv1beta1 = `"kind":"AdmissionReview","apiVersion":"admission.k8s.io/v1beta1"`
41+
)
42+
3843
Describe("HTTP Handler", func() {
3944
var respRecorder *httptest.ResponseRecorder
4045
webhook := &Webhook{
@@ -51,10 +56,10 @@ var _ = Describe("Admission Webhooks", func() {
5156
It("should return bad-request when given an empty body", func() {
5257
req := &http.Request{Body: nil}
5358

54-
expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}}
55-
`)
59+
expected := `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}}
60+
`
5661
webhook.ServeHTTP(respRecorder, req)
57-
Expect(respRecorder.Body.Bytes()).To(Equal(expected))
62+
Expect(respRecorder.Body.String()).To(Equal(expected))
5863
})
5964

6065
It("should return bad-request when given the wrong content-type", func() {
@@ -63,10 +68,11 @@ var _ = Describe("Admission Webhooks", func() {
6368
Body: nopCloser{Reader: bytes.NewBuffer(nil)},
6469
}
6570

66-
expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expected application/json","code":400}}}
67-
`)
71+
expected :=
72+
`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expected application/json","code":400}}}
73+
`
6874
webhook.ServeHTTP(respRecorder, req)
69-
Expect(respRecorder.Body.Bytes()).To(Equal(expected))
75+
Expect(respRecorder.Body.String()).To(Equal(expected))
7076
})
7177

7278
It("should return bad-request when given an undecodable body", func() {
@@ -75,14 +81,14 @@ var _ = Describe("Admission Webhooks", func() {
7581
Body: nopCloser{Reader: bytes.NewBufferString("{")},
7682
}
7783

78-
expected := []byte(
84+
expected :=
7985
`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"couldn't get version/kind; json parse error: unexpected end of JSON input","code":400}}}
80-
`)
86+
`
8187
webhook.ServeHTTP(respRecorder, req)
82-
Expect(respRecorder.Body.Bytes()).To(Equal(expected))
88+
Expect(respRecorder.Body.String()).To(Equal(expected))
8389
})
8490

85-
It("should return the response given by the handler", func() {
91+
It("should return the response given by the handler with version defaulted to v1", func() {
8692
req := &http.Request{
8793
Header: http.Header{"Content-Type": []string{"application/json"}},
8894
Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)},
@@ -92,10 +98,42 @@ var _ = Describe("Admission Webhooks", func() {
9298
log: logf.RuntimeLog.WithName("webhook"),
9399
}
94100

95-
expected := []byte(`{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}}
96-
`)
101+
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}}
102+
`, gvkJSONv1)
103+
webhook.ServeHTTP(respRecorder, req)
104+
Expect(respRecorder.Body.String()).To(Equal(expected))
105+
})
106+
107+
It("should return the v1 response given by the handler", func() {
108+
req := &http.Request{
109+
Header: http.Header{"Content-Type": []string{"application/json"}},
110+
Body: nopCloser{Reader: bytes.NewBufferString(fmt.Sprintf(`{%s,"request":{}}`, gvkJSONv1))},
111+
}
112+
webhook := &Webhook{
113+
Handler: &fakeHandler{},
114+
log: logf.RuntimeLog.WithName("webhook"),
115+
}
116+
117+
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}}
118+
`, gvkJSONv1)
119+
webhook.ServeHTTP(respRecorder, req)
120+
Expect(respRecorder.Body.String()).To(Equal(expected))
121+
})
122+
123+
It("should return the v1beta1 response given by the handler", func() {
124+
req := &http.Request{
125+
Header: http.Header{"Content-Type": []string{"application/json"}},
126+
Body: nopCloser{Reader: bytes.NewBufferString(fmt.Sprintf(`{%s,"request":{}}`, gvkJSONv1beta1))},
127+
}
128+
webhook := &Webhook{
129+
Handler: &fakeHandler{},
130+
log: logf.RuntimeLog.WithName("webhook"),
131+
}
132+
133+
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}}
134+
`, gvkJSONv1beta1)
97135
webhook.ServeHTTP(respRecorder, req)
98-
Expect(respRecorder.Body.Bytes()).To(Equal(expected))
136+
Expect(respRecorder.Body.String()).To(Equal(expected))
99137
})
100138

101139
It("should present the Context from the HTTP request, if any", func() {
@@ -116,13 +154,13 @@ var _ = Describe("Admission Webhooks", func() {
116154
log: logf.RuntimeLog.WithName("webhook"),
117155
}
118156

119-
expected := []byte(fmt.Sprintf(`{"response":{"uid":"","allowed":true,"status":{"metadata":{},"reason":%q,"code":200}}}
120-
`, value))
157+
expected := fmt.Sprintf(`{%s,"response":{"uid":"","allowed":true,"status":{"metadata":{},"reason":%q,"code":200}}}
158+
`, gvkJSONv1, value)
121159

122160
ctx, cancel := context.WithCancel(context.WithValue(context.Background(), key, value))
123161
cancel()
124162
webhook.ServeHTTP(respRecorder, req.WithContext(ctx))
125-
Expect(respRecorder.Body.Bytes()).To(Equal(expected))
163+
Expect(respRecorder.Body.String()).To(Equal(expected))
126164
})
127165
})
128166
})

0 commit comments

Comments
 (0)