From 0bdf29801c6345ce797c247a6b3e8ca36b6b87e2 Mon Sep 17 00:00:00 2001 From: Walter Fender Date: Mon, 29 Jan 2018 18:07:28 -0800 Subject: [PATCH] Fix flaky AdmissionWebhook e2e-crd tests Several of the tests("It") in the e2e suite reuse the CRD. However they each try to setup and tear down the CRD independently. Since these tests can be running in parallel, causing intermittant failures. Added a new framework utility for creating CRDs per test. Then making the relevant tests use the utility to prevent name collision/race. Fixed bazel build. Factored in fixes for @caesarxuchao Making suggested change for @janetkuo --- test/e2e/apimachinery/webhook.go | 107 +++++++------------------- test/e2e/framework/BUILD | 4 + test/e2e/framework/crd_util.go | 126 +++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 79 deletions(-) create mode 100644 test/e2e/framework/crd_util.go diff --git a/test/e2e/apimachinery/webhook.go b/test/e2e/apimachinery/webhook.go index ab00fca6d0..a139bf8911 100644 --- a/test/e2e/apimachinery/webhook.go +++ b/test/e2e/apimachinery/webhook.go @@ -27,8 +27,6 @@ import ( extensions "k8s.io/api/extensions/v1beta1" rbacv1beta1 "k8s.io/api/rbac/v1beta1" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" - crdclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" - "k8s.io/apiextensions-apiserver/test/integration/testserver" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -67,10 +65,6 @@ const ( hangingPodName = "hanging-pod" disallowedConfigMapName = "disallowed-configmap" allowedConfigMapName = "allowed-configmap" - crdName = "e2e-test-webhook-crd" - crdKind = "E2e-test-webhook-crd" - crdAPIGroup = "webhook-crd-test.k8s.io" - crdAPIVersion = "v1" failNamespaceLabelKey = "fail-closed-webhook" failNamespaceLabelValue = "yes" failNamespaceName = "fail-closed-namesapce" @@ -107,6 +101,7 @@ var _ = SIGDescribe("AdmissionWebhook", func() { // the development 1.9 cycle. deployWebhookAndService(f, imageutils.GetE2EImage(imageutils.AdmissionWebhook), context) }) + AfterEach(func() { cleanWebhookTest(client, namespaceName) }) @@ -118,11 +113,14 @@ var _ = SIGDescribe("AdmissionWebhook", func() { }) It("Should be able to deny custom resource creation", func() { - crdCleanup, dynamicClient := createCRD(f) - defer crdCleanup() - webhookCleanup := registerWebhookForCRD(f, context) + testcrd, err := framework.CreateTestCRD(f) + if err != nil { + return + } + defer testcrd.CleanUp() + webhookCleanup := registerWebhookForCRD(f, context, testcrd) defer webhookCleanup() - testCRDWebhook(f, dynamicClient) + testCRDWebhook(f, testcrd.Crd, testcrd.DynamicClient) }) It("Should unconditionally reject operations on fail closed webhook", func() { @@ -144,11 +142,14 @@ var _ = SIGDescribe("AdmissionWebhook", func() { }) It("Should mutate crd", func() { - crdCleanup, dynamicClient := createCRD(f) - defer crdCleanup() - webhookCleanup := registerMutatingWebhookForCRD(f, context) + testcrd, err := framework.CreateTestCRD(f) + if err != nil { + return + } + defer testcrd.CleanUp() + webhookCleanup := registerMutatingWebhookForCRD(f, context, testcrd) defer webhookCleanup() - testMutatingCRDWebhook(f, dynamicClient) + testMutatingCRDWebhook(f, testcrd.Crd, testcrd.DynamicClient) }) // TODO: add more e2e tests for mutating webhooks @@ -814,57 +815,7 @@ func cleanWebhookTest(client clientset.Interface, namespaceName string) { _ = client.RbacV1beta1().RoleBindings("kube-system").Delete(roleBindingName, nil) } -// newCRDForAdmissionWebhookTest generates a CRD -func newCRDForAdmissionWebhookTest() *apiextensionsv1beta1.CustomResourceDefinition { - return &apiextensionsv1beta1.CustomResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{Name: crdName + "s." + crdAPIGroup}, - Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ - Group: crdAPIGroup, - Version: crdAPIVersion, - Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ - Plural: crdName + "s", - Singular: crdName, - Kind: crdKind, - ListKind: crdName + "List", - }, - Scope: apiextensionsv1beta1.NamespaceScoped, - }, - } -} - -func createCRD(f *framework.Framework) (func(), dynamic.ResourceInterface) { - config, err := framework.LoadConfig() - if err != nil { - framework.Failf("failed to load config: %v", err) - } - - apiExtensionClient, err := crdclientset.NewForConfig(config) - if err != nil { - framework.Failf("failed to initialize apiExtensionClient: %v", err) - } - - crd := newCRDForAdmissionWebhookTest() - - //create CRD and waits for the resource to be recognized and available. - dynamicClient, err := testserver.CreateNewCustomResourceDefinitionWatchUnsafe(crd, apiExtensionClient, f.ClientPool) - if err != nil { - framework.Failf("failed to create CustomResourceDefinition: %v", err) - } - - resourceClient := dynamicClient.Resource(&metav1.APIResource{ - Name: crd.Spec.Names.Plural, - Namespaced: true, - }, f.Namespace.Name) - - return func() { - err = testserver.DeleteCustomResourceDefinition(crd, apiExtensionClient) - if err != nil { - framework.Failf("failed to delete CustomResourceDefinition: %v", err) - } - }, resourceClient -} - -func registerWebhookForCRD(f *framework.Framework, context *certContext) func() { +func registerWebhookForCRD(f *framework.Framework, context *certContext, testcrd *framework.TestCrd) func() { client := f.ClientSet By("Registering the crd webhook via the AdmissionRegistration API") @@ -880,9 +831,9 @@ func registerWebhookForCRD(f *framework.Framework, context *certContext) func() Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{v1beta1.Create}, Rule: v1beta1.Rule{ - APIGroups: []string{crdAPIGroup}, - APIVersions: []string{crdAPIVersion}, - Resources: []string{crdName + "s"}, + APIGroups: []string{testcrd.ApiGroup}, + APIVersions: []string{testcrd.ApiVersion}, + Resources: []string{testcrd.GetPluralName()}, }, }}, ClientConfig: v1beta1.WebhookClientConfig{ @@ -905,7 +856,7 @@ func registerWebhookForCRD(f *framework.Framework, context *certContext) func() } } -func registerMutatingWebhookForCRD(f *framework.Framework, context *certContext) func() { +func registerMutatingWebhookForCRD(f *framework.Framework, context *certContext, testcrd *framework.TestCrd) func() { client := f.ClientSet By("Registering the mutating webhook for crd via the AdmissionRegistration API") @@ -921,9 +872,9 @@ func registerMutatingWebhookForCRD(f *framework.Framework, context *certContext) Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{v1beta1.Create}, Rule: v1beta1.Rule{ - APIGroups: []string{crdAPIGroup}, - APIVersions: []string{crdAPIVersion}, - Resources: []string{crdName + "s"}, + APIGroups: []string{testcrd.ApiGroup}, + APIVersions: []string{testcrd.ApiVersion}, + Resources: []string{testcrd.GetPluralName()}, }, }}, ClientConfig: v1beta1.WebhookClientConfig{ @@ -940,9 +891,9 @@ func registerMutatingWebhookForCRD(f *framework.Framework, context *certContext) Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{v1beta1.Create}, Rule: v1beta1.Rule{ - APIGroups: []string{crdAPIGroup}, - APIVersions: []string{crdAPIVersion}, - Resources: []string{crdName + "s"}, + APIGroups: []string{testcrd.ApiGroup}, + APIVersions: []string{testcrd.ApiVersion}, + Resources: []string{testcrd.GetPluralName()}, }, }}, ClientConfig: v1beta1.WebhookClientConfig{ @@ -964,9 +915,8 @@ func registerMutatingWebhookForCRD(f *framework.Framework, context *certContext) return func() { client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(configName, nil) } } -func testCRDWebhook(f *framework.Framework, crdClient dynamic.ResourceInterface) { +func testCRDWebhook(f *framework.Framework, crd *apiextensionsv1beta1.CustomResourceDefinition, crdClient dynamic.ResourceInterface) { By("Creating a custom resource that should be denied by the webhook") - crd := newCRDForAdmissionWebhookTest() crInstance := &unstructured.Unstructured{ Object: map[string]interface{}{ "kind": crd.Spec.Names.Kind, @@ -988,9 +938,8 @@ func testCRDWebhook(f *framework.Framework, crdClient dynamic.ResourceInterface) } } -func testMutatingCRDWebhook(f *framework.Framework, crdClient dynamic.ResourceInterface) { +func testMutatingCRDWebhook(f *framework.Framework, crd *apiextensionsv1beta1.CustomResourceDefinition, crdClient dynamic.ResourceInterface) { By("Creating a custom resource that should be mutated by the webhook") - crd := newCRDForAdmissionWebhookTest() cr := &unstructured.Unstructured{ Object: map[string]interface{}{ "kind": crd.Spec.Names.Kind, diff --git a/test/e2e/framework/BUILD b/test/e2e/framework/BUILD index b8a0567417..a5cebe8098 100644 --- a/test/e2e/framework/BUILD +++ b/test/e2e/framework/BUILD @@ -10,6 +10,7 @@ go_library( srcs = [ "authorizer_util.go", "cleanup.go", + "crd_util.go", "deployment_util.go", "exec_util.go", "firewall_util.go", @@ -110,6 +111,9 @@ go_library( "//vendor/k8s.io/api/extensions/v1beta1:go_default_library", "//vendor/k8s.io/api/policy/v1beta1:go_default_library", "//vendor/k8s.io/api/rbac/v1beta1:go_default_library", + "//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", + "//vendor/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset:go_default_library", + "//vendor/k8s.io/apiextensions-apiserver/test/integration/testserver:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", diff --git a/test/e2e/framework/crd_util.go b/test/e2e/framework/crd_util.go new file mode 100644 index 0000000000..8fb19abbc4 --- /dev/null +++ b/test/e2e/framework/crd_util.go @@ -0,0 +1,126 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package framework + +import ( + "fmt" + + apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + crdclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + "k8s.io/apiextensions-apiserver/test/integration/testserver" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/dynamic" +) + +// CleanCrdFn declares the clean up function needed to remove the CRD +type CleanCrdFn func() error + +// TestCrd holds all the pieces needed to test with the CRD +type TestCrd struct { + Name string + Kind string + ApiGroup string + ApiVersion string + ApiExtensionClient *crdclientset.Clientset + Crd *apiextensionsv1beta1.CustomResourceDefinition + DynamicClient dynamic.ResourceInterface + CleanUp CleanCrdFn +} + +// CreateTestCRD creates a new CRD specifically for the calling test. +func CreateTestCRD(f *Framework) (*TestCrd, error) { + suffix := randomSuffix() + name := fmt.Sprintf("e2e-test-%s-%s-crd", f.BaseName, suffix) + kind := fmt.Sprintf("E2e-test-%s-%s-crd", f.BaseName, suffix) + group := fmt.Sprintf("%s-crd-test.k8s.io", f.BaseName) + apiVersion := "v1" + testcrd := &TestCrd{ + Name: name, + Kind: kind, + ApiGroup: group, + ApiVersion: apiVersion, + } + + // Creating a custom resource definition for use by assorted tests. + config, err := LoadConfig() + if err != nil { + Failf("failed to load config: %v", err) + return nil, err + } + apiExtensionClient, err := crdclientset.NewForConfig(config) + if err != nil { + Failf("failed to initialize apiExtensionClient: %v", err) + return nil, err + } + crd := newCRDForTest(testcrd) + + //create CRD and waits for the resource to be recognized and available. + dynamicClient, err := testserver.CreateNewCustomResourceDefinitionWatchUnsafe(crd, apiExtensionClient, f.ClientPool) + if err != nil { + Failf("failed to create CustomResourceDefinition: %v", err) + return nil, err + } + resourceClient := dynamicClient.Resource(&metav1.APIResource{ + Name: crd.Spec.Names.Plural, + Namespaced: true, + }, f.Namespace.Name) + + testcrd.ApiExtensionClient = apiExtensionClient + testcrd.Crd = crd + testcrd.DynamicClient = resourceClient + testcrd.CleanUp = func() error { + err := testserver.DeleteCustomResourceDefinition(crd, apiExtensionClient) + if err != nil { + Failf("failed to delete CustomResourceDefinition(%s): %v", name, err) + } + return err + } + return testcrd, nil +} + +// newCRDForTest generates a CRD definition for the test +func newCRDForTest(testcrd *TestCrd) *apiextensionsv1beta1.CustomResourceDefinition { + return &apiextensionsv1beta1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: testcrd.GetMetaName()}, + Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ + Group: testcrd.ApiGroup, + Version: testcrd.ApiVersion, + Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ + Plural: testcrd.GetPluralName(), + Singular: testcrd.Name, + Kind: testcrd.Kind, + ListKind: testcrd.GetListName(), + }, + Scope: apiextensionsv1beta1.NamespaceScoped, + }, + } +} + +// GetMetaName returns the metaname for the CRD. +func (c *TestCrd) GetMetaName() string { + return c.Name + "s." + c.ApiGroup +} + +// GetPluralName returns the plural form of the CRD name +func (c *TestCrd) GetPluralName() string { + return c.Name + "s" +} + +// GetListName returns the name for the CRD list resources +func (c *TestCrd) GetListName() string { + return c.Name + "List" +}