Merge pull request #64681 from CaoShuFeng/webhook_validating_test_case

Automatic merge from submit-queue (batch tested with PRs 64681, 65907). 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>.

fix a TODO in ValidatingAdmissionWebhook

Tests still passes without this filter.


**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:
/assign @sttts 

**Release note**:

```release-note
NONE
```

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go

@@ -50,7 +50,10 @@ func TestAdmit(t *testing.T) {
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 
-	for _, tt := range webhooktesting.NewTestCases(serverURL) {
+	testCases := append(webhooktesting.NewMutatingTestCases(serverURL),
+		webhooktesting.NewNonMutatingTestCases(serverURL)...)
+
+	for _, tt := range testCases {
 		wh, err := NewMutatingWebhook(nil)
 		if err != nil {
 			t.Errorf("%s: failed to create mutating webhook: %v", tt.Name, err)

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

@@ -155,8 +155,11 @@ type Test struct {
 	ErrorContains    string
 }
 
-// NewTestCases returns test cases with a given base url.
+// NewNonMutatingTestCases returns test cases with a given base url.
-func NewTestCases(url *url.URL) []Test {
+// All test cases in NewNonMutatingTestCases have no Patch set in
+// AdmissionResponse. The test cases are used by both MutatingAdmissionWebhook
+// and ValidatingAdmissionWebhook.
+func NewNonMutatingTestCases(url *url.URL) []Test {
 	policyFail := registrationv1beta1.Fail
 	policyIgnore := registrationv1beta1.Ignore
 	ccfgURL := urlConfigGenerator{url}.ccfgURL
@@ -184,64 +187,6 @@ func NewTestCases(url *url.URL) []Test {
 			}},
 			ExpectAllow: true,
 		},
-		{
-			Name: "match & remove label",
-			Webhooks: []registrationv1beta1.Webhook{{
-				Name:              "removeLabel",
-				ClientConfig:      ccfgSVC("removeLabel"),
-				Rules:             matchEverythingRules,
-				NamespaceSelector: &metav1.LabelSelector{},
-			}},
-			ExpectAllow:      true,
-			AdditionalLabels: map[string]string{"remove": "me"},
-			ExpectLabels:     map[string]string{"pod.name": "my-pod"},
-		},
-		{
-			Name: "match & add label",
-			Webhooks: []registrationv1beta1.Webhook{{
-				Name:              "addLabel",
-				ClientConfig:      ccfgSVC("addLabel"),
-				Rules:             matchEverythingRules,
-				NamespaceSelector: &metav1.LabelSelector{},
-			}},
-			ExpectAllow:  true,
-			ExpectLabels: map[string]string{"pod.name": "my-pod", "added": "test"},
-		},
-		{
-			Name: "match CRD & add label",
-			Webhooks: []registrationv1beta1.Webhook{{
-				Name:              "addLabel",
-				ClientConfig:      ccfgSVC("addLabel"),
-				Rules:             matchEverythingRules,
-				NamespaceSelector: &metav1.LabelSelector{},
-			}},
-			IsCRD:        true,
-			ExpectAllow:  true,
-			ExpectLabels: map[string]string{"crd.name": "my-test-crd", "added": "test"},
-		},
-		{
-			Name: "match CRD & remove label",
-			Webhooks: []registrationv1beta1.Webhook{{
-				Name:              "removeLabel",
-				ClientConfig:      ccfgSVC("removeLabel"),
-				Rules:             matchEverythingRules,
-				NamespaceSelector: &metav1.LabelSelector{},
-			}},
-			IsCRD:            true,
-			ExpectAllow:      true,
-			AdditionalLabels: map[string]string{"remove": "me"},
-			ExpectLabels:     map[string]string{"crd.name": "my-test-crd"},
-		},
-		{
-			Name: "match & invalid mutation",
-			Webhooks: []registrationv1beta1.Webhook{{
-				Name:              "invalidMutation",
-				ClientConfig:      ccfgSVC("invalidMutation"),
-				Rules:             matchEverythingRules,
-				NamespaceSelector: &metav1.LabelSelector{},
-			}},
-			ErrorContains: "invalid character",
-		},
 		{
 			Name: "match & disallow",
 			Webhooks: []registrationv1beta1.Webhook{{
@@ -409,6 +354,74 @@ func NewTestCases(url *url.URL) []Test {
 	}
 }
 
+// NewMutatingTestCases returns test cases with a given base url.
+// All test cases in NewMutatingTestCases have Patch set in
+// AdmissionResponse. The test cases are only used by both MutatingAdmissionWebhook.
+func NewMutatingTestCases(url *url.URL) []Test {
+	return []Test{
+		{
+			Name: "match & remove label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "removeLabel",
+				ClientConfig:      ccfgSVC("removeLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			ExpectAllow:      true,
+			AdditionalLabels: map[string]string{"remove": "me"},
+			ExpectLabels:     map[string]string{"pod.name": "my-pod"},
+		},
+		{
+			Name: "match & add label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "addLabel",
+				ClientConfig:      ccfgSVC("addLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			ExpectAllow:  true,
+			ExpectLabels: map[string]string{"pod.name": "my-pod", "added": "test"},
+		},
+		{
+			Name: "match CRD & add label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "addLabel",
+				ClientConfig:      ccfgSVC("addLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsCRD:        true,
+			ExpectAllow:  true,
+			ExpectLabels: map[string]string{"crd.name": "my-test-crd", "added": "test"},
+		},
+		{
+			Name: "match CRD & remove label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "removeLabel",
+				ClientConfig:      ccfgSVC("removeLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsCRD:            true,
+			ExpectAllow:      true,
+			AdditionalLabels: map[string]string{"remove": "me"},
+			ExpectLabels:     map[string]string{"crd.name": "my-test-crd"},
+		},
+		{
+			Name: "match & invalid mutation",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "invalidMutation",
+				ClientConfig:      ccfgSVC("invalidMutation"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			ErrorContains: "invalid character",
+		},
+		// No need to test everything with the url case, since only the
+		// connection is different.
+	}
+}
+
 // CachedTest is a test case for the client manager.
 type CachedTest struct {
 	Name            string

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go

@@ -48,12 +48,7 @@ func TestValidate(t *testing.T) {
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 
-	for _, tt := range webhooktesting.NewTestCases(serverURL) {
+	for _, tt := range webhooktesting.NewNonMutatingTestCases(serverURL) {
-		// TODO: re-enable all tests
-		if !strings.Contains(tt.Name, "no match") {
-			continue
-		}
-
 		wh, err := NewValidatingAdmissionWebhook(nil)
 		if err != nil {
 			t.Errorf("%s: failed to create validating webhook: %v", tt.Name, err)