From a777e53a2dda912dfd0f81a222d1cf6fdfd114b4 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Mon, 16 May 2022 12:04:26 +0100 Subject: [PATCH 1/8] test/e2e: add new helpers to retry updates on conflict errors --- test/e2e/util.go | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/test/e2e/util.go b/test/e2e/util.go index ebda888a71..9bb9c09e4e 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -10,12 +10,14 @@ import ( "testing" "time" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -285,3 +287,77 @@ func probe(timeout, period, success, failure int) *corev1.Probe { FailureThreshold: int32(failure), } } + +// updateIngressControllerSpecWithRetryOnConflict gets a fresh copy of +// the named ingresscontroller, calls mutateSpecFn() where callers can +// modify fields of the spec, and then updates the ingresscontroller +// object. If there is a conflict error on update then the complete +// sequence of get, mutate, and update is retried until timeout is +// reached. +func updateIngressControllerSpecWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(*operatorv1.IngressControllerSpec)) error { + ic := operatorv1.IngressController{} + return wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), name, &ic); err != nil { + t.Logf("error getting ingress controller %v: %v, retrying...", name, err) + return false, nil + } + mutateSpecFn(&ic.Spec) + if err := kclient.Update(context.TODO(), &ic); err != nil { + if errors.IsConflict(err) { + t.Logf("conflict when updating ingress controller %v: %v, retrying...", name, err) + return false, nil + } + return false, err + } + return true, nil + }) +} + +// updateIngressConfigSpecWithRetryOnConflict gets a fresh copy of the +// name ingress config, calls updateSpecFn() where callers can modify +// fields of the spec, and then updates the ingress config object. If +// there is a conflict error on update then the complete operation of +// get, mutate, and update is retried until timeout is reached. +func updateIngressConfigSpecWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(*configv1.IngressSpec)) error { + ingressConfig := configv1.Ingress{} + return wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), name, &ingressConfig); err != nil { + t.Logf("error getting ingress config %v: %v, retrying...", name, err) + return false, nil + } + mutateSpecFn(&ingressConfig.Spec) + if err := kclient.Update(context.TODO(), &ingressConfig); err != nil { + if errors.IsConflict(err) { + t.Logf("conflict when updating ingress config %v: %v, retrying...", name, err) + return false, nil + } + return false, err + } + return true, nil + }) +} + +// updateInfrastructureConfigSpecWithRetryOnConflict gets a fresh copy +// of the named infrastructure object, calls updateSpecFn() where +// callers can modify fields of the spec, and then updates the infrastructure +// config object. If there is a conflict error on update then the +// complete operation of get, mutate, and update is retried until +// timeout is reached. +func updateInfrastructureConfigSpecWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(*configv1.InfrastructureSpec)) error { + infraConfig := configv1.Infrastructure{} + return wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { + if err := kclient.Get(context.TODO(), name, &infraConfig); err != nil { + t.Logf("error getting infrastructure config %v: %v, retrying...", name, err) + return false, nil + } + mutateSpecFn(&infraConfig.Spec) + if err := kclient.Update(context.TODO(), &infraConfig); err != nil { + if errors.IsConflict(err) { + t.Logf("conflict when updating infrastructure config %v: %v, retrying...", name, err) + return false, nil + } + return false, err + } + return true, nil + }) +} From a22322b25569059c61e1973f37f0a4b49e9407bc Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Thu, 12 May 2022 10:35:39 +0100 Subject: [PATCH 2/8] test/e2e: mark parallel tests with calls to t.Parallel() --- test/e2e/certificate_publisher_test.go | 2 ++ test/e2e/checkinterval_test.go | 1 + test/e2e/client_tls_test.go | 1 + test/e2e/forwarded_header_policy_test.go | 4 +++ test/e2e/haproxy_timeouts_test.go | 2 ++ test/e2e/http_header_buffer_test.go | 1 + .../http_header_name_case_adjustment_test.go | 1 + test/e2e/maxconn_test.go | 2 ++ test/e2e/operator_test.go | 27 +++++++++++++++++++ test/e2e/router_compression_test.go | 1 + 10 files changed, 42 insertions(+) diff --git a/test/e2e/certificate_publisher_test.go b/test/e2e/certificate_publisher_test.go index 4889efbaa5..f6704a3215 100644 --- a/test/e2e/certificate_publisher_test.go +++ b/test/e2e/certificate_publisher_test.go @@ -24,6 +24,7 @@ import ( // references a secret that does not exist, then creates the secret and verifies // that the operator updates the "router-certs" global secret. func TestCreateIngressControllerThenSecret(t *testing.T) { + t.Parallel() name := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("test-")} ic := newPrivateController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) ic.Spec.DefaultCertificate = &corev1.LocalObjectReference{ @@ -73,6 +74,7 @@ func TestCreateIngressControllerThenSecret(t *testing.T) { // ingresscontroller that references the secret and verifies that the operator // updates the "router-certs" global secret. func TestCreateSecretThenIngressController(t *testing.T) { + t.Parallel() name := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("test-")} // Create the secret. diff --git a/test/e2e/checkinterval_test.go b/test/e2e/checkinterval_test.go index 6750410e6f..70d3f4fa5f 100644 --- a/test/e2e/checkinterval_test.go +++ b/test/e2e/checkinterval_test.go @@ -24,6 +24,7 @@ const ( ) func TestHealthCheckIntervalIngressController(t *testing.T) { + t.Parallel() name := types.NamespacedName{Namespace: operatorNamespace, Name: "healthcheckinterval"} domain := name.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(name, domain) diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index 0478838265..8b85b4264c 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -55,6 +55,7 @@ import ( // accepts connections with a valid, matching certificate and rejects other // connections. func TestClientTLS(t *testing.T) { + t.Parallel() // We will configure the ingresscontroller to recognize certificates // signed by this CA. ca, caKey, err := generateClientCA() diff --git a/test/e2e/forwarded_header_policy_test.go b/test/e2e/forwarded_header_policy_test.go index a5d5ec9c7a..1473a46dd6 100644 --- a/test/e2e/forwarded_header_policy_test.go +++ b/test/e2e/forwarded_header_policy_test.go @@ -121,6 +121,7 @@ func testRouteHeaders(t *testing.T, image string, route *routev1.Route, address // client specifies 2 X-Forwarded-For headers, then the router should append a // 3rd. func TestForwardedHeaderPolicyAppend(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -215,6 +216,7 @@ func TestForwardedHeaderPolicyAppend(t *testing.T) { // expected behavior if its policy is "Replace". A forwarded client request // should always have exactly 1 X-Forwarded-For header. func TestForwardedHeaderPolicyReplace(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -284,6 +286,7 @@ func TestForwardedHeaderPolicyReplace(t *testing.T) { // should always have exactly as many X-Forwarded-For headers as the client // specified. func TestForwardedHeaderPolicyNever(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -354,6 +357,7 @@ func TestForwardedHeaderPolicyNever(t *testing.T) { // specifies more than 1 X-Forwarded-For header, the forwarded request should // include exactly as many X-Forwarded-For headers as the client specified. func TestForwardedHeaderPolicyIfNone(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) diff --git a/test/e2e/haproxy_timeouts_test.go b/test/e2e/haproxy_timeouts_test.go index 198e751fb2..b899297c71 100644 --- a/test/e2e/haproxy_timeouts_test.go +++ b/test/e2e/haproxy_timeouts_test.go @@ -24,6 +24,7 @@ import ( ) func TestHAProxyTimeouts(t *testing.T) { + t.Parallel() const ( clientTimeoutInput = 45 * time.Second clientTimeoutOutput = "45s" @@ -178,6 +179,7 @@ func TestHAProxyTimeouts(t *testing.T) { } func TestHAProxyTimeoutsRejection(t *testing.T) { + t.Parallel() const ( clientTimeoutInput = -45 * time.Second clientFinTimeoutInput = 0 * time.Millisecond diff --git a/test/e2e/http_header_buffer_test.go b/test/e2e/http_header_buffer_test.go index 0866e510fa..350119615e 100644 --- a/test/e2e/http_header_buffer_test.go +++ b/test/e2e/http_header_buffer_test.go @@ -29,6 +29,7 @@ import ( ) func TestHTTPHeaderBufferSize(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "header-buffer-size"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) diff --git a/test/e2e/http_header_name_case_adjustment_test.go b/test/e2e/http_header_name_case_adjustment_test.go index ed82e2cdf7..1a57a5ee88 100644 --- a/test/e2e/http_header_name_case_adjustment_test.go +++ b/test/e2e/http_header_name_case_adjustment_test.go @@ -27,6 +27,7 @@ import ( ) func TestHeaderNameCaseAdjustment(t *testing.T) { + t.Parallel() testHeaderNames := []operatorv1.IngressControllerHTTPHeaderNameCaseAdjustment{ "X-Forwarded-For", "Cache-Control", diff --git a/test/e2e/maxconn_test.go b/test/e2e/maxconn_test.go index 751a766c86..7ef5f65919 100644 --- a/test/e2e/maxconn_test.go +++ b/test/e2e/maxconn_test.go @@ -20,6 +20,7 @@ import ( ) func TestTunableMaxConnectionsValidValues(t *testing.T) { + t.Parallel() updateMaxConnections := func(t *testing.T, client client.Client, timeout time.Duration, maxConnections int32, name types.NamespacedName) error { return wait.PollImmediate(time.Second, timeout, func() (bool, error) { ic := operatorv1.IngressController{} @@ -95,6 +96,7 @@ func TestTunableMaxConnectionsValidValues(t *testing.T) { // so we test outside of those value and expect validation failures // when we attempt to set them. func TestTunableMaxConnectionsInvalidValues(t *testing.T) { + t.Parallel() updateMaxConnections := func(t *testing.T, client client.Client, maxConnections int32, name types.NamespacedName) error { ic := operatorv1.IngressController{} if err := client.Get(context.TODO(), name, &ic); err != nil { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 6ca8c564bf..e6aaa98629 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -231,6 +231,7 @@ func TestDefaultIngressClass(t *testing.T) { // ingressclass for a custom ingresscontroller and deletes the ingressclass if // the ingresscontroller is deleted. func TestCustomIngressClass(t *testing.T) { + t.Parallel() icName := types.NamespacedName{ Namespace: operatorNamespace, Name: "testcustomingressclass", @@ -290,6 +291,7 @@ func TestCustomIngressClass(t *testing.T) { } func TestUserDefinedIngressController(t *testing.T) { + t.Parallel() name := types.NamespacedName{Namespace: operatorNamespace, Name: "test"} ing := newLoadBalancerController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing); err != nil { @@ -303,6 +305,7 @@ func TestUserDefinedIngressController(t *testing.T) { } func TestUniqueDomainRejection(t *testing.T) { + t.Parallel() def := &operatorv1.IngressController{} if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { t.Fatalf("failed to observe expected conditions: %v", err) @@ -369,6 +372,7 @@ func TestProxyProtocolOnAWS(t *testing.T) { // TestProxyProtocolAPI verifies that the operator configures router pod // replicas to use PROXY protocol if it is specified on an ingresscontroller. func TestProxyProtocolAPI(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "proxy-protocol"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newNodePortController(icName, domain) @@ -515,6 +519,7 @@ func TestUpdateDefaultIngressController(t *testing.T) { // scale client and uses it to scale the ingresscontroller up to 2 replicas and // then back down to 1 replica. func TestIngressControllerScale(t *testing.T) { + t.Parallel() // Create a new ingresscontroller. name := types.NamespacedName{Namespace: operatorNamespace, Name: "scale"} domain := name.Name + "." + dnsConfig.Spec.BaseDomain @@ -749,6 +754,7 @@ func TestPodDisruptionBudgetExists(t *testing.T) { // operator creates a router and that the router becomes available. func TestHostNetworkEndpointPublishingStrategy(t *testing.T) { name := types.NamespacedName{Namespace: operatorNamespace, Name: "host"} + t.Parallel() ing := newHostNetworkController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing); err != nil { t.Fatalf("failed to create ingresscontroller: %v", err) @@ -770,6 +776,7 @@ func TestHostNetworkEndpointPublishingStrategy(t *testing.T) { // TestHostNetworkPortBinding creates two ingresscontrollers on the same node // with different port bindings and verifies that both routers are available. func TestHostNetworkPortBinding(t *testing.T) { + t.Parallel() // deploy first ingresscontroller with the default port bindings name1 := types.NamespacedName{Namespace: operatorNamespace, Name: "host"} ing1 := newHostNetworkController(name1, name1.Name+"."+dnsConfig.Spec.BaseDomain) @@ -872,6 +879,7 @@ func assertContainerHasPort(t *testing.T, container corev1.Container, name strin // "Internal" and verifies that the operator creates a load balancer and that // the load balancer has a private IP address. func TestInternalLoadBalancer(t *testing.T) { + t.Parallel() platform := infraConfig.Status.Platform supportedPlatforms := map[configv1.PlatformType]struct{}{ @@ -958,6 +966,7 @@ func TestInternalLoadBalancer(t *testing.T) { // parameter set to both "Global" and "local" to verify that the // Load Balancer service is created properly. func TestInternalLoadBalancerGlobalAccessGCP(t *testing.T) { + t.Parallel() platform := infraConfig.Status.Platform supportedPlatforms := map[configv1.PlatformType]struct{}{ @@ -1057,6 +1066,7 @@ func TestInternalLoadBalancerGlobalAccessGCP(t *testing.T) { // recreating the LoadBalancer service to change its scope, then the operator // should delete and recreate the service automatically. func TestScopeChange(t *testing.T) { + t.Parallel() platform := infraConfig.Status.Platform supportedPlatforms := map[configv1.PlatformType]struct{}{ configv1.AlibabaCloudPlatformType: {}, @@ -1225,6 +1235,7 @@ func TestScopeChange(t *testing.T) { // verifies that the operator does not add the port back. See // . func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) { + t.Parallel() name := types.NamespacedName{Namespace: operatorNamespace, Name: "nodeport"} ing := newNodePortController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing); err != nil { @@ -1297,6 +1308,7 @@ func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) { // profile, then updates the ingresscontroller to use a custom TLS profile, and // then verifies that the operator reflects the custom profile in its status. func TestTLSSecurityProfile(t *testing.T) { + t.Parallel() name := types.NamespacedName{Namespace: operatorNamespace, Name: "test"} domain := name.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(name, domain) @@ -1358,6 +1370,7 @@ func TestTLSSecurityProfile(t *testing.T) { } func TestRouteAdmissionPolicy(t *testing.T) { + t.Parallel() // Set up an ingresscontroller which only selects routes created by this test icName := types.NamespacedName{Namespace: operatorNamespace, Name: "routeadmission"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain @@ -1572,6 +1585,7 @@ func TestRouteAdmissionPolicy(t *testing.T) { } func TestSyslogLogging(t *testing.T) { + t.Parallel() ic := &operatorv1.IngressController{} if err := kclient.Get(context.TODO(), defaultName, ic); err != nil { t.Fatalf("failed to get default ingresscontroller: %v", err) @@ -1743,6 +1757,7 @@ $ModLoad omstdout.so } func TestContainerLogging(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "containerlogging"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -1846,6 +1861,7 @@ func TestIngressControllerCustomEndpoints(t *testing.T) { } func TestHTTPHeaderCapture(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "headercapture"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newNodePortController(icName, domain) @@ -1987,6 +2003,7 @@ func TestHTTPHeaderCapture(t *testing.T) { } func TestHTTPCookieCapture(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "cookiecapture"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newNodePortController(icName, domain) @@ -2126,6 +2143,7 @@ func TestHTTPCookieCapture(t *testing.T) { // "LoadBalancerService" endpoint publishing strategy type with // an AWS Network Load Balancer (NLB). func TestNetworkLoadBalancer(t *testing.T) { + t.Parallel() platform := infraConfig.Status.PlatformStatus.Type if platform != configv1.AWSPlatformType { @@ -2170,6 +2188,7 @@ func TestNetworkLoadBalancer(t *testing.T) { // TestAWSELBConnectionIdleTimeout verifies that the AWS ELB connection-idle // timeout works as expected. func TestAWSELBConnectionIdleTimeout(t *testing.T) { + t.Parallel() if platform := infraConfig.Status.PlatformStatus.Type; platform != configv1.AWSPlatformType { t.Skipf("test skipped on platform %q", platform) } @@ -2388,6 +2407,7 @@ func TestAWSELBConnectionIdleTimeout(t *testing.T) { } func TestUniqueIdHeader(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "uniqueid"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -2514,6 +2534,7 @@ func TestUniqueIdHeader(t *testing.T) { // the operator always configures router pod replicas to use the "source" // algorithm for passthrough routes irrespective of the override. func TestLoadBalancingAlgorithmUnsupportedConfigOverride(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "leastconn"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -2560,6 +2581,7 @@ func TestLoadBalancingAlgorithmUnsupportedConfigOverride(t *testing.T) { // configures router pod replicas to use the dynamic config manager if the // ingresscontroller is so configured using an unsupported config override. func TestDynamicConfigManagerUnsupportedConfigOverride(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "dynamic-config-manager"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -2666,6 +2688,7 @@ func TestLocalWithFallbackOverrideForLoadBalancerService(t *testing.T) { // does not set the local-with-fallback annotation on a NodePort service if the // the localWithFallback unsupported config override is set to "false". func TestLocalWithFallbackOverrideForNodePortService(t *testing.T) { + t.Parallel() icName := types.NamespacedName{ Namespace: operatorNamespace, Name: "local-with-fallback", @@ -2721,6 +2744,7 @@ func TestLocalWithFallbackOverrideForNodePortService(t *testing.T) { // if one is specified using an unsupported config override on the // ingresscontroller. func TestReloadIntervalUnsupportedConfigOverride(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "reload-interval"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -2760,6 +2784,7 @@ func TestReloadIntervalUnsupportedConfigOverride(t *testing.T) { // error-page configmap when it is deleted or when the user-provided configmap // is updated. func TestCustomErrorpages(t *testing.T) { + t.Parallel() icName := types.NamespacedName{Namespace: operatorNamespace, Name: "errorpage"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) @@ -2867,6 +2892,7 @@ func TestCustomErrorpages(t *testing.T) { // operator allows changes to the kubelet probe timeouts for the router // deployment associated with a custom ingresscontroller. func TestTunableRouterKubeletProbesForCustomIngressController(t *testing.T) { + t.Parallel() icName := types.NamespacedName{ Namespace: operatorNamespace, Name: "tunable-kubelet-probes", @@ -2939,6 +2965,7 @@ func TestTunableRouterKubeletProbesForCustomIngressController(t *testing.T) { // It creates a service with the same naming convention as the ingress controller creates its own load balancing services. // Then it triggers a reconcilation of the ingress operator to see if it will delete our service. func TestIngressControllerServiceNameCollision(t *testing.T) { + t.Parallel() // Create the new private controller that we will later create a service to collide with the naming scheme of this. icName := types.NamespacedName{ Namespace: operatorNamespace, diff --git a/test/e2e/router_compression_test.go b/test/e2e/router_compression_test.go index 14e93b6117..ab205d6855 100644 --- a/test/e2e/router_compression_test.go +++ b/test/e2e/router_compression_test.go @@ -24,6 +24,7 @@ import ( ) func TestRouterCompressionParsing(t *testing.T) { + t.Parallel() // Test compression policies for the ingress config mimeTypesNormative := []operatorv1.CompressionMIMEType{"text/html", "application/json", "x-custom/allow-custom"} compressionPolicyNormative := operatorv1.HTTPCompressionPolicy{MimeTypes: mimeTypesNormative} From 7dd007f96044eb71fe59f858d0768ff9ca7fc5d2 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Wed, 11 May 2022 14:57:49 +0100 Subject: [PATCH 3/8] test/e2e: http_header_buffer_test: s/t.Error/t.Fatal for certain conditions Call t.Fatal for those failure cases where it is not really practical to continue running the test. --- test/e2e/http_header_buffer_test.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/test/e2e/http_header_buffer_test.go b/test/e2e/http_header_buffer_test.go index 350119615e..3ea3676430 100644 --- a/test/e2e/http_header_buffer_test.go +++ b/test/e2e/http_header_buffer_test.go @@ -168,7 +168,7 @@ func TestHTTPHeaderBufferSize(t *testing.T) { Name: clientPodValidRequest.Name, } if err := kclient.Get(context.TODO(), podName, pod); err != nil { - t.Errorf("failed to get pod %s: %v", clientPodValidRequest.Name, err) + t.Fatalf("failed to get pod %s: %v", clientPodValidRequest.Name, err) } logs, err := cl.CoreV1().Pods(clientPodValidRequest.Namespace).GetLogs(clientPodValidRequest.Name, &corev1.PodLogOptions{ @@ -176,7 +176,7 @@ func TestHTTPHeaderBufferSize(t *testing.T) { Follow: false, }).DoRaw(context.TODO()) if err != nil { - t.Errorf("failed to get logs from pod %s: %v", clientPodValidRequest.Name, err) + t.Fatalf("failed to get logs from pod %s: %v", clientPodValidRequest.Name, err) } output := fmt.Sprintf("failed to observe the expected output: %v\nclient pod spec: %#v\nclient pod logs:\n%s", pollErr, pod, logs) @@ -185,7 +185,7 @@ func TestHTTPHeaderBufferSize(t *testing.T) { } if err := kclient.Get(context.TODO(), types.NamespacedName{Name: ic.Name, Namespace: ic.Namespace}, ic); err != nil { - t.Errorf("failed to get ingresscontroller %s: %v", ic.Name, err) + t.Fatalf("failed to get ingresscontroller %s: %v", ic.Name, err) } // Get the name of the current router pod for the test ingress controller @@ -194,11 +194,11 @@ func TestHTTPHeaderBufferSize(t *testing.T) { controller.ControllerDeploymentLabel: "header-buffer-size", } if err := kclient.List(context.TODO(), podList, client.InNamespace(deployment.Namespace), client.MatchingLabels(labels)); err != nil { - t.Errorf("failed to list pods for ingress controllers %s: %v", ic.Name, err) + t.Fatalf("failed to list pods for ingress controllers %s: %v", ic.Name, err) } if len(podList.Items) != 1 { - t.Errorf("expected ingress controller %s to have exactly 1 router pod, but it has %d", ic.Name, len(podList.Items)) + t.Fatalf("expected ingress controller %s to have exactly 1 router pod, but it has %d", ic.Name, len(podList.Items)) } oldRouterPodName := podList.Items[0].Name @@ -221,7 +221,8 @@ func TestHTTPHeaderBufferSize(t *testing.T) { pollErr = wait.PollImmediate(2*time.Second, 5*time.Minute, func() (bool, error) { podList := &corev1.PodList{} if err := kclient.List(context.TODO(), podList, client.InNamespace(deployment.Namespace), client.MatchingLabels(labels)); err != nil { - t.Errorf("failed to list pods for ingress controllers %s: %v", ic.Name, err) + t.Logf("failed to list pods for ingress controllers %s: %v, retrying...", ic.Name, err) + return false, nil } for _, pod := range podList.Items { @@ -244,7 +245,7 @@ func TestHTTPHeaderBufferSize(t *testing.T) { }) if pollErr != nil { - t.Errorf("timed out waiting for new router pod for %s to become ready: %v", ic.Name, pollErr) + t.Fatalf("timed out waiting for new router pod for %s to become ready: %v", ic.Name, pollErr) } name = name + "-fail-case" @@ -295,7 +296,7 @@ func TestHTTPHeaderBufferSize(t *testing.T) { Name: clientPodInvalidRequest.Name, } if err := kclient.Get(context.TODO(), podName, pod); err != nil { - t.Errorf("failed to get pod %s: %v", clientPodInvalidRequest.Name, err) + t.Fatalf("failed to get pod %s: %v", clientPodInvalidRequest.Name, err) } logs, err := cl.CoreV1().Pods(clientPodInvalidRequest.Namespace).GetLogs(clientPodInvalidRequest.Name, &corev1.PodLogOptions{ @@ -303,7 +304,7 @@ func TestHTTPHeaderBufferSize(t *testing.T) { Follow: false, }).DoRaw(context.TODO()) if err != nil { - t.Errorf("failed to get logs from pod %s: %v", clientPodInvalidRequest.Name, err) + t.Fatalf("failed to get logs from pod %s: %v", clientPodInvalidRequest.Name, err) } output := fmt.Sprintf("failed to observe the expected output: %v\nclient pod spec: %#v\nclient pod logs:\n%s", pollErr, pod, logs) From a72396a0d614d067ef0006ccd79bda32f9e71d43 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Mon, 16 May 2022 18:30:35 +0100 Subject: [PATCH 4/8] test/e2e: ensure resource names are unique for parallel tests e2e tests that can run in parallel must create resource names with unique names. --- test/e2e/forwarded_header_policy_test.go | 10 +++++----- test/e2e/operator_test.go | 10 +++++----- test/e2e/util.go | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/test/e2e/forwarded_header_policy_test.go b/test/e2e/forwarded_header_policy_test.go index 1473a46dd6..8dc34af19a 100644 --- a/test/e2e/forwarded_header_policy_test.go +++ b/test/e2e/forwarded_header_policy_test.go @@ -52,7 +52,7 @@ func testRouteHeaders(t *testing.T, image string, route *routev1.Route, address extraCurlArgs = append(extraCurlArgs, "-H", header) } testPodCount++ - name := fmt.Sprintf("forwardedheader%d", testPodCount) + name := fmt.Sprintf("%s%d", route.Name, testPodCount) clientPod := buildCurlPod(name, route.Namespace, image, route.Spec.Host, address, extraCurlArgs...) if err := kclient.Create(context.TODO(), clientPod); err != nil { t.Fatalf("failed to create pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) @@ -122,7 +122,7 @@ func testRouteHeaders(t *testing.T, image string, route *routev1.Route, address // 3rd. func TestForwardedHeaderPolicyAppend(t *testing.T) { t.Parallel() - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader-append"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) if err := kclient.Create(context.TODO(), ic); err != nil { @@ -217,7 +217,7 @@ func TestForwardedHeaderPolicyAppend(t *testing.T) { // should always have exactly 1 X-Forwarded-For header. func TestForwardedHeaderPolicyReplace(t *testing.T) { t.Parallel() - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader-replace"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) ic.Spec.HTTPHeaders = &operatorv1.IngressControllerHTTPHeaders{ @@ -287,7 +287,7 @@ func TestForwardedHeaderPolicyReplace(t *testing.T) { // specified. func TestForwardedHeaderPolicyNever(t *testing.T) { t.Parallel() - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader-never"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) ic.Spec.HTTPHeaders = &operatorv1.IngressControllerHTTPHeaders{ @@ -358,7 +358,7 @@ func TestForwardedHeaderPolicyNever(t *testing.T) { // include exactly as many X-Forwarded-For headers as the client specified. func TestForwardedHeaderPolicyIfNone(t *testing.T) { t.Parallel() - icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader"} + icName := types.NamespacedName{Namespace: operatorNamespace, Name: "forwardedheader-none"} domain := icName.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(icName, domain) ic.Spec.HTTPHeaders = &operatorv1.IngressControllerHTTPHeaders{ diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index e6aaa98629..308411b7d8 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -292,7 +292,7 @@ func TestCustomIngressClass(t *testing.T) { func TestUserDefinedIngressController(t *testing.T) { t.Parallel() - name := types.NamespacedName{Namespace: operatorNamespace, Name: "test"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: "testuserdefinedingresscontroller"} ing := newLoadBalancerController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing); err != nil { t.Fatalf("failed to create ingresscontroller: %v", err) @@ -753,8 +753,8 @@ func TestPodDisruptionBudgetExists(t *testing.T) { // the "HostNetwork" endpoint publishing strategy type and verifies that the // operator creates a router and that the router becomes available. func TestHostNetworkEndpointPublishingStrategy(t *testing.T) { - name := types.NamespacedName{Namespace: operatorNamespace, Name: "host"} t.Parallel() + name := types.NamespacedName{Namespace: operatorNamespace, Name: "hostnetworkendpointpublishingstrategy"} ing := newHostNetworkController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing); err != nil { t.Fatalf("failed to create ingresscontroller: %v", err) @@ -778,7 +778,7 @@ func TestHostNetworkEndpointPublishingStrategy(t *testing.T) { func TestHostNetworkPortBinding(t *testing.T) { t.Parallel() // deploy first ingresscontroller with the default port bindings - name1 := types.NamespacedName{Namespace: operatorNamespace, Name: "host"} + name1 := types.NamespacedName{Namespace: operatorNamespace, Name: "hostnetworkportbinding"} ing1 := newHostNetworkController(name1, name1.Name+"."+dnsConfig.Spec.BaseDomain) if err := kclient.Create(context.TODO(), ing1); err != nil { t.Fatalf("failed to create the first ingresscontroller: %v", err) @@ -895,7 +895,7 @@ func TestInternalLoadBalancer(t *testing.T) { annotation := ingresscontroller.InternalLBAnnotations[platform] - name := types.NamespacedName{Namespace: operatorNamespace, Name: "test"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: "testinternalloadbalancer"} ic := newLoadBalancerController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{ Scope: operatorv1.InternalLoadBalancer, @@ -1309,7 +1309,7 @@ func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) { // then verifies that the operator reflects the custom profile in its status. func TestTLSSecurityProfile(t *testing.T) { t.Parallel() - name := types.NamespacedName{Namespace: operatorNamespace, Name: "test"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: "testtlssecurityprofile"} domain := name.Name + "." + dnsConfig.Spec.BaseDomain ic := newPrivateController(name, domain) if err := kclient.Create(context.TODO(), ic); err != nil { diff --git a/test/e2e/util.go b/test/e2e/util.go index 9bb9c09e4e..86d5f134c6 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -36,7 +36,7 @@ func buildEchoPod(name, namespace string) *corev1.Pod { return &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - "app": "echo", + "app": name, }, Name: name, Namespace: namespace, From b7030188afb9f55413698e9a5aa70619037bf807 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Wed, 11 May 2022 13:05:08 +0100 Subject: [PATCH 5/8] test/e2e: be explicit with some test expectations Explicitly assert tests TestHTTPHeaderCapture and TestHTTPCookieCapture only find 1 pod. These two tests were flaking in CI because they sometimes coincided with tests that modified the ingress config object. When that object is modified a rolling deployment of all ingresscontroller pods can take place and these two tests fell foul of the expectation that only one pod was running. The CI logs show that "the pod does not exist"--and that's because the pod was in terminating state post the rolling deployment. --- test/e2e/operator_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 308411b7d8..4134e40b45 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -1909,8 +1909,12 @@ func TestHTTPHeaderCapture(t *testing.T) { if err := kclient.List(context.TODO(), podList, client.MatchingLabelsSelector{Selector: selector}); err != nil { t.Fatalf("failed to list pods for ingresscontroller: %v", err) } - if len(podList.Items) < 1 { - t.Fatalf("found no pods for ingresscontroller: %v", err) + if len(podList.Items) != 1 { + var podNames []string + for i := range podList.Items { + podNames = append(podNames, podList.Items[i].Name) + } + t.Fatalf("expected ingress controller %s to have exactly 1 router pod, but it has %d: %s", ic.Name, len(podList.Items), strings.Join(podNames, ", ")) } // Make a request to the console route. @@ -2049,8 +2053,8 @@ func TestHTTPCookieCapture(t *testing.T) { if err := kclient.List(context.TODO(), podList, client.MatchingLabelsSelector{Selector: selector}); err != nil { t.Fatalf("failed to list pods for ingresscontroller: %v", err) } - if len(podList.Items) < 1 { - t.Fatalf("found no pods for ingresscontroller: %v", err) + if len(podList.Items) != 1 { + t.Fatalf("expected ingress controller %s to have exactly 1 router pod, but it has %d", ic.Name, len(podList.Items)) } // Make a request to the console route. From 42880c4ef8dafb9add4ed6f1d42bdaa8a6c6c2ab Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Mon, 16 May 2022 18:51:31 +0100 Subject: [PATCH 6/8] test/e2e: retry update on conflict errors Some tests, notably in test teardown (i.e., defer blocks) fail hard because the object that is being updated has been changed since it was last refreshed. We use the new update*SpecWithRetryOnConflict() functions that allow finer grained updates on the Spec itself and, if the update fails with a conflict error, the sequence of get, mutate, update will be retried. --- test/e2e/hsts_policy_test.go | 5 +-- test/e2e/operator_test.go | 59 +++++++++++++++++++----------------- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/test/e2e/hsts_policy_test.go b/test/e2e/hsts_policy_test.go index d0940b141c..6499a85cad 100644 --- a/test/e2e/hsts_policy_test.go +++ b/test/e2e/hsts_policy_test.go @@ -61,8 +61,9 @@ func TestHstsPolicyWorks(t *testing.T) { } defer func() { // Remove the HSTS policies from the ingress config - ing.Spec.RequiredHSTSPolicies = nil - if err := kclient.Update(context.TODO(), ing); err != nil { + if err := updateIngressConfigSpecWithRetryOnConflict(t, clusterConfigName, timeout, func(spec *configv1.IngressSpec) { + spec.RequiredHSTSPolicies = nil + }); err != nil { t.Fatalf("failed to restore ingress config: %v", err) } }() diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 4134e40b45..de65aaf7b2 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -452,10 +452,12 @@ func TestUpdateDefaultIngressController(t *testing.T) { } }() - ic.Spec.DefaultCertificate = &corev1.LocalObjectReference{Name: secret.Name} - if err := kclient.Update(context.TODO(), ic); err != nil { - t.Fatalf("failed to update default ingresscontroller: %v", err) + if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { + spec.DefaultCertificate = &corev1.LocalObjectReference{Name: secret.Name} + }); err != nil { + t.Fatalf("failed to update default ingress controller: %v", err) } + name := types.NamespacedName{Namespace: deployment.Namespace, Name: deployment.Name} err = wait.PollImmediate(1*time.Second, 15*time.Second, func() (bool, error) { if err := kclient.Get(context.TODO(), name, deployment); err != nil { @@ -488,12 +490,10 @@ func TestUpdateDefaultIngressController(t *testing.T) { } // Reset .spec.defaultCertificate to its original value. - if err := kclient.Get(context.TODO(), defaultName, ic); err != nil { - t.Fatalf("failed to get default ingresscontroller: %v", err) - } - ic.Spec.DefaultCertificate = originalSecret - if err := kclient.Update(context.TODO(), ic); err != nil { - t.Errorf("failed to reset default ingresscontroller: %v", err) + if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { + spec.DefaultCertificate = originalSecret + }); err != nil { + t.Fatalf("failed to reset default ingresscontroller: %v", err) } // Wait for the default ingress configmap to be updated back to the original @@ -1813,14 +1813,27 @@ func TestIngressControllerCustomEndpoints(t *testing.T) { Name: "elasticloadbalancing", URL: fmt.Sprintf("https://elasticloadbalancing.%s.amazonaws.com", platform.AWS.Region), } - endpoints := []configv1.AWSServiceEndpoint{route53Endpoint, taggingEndpoint, elbEndpoint} - awsSpec := configv1.AWSPlatformSpec{ServiceEndpoints: endpoints} - infraConfig.Spec.PlatformSpec.AWS = &awsSpec - if err := kclient.Update(context.TODO(), &infraConfig); err != nil { + if err := updateInfrastructureConfigSpecWithRetryOnConflict(t, types.NamespacedName{Name: "cluster"}, timeout, func(spec *configv1.InfrastructureSpec) { + spec.PlatformSpec.AWS = &configv1.AWSPlatformSpec{ + ServiceEndpoints: []configv1.AWSServiceEndpoint{ + route53Endpoint, + taggingEndpoint, + elbEndpoint, + }, + } + }); err != nil { t.Fatalf("failed to update infrastructure config: %v\n", err) } + defer func() { + // Remove the custom endpoints from the infrastructure config. + if err := updateInfrastructureConfigSpecWithRetryOnConflict(t, types.NamespacedName{Name: "cluster"}, timeout, func(spec *configv1.InfrastructureSpec) { + spec.PlatformSpec.AWS = nil + }); err != nil { + t.Fatalf("failed to update infrastructure config: %v", err) + } + }() // Wait for infrastructure status to update with custom endpoints. - err := wait.PollImmediate(1*time.Second, 15*time.Second, func() (bool, error) { + if err := wait.PollImmediate(1*time.Second, 15*time.Second, func() (bool, error) { if err := kclient.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, &infraConfig); err != nil { t.Logf("failed to get infrastructure config: %v\n", err) return false, err @@ -1829,17 +1842,9 @@ func TestIngressControllerCustomEndpoints(t *testing.T) { return false, nil } return true, nil - }) - if err != nil { + }); err != nil { t.Fatalf("failed to observe status update for infrastructure config %s", infraConfig.Name) } - defer func() { - // Remove the custom endpoints from the infrastructure config. - infraConfig.Spec.PlatformSpec.AWS = nil - if err := kclient.Update(context.TODO(), &infraConfig); err != nil { - t.Fatalf("failed to update infrastructure config: %v\n", err) - } - }() default: t.Skipf("skipping TestIngressControllerCustomEndpoints test due to platform type: %s", platform.Type) } @@ -2666,11 +2671,9 @@ func TestLocalWithFallbackOverrideForLoadBalancerService(t *testing.T) { t.Fatalf("failed to update ingresscontroller %q with override: %v", defaultName, err) } defer func() { - if err := kclient.Get(context.TODO(), defaultName, ic); err != nil { - t.Fatalf("failed to get ingresscontroller %q: %v", defaultName, err) - } - ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{} - if err := kclient.Update(context.TODO(), ic); err != nil { + if err := updateIngressControllerSpecWithRetryOnConflict(t, defaultName, timeout, func(spec *operatorv1.IngressControllerSpec) { + spec.UnsupportedConfigOverrides = runtime.RawExtension{} + }); err != nil { t.Fatalf("failed to update ingresscontroller %q to remove the override: %v", defaultName, err) } }() From cc23d9b9fd73cca8fcbb2d878000c7e53720c345 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Thu, 12 May 2022 10:40:58 +0100 Subject: [PATCH 7/8] test/e2e: remove TestCanaryRouteRotationAnnotation This tests uses an unsupported annotation. --- test/e2e/canary_test.go | 85 ----------------------------------------- 1 file changed, 85 deletions(-) diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go index 8e874429e9..3e4bfc6600 100644 --- a/test/e2e/canary_test.go +++ b/test/e2e/canary_test.go @@ -6,14 +6,10 @@ package e2e import ( "bufio" "context" - "fmt" - "strconv" "strings" "testing" "time" - "github.com/google/go-cmp/cmp" - operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" @@ -152,84 +148,3 @@ func buildCanaryCurlPod(name, namespace, image, host string) *corev1.Pod { }, } } - -// TestCanaryRouteRotationAnnotation verifies that the -// canary controller respects the canary route rotation -// annotation for the default ingress controller. -// -// Note that this test will mutate the default ingress controller -func TestCanaryRouteRotationAnnotation(t *testing.T) { - // Set the CanaryRouteRotation annotation to true - // on the default ingress controller. - if err := setDefaultIngressControllerRotationAnnotation(t, true); err != nil { - t.Fatalf("failed to set canary route rotation annotation: %v", err) - } - - // Cleanup default ingress controller by setting the canary rotation - // annotation to false (and verifying that we were successful in doing so). - defer func() { - if err := setDefaultIngressControllerRotationAnnotation(t, false); err != nil { - t.Fatalf("failed to set canary route rotation annotation: %v", err) - } - }() - - // Get canary route. - canaryRoute := &routev1.Route{} - name := controller.CanaryRouteName() - err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { - if err := kclient.Get(context.TODO(), name, canaryRoute); err != nil { - t.Logf("failed to get canary route %s: %v", name, err) - return false, nil - } - return true, nil - }) - - if err != nil { - t.Fatalf("failed to observe canary route: %v", err) - } - - // The canary controller should update the canary route after 5 successful - // canary checks. Canary checks happen once a minute. - updatedCanaryRoute := &routev1.Route{} - err = wait.PollImmediate(5*time.Second, 10*time.Minute, func() (bool, error) { - if err := kclient.Get(context.TODO(), name, updatedCanaryRoute); err != nil { - t.Logf("failed to get canary route %s: %v", name, err) - return false, nil - } - // If the canaryRoute and updatedCanaryRoute do not have the same targetPort, - // then the canary route rotation annotation is working. - if cmp.Equal(canaryRoute.Spec.Port.TargetPort, updatedCanaryRoute.Spec.Port.TargetPort) { - return false, nil - } - - return true, nil - }) - - if err != nil { - t.Fatalf("failed to observe canary route rotation: %v", err) - } -} - -func setDefaultIngressControllerRotationAnnotation(t *testing.T, val bool) error { - t.Helper() - ic := &operatorv1.IngressController{} - if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { - if err := kclient.Get(context.TODO(), defaultName, ic); err != nil { - t.Logf("Get failed: %v, retrying...", err) - return false, nil - } - if ic.Annotations == nil { - ic.Annotations = map[string]string{} - } - ic.Annotations[canarycontroller.CanaryRouteRotationAnnotation] = strconv.FormatBool(val) - if err := kclient.Update(context.TODO(), ic); err != nil { - t.Logf("failed to update ingress controller: %v", err) - return false, nil - } - return true, nil - }); err != nil { - return fmt.Errorf("failed to update ingress controller: %v", err) - } - - return nil -} From a449e497e35fafeecbee9ea656e0631393182f70 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Wed, 11 May 2022 12:52:07 +0100 Subject: [PATCH 8/8] Makefile: change test-e2e entrypoint to TestAll() Previously we ran all tests via the wildcard: TEST='.*'. The new TestALL e2e function groups tests that can be run in parallel and those that must run in series via two overarching subtests. The new TestALL function provides for consistent ordering of all e2e tests; serial tests will not start until the parallel tests complete. And those tests that can run in parallel can, and should, run in any order because, by definition, they are deemed to be self-contained. But some tests which are run in parallel are sensitive to any other test that modifies the ingress config object; changes to that object will cause a rolling update of all ingresscontroller's and some of the parallel tests do not accommodate such an event. Any test that does modify the ingress config object are always grouped into the serial subtests. --- Makefile | 7 +- hack/verify-e2e-test-all-presence.sh | 64 +++++++++++++++++++ test/e2e/all_test.go | 95 ++++++++++++++++++++++++++++ test/e2e/operator_test.go | 9 +++ 4 files changed, 174 insertions(+), 1 deletion(-) create mode 100755 hack/verify-e2e-test-all-presence.sh create mode 100644 test/e2e/all_test.go diff --git a/Makefile b/Makefile index 020719d383..c347fe0c32 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ endif GO=GO111MODULE=on GOFLAGS=-mod=vendor go GO_BUILD_RECIPE=CGO_ENABLED=0 $(GO) build -o $(BIN) $(GO_GCFLAGS) $(MAIN_PACKAGE) -TEST ?= .* +TEST ?= TestAll .PHONY: build build: @@ -55,6 +55,10 @@ release-local: test-e2e: $(GO) test -timeout 1h -count 1 -v -tags e2e -run "$(TEST)" ./test/e2e +.PHONY: test-e2e-list +test-e2e-list: + @(cd ./test/e2e; E2E_TEST_MAIN_SKIP_SETUP=1 $(GO) test -list . -tags e2e | grep ^Test | sort) + .PHONY: clean clean: $(GO) clean @@ -67,6 +71,7 @@ verify: hack/verify-profile-manifests.sh hack/verify-generated-bindata.sh hack/verify-deps.sh + hack/verify-e2e-test-all-presence.sh .PHONY: uninstall uninstall: diff --git a/hack/verify-e2e-test-all-presence.sh b/hack/verify-e2e-test-all-presence.sh new file mode 100755 index 0000000000..b13d4b4642 --- /dev/null +++ b/hack/verify-e2e-test-all-presence.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash + +# This script verifies that all the e2e tests defined in package +# test/e2e have a corresponding invocation in the TestAll function +# defined in test/e2e/all_test.go. +# +# The TestAll function provides ordering of all e2e tests. Tests that +# can be run in parallel are invoked first and will run to completion +# before starting those that must run serially. +# +# The CI job runs `make verify` before starting the e2e tests and the +# verify target will run this script. If this script detects any Go +# Test function by name that is not invoked by the TestAll function +# then it will list the omission and exit with an error, preventing +# the e2e tests from starting. + +# This script has been tested on Linux and macOS. + +set -u +set -o pipefail + +thisdir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +e2e_test_dir="$thisdir/../test/e2e" +e2e_test_file="$e2e_test_dir/all_test.go" + +if ! [[ -f ${e2e_test_file} ]]; then + echo "error: $e2e_test_file is missing." >&2 + exit 1 +fi + +# "go test -list" must run in the directory where the test files are. +pushd "$e2e_test_dir" >/dev/null || { + echo "error: pushd $e2e_test_dir failed" >&2; + exit 2 # ENOENT +} + +go_test_list_output=$(E2E_TEST_MAIN_SKIP_SETUP=1 go test -list . -tags e2e) +go_test_list_status=$? + +if [[ $go_test_list_status -ne 0 ]]; then + echo "error: go test -list failed" >&2 + echo "$go_test_list_output" >&2 + exit $go_test_list_status +fi + +popd >/dev/null || { + echo "error: popd failed" >&2; + exit 1 +} + +errors=0 + +for i in $go_test_list_output; do + if ! [[ $i =~ ^Test ]] || [[ $i == "TestAll" ]]; then + continue + fi + pattern="^ \+t\.Run(\"$i\", $i)$" # ^multiples. + if ! grep -q -e "$pattern" -- "$e2e_test_file"; then + echo "error: test function '$i' not called by 'TestAll' in $e2e_test_file" >&2 + errors=1 + fi +done + +exit $errors diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go new file mode 100644 index 0000000000..db0bf69ffe --- /dev/null +++ b/test/e2e/all_test.go @@ -0,0 +1,95 @@ +//go:build e2e +// +build e2e + +package e2e + +import "testing" + +// TestAll is the entrypoint for `make test-e2e` unless you override +// with: make TEST=Test test-e2e. +// +// The overriding goal of this test is to run as many tests in +// parallel as possible before running those tests that must run serially. There +// are two goals: 1) cut down on test execution time and 2) provide +// explicit ordering for tests that do not expect a rolling update of +// ingresscontroller pods because a previous test modified the +// ingressconfig object and the defer logic for cleanup is still +// runnng when the new test starts. +func TestAll(t *testing.T) { + // This call to Run() will not return until all of its + // parallel subtests complete. Each "parallel" test must + // invoke t.Parallel(). + t.Run("parallel", func(t *testing.T) { + t.Run("TestAWSELBConnectionIdleTimeout", TestAWSELBConnectionIdleTimeout) + t.Run("TestClientTLS", TestClientTLS) + t.Run("TestContainerLogging", TestContainerLogging) + t.Run("TestCreateIngressControllerThenSecret", TestCreateIngressControllerThenSecret) + t.Run("TestCreateSecretThenIngressController", TestCreateSecretThenIngressController) + t.Run("TestCustomErrorpages", TestCustomErrorpages) + t.Run("TestCustomIngressClass", TestCustomIngressClass) + t.Run("TestDynamicConfigManagerUnsupportedConfigOverride", TestDynamicConfigManagerUnsupportedConfigOverride) + t.Run("TestForwardedHeaderPolicyAppend", TestForwardedHeaderPolicyAppend) + t.Run("TestForwardedHeaderPolicyIfNone", TestForwardedHeaderPolicyIfNone) + t.Run("TestForwardedHeaderPolicyNever", TestForwardedHeaderPolicyNever) + t.Run("TestForwardedHeaderPolicyReplace", TestForwardedHeaderPolicyReplace) + t.Run("TestHAProxyTimeouts", TestHAProxyTimeouts) + t.Run("TestHAProxyTimeoutsRejection", TestHAProxyTimeoutsRejection) + t.Run("TestHTTPCookieCapture", TestHTTPCookieCapture) + t.Run("TestHTTPHeaderBufferSize", TestHTTPHeaderBufferSize) + t.Run("TestHTTPHeaderCapture", TestHTTPHeaderCapture) + t.Run("TestHeaderNameCaseAdjustment", TestHeaderNameCaseAdjustment) + t.Run("TestHealthCheckIntervalIngressController", TestHealthCheckIntervalIngressController) + t.Run("TestHostNetworkEndpointPublishingStrategy", TestHostNetworkEndpointPublishingStrategy) + t.Run("TestIngressControllerScale", TestIngressControllerScale) + t.Run("TestIngressControllerServiceNameCollision", TestIngressControllerServiceNameCollision) + t.Run("TestInternalLoadBalancer", TestInternalLoadBalancer) + t.Run("TestInternalLoadBalancerGlobalAccessGCP", TestInternalLoadBalancerGlobalAccessGCP) + t.Run("TestLoadBalancingAlgorithmUnsupportedConfigOverride", TestLoadBalancingAlgorithmUnsupportedConfigOverride) + t.Run("TestLocalWithFallbackOverrideForNodePortService", TestLocalWithFallbackOverrideForNodePortService) + t.Run("TestNetworkLoadBalancer", TestNetworkLoadBalancer) + t.Run("TestNodePortServiceEndpointPublishingStrategy", TestNodePortServiceEndpointPublishingStrategy) + t.Run("TestProxyProtocolAPI", TestProxyProtocolAPI) + t.Run("TestReloadIntervalUnsupportedConfigOverride", TestReloadIntervalUnsupportedConfigOverride) + t.Run("TestRouteAdmissionPolicy", TestRouteAdmissionPolicy) + t.Run("TestRouterCompressionParsing", TestRouterCompressionParsing) + t.Run("TestScopeChange", TestScopeChange) + t.Run("TestSyslogLogging", TestSyslogLogging) + t.Run("TestTLSSecurityProfile", TestTLSSecurityProfile) + t.Run("TestTunableMaxConnectionsInvalidValues", TestTunableMaxConnectionsInvalidValues) + t.Run("TestTunableMaxConnectionsValidValues", TestTunableMaxConnectionsValidValues) + t.Run("TestTunableRouterKubeletProbesForCustomIngressController", TestTunableRouterKubeletProbesForCustomIngressController) + t.Run("TestUniqueDomainRejection", TestUniqueDomainRejection) + t.Run("TestUniqueIdHeader", TestUniqueIdHeader) + t.Run("TestUserDefinedIngressController", TestUserDefinedIngressController) + }) + + t.Run("serial", func(t *testing.T) { + t.Run("TestDefaultIngressControllerSteadyConditions", TestDefaultIngressControllerSteadyConditions) + t.Run("TestClusterOperatorStatusRelatedObjects", TestClusterOperatorStatusRelatedObjects) + t.Run("TestConfigurableRouteNoConsumingUserNoRBAC", TestConfigurableRouteNoConsumingUserNoRBAC) + t.Run("TestConfigurableRouteNoSecretNoRBAC", TestConfigurableRouteNoSecretNoRBAC) + t.Run("TestConfigurableRouteRBAC", TestConfigurableRouteRBAC) + t.Run("TestDefaultIngressCertificate", TestDefaultIngressCertificate) + t.Run("TestDefaultIngressClass", TestDefaultIngressClass) + t.Run("TestHstsPolicyWorks", TestHstsPolicyWorks) + t.Run("TestIngressControllerCustomEndpoints", TestIngressControllerCustomEndpoints) + t.Run("TestIngressStatus", TestIngressStatus) + t.Run("TestLocalWithFallbackOverrideForLoadBalancerService", TestLocalWithFallbackOverrideForLoadBalancerService) + t.Run("TestOperatorSteadyConditions", TestOperatorSteadyConditions) + t.Run("TestPodDisruptionBudgetExists", TestPodDisruptionBudgetExists) + t.Run("TestProxyProtocolOnAWS", TestProxyProtocolOnAWS) + t.Run("TestRouteHTTP2EnableAndDisableIngressController", TestRouteHTTP2EnableAndDisableIngressController) + t.Run("TestRouteHardStopAfterEnableOnIngressController", TestRouteHardStopAfterEnableOnIngressController) + t.Run("TestRouteHardStopAfterTestInvalidDuration", TestRouteHardStopAfterTestInvalidDuration) + t.Run("TestRouteHardStopAfterTestOneDayDuration", TestRouteHardStopAfterTestOneDayDuration) + t.Run("TestRouteHardStopAfterTestZeroLengthDuration", TestRouteHardStopAfterTestZeroLengthDuration) + t.Run("TestRouteNbthreadIngressController", TestRouteNbthreadIngressController) + t.Run("TestRouterCompressionOperation", TestRouterCompressionOperation) + t.Run("TestUpdateDefaultIngressController", TestUpdateDefaultIngressController) + t.Run("TestCanaryRoute", TestCanaryRoute) + t.Run("TestRouteHTTP2EnableAndDisableIngressConfig", TestRouteHTTP2EnableAndDisableIngressConfig) + t.Run("TestRouteHardStopAfterEnableOnIngressConfig", TestRouteHardStopAfterEnableOnIngressConfig) + t.Run("TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig", TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig) + t.Run("TestHostNetworkPortBinding", TestHostNetworkPortBinding) + }) +} diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index de65aaf7b2..98c6ddb502 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -103,6 +103,15 @@ var defaultName = types.NamespacedName{Namespace: operatorNamespace, Name: manif var clusterConfigName = types.NamespacedName{Namespace: operatorNamespace, Name: manifests.ClusterIngressConfigName} func TestMain(m *testing.M) { + if os.Getenv("E2E_TEST_MAIN_SKIP_SETUP") == "1" { + // If we are deriving the set of tests via `go test + // -list` then we don't always have a KUBECONFIG + // (e.g., CI, or local dev) so we do none of the + // overall test setup below because calls to + // getConfig() will fail and no test names will be + // discovered. + os.Exit(m.Run()) + } kubeConfig, err := config.GetConfig() if err != nil { fmt.Printf("failed to get kube config: %s\n", err)