cmd/webhook: refactor error handling (#2521)

* return admission response with Allowed=false in case of validation errors * fix approved and rejected request metrics * add tests for malformed requests Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Aug 23, 2023 · 29ae205 · 29ae205
1 parent 81328c0
commit 29ae205
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 44 deletions.
diff --git 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,43 +88,56 @@ 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"
 		}
 
 		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
@@ -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
@@ -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
@@ -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
@@ -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{