From 43c95c4fce86995b763cefe91d63fb763bab8079 Mon Sep 17 00:00:00 2001 From: awgreene Date: Mon, 21 Dec 2020 16:40:57 -0500 Subject: [PATCH] Reduce e2e test failure rate This commit introduces three changes to the e2e test suite: 1. It reduces the failure rate of the CSV e2e test by allowing the operator to reach a succeeded install before searching for resources. 2. An installplan e2e test was failing a diff check against two installplans because the operator controller was applying a component label to one of the installplans. An update was made to still compare the spec and status of the two installplans but ignores other fields. 3. A test that ensures OLM elevates scoped permissions to cluster levels when the CSV is installed in a global OperatorGroup was not elevating them because all of the permissions defined in the role were already provided by a clusterrole defined in the csv. The test was sped up significantly by making the rules defined in the CSV's spec.Permissions field different from those defined in the spec.ClusterPermissions field. --- test/e2e/csv_e2e_test.go | 34 ++++++++++++++------- test/e2e/installplan_e2e_test.go | 51 +++++++++++++++----------------- 2 files changed, 48 insertions(+), 37 deletions(-) diff --git a/test/e2e/csv_e2e_test.go b/test/e2e/csv_e2e_test.go index 86680af7dfe..0a6a8385052 100644 --- a/test/e2e/csv_e2e_test.go +++ b/test/e2e/csv_e2e_test.go @@ -1306,28 +1306,42 @@ var _ = Describe("ClusterServiceVersion", func() { Expect(err).ShouldNot(HaveOccurred(), "error getting expected APIService") // Should create Service - _, err = c.GetService(testNamespace, serviceName) - Expect(err).ShouldNot(HaveOccurred(), "error getting expected Service") + Eventually(func() error { + _, err := c.GetService(testNamespace, serviceName) + return err + }, timeout, interval).ShouldNot(HaveOccurred()) // Should create certificate Secret secretName = fmt.Sprintf("%s-cert", serviceName) - _, err = c.GetSecret(testNamespace, secretName) - Expect(err).ShouldNot(HaveOccurred(), "error getting expected Secret") + Eventually(func() error { + _, err = c.GetSecret(testNamespace, secretName) + return err + }, timeout, interval).ShouldNot(HaveOccurred()) // Should create a Role for the Secret _, err = c.GetRole(testNamespace, secretName) - Expect(err).ShouldNot(HaveOccurred(), "error getting expected Secret Role") + Eventually(func() error { + _, err = c.GetRole(testNamespace, secretName) + return err + }, timeout, interval).ShouldNot(HaveOccurred()) // Should create a RoleBinding for the Secret - _, err = c.GetRoleBinding(testNamespace, secretName) - Expect(err).ShouldNot(HaveOccurred(), "error getting exptected Secret RoleBinding") + Eventually(func() error { + _, err = c.GetRoleBinding(testNamespace, secretName) + return err + }, timeout, interval).ShouldNot(HaveOccurred()) // Should create a system:auth-delegator Cluster RoleBinding - _, err = c.GetClusterRoleBinding(fmt.Sprintf("%s-system:auth-delegator", serviceName)) - Expect(err).ShouldNot(HaveOccurred(), "error getting expected system:auth-delegator ClusterRoleBinding") + Eventually(func() error { + _, err = c.GetClusterRoleBinding(fmt.Sprintf("%s-system:auth-delegator", serviceName)) + return err + }, timeout, interval).ShouldNot(HaveOccurred()) // Should create an extension-apiserver-authentication-reader RoleBinding in kube-system - _, err = c.GetRoleBinding("kube-system", fmt.Sprintf("%s-auth-reader", serviceName)) + Eventually(func() error { + _, err = c.GetRoleBinding("kube-system", fmt.Sprintf("%s-auth-reader", serviceName)) + return err + }, timeout, interval).ShouldNot(HaveOccurred()) Expect(err).ShouldNot(HaveOccurred(), "error getting expected extension-apiserver-authentication-reader RoleBinding") // Should eventually GC the CSV diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 692ead6c6e4..513fa8fe6e2 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -222,11 +222,12 @@ var _ = Describe("Install Plan", func() { // Fetch installplan again to check for unnecessary control loops fetchedInstallPlan, err = fetchInstallPlan(GinkgoT(), crc, fetchedInstallPlan.GetName(), func(fip *operatorsv1alpha1.InstallPlan) bool { - compareResources(GinkgoT(), fetchedInstallPlan, fip) + // Don't compare object meta as labels can be applied by the operator controller. + compareResources(GinkgoT(), fetchedInstallPlan.Spec, fip.Spec) + compareResources(GinkgoT(), fetchedInstallPlan.Status, fip.Status) return true }) require.NoError(GinkgoT(), err) - require.Equal(GinkgoT(), len(expectedStepSources), len(fetchedInstallPlan.Status.Plan), "Number of resolved steps matches the number of expected steps") // Ensure resolved step resources originate from the correct catalog sources @@ -2111,6 +2112,12 @@ var _ = Describe("Install Plan", func() { APIGroups: []string{"cluster.com"}, Resources: []string{crdPlural}, }, + // Permissions must be different than ClusterPermissions defined below if OLM is going to lift role/rolebindings to cluster level. + { + Verbs: []string{rbac.VerbAll}, + APIGroups: []string{corev1.GroupName}, + Resources: []string{corev1.ResourceConfigMaps.String()}, + }, }, }, } @@ -2205,6 +2212,7 @@ var _ = Describe("Install Plan", func() { } return true, nil }) + require.NoError(GinkgoT(), err) } if step.Resource.Kind == "RoleBinding" { err = wait.Poll(pollInterval, pollDuration, func() (bool, error) { @@ -2217,6 +2225,7 @@ var _ = Describe("Install Plan", func() { } return true, nil }) + require.NoError(GinkgoT(), err) } } @@ -2238,16 +2247,10 @@ var _ = Describe("Install Plan", func() { GinkgoT().Logf("Monitoring cluster role binding %v", binding.GetName()) } - // can't query by owner reference, so just use the name we know is in the install plan - createdServiceAccountNames := map[string]struct{}{serviceAccountName: {}} - GinkgoT().Logf("Monitoring service account %v", serviceAccountName) - crWatcher, err := c.KubernetesInterface().RbacV1().ClusterRoles().Watch(context.TODO(), metav1.ListOptions{LabelSelector: fmt.Sprintf("%v=%v", ownerutil.OwnerKey, stableCSVName)}) require.NoError(GinkgoT(), err) crbWatcher, err := c.KubernetesInterface().RbacV1().ClusterRoleBindings().Watch(context.TODO(), metav1.ListOptions{LabelSelector: fmt.Sprintf("%v=%v", ownerutil.OwnerKey, stableCSVName)}) require.NoError(GinkgoT(), err) - saWatcher, err := c.KubernetesInterface().CoreV1().ServiceAccounts(testNamespace).Watch(context.TODO(), metav1.ListOptions{}) - require.NoError(GinkgoT(), err) done := make(chan struct{}) errExit := make(chan error) @@ -2266,7 +2269,7 @@ var _ = Describe("Install Plan", func() { continue } delete(createdClusterRoleNames, cr.GetName()) - if len(createdClusterRoleNames) == 0 && len(createdClusterRoleBindingNames) == 0 && len(createdServiceAccountNames) == 0 { + if len(createdClusterRoleNames) == 0 && len(createdClusterRoleBindingNames) == 0 { done <- struct{}{} return } @@ -2282,23 +2285,7 @@ var _ = Describe("Install Plan", func() { continue } delete(createdClusterRoleBindingNames, crb.GetName()) - if len(createdClusterRoleNames) == 0 && len(createdClusterRoleBindingNames) == 0 && len(createdServiceAccountNames) == 0 { - done <- struct{}{} - return - } - } - case evt, ok := <-saWatcher.ResultChan(): - if !ok { - errExit <- errors.New("sa watch channel closed unexpectedly") - return - } - if evt.Type == watch.Deleted { - sa, ok := evt.Object.(*corev1.ServiceAccount) - if !ok { - continue - } - delete(createdServiceAccountNames, sa.GetName()) - if len(createdClusterRoleNames) == 0 && len(createdClusterRoleBindingNames) == 0 && len(createdServiceAccountNames) == 0 { + if len(createdClusterRoleNames) == 0 && len(createdClusterRoleBindingNames) == 0 { done <- struct{}{} return } @@ -2320,7 +2307,17 @@ var _ = Describe("Install Plan", func() { require.Emptyf(GinkgoT(), createdClusterRoleNames, "unexpected cluster role remain: %v", createdClusterRoleNames) require.Emptyf(GinkgoT(), createdClusterRoleBindingNames, "unexpected cluster role binding remain: %v", createdClusterRoleBindingNames) - require.Emptyf(GinkgoT(), createdServiceAccountNames, "unexpected service account remain: %v", createdServiceAccountNames) + + Eventually(func() error { + _, err := c.GetServiceAccount(testNamespace, serviceAccountName) + if err == nil { + return fmt.Errorf("The %v/%v ServiceAccount should have been deleted", testNamespace, serviceAccountName) + } + if !k8serrors.IsNotFound(err) { + return err + } + return nil + }, timeout, interval).Should(BeNil()) }) It("CRD validation", func() {