Allow Delete verb on validation callbacks (#1219)

* Create IsInDelete context

* Set up context with WithinDelete

* Test for callback delete

* fix subresource update

* Return oldObj for deletes

* include delete in webhook config

* include delete in unit test

* don't log on delete

apis/contexts.go

@@ -38,6 +38,21 @@ func IsInCreate(ctx context.Context) bool {
 	return ctx.Value(inCreateKey{}) != nil
 }
 
+// This is attached to contexts passed to webhook interfaces when
+// the receiver being validated is being deleted.
+type inDeleteKey struct{}
+
+// WithinDelete is used to note that the webhook is calling within
+// the context of a Delete operation.
+func WithinDelete(ctx context.Context) context.Context {
+	return context.WithValue(ctx, inDeleteKey{}, struct{}{})
+}
+
+// IsInDelete checks whether the context is a Delete.
+func IsInDelete(ctx context.Context) bool {
+	return ctx.Value(inDeleteKey{}) != nil
+}
+
 // This is attached to contexts passed to webhook interfaces when
 // the receiver being validated is being updated.
 type inUpdateKey struct{}

apis/contexts_test.go

@@ -48,6 +48,21 @@ func TestContexts(t *testing.T) {
 		ctx:   WithinUpdate(ctx, struct{}{}),
 		check: IsInCreate,
 		want:  false,
+	}, {
+		name:  "is in delete",
+		ctx:   WithinDelete(ctx),
+		check: IsInDelete,
+		want:  true,
+	}, {
+		name:  "not in delete (bare)",
+		ctx:   ctx,
+		check: IsInDelete,
+		want:  false,
+	}, {
+		name:  "not in delete (create)",
+		ctx:   WithinCreate(ctx),
+		check: IsInDelete,
+		want:  false,
 	}, {
 		name:  "is in update",
 		ctx:   WithinUpdate(ctx, struct{}{}),
@@ -144,7 +159,7 @@ func TestContexts(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			got := tc.check(tc.ctx)
 			if tc.want != got {
-				t.Errorf("check() = %v, wanted %v", tc.want, got)
+				t.Errorf("check() = %v, wanted %v", got, tc.want)
 			}
 		})
 	}

webhook/resourcesemantics/validation/reconcile_config.go

@@ -97,6 +97,7 @@ func (ac *reconciler) reconcileValidatingWebhook(ctx context.Context, caCert []b
 			Operations: []admissionregistrationv1beta1.OperationType{
 				admissionregistrationv1beta1.Create,
 				admissionregistrationv1beta1.Update,
+				admissionregistrationv1beta1.Delete,
 			},
 			Rule: admissionregistrationv1beta1.Rule{
 				APIGroups:   []string{gvk.Group},

webhook/resourcesemantics/validation/reconcile_config_test.go

@@ -59,28 +59,28 @@ func TestReconcile(t *testing.T) {
 
 	// These are the rules we expect given the context of "handlers".
 	expectedRules := []admissionregistrationv1beta1.RuleWithOperations{{
-		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE"},
+		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1beta1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1alpha1"},
 			Resources:   []string{"innerdefaultresources/*"},
 		},
 	}, {
-		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE"},
+		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1beta1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1alpha1"},
 			Resources:   []string{"resources/*"},
 		},
 	}, {
-		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE"},
+		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1beta1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1beta1"},
 			Resources:   []string{"resources/*"},
 		},
 	}, {
-		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE"},
+		Operations: []admissionregistrationv1beta1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1beta1.Rule{
 			APIGroups:   []string{"pkg.knative.io"},
 			APIVersions: []string{"v1alpha1"},

webhook/resourcesemantics/validation/validation_admit.go

@@ -42,11 +42,11 @@ type Callback struct {
 	function func(ctx context.Context, unstructured *unstructured.Unstructured) error
 
 	// supportedVerbs are the verbs supported for the callback.
-	// The function will only be called on these acitons.
+	// The function will only be called on these actions.
 	supportedVerbs map[webhook.Operation]struct{}
 }
 
-// NewCallback creates a new callback function to be invoked on supported vebs.
+// NewCallback creates a new callback function to be invoked on supported verbs.
 func NewCallback(function func(context.Context, *unstructured.Unstructured) error, supportedVerbs ...webhook.Operation) Callback {
 	m := make(map[webhook.Operation]struct{})
 	for _, op := range supportedVerbs {
@@ -131,23 +131,26 @@ func (ac *reconciler) decodeRequestAndPrepareContext(
 		}
 	}
 
-	// Set up the context for validation
-	if oldObj != nil {
-		if req.SubResource == "" {
-			ctx = apis.WithinUpdate(ctx, oldObj)
-		} else {
-			ctx = apis.WithinSubResourceUpdate(ctx, oldObj, req.SubResource)
-		}
-	} else {
-		ctx = apis.WithinCreate(ctx)
-	}
 	ctx = apis.WithUserInfo(ctx, &req.UserInfo)
 	ctx = context.WithValue(ctx, kubeclient.Key{}, ac.client)
-
 	if req.DryRun != nil && *req.DryRun {
 		ctx = apis.WithDryRun(ctx)
 	}
 
+	if newObj != nil && oldObj != nil && req.SubResource == "" {
+		ctx = apis.WithinSubResourceUpdate(ctx, oldObj, req.SubResource)
+	}
+
+	switch req.Operation {
+	case admissionv1beta1.Update:
+		ctx = apis.WithinUpdate(ctx, oldObj)
+	case admissionv1beta1.Create:
+		ctx = apis.WithinCreate(ctx)
+	case admissionv1beta1.Delete:
+		ctx = apis.WithinDelete(ctx)
+		return ctx, oldObj, nil
+	}
+
 	return ctx, newObj, nil
 }
 
@@ -158,6 +161,8 @@ func validate(ctx context.Context, resource resourcesemantics.GenericCRD, req *a
 	switch req.Operation {
 	case admissionv1beta1.Create, admissionv1beta1.Update:
 		// Supported verbs
+	case admissionv1beta1.Delete:
+		return nil // Validation handled by optional Callback, but not validatable.
 	default:
 		logger.Infof("Unhandled webhook validation operation, letting it through %v", req.Operation)
 		return nil

webhook/resourcesemantics/validation/validation_admit_test.go

@@ -84,12 +84,12 @@ var (
 			Group:   "pkg.knative.dev",
 			Version: "v1alpha1",
 			Kind:    "Resource",
-		}: NewCallback(resourceCallback, webhook.Create, webhook.Update),
+		}: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete),
 		{
 			Group:   "pkg.knative.dev",
 			Version: "v1beta1",
 			Kind:    "Resource",
-		}: NewCallback(resourceCallback, webhook.Create, webhook.Update),
+		}: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete),
 	}
 	initialResourceWebhook = &admissionregistrationv1beta1.ValidatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
@@ -272,6 +272,14 @@ func resourceCallback(ctx context.Context, uns *unstructured.Unstructured) error
 		return err
 	}
 
+	if apis.IsInDelete(ctx) {
+		if resource.Spec.FieldForCallbackValidation != "magic delete" {
+			return errors.New("no magic delete")
+		} else {
+			return nil
+		}
+	}
+
 	if apis.IsDryRun(ctx) {
 		return errors.New("dryRun fail")
 	}
@@ -283,7 +291,7 @@ func resourceCallback(ctx context.Context, uns *unstructured.Unstructured) error
 	return nil
 }
 
-func TestValidaitonCallback(t *testing.T) {
+func TestValidationCreateCallback(t *testing.T) {
 	tests := []struct {
 		name      string
 		dryRun    bool
@@ -343,6 +351,70 @@ func TestValidaitonCallback(t *testing.T) {
 	}
 }
 
+func TestValidationDeleteCallback(t *testing.T) {
+	tests := []struct {
+		name      string
+		setup     func(context.Context, *Resource)
+		rejection string
+	}{{
+		name: "with field magic delete",
+		setup: func(ctx context.Context, r *Resource) {
+			// Put a good value in.
+			r.Spec.FieldForCallbackValidation = "magic delete"
+		},
+	}, {
+		name: "with field reject value",
+		setup: func(ctx context.Context, r *Resource) {
+			// Put a bad value in.
+			r.Spec.FieldForCallbackValidation = "no magic delete"
+		},
+		rejection: "validation callback failed: no magic delete",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			r := CreateResource("a name")
+			ctx := apis.WithinCreate(apis.WithUserInfo(
+				TestContextWithLogger(t),
+				&authenticationv1.UserInfo{Username: user1}))
+
+			// Setup the resource.
+			tc.setup(ctx, r)
+
+			_, ac := newNonRunningTestResourceAdmissionController(t)
+			req := createDeleteResource(ctx, t, r)
+
+			resp := ac.Admit(ctx, req)
+
+			if tc.rejection == "" {
+				ExpectAllowed(t, resp)
+			} else {
+				ExpectFailsWith(t, resp, tc.rejection)
+			}
+		})
+	}
+}
+
+func createDeleteResource(ctx context.Context, t *testing.T, old *Resource) *admissionv1beta1.AdmissionRequest {
+	t.Helper()
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Delete,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+		UserInfo: *apis.GetUserInfo(ctx),
+	}
+	marshaledOld, err := json.Marshal(old)
+	if err != nil {
+		t.Fatalf("Failed to marshal resource: %v", err)
+	}
+	req.OldObject.Raw = marshaledOld
+	req.Resource.Group = "pkg.knative.dev"
+	return req
+}
+
 func createCreateResource(ctx context.Context, t *testing.T, r *Resource) *admissionv1beta1.AdmissionRequest {
 	t.Helper()
 	req := &admissionv1beta1.AdmissionRequest{