From 87ce3f75df60d56e80ac9bfe195e433739ad5c16 Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Thu, 17 Aug 2023 23:32:28 +0200 Subject: [PATCH] cmd/webhook: refactor error handling * return admission response with Allowed=false in case of validation errors * fix approved and rejected request metrics * add tests for malformed requests --- cmd/webhook/admission/admission.go | 62 +++++++++---------- cmd/webhook/admission/admission_test.go | 80 +++++++++++++++++++++++++ cmd/webhook/admission/definitions.go | 5 ++ cmd/webhook/admission/ingress.go | 15 ++--- cmd/webhook/admission/routegroup.go | 15 ++--- 5 files changed, 133 insertions(+), 44 deletions(-) diff --git a/cmd/webhook/admission/admission.go b/cmd/webhook/admission/admission.go index a83b0d7c2c..09d0c982b2 100644 --- a/cmd/webhook/admission/admission.go +++ b/cmd/webhook/admission/admission.go @@ -2,7 +2,6 @@ package admission import ( "encoding/json" - "fmt" "io" "net/http" "time" @@ -80,7 +79,7 @@ func Handler(admitter admitter) http.HandlerFunc { return } - review := admissionReview{} + var review admissionReview err = json.Unmarshal(body, &review) if err != nil { log.Errorf("Failed to parse request: %v", err) @@ -89,15 +88,23 @@ func Handler(admitter admitter) http.HandlerFunc { return } - operationInfo := fmt.Sprintf( - "%s %s %s/%s", - review.Request.Operation, - review.Request.Kind, - review.Request.Namespace, - extractName(review.Request), - ) + request := review.Request + if request == nil { + log.Errorf("Missing review request") + w.WriteHeader(http.StatusBadRequest) + invalidRequests.WithLabelValues(admitterName).Inc() + return + } - gvr := review.Request.Resource + log := log.WithFields(log.Fields{ + "operation": request.Operation, + "kind": request.Kind, + "namespace": request.Namespace, + "name": extractName(request), + "user": request.UserInfo.Username, + }) + + gvr := request.Resource group := gvr.Group if group == "" { group = "zalando.org" @@ -105,27 +112,32 @@ func Handler(admitter admitter) http.HandlerFunc { labelValues := prometheus.Labels{ "admitter": admitterName, - "operation": string(review.Request.Operation), + "operation": request.Operation, "group": group, "version": gvr.Version, "resource": gvr.Resource, - "sub_resource": review.Request.SubResource, + "sub_resource": request.SubResource, } start := time.Now() - defer admissionDuration.With(labelValues). - Observe(float64(time.Since(start)) / float64(time.Second)) + defer admissionDuration.With(labelValues).Observe(float64(time.Since(start)) / float64(time.Second)) - admResp, err := admitter.admit(review.Request) + admResp, err := admitter.admit(request) if err != nil { - log.Errorf("Rejected %s: %v", operationInfo, err) - writeResponse(w, errorResponse(review.Request.UID, err)) - rejectedRequests.With(labelValues).Inc() + log.Errorf("Failed to admit request: %v", err) + w.WriteHeader(http.StatusBadRequest) + invalidRequests.WithLabelValues(admitterName).Inc() return } - log.Debugf("Allowed %s", operationInfo) - approvedRequests.With(labelValues).Inc() + if admResp.Allowed { + log.Debugf("Allowed") + approvedRequests.With(labelValues).Inc() + } else { + log.Debugf("Rejected") + rejectedRequests.With(labelValues).Inc() + } + writeResponse(w, admResp) } } @@ -148,16 +160,6 @@ func writeResponse(writer http.ResponseWriter, response *admissionResponse) { } } -func errorResponse(uid string, err error) *admissionResponse { - return &admissionResponse{ - Allowed: false, - UID: uid, - Result: &status{ - Message: err.Error(), - }, - } -} - func extractName(request *admissionRequest) string { if request.Name != "" { return request.Name diff --git a/cmd/webhook/admission/admission_test.go b/cmd/webhook/admission/admission_test.go index e00dde1cf1..d3869de555 100644 --- a/cmd/webhook/admission/admission_test.go +++ b/cmd/webhook/admission/admission_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -214,3 +215,82 @@ func TestIngressAdmitter(t *testing.T) { }) } } + +func TestMalformedRequests(t *testing.T) { + routeGroupHandler := Handler(&RouteGroupAdmitter{}) + ingressHandler := Handler(&IngressAdmitter{}) + + for _, tc := range []struct { + name string + method string + contentType string + input string + expectedStatus int + }{ + { + name: "unsupported method", + method: "GET", + input: `{"foo": "bar"}`, + expectedStatus: http.StatusBadRequest, + }, + { + name: "unsupported content type", + contentType: "text/plain", + input: "hello world", + expectedStatus: http.StatusBadRequest, + }, + { + name: "malformed json", + input: "not a json", + expectedStatus: http.StatusBadRequest, + }, + { + name: "missing review request", + input: `{"foo": "bar"}`, + expectedStatus: http.StatusBadRequest, + }, + { + name: "malformed object", + input: ` + { + "request": { + "uid": "req-uid", + "name": "req1", + "namespace": "ns1", + "object": "not an object" + } + } + `, + expectedStatus: http.StatusBadRequest, + }, + } { + t.Run(tc.name, func(t *testing.T) { + makeRequest := func() *http.Request { + method := "POST" + if tc.method != "" { + method = tc.method + } + + req := httptest.NewRequest(method, "http://example.com/foo", strings.NewReader(tc.input)) + if tc.contentType != "" { + req.Header.Set("Content-Type", tc.contentType) + } else { + req.Header.Set("Content-Type", "application/json") + } + return req + } + + t.Run("route group", func(t *testing.T) { + w := httptest.NewRecorder() + routeGroupHandler(w, makeRequest()) + assert.Equal(t, tc.expectedStatus, w.Code) + }) + + t.Run("ingress", func(t *testing.T) { + w := httptest.NewRecorder() + ingressHandler(w, makeRequest()) + assert.Equal(t, tc.expectedStatus, w.Code) + }) + }) + } +} diff --git a/cmd/webhook/admission/definitions.go b/cmd/webhook/admission/definitions.go index 0e75475b48..68c2626acf 100644 --- a/cmd/webhook/admission/definitions.go +++ b/cmd/webhook/admission/definitions.go @@ -43,6 +43,7 @@ type admissionRequest struct { Name string `json:"name,omitempty"` Namespace string `json:"namespace,omitempty"` Operation string `json:"operation"` + UserInfo userInfo `json:"userInfo"` Object json.RawMessage `json:"object,omitempty"` } @@ -55,3 +56,7 @@ type admissionResponse struct { type status struct { Message string `json:"message,omitempty"` } + +type userInfo struct { + Username string `json:"username,omitempty"` +} diff --git a/cmd/webhook/admission/ingress.go b/cmd/webhook/admission/ingress.go index f4313da106..536ac1c238 100644 --- a/cmd/webhook/admission/ingress.go +++ b/cmd/webhook/admission/ingress.go @@ -15,16 +15,17 @@ func (iga *IngressAdmitter) name() string { } func (iga *IngressAdmitter) admit(req *admissionRequest) (*admissionResponse, error) { - - ingressItem := definitions.IngressV1Item{} - err := json.Unmarshal(req.Object, &ingressItem) - if err != nil { + var ingressItem definitions.IngressV1Item + if err := json.Unmarshal(req.Object, &ingressItem); err != nil { return nil, err } - err = iga.IngressValidator.Validate(&ingressItem) - if err != nil { - return nil, err + if err := iga.IngressValidator.Validate(&ingressItem); err != nil { + return &admissionResponse{ + UID: req.UID, + Allowed: false, + Result: &status{Message: err.Error()}, + }, nil } return &admissionResponse{ diff --git a/cmd/webhook/admission/routegroup.go b/cmd/webhook/admission/routegroup.go index 42c70810d7..be26fd1dce 100644 --- a/cmd/webhook/admission/routegroup.go +++ b/cmd/webhook/admission/routegroup.go @@ -15,16 +15,17 @@ func (rga *RouteGroupAdmitter) name() string { } func (rga *RouteGroupAdmitter) admit(req *admissionRequest) (*admissionResponse, error) { - - rgItem := definitions.RouteGroupItem{} - err := json.Unmarshal(req.Object, &rgItem) - if err != nil { + var rgItem definitions.RouteGroupItem + if err := json.Unmarshal(req.Object, &rgItem); err != nil { return nil, err } - err = rga.RouteGroupValidator.Validate(&rgItem) - if err != nil { - return nil, err + if err := rga.RouteGroupValidator.Validate(&rgItem); err != nil { + return &admissionResponse{ + UID: req.UID, + Allowed: false, + Result: &status{Message: err.Error()}, + }, nil } return &admissionResponse{