Merge pull request #3223 from troy0820/troy0820/return-warnings-on-webhooks

🐛Return warnings on webhook response

Author: k8s-ci-robot

pkg/webhook/admission/multi.go

@@ -31,6 +31,7 @@ type multiMutating []Handler
 
 func (hs multiMutating) Handle(ctx context.Context, req Request) Response {
 	patches := []jsonpatch.JsonPatchOperation{}
+	warnings := []string{}
 	for _, handler := range hs {
 		resp := handler.Handle(ctx, req)
 		if !resp.Allowed {
@@ -42,6 +43,7 @@ func (hs multiMutating) Handle(ctx context.Context, req Request) Response {
 					resp.PatchType, admissionv1.PatchTypeJSONPatch))
 		}
 		patches = append(patches, resp.Patches...)
+		warnings = append(warnings, resp.Warnings...)
 	}
 	var err error
 	marshaledPatch, err := json.Marshal(patches)
@@ -55,6 +57,7 @@ func (hs multiMutating) Handle(ctx context.Context, req Request) Response {
 				Code: http.StatusOK,
 			},
 			Patch:     marshaledPatch,
+			Warnings:  warnings,
 			PatchType: func() *admissionv1.PatchType { pt := admissionv1.PatchTypeJSONPatch; return &pt }(),
 		},
 	}
@@ -71,18 +74,21 @@ func MultiMutatingHandler(handlers ...Handler) Handler {
 type multiValidating []Handler
 
 func (hs multiValidating) Handle(ctx context.Context, req Request) Response {
+	warnings := []string{}
 	for _, handler := range hs {
 		resp := handler.Handle(ctx, req)
 		if !resp.Allowed {
 			return resp
 		}
+		warnings = append(warnings, resp.Warnings...)
 	}
 	return Response{
 		AdmissionResponse: admissionv1.AdmissionResponse{
 			Allowed: true,
 			Result: &metav1.Status{
 				Code: http.StatusOK,
 			},
+			Warnings: warnings,
 		},
 	}
 }

pkg/webhook/admission/multi_test.go

@@ -46,6 +46,17 @@ var _ = Describe("Multi-Handler Admission Webhooks", func() {
 		},
 	}
 
+	withWarnings := &fakeHandler{
+		fn: func(ctx context.Context, req Request) Response {
+			return Response{
+				AdmissionResponse: admissionv1.AdmissionResponse{
+					Allowed:  true,
+					Warnings: []string{"handler-warning"},
+				},
+			}
+		},
+	}
+
 	Context("with validating handlers", func() {
 		It("should deny the request if any handler denies the request", func() {
 			By("setting up a handler with accept and deny")
@@ -54,6 +65,7 @@ var _ = Describe("Multi-Handler Admission Webhooks", func() {
 			By("checking that the handler denies the request")
 			resp := handler.Handle(context.Background(), Request{})
 			Expect(resp.Allowed).To(BeFalse())
+			Expect(resp.Warnings).To(BeEmpty())
 		})
 
 		It("should allow the request if all handlers allow the request", func() {
@@ -63,6 +75,17 @@ var _ = Describe("Multi-Handler Admission Webhooks", func() {
 			By("checking that the handler allows the request")
 			resp := handler.Handle(context.Background(), Request{})
 			Expect(resp.Allowed).To(BeTrue())
+			Expect(resp.Warnings).To(BeEmpty())
+		})
+
+		It("should show the warnings if all handlers allow the request", func() {
+			By("setting up a handler with only accept")
+			handler := MultiValidatingHandler(alwaysAllow, withWarnings)
+
+			By("checking that the handler allows the request")
+			resp := handler.Handle(context.Background(), Request{})
+			Expect(resp.Allowed).To(BeTrue())
+			Expect(resp.Warnings).To(HaveLen(1))
 		})
 	})
 
@@ -107,6 +130,25 @@ var _ = Describe("Multi-Handler Admission Webhooks", func() {
 			},
 		}
 
+		patcher3 := &fakeHandler{
+			fn: func(ctx context.Context, req Request) Response {
+				return Response{
+					Patches: []jsonpatch.JsonPatchOperation{
+						{
+							Operation: "add",
+							Path:      "/metadata/annotation/newest-key",
+							Value:     "value",
+						},
+					},
+					AdmissionResponse: admissionv1.AdmissionResponse{
+						Allowed:   true,
+						Warnings:  []string{"annotation-warning"},
+						PatchType: func() *admissionv1.PatchType { pt := admissionv1.PatchTypeJSONPatch; return &pt }(),
+					},
+				}
+			},
+		}
+
 		It("should not return any patches if the request is denied", func() {
 			By("setting up a webhook with some patches and a deny")
 			handler := MultiMutatingHandler(patcher1, patcher2, alwaysDeny)
@@ -128,5 +170,20 @@ var _ = Describe("Multi-Handler Admission Webhooks", func() {
 				`[{"op":"add","path":"/metadata/annotation/new-key","value":"new-value"},` +
 					`{"op":"replace","path":"/spec/replicas","value":"2"},{"op":"add","path":"/metadata/annotation/hello","value":"world"}]`)))
 		})
+
+		It("should produce all patches if the requests are all allowed and show warnings", func() {
+			By("setting up a webhook with some patches")
+			handler := MultiMutatingHandler(patcher1, patcher2, alwaysAllow, patcher3)
+
+			By("checking that the handler accepts the request and returns all patches")
+			resp := handler.Handle(context.Background(), Request{})
+			Expect(resp.Allowed).To(BeTrue())
+			Expect(resp.Patch).To(Equal([]byte(
+				`[{"op":"add","path":"/metadata/annotation/new-key","value":"new-value"},` +
+					`{"op":"replace","path":"/spec/replicas","value":"2"},{"op":"add","path":"/metadata/annotation/hello","value":"world"},` +
+					`{"op":"add","path":"/metadata/annotation/newest-key","value":"value"}]`)))
+			Expect(resp.Warnings).To(HaveLen(1))
+		})
+
 	})
 })