From 91dadf66f14d6b77033a4ad9250dce98c2046dfc Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Thu, 26 Oct 2023 09:21:57 -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 145ca0584f..d4d762862e 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 9930a8e3b70b09f9c4c39ee2771f619a3beef858 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 d4d762862e..72c8e82c73 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -688,3 +688,19 @@ func assertDeletedWaitForCleanup(t *testing.T, cl client.Client, thing client.Ob t.Fatalf("Timed out waiting for %s to be cleaned up: %v", thing.GetName(), err) } } + +// 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 549601b4b21a879b9ca54d3d6587315acdb7a284 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 0e4317fe88..3d12702e27 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -735,15 +735,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 72c8e82c73..f6cef991e8 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" @@ -704,3 +705,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 +}