diff --git a/CHANGELOG.md b/CHANGELOG.md index 89a4950..35fb006 100644 --- 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 index 9f3e9a6..a8ecb37 100644 --- 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 index 2fd6023..f52b5e9 100644 --- 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 index 07c9d6e..c04488a 100644 --- 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 index 500c559..e89d907 100644 --- 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 index feaec2c..55d7e12 100644 --- 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{ diff --git a/test/integration/webhook/validating_test.go b/test/integration/webhook/validating_test.go index fde47fc..bac6903 100644 --- a/test/integration/webhook/validating_test.go +++ b/test/integration/webhook/validating_test.go @@ -28,6 +28,7 @@ import ( func getValidatingWebhookConfig(t *testing.T, cfg helperconfig.TestEnvConfig, rules []arv1.RuleWithOperations) *arv1.ValidatingWebhookConfiguration { whSideEffect := arv1.SideEffectClassNone + whFailurePolicy := arv1.Fail var timeoutSecs int32 = 30 return &arv1.ValidatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ @@ -37,6 +38,7 @@ func getValidatingWebhookConfig(t *testing.T, cfg helperconfig.TestEnvConfig, ru { Name: "test.slok.dev", AdmissionReviewVersions: []string{"v1beta1"}, + FailurePolicy: &whFailurePolicy, TimeoutSeconds: &timeoutSecs, SideEffects: &whSideEffect, ClientConfig: arv1.WebhookClientConfig{ @@ -74,7 +76,6 @@ func TestValidatingWebhook(t *testing.T) { Valid: false, Message: "test message from validator", }, nil - }) vwh, _ := validating.NewWebhook(validating.WebhookConfig{ Name: "pod-validating-label", @@ -90,7 +91,7 @@ func TestValidatingWebhook(t *testing.T) { Namespace: "default", }, Spec: corev1.PodSpec{ - Containers: []corev1.Container{corev1.Container{Name: "test", Image: "wrong"}}, + Containers: []corev1.Container{{Name: "test", Image: "wrong"}}, }, } _, err := cli.CoreV1().Pods(p.Namespace).Create(context.TODO(), p, metav1.CreateOptions{}) @@ -128,7 +129,7 @@ func TestValidatingWebhook(t *testing.T) { Namespace: "default", }, Spec: corev1.PodSpec{ - Containers: []corev1.Container{corev1.Container{Name: "test", Image: "wrong"}}, + Containers: []corev1.Container{{Name: "test", Image: "wrong"}}, }, } _, err := cli.CoreV1().Pods(p.Namespace).Create(context.TODO(), p, metav1.CreateOptions{}) @@ -166,7 +167,7 @@ func TestValidatingWebhook(t *testing.T) { Namespace: "default", }, Spec: corev1.PodSpec{ - Containers: []corev1.Container{corev1.Container{Name: "test", Image: "wrong"}}, + Containers: []corev1.Container{{Name: "test", Image: "wrong"}}, }, } _, err := cli.CoreV1().Pods(p.Namespace).Create(context.TODO(), p, metav1.CreateOptions{}) @@ -328,6 +329,102 @@ func TestValidatingWebhook(t *testing.T) { assert.NoError(t, err, "house should be present") }, }, + + "Having a static webhook, a validating webhook should allow deleting the pod.": { + webhookRegisterCfg: getValidatingWebhookConfig(t, cfg, []arv1.RuleWithOperations{webhookRulesDeletePod}), + webhook: func() webhook.Webhook { + val := validating.ValidatorFunc(func(ctx context.Context, obj metav1.Object) (bool, validating.ValidatorResult, error) { + // Allow if it has our label. + if l := obj.GetLabels()["kubewebhook"]; l == "test" { + return true, validating.ValidatorResult{Valid: true}, nil + } + + return true, validating.ValidatorResult{ + Valid: false, + Message: "test message from validator", + }, nil + }) + vwh, _ := validating.NewWebhook(validating.WebhookConfig{ + Name: "pod-validating-delete", + Obj: &corev1.Pod{}, + }, val, nil, nil, nil) + return vwh + }, + execTest: func(t *testing.T, cli kubernetes.Interface, _ kubewebhookcrd.Interface) { + assert := assert.New(t) + require := require.New(t) + + p := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("test-%d", time.Now().UnixNano()), + Namespace: "default", + Labels: map[string]string{"kubewebhook": "test"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "wrong"}}, + }, + } + _, err := cli.CoreV1().Pods(p.Namespace).Create(context.TODO(), p, metav1.CreateOptions{}) + require.NoError(err) + + err = cli.CoreV1().Pods(p.Namespace).Delete(context.TODO(), p.Name, metav1.DeleteOptions{}) + require.NoError(err) + + // Give time so deleting takes place. + time.Sleep(5 * time.Second) + + // Check expectations. + _, err = cli.CoreV1().Pods(p.Namespace).Get(context.TODO(), p.Name, metav1.GetOptions{}) + assert.Error(err, "pod shouldn't be present") + }, + }, + + "Having a dynamic webhook, a validating webhook should allow deleting the CRD.": { + webhookRegisterCfg: getValidatingWebhookConfig(t, cfg, []arv1.RuleWithOperations{webhookRulesDeletePod}), + webhook: func() webhook.Webhook { + val := validating.ValidatorFunc(func(ctx context.Context, obj metav1.Object) (bool, validating.ValidatorResult, error) { + // Allow if it has our label. + if l := obj.GetLabels()["city"]; l == "Bilbo" { + return true, validating.ValidatorResult{Valid: true}, nil + } + + return true, validating.ValidatorResult{ + Valid: false, + Message: "test message from validator", + }, nil + }) + vwh, _ := validating.NewWebhook(validating.WebhookConfig{ + Name: "pod-dynamic-validating-delete", + }, val, nil, nil, nil) + return vwh + }, + execTest: func(t *testing.T, _ kubernetes.Interface, crdcli kubewebhookcrd.Interface) { + assert := assert.New(t) + require := require.New(t) + + h := &buildingv1.House{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("test-%d", time.Now().UnixNano()), + Namespace: "default", + Labels: map[string]string{"city": "Bilbo"}, + }, + Spec: buildingv1.HouseSpec{Name: "newHouse"}, + } + + _, err := crdcli.BuildingV1().Houses(h.Namespace).Create(context.TODO(), h, metav1.CreateOptions{}) + require.NoError(err) + + err = crdcli.BuildingV1().Houses(h.Namespace).Delete(context.TODO(), h.Name, metav1.DeleteOptions{}) + require.NoError(err) + + // Give time so deleting takes place. + time.Sleep(5 * time.Second) + + // Check expectations. + _, err = cli.CoreV1().Pods(h.Namespace).Get(context.TODO(), h.Name, metav1.GetOptions{}) + assert.Error(err, "house shouldn't be present") + }, + }, } for name, test := range tests {