Added Rejected field to MutatorResult, fixes slok#189

SOF3 · Jun 9, 2022 · 7155978 · 7155978
1 parent 7cda665
commit 7155978
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 7 deletions.
diff --git a/pkg/http/handler.go b/pkg/http/handler.go
@@ -278,6 +278,15 @@ func (h handler) validatingModelResponseToJSON(ctx context.Context, review model
 }
 
 func (h handler) mutatingModelResponseToJSON(ctx context.Context, review model.AdmissionReview, resp *model.MutatingAdmissionResponse) (data []byte, err error) {
+	var resultStatus *metav1.Status
+	if resp.Rejected {
+		resultStatus = &metav1.Status{
+			Message: resp.Message,
+			Status:  metav1.StatusFailure,
+			Code:    http.StatusBadRequest,
+		}
+	}
+
 	switch review.OriginalAdmissionReview.(type) {
 	case *admissionv1beta1.AdmissionReview:
 		if len(resp.Warnings) > 0 {
@@ -290,7 +299,8 @@ func (h handler) mutatingModelResponseToJSON(ctx context.Context, review model.A
 				UID:       types.UID(review.ID),
 				PatchType: v1beta1JSONPatchType,
 				Patch:     resp.JSONPatchPatch,
-				Allowed:   true,
+				Allowed:   !resp.Rejected,
+				Result:    resultStatus,
 			},
 		})
 		return data, err
@@ -302,8 +312,9 @@ func (h handler) mutatingModelResponseToJSON(ctx context.Context, review model.A
 				UID:       types.UID(review.ID),
 				PatchType: v1JSONPatchType,
 				Patch:     resp.JSONPatchPatch,
-				Allowed:   true,
+				Allowed:   !resp.Rejected,
 				Warnings:  resp.Warnings,
+				Result:    resultStatus,
 			},
 		})
 

diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go
@@ -174,13 +174,29 @@ func TestDefaultWebhookFlow(t *testing.T) {
 			expCode: 200,
 		},
 
+		"A correct rejected mutating admission v1beta1 webhook without mutation should not fail.": {
+			body: getTestAdmissionReviewV1beta1RequestStr("1234567890"),
+			mock: func(mw *webhookmock.Webhook) {
+				resp := &model.MutatingAdmissionResponse{
+					ID:             "1234567890",
+					Rejected:       true,
+					Message:        "this is not valid because reasons",
+					JSONPatchPatch: []byte(``),
+					Warnings:       []string{"warn1", "warn2"}, // v1beta1 ignores warnings.
+				}
+				mw.On("Review", mock.Anything, mock.Anything).Once().Return(resp, nil)
+			},
+			expBody: `{"kind":"AdmissionReview","apiVersion":"admission.k8s.io/v1beta1","response":{"uid":"1234567890","allowed":false,"status":{"metadata":{},"status":"Failure","message":"this is not valid because reasons","code":400},"patchType":"JSONPatch"}}`,
+			expCode: 200,
+		},
+
 		"A correct validation admission v1 webhook that allows should not fail.": {
 			body: getTestAdmissionReviewV1RequestStr("1234567890"),
 			mock: func(mw *webhookmock.Webhook) {
 				resp := &model.ValidatingAdmissionResponse{
 					ID:       "1234567890",
 					Allowed:  true,
-					Warnings: []string{"warn1", "warn2"}, // v1beta1 ignores warnings.
+					Warnings: []string{"warn1", "warn2"}, // v1 includes warnings.
 				}
 				mw.On("Review", mock.Anything, mock.Anything).Once().Return(resp, nil)
 			},
@@ -195,7 +211,7 @@ func TestDefaultWebhookFlow(t *testing.T) {
 					ID:       "1234567890",
 					Allowed:  false,
 					Message:  "this is not valid because reasons",
-					Warnings: []string{"warn1", "warn2"}, // v1beta1 ignores warnings.
+					Warnings: []string{"warn1", "warn2"}, // v1 includes warnings.
 				}
 				mw.On("Review", mock.Anything, mock.Anything).Once().Return(resp, nil)
 			},
@@ -209,7 +225,7 @@ func TestDefaultWebhookFlow(t *testing.T) {
 				resp := &model.MutatingAdmissionResponse{
 					ID:             "1234567890",
 					JSONPatchPatch: []byte(`{"something": something}`),
-					Warnings:       []string{"warn1", "warn2"}, // v1beta1 ignores warnings.
+					Warnings:       []string{"warn1", "warn2"}, // v1 includes warnings.
 				}
 				mw.On("Review", mock.Anything, mock.Anything).Once().Return(resp, nil)
 			},
@@ -223,14 +239,30 @@ func TestDefaultWebhookFlow(t *testing.T) {
 				resp := &model.MutatingAdmissionResponse{
 					ID:             "1234567890",
 					JSONPatchPatch: []byte(``),
-					Warnings:       []string{"warn1", "warn2"}, // v1beta1 ignores warnings.
+					Warnings:       []string{"warn1", "warn2"}, // v1 includes warnings.
 				}
 				mw.On("Review", mock.Anything, mock.Anything).Once().Return(resp, nil)
 			},
 			expBody: `{"kind":"AdmissionReview","apiVersion":"admission.k8s.io/v1","response":{"uid":"1234567890","allowed":true,"patchType":"JSONPatch","warnings":["warn1","warn2"]}}`,
 			expCode: 200,
 		},
 
+		"A correct rejected mutating admission v1 webhook without mutation should not fail.": {
+			body: getTestAdmissionReviewV1RequestStr("1234567890"),
+			mock: func(mw *webhookmock.Webhook) {
+				resp := &model.MutatingAdmissionResponse{
+					ID:             "1234567890",
+					Rejected:       true,
+					Message:        "this is not valid because reasons",
+					JSONPatchPatch: []byte(``),
+					Warnings:       []string{"warn1", "warn2"}, // v1 includes warnings.
+				}
+				mw.On("Review", mock.Anything, mock.Anything).Once().Return(resp, nil)
+			},
+			expBody: `{"kind":"AdmissionReview","apiVersion":"admission.k8s.io/v1","response":{"uid":"1234567890","allowed":false,"status":{"metadata":{},"status":"Failure","message":"this is not valid because reasons","code":400},"patchType":"JSONPatch","warnings":["warn1","warn2"]}}`,
+			expCode: 200,
+		},
+
 		"A regular mutating admission v1beta1 call to the webhook handler should execute the webhook and return error if something failed": {
 			body: getTestAdmissionReviewV1beta1RequestStr("1234567890"),
 			mock: func(mw *webhookmock.Webhook) {

diff --git a/pkg/model/response.go b/pkg/model/response.go
@@ -21,6 +21,8 @@ type MutatingAdmissionResponse struct {
 	admissionResponse
 
 	ID             string
+	Rejected       bool
+	Message        string
 	JSONPatchPatch []byte
 	Warnings       []string
 }

diff --git a/pkg/webhook/mutating/mutator.go b/pkg/webhook/mutating/mutator.go
@@ -14,6 +14,12 @@ import (
 type MutatorResult struct {
 	// StopChain will stop the chain of validators in case there is a chain set.
 	StopChain bool
+	// Reject tells the apiserver that the resource is incorrect and it should reject the request.
+	// Contrary to ValidatorResult, Reject is used instead of Allowed so that the default value is to allow.
+	// If Reject is true, this implies StopChain.
+	Reject bool
+	// Message will be used by the apiserver to give more information in case the resource is not valid.
+	Message string
 	// MutatedObject is the object that has been mutated. If is nil, it will be used the one
 	// received by the Mutator.
 	MutatedObject metav1.Object
@@ -83,7 +89,7 @@ func (c *Chain) Mutate(ctx context.Context, ar *model.AdmissionReview, obj metav
 				obj = res.MutatedObject
 			}
 
-			if res.StopChain {
+			if res.StopChain || res.Reject {
 				res.Warnings = warnings
 				return res, nil
 			}

diff --git a/pkg/webhook/mutating/webhook.go b/pkg/webhook/mutating/webhook.go
@@ -146,6 +146,8 @@ func (w mutatingWebhook) mutatingAdmissionReview(ctx context.Context, ar model.A
 	// Forge response.
 	return &model.MutatingAdmissionResponse{
 		ID:             ar.ID,
+		Rejected:       res.Reject,
+		Message:        res.Message,
 		JSONPatchPatch: marshalledPatch,
 		Warnings:       res.Warnings,
 	}, nil