address comments

Author: Mengqi Yu

pkg/webhook/admission/admission_suite_test.go

@@ -26,7 +26,7 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
 
-func TestSource(t *testing.T) {
+func TestAdmissionWebhook(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecsWithDefaultAndCustomReporters(t, "application Suite", []Reporter{envtest.NewlineReporter{}})
 }

pkg/webhook/admission/decode_test.go

@@ -27,6 +27,15 @@ import (
 )
 
 var _ = Describe("admission webhook decoder", func() {
+	var decoder Decoder
+	BeforeEach(func(done Done) {
+		var err error
+		decoder, err = NewDecoder(scheme.Scheme)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(decoder).NotTo(BeNil())
+		close(done)
+	})
+
 	Describe("NewDecoder", func() {
 		It("should return a decoder without an error", func() {
 			decoder, err := NewDecoder(scheme.Scheme)
@@ -36,7 +45,6 @@ var _ = Describe("admission webhook decoder", func() {
 	})
 
 	Describe("Decode", func() {
-		var decoder Decoder
 		req := Request{
 			AdmissionRequest: &admissionv1beta1.AdmissionRequest{
 				Object: runtime.RawExtension{
@@ -59,13 +67,6 @@ var _ = Describe("admission webhook decoder", func() {
 				},
 			},
 		}
-		BeforeEach(func(done Done) {
-			var err error
-			decoder, err = NewDecoder(scheme.Scheme)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(decoder).NotTo(BeNil())
-			close(done)
-		})
 
 		It("should be able to decode", func() {
 			err := decoder.Decode(req, &corev1.Pod{})

pkg/webhook/admission/http_test.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"net/http/httptest"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -30,18 +31,25 @@ import (
 )
 
 var _ = Describe("admission webhook http handler", func() {
+	var w *httptest.ResponseRecorder
+	BeforeEach(func(done Done) {
+		w = &httptest.ResponseRecorder{
+			Body: bytes.NewBuffer(nil),
+		}
+		close(done)
+	})
+
 	Describe("empty request body", func() {
 		req := &http.Request{Body: nil}
 		wh := &Webhook{
 			Handlers: []Handler{},
 		}
-		w := &fakeResponseWriter{}
+
 		expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}}
 `)
-		It("should return a response with an error", func() {
+		It("should return an error with bad-request status code", func() {
 			wh.ServeHTTP(w, req)
-			Expect(w.response).NotTo(BeNil())
-			Expect(w.response).To(Equal(expected))
+			Expect(w.Body.Bytes()).To(Equal(expected))
 		})
 	})
 
@@ -53,13 +61,12 @@ var _ = Describe("admission webhook http handler", func() {
 		wh := &Webhook{
 			Handlers: []Handler{},
 		}
-		w := &fakeResponseWriter{}
 		expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expect application/json","code":400}}}
 `)
-		It("should return a response with an error", func() {
+		It("should return an error with bad-request status code", func() {
 			wh.ServeHTTP(w, req)
-			Expect(w.response).NotTo(BeNil())
-			Expect(w.response).To(Equal(expected))
+			Expect(w.Body.Bytes()).To(Equal(expected))
+
 		})
 	})
 
@@ -72,14 +79,13 @@ var _ = Describe("admission webhook http handler", func() {
 			Type:     types.WebhookTypeMutating,
 			Handlers: []Handler{},
 		}
-		w := &fakeResponseWriter{}
 		expected := []byte(
 			`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"couldn't get version/kind; json parse error: unexpected end of JSON input","code":400}}}
 `)
-		It("should return a response with an error", func() {
+		It("should return an error with bad-request status code", func() {
 			wh.ServeHTTP(w, req)
-			Expect(w.response).NotTo(BeNil())
-			Expect(w.response).To(Equal(expected))
+			Expect(w.Body.Bytes()).To(Equal(expected))
+
 		})
 	})
 
@@ -92,13 +98,11 @@ var _ = Describe("admission webhook http handler", func() {
 			Type:     types.WebhookTypeMutating,
 			Handlers: []Handler{},
 		}
-		w := &fakeResponseWriter{}
 		expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"got an empty AdmissionRequest","code":400}}}
 `)
-		It("should return a response with an error", func() {
+		It("should return an error with bad-request status code", func() {
 			wh.ServeHTTP(w, req)
-			Expect(w.response).NotTo(BeNil())
-			Expect(w.response).To(Equal(expected))
+			Expect(w.Body.Bytes()).To(Equal(expected))
 		})
 	})
 
@@ -110,13 +114,12 @@ var _ = Describe("admission webhook http handler", func() {
 		wh := &Webhook{
 			Handlers: []Handler{},
 		}
-		w := &fakeResponseWriter{}
-		expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"you must specify your webhook type","code":400}}}
+		expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"you must specify your webhook type","code":500}}}
 `)
-		It("should return a response with an error", func() {
+		It("should return an error with internal-error status code", func() {
 			wh.ServeHTTP(w, req)
-			Expect(w.response).NotTo(BeNil())
-			Expect(w.response).To(Equal(expected))
+			Expect(w.Body.Bytes()).To(Equal(expected))
+
 		})
 	})
 
@@ -131,36 +134,17 @@ var _ = Describe("admission webhook http handler", func() {
 			Handlers: []Handler{h},
 			KVMap:    map[string]interface{}{"foo": "bar"},
 		}
-		w := &fakeResponseWriter{}
 		expected := []byte(`{"response":{"uid":"","allowed":true}}
 `)
 		It("should return a response successfully", func() {
 			wh.ServeHTTP(w, req)
-			Expect(w.response).NotTo(BeNil())
-			Expect(w.response).To(Equal(expected))
+			Expect(w.Body.Bytes()).To(Equal(expected))
 			Expect(h.invoked).To(BeTrue())
 			Expect(h.valueFromContext).To(Equal("bar"))
 		})
 	})
 })
 
-type fakeResponseWriter struct {
-	response []byte
-}
-
-var _ http.ResponseWriter = &fakeResponseWriter{}
-
-func (w *fakeResponseWriter) Header() http.Header {
-	return nil
-}
-
-func (w *fakeResponseWriter) Write(resp []byte) (int, error) {
-	w.response = append(w.response, resp...)
-	return len(resp), nil
-}
-
-func (w *fakeResponseWriter) WriteHeader(statusCode int) {}
-
 type nopCloser struct {
 	io.Reader
 }

pkg/webhook/admission/webhook.go

@@ -72,6 +72,7 @@ type Webhook struct {
 	KVMap map[string]interface{}
 	// Handlers contains a list of handlers. Each handler may only contains the business logic for its own feature.
 	// For example, feature foo and bar can be in the same webhook if all the other configurations are the same.
+	// The handler will be invoked sequentially as the order in the list.
 	Handlers []Handler
 
 	once sync.Once
@@ -117,7 +118,7 @@ func (w *Webhook) Handle(ctx context.Context, req Request) Response {
 	case types.WebhookTypeValidating:
 		resp = w.handleValidating(ctx, req)
 	default:
-		return ErrorResponse(http.StatusBadRequest, errors.New("you must specify your webhook type"))
+		return ErrorResponse(http.StatusInternalServerError, errors.New("you must specify your webhook type"))
 	}
 	resp.Response.UID = req.AdmissionRequest.UID
 	return resp
@@ -135,44 +136,45 @@ func (w *Webhook) kvMapToContext(ctx context.Context) (context.Context, error) {
 }
 
 func (w *Webhook) handleMutating(ctx context.Context, req Request) Response {
-	r := Response{
-		Response: &admissionv1beta1.AdmissionResponse{},
-	}
 	patches := []jsonpatch.JsonPatchOperation{}
 	for _, handler := range w.Handlers {
 		resp := handler.Handle(ctx, req)
 		if !resp.Response.Allowed {
 			return resp
 		}
-		if resp.Response.PatchType == nil || *resp.Response.PatchType != admissionv1beta1.PatchTypeJSONPatch {
+		if resp.Response.PatchType != nil && *resp.Response.PatchType != admissionv1beta1.PatchTypeJSONPatch {
 			return ErrorResponse(http.StatusInternalServerError,
 				fmt.Errorf("unexpected patch type returned by the handler: %v, only allow: %v",
 					resp.Response.PatchType, admissionv1beta1.PatchTypeJSONPatch))
 		}
 		patches = append(patches, resp.Patches...)
 	}
 	var err error
-	r.Response.Patch, err = json.Marshal(patches)
+	marshaledPatch, err := json.Marshal(patches)
 	if err != nil {
 		return ErrorResponse(http.StatusBadRequest, fmt.Errorf("error when marshaling the patch: %v", err))
 	}
-	return r
-}
-
-func (w *Webhook) handleValidating(ctx context.Context, req Request) Response {
-	r := Response{
+	return Response{
 		Response: &admissionv1beta1.AdmissionResponse{
-			Allowed: true,
+			Allowed:   true,
+			Patch:     marshaledPatch,
+			PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(),
 		},
 	}
+}
+
+func (w *Webhook) handleValidating(ctx context.Context, req Request) Response {
 	for _, handler := range w.Handlers {
 		resp := handler.Handle(ctx, req)
 		if !resp.Response.Allowed {
-			r.Response.Allowed = false
-			break
+			return resp
 		}
 	}
-	return r
+	return Response{
+		Response: &admissionv1beta1.AdmissionResponse{
+			Allowed: true,
+		},
+	}
 }
 
 // GetName returns the name of the webhook.