From 0511e4e41d20268d24bdb472c35148457a27a54b Mon Sep 17 00:00:00 2001 From: Cao Shufeng Date: Mon, 4 Jun 2018 14:55:46 +0800 Subject: [PATCH] fix a TODO in ValidatingAdmissionWebhook Kubernetes-commit: 162499515c0813f579770091dc30925207d063b2 --- .../plugin/webhook/mutating/plugin_test.go | 5 +- .../plugin/webhook/testing/testcase.go | 133 ++++++++++-------- .../plugin/webhook/validating/plugin_test.go | 7 +- 3 files changed, 78 insertions(+), 67 deletions(-) diff --git a/pkg/admission/plugin/webhook/mutating/plugin_test.go b/pkg/admission/plugin/webhook/mutating/plugin_test.go index 12715d5dd..ba8f44a66 100644 --- a/pkg/admission/plugin/webhook/mutating/plugin_test.go +++ b/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) diff --git a/pkg/admission/plugin/webhook/testing/testcase.go b/pkg/admission/plugin/webhook/testing/testcase.go index 96d696788..e2bb01bd6 100644 --- a/pkg/admission/plugin/webhook/testing/testcase.go +++ b/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. -func NewTestCases(url *url.URL) []Test { +// NewNonMutatingTestCases returns test cases with a given base url. +// 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 diff --git a/pkg/admission/plugin/webhook/validating/plugin_test.go b/pkg/admission/plugin/webhook/validating/plugin_test.go index a616e9f0f..95b7d77bd 100644 --- a/pkg/admission/plugin/webhook/validating/plugin_test.go +++ b/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) { - // TODO: re-enable all tests - if !strings.Contains(tt.Name, "no match") { - continue - } - + for _, tt := range webhooktesting.NewNonMutatingTestCases(serverURL) { wh, err := NewValidatingAdmissionWebhook(nil) if err != nil { t.Errorf("%s: failed to create validating webhook: %v", tt.Name, err)