Run deleteValidation at the storage layer so that it will be retried on

conflict.

Adding unit test verify that deleteValidation is retried.

adding e2e test verifying the webhook can intercept configmap and custom
resource deletion, and the existing object is sent via the
admissionreview.OldObject.

update the admission integration test to verify that the existing object
is passed to the deletion admission webhook as oldObject, in case of an
immediate deletion and in case of an update-on-delete.

test/e2e/apimachinery/webhook.go

@@ -73,19 +73,20 @@ const (
 	crdWebhookConfigName                   = "e2e-test-webhook-config-crd"
 	slowWebhookConfigName                  = "e2e-test-webhook-config-slow"
 
-	skipNamespaceLabelKey   = "skip-webhook-admission"
-	skipNamespaceLabelValue = "yes"
-	skippedNamespaceName    = "exempted-namesapce"
-	disallowedPodName       = "disallowed-pod"
-	toBeAttachedPodName     = "to-be-attached-pod"
-	hangingPodName          = "hanging-pod"
-	disallowedConfigMapName = "disallowed-configmap"
-	allowedConfigMapName    = "allowed-configmap"
-	failNamespaceLabelKey   = "fail-closed-webhook"
-	failNamespaceLabelValue = "yes"
-	failNamespaceName       = "fail-closed-namesapce"
-	addedLabelKey           = "added-label"
-	addedLabelValue         = "yes"
+	skipNamespaceLabelKey     = "skip-webhook-admission"
+	skipNamespaceLabelValue   = "yes"
+	skippedNamespaceName      = "exempted-namesapce"
+	disallowedPodName         = "disallowed-pod"
+	toBeAttachedPodName       = "to-be-attached-pod"
+	hangingPodName            = "hanging-pod"
+	disallowedConfigMapName   = "disallowed-configmap"
+	nonDeletableConfigmapName = "nondeletable-configmap"
+	allowedConfigMapName      = "allowed-configmap"
+	failNamespaceLabelKey     = "fail-closed-webhook"
+	failNamespaceLabelValue   = "yes"
+	failNamespaceName         = "fail-closed-namesapce"
+	addedLabelKey             = "added-label"
+	addedLabelValue           = "yes"
 )
 
 var serverWebhookVersion = utilversion.MustParseSemantic("v1.8.0")
@@ -136,7 +137,7 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
 		testAttachingPodWebhook(f)
 	})
 
-	ginkgo.It("Should be able to deny custom resource creation", func() {
+	ginkgo.It("Should be able to deny custom resource creation and deletion", func() {
 		testcrd, err := crd.CreateTestCRD(f)
 		if err != nil {
 			return
@@ -145,6 +146,7 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
 		webhookCleanup := registerWebhookForCustomResource(f, context, testcrd)
 		defer webhookCleanup()
 		testCustomResourceWebhook(f, testcrd.Crd, testcrd.DynamicClients["v1"])
+		testBlockingCustomResourceDeletion(f, testcrd.Crd, testcrd.DynamicClients["v1"])
 	})
 
 	ginkgo.It("Should unconditionally reject operations on fail closed webhook", func() {
@@ -458,7 +460,7 @@ func registerWebhook(f *framework.Framework, context *certContext) func() {
 			{
 				Name: "deny-unwanted-configmap-data.k8s.io",
 				Rules: []v1beta1.RuleWithOperations{{
-					Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update},
+					Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update, v1beta1.Delete},
 					Rule: v1beta1.Rule{
 						APIGroups:   []string{""},
 						APIVersions: []string{"v1"},
@@ -788,6 +790,36 @@ func testWebhook(f *framework.Framework) {
 	framework.ExpectNoError(err, "failed to create configmap %s in namespace: %s", configmap.Name, skippedNamespaceName)
 }
 
+func testBlockingConfigmapDeletion(f *framework.Framework) {
+	ginkgo.By("create a configmap that should be denied by the webhook when deleting")
+	client := f.ClientSet
+	configmap := nonDeletableConfigmap(f)
+	_, err := client.CoreV1().ConfigMaps(f.Namespace.Name).Create(configmap)
+	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to create configmap %s in namespace: %s", configmap.Name, f.Namespace.Name)
+
+	ginkgo.By("deleting the configmap should be denied by the webhook")
+	err = client.CoreV1().ConfigMaps(f.Namespace.Name).Delete(configmap.Name, &metav1.DeleteOptions{})
+	gomega.Expect(err).To(gomega.HaveOccurred(), "deleting configmap %s in namespace: %s should be denied", configmap.Name, f.Namespace.Name)
+	expectedErrMsg1 := "the configmap cannot be deleted because it contains unwanted key and value"
+	if !strings.Contains(err.Error(), expectedErrMsg1) {
+		framework.Failf("expect error contains %q, got %q", expectedErrMsg1, err.Error())
+	}
+
+	ginkgo.By("remove the offending key and value from the configmap data")
+	toCompliantFn := func(cm *v1.ConfigMap) {
+		if cm.Data == nil {
+			cm.Data = map[string]string{}
+		}
+		cm.Data["webhook-e2e-test"] = "webhook-allow"
+	}
+	_, err = updateConfigMap(client, f.Namespace.Name, configmap.Name, toCompliantFn)
+	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to update configmap %s in namespace: %s", configmap.Name, f.Namespace.Name)
+
+	ginkgo.By("deleting the updated configmap should be successful")
+	err = client.CoreV1().ConfigMaps(f.Namespace.Name).Delete(configmap.Name, &metav1.DeleteOptions{})
+	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to delete configmap %s in namespace: %s", configmap.Name, f.Namespace.Name)
+}
+
 func testAttachingPodWebhook(f *framework.Framework) {
 	ginkgo.By("create a pod")
 	client := f.ClientSet
@@ -1187,6 +1219,17 @@ func nonCompliantConfigMap(f *framework.Framework) *v1.ConfigMap {
 	}
 }
 
+func nonDeletableConfigmap(f *framework.Framework) *v1.ConfigMap {
+	return &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: nonDeletableConfigmapName,
+		},
+		Data: map[string]string{
+			"webhook-e2e-test": "webhook-nondeletable",
+		},
+	}
+}
+
 func toBeMutatedConfigMap(f *framework.Framework) *v1.ConfigMap {
 	return &v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1224,6 +1267,28 @@ func updateConfigMap(c clientset.Interface, ns, name string, update updateConfig
 	return cm, pollErr
 }
 
+type updateCustomResourceFn func(cm *unstructured.Unstructured)
+
+func updateCustomResource(c dynamic.ResourceInterface, ns, name string, update updateCustomResourceFn) (*unstructured.Unstructured, error) {
+	var cr *unstructured.Unstructured
+	pollErr := wait.PollImmediate(2*time.Second, 1*time.Minute, func() (bool, error) {
+		var err error
+		if cr, err = c.Get(name, metav1.GetOptions{}); err != nil {
+			return false, err
+		}
+		update(cr)
+		if cr, err = c.Update(cr, metav1.UpdateOptions{}); err == nil {
+			return true, nil
+		}
+		// Only retry update on conflict
+		if !errors.IsConflict(err) {
+			return false, err
+		}
+		return false, nil
+	})
+	return cr, pollErr
+}
+
 func cleanWebhookTest(client clientset.Interface, namespaceName string) {
 	_ = client.CoreV1().Services(namespaceName).Delete(serviceName, nil)
 	_ = client.AppsV1().Deployments(namespaceName).Delete(deploymentName, nil)
@@ -1245,7 +1310,7 @@ func registerWebhookForCustomResource(f *framework.Framework, context *certConte
 			{
 				Name: "deny-unwanted-custom-resource-data.k8s.io",
 				Rules: []v1beta1.RuleWithOperations{{
-					Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update},
+					Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update, v1beta1.Delete},
 					Rule: v1beta1.Rule{
 						APIGroups:   []string{testcrd.Crd.Spec.Group},
 						APIVersions: servedAPIVersions(testcrd.Crd),
@@ -1358,6 +1423,50 @@ func testCustomResourceWebhook(f *framework.Framework, crd *apiextensionsv1beta1
 	}
 }
 
+func testBlockingCustomResourceDeletion(f *framework.Framework, crd *apiextensionsv1beta1.CustomResourceDefinition, customResourceClient dynamic.ResourceInterface) {
+	ginkgo.By("Creating a custom resource whose deletion would be denied by the webhook")
+	crInstanceName := "cr-instance-2"
+	crInstance := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"kind":       crd.Spec.Names.Kind,
+			"apiVersion": crd.Spec.Group + "/" + crd.Spec.Version,
+			"metadata": map[string]interface{}{
+				"name":      crInstanceName,
+				"namespace": f.Namespace.Name,
+			},
+			"data": map[string]interface{}{
+				"webhook-e2e-test": "webhook-nondeletable",
+			},
+		},
+	}
+	_, err := customResourceClient.Create(crInstance, metav1.CreateOptions{})
+	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to create custom resource %s in namespace: %s", crInstanceName, f.Namespace.Name)
+
+	ginkgo.By("Deleting the custom resource should be denied")
+	err = customResourceClient.Delete(crInstanceName, &metav1.DeleteOptions{})
+	gomega.Expect(err).To(gomega.HaveOccurred(), "deleting custom resource %s in namespace: %s should be denied", crInstanceName, f.Namespace.Name)
+	expectedErrMsg1 := "the custom resource cannot be deleted because it contains unwanted key and value"
+	if !strings.Contains(err.Error(), expectedErrMsg1) {
+		framework.Failf("expect error contains %q, got %q", expectedErrMsg1, err.Error())
+	}
+
+	ginkgo.By("Remove the offending key and value from the custom resource data")
+	toCompliantFn := func(cr *unstructured.Unstructured) {
+		if _, ok := cr.Object["data"]; !ok {
+			cr.Object["data"] = map[string]interface{}{}
+		}
+		data := cr.Object["data"].(map[string]interface{})
+		data["webhook-e2e-test"] = "webhook-allow"
+	}
+	_, err = updateCustomResource(customResourceClient, f.Namespace.Name, crInstanceName, toCompliantFn)
+	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to update custom resource %s in namespace: %s", crInstanceName, f.Namespace.Name)
+
+	ginkgo.By("Deleting the updated custom resource should be successful")
+	err = customResourceClient.Delete(crInstanceName, &metav1.DeleteOptions{})
+	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to delete custom resource %s in namespace: %s", crInstanceName, f.Namespace.Name)
+
+}
+
 func testMutatingCustomResourceWebhook(f *framework.Framework, crd *apiextensionsv1beta1.CustomResourceDefinition, customResourceClient dynamic.ResourceInterface, prune bool) {
 	ginkgo.By("Creating a custom resource that should be mutated by the webhook")
 	crName := "cr-instance-1"

test/images/webhook/VERSION

@@ -1 +1 @@
-1.14v1
+1.15v1

test/images/webhook/configmap.go

@@ -41,7 +41,12 @@ func admitConfigMaps(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
 		return nil
 	}
 
-	raw := ar.Request.Object.Raw
+	var raw []byte
+	if ar.Request.Operation == v1beta1.Delete {
+		raw = ar.Request.OldObject.Raw
+	} else {
+		raw = ar.Request.Object.Raw
+	}
 	configmap := corev1.ConfigMap{}
 	deserializer := codecs.UniversalDeserializer()
 	if _, _, err := deserializer.Decode(raw, nil, &configmap); err != nil {
@@ -51,12 +56,19 @@ func admitConfigMaps(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
 	reviewResponse := v1beta1.AdmissionResponse{}
 	reviewResponse.Allowed = true
 	for k, v := range configmap.Data {
-		if k == "webhook-e2e-test" && v == "webhook-disallow" {
+		if k == "webhook-e2e-test" && v == "webhook-disallow" &&
+			(ar.Request.Operation == v1beta1.Create || ar.Request.Operation == v1beta1.Update) {
 			reviewResponse.Allowed = false
 			reviewResponse.Result = &metav1.Status{
 				Reason: "the configmap contains unwanted key and value",
 			}
 		}
+		if k == "webhook-e2e-test" && v == "webhook-nondeletable" && ar.Request.Operation == v1beta1.Delete {
+			reviewResponse.Allowed = false
+			reviewResponse.Result = &metav1.Status{
+				Reason: "the configmap cannot be deleted because it contains unwanted key and value",
+			}
+		}
 	}
 	return &reviewResponse
 }

test/images/webhook/customresource.go

@@ -69,7 +69,12 @@ func admitCustomResource(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse
 		Data map[string]string
 	}{}
 
-	raw := ar.Request.Object.Raw
+	var raw []byte
+	if ar.Request.Operation == v1beta1.Delete {
+		raw = ar.Request.OldObject.Raw
+	} else {
+		raw = ar.Request.Object.Raw
+	}
 	err := json.Unmarshal(raw, &cr)
 	if err != nil {
 		klog.Error(err)
@@ -79,12 +84,19 @@ func admitCustomResource(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse
 	reviewResponse := v1beta1.AdmissionResponse{}
 	reviewResponse.Allowed = true
 	for k, v := range cr.Data {
-		if k == "webhook-e2e-test" && v == "webhook-disallow" {
+		if k == "webhook-e2e-test" && v == "webhook-disallow" &&
+			(ar.Request.Operation == v1beta1.Create || ar.Request.Operation == v1beta1.Update) {
 			reviewResponse.Allowed = false
 			reviewResponse.Result = &metav1.Status{
 				Reason: "the custom resource contains unwanted data",
 			}
 		}
+		if k == "webhook-e2e-test" && v == "webhook-nondeletable" && ar.Request.Operation == v1beta1.Delete {
+			reviewResponse.Allowed = false
+			reviewResponse.Result = &metav1.Status{
+				Reason: "the custom resource cannot be deleted because it contains unwanted key and value",
+			}
+		}
 	}
 	return &reviewResponse
 }

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -527,6 +527,7 @@ func testResourcePatch(c *testContext) {
 }
 
 func testResourceDelete(c *testContext) {
+	// Verify that an immediate delete triggers the webhook and populates the admisssionRequest.oldObject.
 	obj, err := createOrGetResource(c.client, c.gvr, c.resource)
 	if err != nil {
 		c.t.Error(err)
@@ -534,12 +535,13 @@ func testResourceDelete(c *testContext) {
 	}
 	background := metav1.DeletePropagationBackground
 	zero := int64(0)
-	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, false, true)
+	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, true, true)
 	err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background})
 	if err != nil {
 		c.t.Error(err)
 		return
 	}
+	c.admissionHolder.verify(c.t)
 
 	// wait for the item to be gone
 	err = wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) {
@@ -557,6 +559,77 @@ func testResourceDelete(c *testContext) {
 		c.t.Error(err)
 		return
 	}
+
+	// Verify that an update-on-delete triggers the webhook and populates the admisssionRequest.oldObject.
+	obj, err = createOrGetResource(c.client, c.gvr, c.resource)
+	if err != nil {
+		c.t.Error(err)
+		return
+	}
+	// Adding finalizer to the object, then deleting it.
+	// We don't add finalizers by setting DeleteOptions.PropagationPolicy
+	// because some resource (e.g., events) do not support garbage
+	// collector finalizers.
+	_, err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Patch(
+		obj.GetName(),
+		types.MergePatchType,
+		[]byte(`{"metadata":{"finalizers":["test/k8s.io"]}}`),
+		metav1.PatchOptions{})
+	if err != nil {
+		c.t.Error(err)
+		return
+	}
+	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, true, true)
+	err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background})
+	if err != nil {
+		c.t.Error(err)
+		return
+	}
+	c.admissionHolder.verify(c.t)
+
+	// wait other finalizers (e.g., crd's customresourcecleanup finalizer) to be removed.
+	err = wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) {
+		obj, err := c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Get(obj.GetName(), metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		finalizers := obj.GetFinalizers()
+		if len(finalizers) != 1 {
+			c.t.Logf("waiting for other finalizers on %#v %s to be removed, existing finalizers are %v", c.gvr, obj.GetName(), obj.GetFinalizers())
+			return false, nil
+		}
+		if finalizers[0] != "test/k8s.io" {
+			return false, fmt.Errorf("expected the single finalizer on %#v %s to be test/k8s.io, got %v", c.gvr, obj.GetName(), obj.GetFinalizers())
+		}
+		return true, nil
+	})
+
+	// remove the finalizer
+	_, err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Patch(
+		obj.GetName(),
+		types.MergePatchType,
+		[]byte(`{"metadata":{"finalizers":[]}}`),
+		metav1.PatchOptions{})
+	if err != nil {
+		c.t.Error(err)
+		return
+	}
+	// wait for the item to be gone
+	err = wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) {
+		obj, err := c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Get(obj.GetName(), metav1.GetOptions{})
+		if errors.IsNotFound(err) {
+			return true, nil
+		}
+		if err == nil {
+			c.t.Logf("waiting for %#v to be deleted (name: %s, finalizers: %v)...\n", c.gvr, obj.GetName(), obj.GetFinalizers())
+			return false, nil
+		}
+		return false, err
+	})
+	if err != nil {
+		c.t.Error(err)
+		return
+	}
 }
 
 func testResourceDeletecollection(c *testContext) {
@@ -580,7 +653,7 @@ func testResourceDeletecollection(c *testContext) {
 	}
 
 	// set expectations
-	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, "", obj.GetNamespace(), false, false, true)
+	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, "", obj.GetNamespace(), false, true, true)
 
 	// delete
 	err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).DeleteCollection(&metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background}, metav1.ListOptions{LabelSelector: "webhooktest=true"})
@@ -708,7 +781,7 @@ func testNamespaceDelete(c *testContext) {
 	background := metav1.DeletePropagationBackground
 	zero := int64(0)
 
-	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, false, true)
+	c.admissionHolder.expect(c.gvr, gvk(c.resource.Group, c.resource.Version, c.resource.Kind), gvkDeleteOptions, v1beta1.Delete, obj.GetName(), obj.GetNamespace(), false, true, true)
 	err = c.client.Resource(c.gvr).Namespace(obj.GetNamespace()).Delete(obj.GetName(), &metav1.DeleteOptions{GracePeriodSeconds: &zero, PropagationPolicy: &background})
 	if err != nil {
 		c.t.Error(err)

test/utils/image/manifest.go

@@ -196,7 +196,7 @@ const (
 func initImageConfigs() map[int]Config {
 	configs := map[int]Config{}
 	configs[CRDConversionWebhook] = Config{e2eRegistry, "crd-conversion-webhook", "1.13rev2"}
-	configs[AdmissionWebhook] = Config{e2eRegistry, "webhook", "1.14v1"}
+	configs[AdmissionWebhook] = Config{e2eRegistry, "webhook", "1.15v1"}
 	configs[Agnhost] = Config{e2eRegistry, "agnhost", "1.0"}
 	configs[APIServer] = Config{e2eRegistry, "sample-apiserver", "1.10"}
 	configs[AppArmorLoader] = Config{e2eRegistry, "apparmor-loader", "1.0"}