From 962c6992a381ce22572adc1770832f389954a628 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Thu, 3 Aug 2023 20:12:25 -0400 Subject: [PATCH 1/3] test/e2e: Don't use openshift/origin-node Use the "openshift/tools" image from the cluster image registry instead of using the "openshift/origin-node" image pullspec in E2E tests. Before this commit, the E2E tests were inadvertently pulling the "openshift/origin-node" image from Docker Hub and getting rate-limited. The choice to use "openshift/tools" is based on a similar change here: https://github.com/openshift/origin/pull/24887/commits/4cbb844d1d5f3a3ac6dd08325db17b807e7129f2 Follow-up to commit 167bcc2da5d25fe1f4894f447e5d65c2e5919537 and commit a635566b0ae79bfd82823ed2c0fb42ba37a4a151. This commit fixes OCPBUGS-17359. https://issues.redhat.com/browse/OCPBUGS-17359 * test/e2e/util_test.go (buildEchoPod, buildSlowHTTPDPod): Replace the "openshift/origin-node" image pullspec with "image-registry.openshift-image-registry.svc:5000/openshift/tools:latest". --- test/e2e/util_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index 2a68b758f1..b8efbec743 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -55,7 +55,7 @@ func buildEchoPod(name, namespace string) *corev1.Pod { `EXEC:'/bin/bash -c \"printf \\\"HTTP/1.0 200 OK\r\n\r\n\\\"; sed -e \\\"/^\r/q\\\"\"'`, }, Command: []string{"/bin/socat"}, - Image: "openshift/origin-node", + Image: "image-registry.openshift-image-registry.svc:5000/openshift/tools:latest", Name: "echo", Ports: []corev1.ContainerPort{ { @@ -219,7 +219,7 @@ func buildSlowHTTPDPod(name, namespace string) *corev1.Pod { `EXEC:'/bin/bash -c \"sleep 40; printf \\\"HTTP/1.0 200 OK\r\n\r\nfin\r\n\\\"\"'`, }, Command: []string{"/bin/socat"}, - Image: "openshift/origin-node", + Image: "image-registry.openshift-image-registry.svc:5000/openshift/tools:latest", Name: "echo", Ports: []corev1.ContainerPort{ { From e32c79f0900e87185f80be429f40c2cbefb61c3b Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Fri, 4 Aug 2023 21:05:01 -0400 Subject: [PATCH 2/3] TestHstsPolicyWorks: Dump events if test fails * test/e2e/hsts_policy_test.go (TestHstsPolicyWorks): Dump events in case of test failure, using the new dumpEventsInNamespace helper. * test/e2e/util_test.go (dumpEventsInNamespace): New helper function to log all events in a namespace. --- test/e2e/hsts_policy_test.go | 1 + test/e2e/util_test.go | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/test/e2e/hsts_policy_test.go b/test/e2e/hsts_policy_test.go index 6499a85cad..5ff1db6b53 100644 --- a/test/e2e/hsts_policy_test.go +++ b/test/e2e/hsts_policy_test.go @@ -144,6 +144,7 @@ func TestHstsPolicyWorks(t *testing.T) { t.Logf("request to %s got correct HSTS header: [%s]", validRoute.Spec.Host, header) return true, nil }); err != nil { + dumpEventsInNamespace(t, echoPod.Namespace) t.Fatalf("failed to find header [%s]: %v", exampleHeader, err) } } diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index b8efbec743..29d0f8b93d 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -712,3 +712,19 @@ func getRouteHost(t *testing.T, route *routev1.Route, router string) string { t.Fatalf("failed to find host name for default router in route: %#v", route) return "" } + +// dumpEventsInNamespace gets the events in the specified namespace and logs +// them. +func dumpEventsInNamespace(t *testing.T, ns string) { + t.Helper() + + eventList := &corev1.EventList{} + if err := kclient.List(context.TODO(), eventList, client.InNamespace(ns)); err != nil { + t.Errorf("failed to list events for namespace %s: %v", ns, err) + return + } + + for _, e := range eventList.Items { + t.Log(e.FirstTimestamp, e.Source, e.InvolvedObject.Kind, e.InvolvedObject.Name, e.Reason, e.Message) + } +} From 42c4f827df32ca5166e26dc2a2d75aea4dd8ebc4 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Fri, 4 Aug 2023 21:38:24 -0400 Subject: [PATCH 3/3] TestHstsPolicyWorks: Wait for NS to be provisioned When creating a new namespace for the TestHstsPolicyWorks test, wait for the "default" ServiceAccount and the "system:image-pullers" RoleBinding to be provisioned in the newly created namespace before proceeding with the test. Make a similar change for the TestMTLSWithCRLsCerts test. Before this commit, TestHstsPolicyWorks sometimes failed because it tried to create a pod before the ServiceAccount had been provisioned and granted access to pull images. As a result, the test would randomly fail with the following error: Failed to pull image "image-registry.openshift-image-registry.svc:5000/openshift/tools:latest": rpc error: code = Unknown desc = reading manifest This change should prevent such failures. Because TestMTLSWithCRLsCerts also creates a namespace and then creates pods in this namespace, this commit makes the same change to this test as well. Some other tests create namespaces but do not create pods in those namespaces; those tests do not necessarily need to wait for the ServiceAccount and RoleBinding. Inspired by https://github.com/openshift/origin/pull/21905/commits/877c652ad6beadcd016396b627d01d9eeb01bb9c. * test/e2e/client_tls_test.go (TestMTLSWithCRLs): * test/e2e/hsts_policy_test.go (TestHstsPolicyWorks): Use the new createNamespace helper. * test/e2e/util_test.go (createNamespace): New helper function. Create a namespace with the specified name, register a cleanup handler to delete the namespace when the test finishes, wait for the "default" ServiceAccount and "system:image-pullers" RoleBinding to be created, and return the namespace. --- test/e2e/client_tls_test.go | 10 +---- test/e2e/hsts_policy_test.go | 19 +--------- test/e2e/util_test.go | 71 ++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 27 deletions(-) diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index 16672f2246..dc7ee26ae7 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -737,15 +737,7 @@ func TestMTLSWithCRLs(t *testing.T) { }, } - namespace := corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: namespaceName, - }, - } - if err := kclient.Create(context.TODO(), &namespace); err != nil { - t.Fatalf("Failed to create namespace %q: %v", namespace.Name, err) - } - defer assertDeletedWaitForCleanup(t, kclient, &namespace) + namespace := createNamespace(t, namespaceName) for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { tcCerts := tc.CreateCerts() diff --git a/test/e2e/hsts_policy_test.go b/test/e2e/hsts_policy_test.go index 5ff1db6b53..2400502a2a 100644 --- a/test/e2e/hsts_policy_test.go +++ b/test/e2e/hsts_policy_test.go @@ -14,9 +14,6 @@ import ( configv1 "github.com/openshift/api/config/v1" routev1 "github.com/openshift/api/route/v1" - corev1 "k8s.io/api/core/v1" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/pointer" @@ -72,20 +69,7 @@ func TestHstsPolicyWorks(t *testing.T) { t.Logf("created a RequiredHSTSPolicy with DomainPatterns: %v,\n preload policy: %s,\n includeSubDomains policy: %s,\n largest age: %d,\n smallest age: %d\n", p.DomainPatterns, p.PreloadPolicy, p.IncludeSubDomainsPolicy, *p.MaxAge.LargestMaxAge, *p.MaxAge.SmallestMaxAge) // Use the same namespace for route, service, and pod - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hsts-policy-namespace", - }, - } - if err := kclient.Create(context.TODO(), ns); err != nil { - t.Fatalf("failed to create namespace: %v", err) - } - defer func() { - // this will cleanup all components in this namespace - if err := kclient.Delete(context.TODO(), ns); err != nil { - t.Fatalf("failed to delete test namespace %s: %v", ns.Name, err) - } - }() + ns := createNamespace(t, "hsts-policy-namespace") // Create pod echoPod := buildEchoPod("hsts-policy-echo", ns.Name) @@ -144,7 +128,6 @@ func TestHstsPolicyWorks(t *testing.T) { t.Logf("request to %s got correct HSTS header: [%s]", validRoute.Spec.Host, header) return true, nil }); err != nil { - dumpEventsInNamespace(t, echoPod.Namespace) t.Fatalf("failed to find header [%s]: %v", exampleHeader, err) } } diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index 29d0f8b93d..081d09f1af 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -20,6 +20,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/utils/pointer" "k8s.io/apimachinery/pkg/api/errors" @@ -728,3 +729,73 @@ func dumpEventsInNamespace(t *testing.T, ns string) { t.Log(e.FirstTimestamp, e.Source, e.InvolvedObject.Kind, e.InvolvedObject.Name, e.Reason, e.Message) } } + +// createNamespace creates a namespace with the specified name and registers a +// cleanup handler to delete the namespace when the test finishes. +// +// After creating the namespace, this function waits for the "default" +// ServiceAccount and "system:image-pullers" RoleBinding to be created as well, +// which is necessary in order for pods in the new namespace to be able to pull +// images. +func createNamespace(t *testing.T, name string) *corev1.Namespace { + t.Helper() + + t.Logf("Creating namespace %q...", name) + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}} + if err := kclient.Create(context.TODO(), ns); err != nil { + t.Fatalf("failed to create namespace: %v", err) + } + t.Cleanup(func() { + t.Logf("Dumping events in namespace %q...", name) + if t.Failed() { + dumpEventsInNamespace(t, name) + } + t.Logf("Deleting namespace %q...", name) + if err := kclient.Delete(context.TODO(), ns); err != nil { + t.Errorf("failed to delete namespace %s: %v", ns.Name, err) + } + }) + + saName := types.NamespacedName{ + Namespace: name, + Name: "default", + } + t.Logf("Waiting for ServiceAccount %s to be provisioned...", saName) + if err := wait.PollImmediate(1*time.Second, 3*time.Minute, func() (bool, error) { + var sa corev1.ServiceAccount + if err := kclient.Get(context.TODO(), saName, &sa); err != nil { + if errors.IsNotFound(err) { + return false, nil + } + return false, err + } + for _, s := range sa.Secrets { + if strings.Contains(s.Name, "dockercfg") { + return true, nil + } + } + return false, nil + }); err != nil { + t.Fatalf(`Timed out waiting for ServiceAccount %s to be provisioned: %v`, saName, err) + } + + rbName := types.NamespacedName{ + Namespace: name, + Name: "system:image-pullers", + } + t.Logf("Waiting for RoleBinding %s to be created...", rbName) + if err := wait.PollImmediate(1*time.Second, 3*time.Minute, func() (bool, error) { + var rb rbacv1.RoleBinding + if err := kclient.Get(context.TODO(), rbName, &rb); err != nil { + if errors.IsNotFound(err) { + return false, nil + } + return false, err + } + return true, nil + }); err != nil { + t.Fatalf(`Timed out waiting for RoleBinding "default" to be provisioned: %v`, err) + } + + return ns +}