Merge pull request #67099 from jennybuckley/dry-run-admission-3

Automatic merge from submit-queue (batch tested with PRs 67396, 67097, 67395, 67365, 67099). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add unit tests for webhooks with dry run **What this PR does / why we need it**: Fixes an issue with kubernetes/kubernetes#67085 and adds a couple test cases that would catch it. @lavalamp **Release note**: ```release-note NONE ``` Kubernetes-commit: c582a37cae02b4d1a850e6668ea8ea0a81dcd204
kubernetes · Sep 6, 2018 · 94e563a · 94e563a
2 parents e4aa942 + 2010d2a
commit 94e563a
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 13 deletions.
diff --git a/pkg/admission/plugin/webhook/mutating/dispatcher.go b/pkg/admission/plugin/webhook/mutating/dispatcher.go
@@ -84,7 +84,7 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr *generic.Version
 func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta1.Webhook, attr *generic.VersionedAttributes) error {
 	if attr.IsDryRun() {
 		// TODO: support this
-		webhookerrors.NewDryRunUnsupportedErr(h.Name)
+		return webhookerrors.NewDryRunUnsupportedErr(h.Name)
 	}
 
 	// Make the webhook request

diff --git a/pkg/admission/plugin/webhook/mutating/plugin_test.go b/pkg/admission/plugin/webhook/mutating/plugin_test.go
@@ -79,9 +79,9 @@ func TestAdmit(t *testing.T) {
 
 		var attr admission.Attributes
 		if tt.IsCRD {
-			attr = webhooktesting.NewAttributeUnstructured(ns, tt.AdditionalLabels)
+			attr = webhooktesting.NewAttributeUnstructured(ns, tt.AdditionalLabels, tt.IsDryRun)
 		} else {
-			attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels)
+			attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels, tt.IsDryRun)
 		}
 
 		err = wh.Admit(attr)
@@ -147,7 +147,7 @@ func TestAdmitCachedClient(t *testing.T) {
 			continue
 		}
 
-		err = wh.Admit(webhooktesting.NewAttribute(ns, nil))
+		err = wh.Admit(webhooktesting.NewAttribute(ns, nil, false))
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}

diff --git a/pkg/admission/plugin/webhook/testing/testcase.go b/pkg/admission/plugin/webhook/testing/testcase.go
@@ -76,7 +76,7 @@ func NewFakeDataSource(name string, webhooks []registrationv1beta1.Webhook, muta
 	return client, informerFactory
 }
 
-func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string) admission.Attributes {
+func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string, dryRun bool) admission.Attributes {
 	object.SetName(name)
 	object.SetNamespace(namespace)
 	objectLabels := map[string]string{resource + ".name": name}
@@ -95,11 +95,11 @@ func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind sch
 		UID:  "webhook-test",
 	}
 
-	return admission.NewAttributesRecord(object.(runtime.Object), oldObject.(runtime.Object), kind, namespace, name, gvr, subResource, admission.Update, false, &userInfo)
+	return admission.NewAttributesRecord(object.(runtime.Object), oldObject.(runtime.Object), kind, namespace, name, gvr, subResource, admission.Update, dryRun, &userInfo)
 }
 
 // NewAttribute returns static admission Attributes for testing.
-func NewAttribute(namespace string, labels map[string]string) admission.Attributes {
+func NewAttribute(namespace string, labels map[string]string, dryRun bool) admission.Attributes {
 	// Set up a test object for the call
 	object := corev1.Pod{
 		TypeMeta: metav1.TypeMeta{
@@ -111,11 +111,11 @@ func NewAttribute(namespace string, labels map[string]string) admission.Attribut
 	kind := corev1.SchemeGroupVersion.WithKind("Pod")
 	name := "my-pod"
 
-	return newAttributesRecord(&object, &oldObject, kind, namespace, name, "pod", labels)
+	return newAttributesRecord(&object, &oldObject, kind, namespace, name, "pod", labels, dryRun)
 }
 
 // NewAttributeUnstructured returns static admission Attributes for testing with custom resources.
-func NewAttributeUnstructured(namespace string, labels map[string]string) admission.Attributes {
+func NewAttributeUnstructured(namespace string, labels map[string]string, dryRun bool) admission.Attributes {
 	// Set up a test object for the call
 	object := unstructured.Unstructured{}
 	object.SetKind("TestCRD")
@@ -126,7 +126,7 @@ func NewAttributeUnstructured(namespace string, labels map[string]string) admiss
 	kind := object.GroupVersionKind()
 	name := "my-test-crd"
 
-	return newAttributesRecord(&object, &oldObject, kind, namespace, name, "crd", labels)
+	return newAttributesRecord(&object, &oldObject, kind, namespace, name, "crd", labels, dryRun)
 }
 
 type urlConfigGenerator struct {
@@ -149,6 +149,7 @@ type Test struct {
 	Webhooks         []registrationv1beta1.Webhook
 	Path             string
 	IsCRD            bool
+	IsDryRun         bool
 	AdditionalLabels map[string]string
 	ExpectLabels     map[string]string
 	ExpectAllow      bool
@@ -349,6 +350,30 @@ func NewNonMutatingTestCases(url *url.URL) []Test {
 			}},
 			ErrorContains: "Webhook response was absent",
 		},
+		{
+			Name: "no match dry run",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:         "nomatch",
+				ClientConfig: ccfgSVC("disallow"),
+				Rules: []registrationv1beta1.RuleWithOperations{{
+					Operations: []registrationv1beta1.OperationType{registrationv1beta1.Create},
+				}},
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsDryRun:    true,
+			ExpectAllow: true,
+		},
+		{
+			Name: "match dry run",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "allow",
+				ClientConfig:      ccfgSVC("allow"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsDryRun:      true,
+			ErrorContains: "does not support dry run",
+		},
 		// No need to test everything with the url case, since only the
 		// connection is different.
 	}
@@ -417,6 +442,17 @@ func NewMutatingTestCases(url *url.URL) []Test {
 			}},
 			ErrorContains: "invalid character",
 		},
+		{
+			Name: "match & remove label dry run",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "removeLabel",
+				ClientConfig:      ccfgSVC("removeLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsDryRun:      true,
+			ErrorContains: "does not support dry run",
+		},
 		// No need to test everything with the url case, since only the
 		// connection is different.
 	}

diff --git a/pkg/admission/plugin/webhook/validating/dispatcher.go b/pkg/admission/plugin/webhook/validating/dispatcher.go
@@ -99,7 +99,7 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr *generic.Versi
 func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook, attr *generic.VersionedAttributes) error {
 	if attr.IsDryRun() {
 		// TODO: support this
-		webhookerrors.NewDryRunUnsupportedErr(h.Name)
+		return webhookerrors.NewDryRunUnsupportedErr(h.Name)
 	}
 
 	// Make the webhook request

diff --git a/pkg/admission/plugin/webhook/validating/plugin_test.go b/pkg/admission/plugin/webhook/validating/plugin_test.go
@@ -72,7 +72,7 @@ func TestValidate(t *testing.T) {
 			continue
 		}
 
-		err = wh.Validate(webhooktesting.NewAttribute(ns, nil))
+		err = wh.Validate(webhooktesting.NewAttribute(ns, nil, tt.IsDryRun))
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}
@@ -130,7 +130,7 @@ func TestValidateCachedClient(t *testing.T) {
 			continue
 		}
 
-		err = wh.Validate(webhooktesting.NewAttribute(ns, nil))
+		err = wh.Validate(webhooktesting.NewAttribute(ns, nil, false))
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}