Merge pull request #64 from slok/slok/delete-body

slok · May 16, 2020 · e8c88a9 · e8c88a9
2 parents 823fc44 + 6976447
commit e8c88a9
Show file tree

Hide file tree

Showing 7 changed files with 286 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,9 @@
 ### Added
 - Dynamic type webhooks without the need to a specific type (can use as multitype webhook).
 
+### Changed
+- Fixed on `DELETE` operations static webhooks not receiving object unmarshalled (#41)
+- Fixed on `DELETE` operations dynamic webhooks having unmarshaling errors (#63)
 
 ## [0.9.1] - 2020-04-09
 ### Changed

diff --git a/pkg/webhook/mutating/webhook.go b/pkg/webhook/mutating/webhook.go
@@ -98,8 +98,15 @@ func (w mutationWebhook) Review(ctx context.Context, ar *admissionv1beta1.Admiss
 
 	w.logger.Debugf("reviewing request %s, named: %s/%s", auid, ar.Request.Namespace, ar.Request.Name)
 
+	// Delete operations don't have body because should be gone on the deletion, instead they have the body
+	// of the object we want to delete as an old object.
+	raw := ar.Request.Object.Raw
+	if ar.Request.Operation == admissionv1beta1.Delete {
+		raw = ar.Request.OldObject.Raw
+	}
+
 	// Create a new object from the raw type.
-	runtimeObj, err := w.objectCreator.NewObject(ar.Request.Object.Raw)
+	runtimeObj, err := w.objectCreator.NewObject(raw)
 	if err != nil {
 		return w.toAdmissionErrorResponse(ar, err)
 	}
@@ -110,11 +117,11 @@ func (w mutationWebhook) Review(ctx context.Context, ar *admissionv1beta1.Admiss
 		return w.toAdmissionErrorResponse(ar, err)
 	}
 
-	return w.mutatingAdmissionReview(ctx, ar, mutatingObj)
+	return w.mutatingAdmissionReview(ctx, ar, raw, mutatingObj)
 
 }
 
-func (w mutationWebhook) mutatingAdmissionReview(ctx context.Context, ar *admissionv1beta1.AdmissionReview, obj metav1.Object) *admissionv1beta1.AdmissionResponse {
+func (w mutationWebhook) mutatingAdmissionReview(ctx context.Context, ar *admissionv1beta1.AdmissionReview, rawObj []byte, obj metav1.Object) *admissionv1beta1.AdmissionResponse {
 	auid := ar.Request.UID
 
 	// Mutate the object.
@@ -128,7 +135,7 @@ func (w mutationWebhook) mutatingAdmissionReview(ctx context.Context, ar *admiss
 		return w.toAdmissionErrorResponse(ar, err)
 	}
 
-	patch, err := jsonpatch.CreatePatch(ar.Request.Object.Raw, mutatedJSON)
+	patch, err := jsonpatch.CreatePatch(rawObj, mutatedJSON)
 	if err != nil {
 		return w.toAdmissionErrorResponse(ar, err)
 	}

diff --git a/pkg/webhook/mutating/webhook_test.go b/pkg/webhook/mutating/webhook_test.go
@@ -174,6 +174,24 @@ func TestPodAdmissionReviewMutation(t *testing.T) {
 			},
 		},
 
+		"A static webhook review of delete operation in a Pod should mutate the pod correctly.": {
+			cfg:     mutating.WebhookConfig{Name: "test", Obj: &corev1.Pod{}},
+			mutator: getPodResourceLimitDeletorMutator(),
+			review: &admissionv1beta1.AdmissionReview{
+				Request: &admissionv1beta1.AdmissionRequest{
+					Operation: admissionv1beta1.Delete,
+					UID:       "test",
+					OldObject: runtime.RawExtension{
+						Raw: getPodJSON(),
+					},
+				},
+			},
+			expPatch: []string{
+				`{"op":"remove","path":"/spec/containers/0/resources/limits"}`,
+				`{"op":"remove","path":"/spec/containers/1/resources/limits"}`,
+			},
+		},
+
 		"A dynamic webhook review of a Pod with an ns mutator should mutate the ns.": {
 			cfg:     mutating.WebhookConfig{Name: "test"},
 			mutator: getPodNSMutator("myChangedNS"),
@@ -280,6 +298,58 @@ func TestPodAdmissionReviewMutation(t *testing.T) {
 				`{"op":"add","path":"/metadata/labels/test2","value":"mutated-value2"}`,
 			},
 		},
+
+		"A dynamic webhook delete operation review of an unknown type should be able to mutate with the common object attributes (check unstructured object mutation).": {
+			cfg: mutating.WebhookConfig{Name: "test"},
+			mutator: mutating.MutatorFunc(func(_ context.Context, obj metav1.Object) (bool, error) {
+				// Just a check to validate that is unstructured.
+				if _, ok := obj.(runtime.Unstructured); !ok {
+					return true, fmt.Errorf("not unstructured")
+				}
+
+				// Mutate.
+				labels := obj.GetLabels()
+				if labels == nil {
+					labels = map[string]string{}
+				}
+				labels["test1"] = "mutated-value1"
+				labels["test2"] = "mutated-value2"
+				obj.SetLabels(labels)
+
+				return false, nil
+			}),
+			review: &admissionv1beta1.AdmissionReview{
+				Request: &admissionv1beta1.AdmissionRequest{
+					UID:       "test",
+					Operation: admissionv1beta1.Delete,
+					OldObject: runtime.RawExtension{
+						Raw: []byte(`
+						{
+							"kind": "whatever",
+							"apiVersion": "v42",
+							"metadata": {
+								"name":"something",
+								"namespace":"someplace",
+								"labels": {
+									"test1": "value1"
+								},
+								"annotations":{
+									"key1":"val1",
+									"key2":"val2"
+								}
+							},
+							"spec": {
+								"n": 42 
+							}
+						}`),
+					},
+				},
+			},
+			expPatch: []string{
+				`{"op":"replace","path":"/metadata/labels/test1","value":"mutated-value1"}`,
+				`{"op":"add","path":"/metadata/labels/test2","value":"mutated-value2"}`,
+			},
+		},
 	}
 
 	for name, test := range tests {

diff --git a/pkg/webhook/validating/webhook.go b/pkg/webhook/validating/webhook.go
@@ -93,8 +93,15 @@ type validateWebhook struct {
 func (w validateWebhook) Review(ctx context.Context, ar *admissionv1beta1.AdmissionReview) *admissionv1beta1.AdmissionResponse {
 	w.logger.Debugf("reviewing request %s, named: %s/%s", ar.Request.UID, ar.Request.Namespace, ar.Request.Name)
 
+	// Delete operations don't have body because should be gone on the deletion, instead they have the body
+	// of the object we want to delete as an old object.
+	raw := ar.Request.Object.Raw
+	if ar.Request.Operation == admissionv1beta1.Delete {
+		raw = ar.Request.OldObject.Raw
+	}
+
 	// Create a new object from the raw type.
-	runtimeObj, err := w.objectCreator.NewObject(ar.Request.Object.Raw)
+	runtimeObj, err := w.objectCreator.NewObject(raw)
 	if err != nil {
 		return w.toAdmissionErrorResponse(ar, err)
 	}

diff --git a/pkg/webhook/validating/webhook_test.go b/pkg/webhook/validating/webhook_test.go
@@ -28,6 +28,9 @@ func getPodJSON() []byte {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "testPod",
 			Namespace: "testNS",
+			Labels: map[string]string{
+				"test1": "value1",
+			},
 		},
 	}
 	bs, _ := json.Marshal(pod)
@@ -92,6 +95,41 @@ func TestPodAdmissionReviewValidation(t *testing.T) {
 			},
 		},
 
+		"A static webhook review of a delete operation on a Pod should allow.": {
+			cfg: validating.WebhookConfig{Name: "test", Obj: &corev1.Pod{}},
+			validator: validating.ValidatorFunc(func(_ context.Context, obj metav1.Object) (bool, validating.ValidatorResult, error) {
+				// Just a check to validate that is unstructured.
+				pod, ok := obj.(*corev1.Pod)
+				if !ok {
+					return true, validating.ValidatorResult{}, fmt.Errorf("not unstructured")
+				}
+
+				// Validate.
+				_, ok = pod.Labels["test1"]
+				return false, validating.ValidatorResult{
+					Valid:   ok,
+					Message: "label present",
+				}, nil
+			}),
+			review: &admissionv1beta1.AdmissionReview{
+				Request: &admissionv1beta1.AdmissionRequest{
+					Operation: admissionv1beta1.Delete,
+					UID:       "test",
+					OldObject: runtime.RawExtension{
+						Raw: getPodJSON(),
+					},
+				},
+			},
+			expResponse: &admissionv1beta1.AdmissionResponse{
+				UID:     "test",
+				Allowed: true,
+				Result: &metav1.Status{
+					Status:  metav1.StatusSuccess,
+					Message: "label present",
+				},
+			},
+		},
+
 		"A dynamic webhook review of a Pod with a valid validator result should return allowed.": {
 			cfg:       validating.WebhookConfig{Name: "test"},
 			validator: getFakeValidator(true, "valid test chain"),
@@ -177,6 +215,52 @@ func TestPodAdmissionReviewValidation(t *testing.T) {
 				},
 			},
 		},
+
+		"A dynamic webhook review of a delete operation on a unknown type should check that a label is present.": {
+			cfg: validating.WebhookConfig{Name: "test"},
+			validator: validating.ValidatorFunc(func(_ context.Context, obj metav1.Object) (bool, validating.ValidatorResult, error) {
+				// Just a check to validate that is unstructured.
+				if _, ok := obj.(runtime.Unstructured); !ok {
+					return true, validating.ValidatorResult{}, fmt.Errorf("not unstructured")
+				}
+
+				// Validate.
+				labels := obj.GetLabels()
+				_, ok := labels["test1"]
+				return false, validating.ValidatorResult{
+					Valid:   ok,
+					Message: "label present",
+				}, nil
+			}),
+			review: &admissionv1beta1.AdmissionReview{
+				Request: &admissionv1beta1.AdmissionRequest{
+					UID:       "test",
+					Operation: admissionv1beta1.Delete,
+					OldObject: runtime.RawExtension{
+						Raw: []byte(`
+						{
+							"kind": "whatever",
+							"apiVersion": "v42",
+							"metadata": {
+								"name":"something",
+								"namespace":"someplace",
+								"labels": {
+									"test1": "value1"
+								}
+							}
+						}`),
+					},
+				},
+			},
+			expResponse: &admissionv1beta1.AdmissionResponse{
+				UID:     "test",
+				Allowed: true,
+				Result: &metav1.Status{
+					Status:  metav1.StatusSuccess,
+					Message: "label present",
+				},
+			},
+		},
 	}
 
 	for name, test := range tests {

diff --git a/test/integration/webhook/helper_test.go b/test/integration/webhook/helper_test.go
@@ -42,6 +42,15 @@ var (
 		},
 	}
 
+	webhookRulesDeletePod = arv1.RuleWithOperations{
+		Operations: []arv1.OperationType{"DELETE"},
+		Rule: arv1.Rule{
+			APIGroups:   []string{""},
+			APIVersions: []string{"v1"},
+			Resources:   []string{"pods"},
+		},
+	}
+
 	webhookRulesHouseCRD = arv1.RuleWithOperations{
 		Operations: []arv1.OperationType{"CREATE"},
 		Rule: arv1.Rule{