diff --git a/pkg/operator/controller/canary/controller.go b/pkg/operator/controller/canary/controller.go index d677a1944d..22982bc482 100644 --- a/pkg/operator/controller/canary/controller.go +++ b/pkg/operator/controller/canary/controller.go @@ -280,7 +280,7 @@ func (r *reconciler) startCanaryRoutePolling(stop <-chan struct{}) error { err = probeRouteEndpoint(route) if err != nil { log.Error(err, "error performing canary route check") - SetCanaryRouteReachableMetric(route.Spec.Host, false) + SetCanaryRouteReachableMetric(getRouteHost(route), false) successiveFail += 1 // Mark the default ingress controller degraded after 5 successive canary check failures if successiveFail >= canaryCheckFailureCount { @@ -291,7 +291,7 @@ func (r *reconciler) startCanaryRoutePolling(stop <-chan struct{}) error { return } - SetCanaryRouteReachableMetric(route.Spec.Host, true) + SetCanaryRouteReachableMetric(getRouteHost(route), true) if err := r.setCanaryPassingStatusCondition(); err != nil { log.Error(err, "error updating canary status condition") } diff --git a/pkg/operator/controller/canary/http.go b/pkg/operator/controller/canary/http.go index 55c04cd675..2f3bf9e397 100644 --- a/pkg/operator/controller/canary/http.go +++ b/pkg/operator/controller/canary/http.go @@ -23,8 +23,9 @@ const ( // probeRouteEndpoint probes the given route's host // and returns an error when applicable. func probeRouteEndpoint(route *routev1.Route) error { - if len(route.Spec.Host) == 0 { - return fmt.Errorf("route.Spec.Host is empty, cannot test route") + routeHost := getRouteHost(route) + if len(routeHost) == 0 { + return fmt.Errorf("route host is empty, cannot test route") } // Create HTTP request @@ -33,7 +34,7 @@ func probeRouteEndpoint(route *routev1.Route) error { // via an external load balancer drop all traffic on port 80, // in which case redirecting insecure traffic is not possible. // See https://bugzilla.redhat.com/show_bug.cgi?id=1934773. - request, err := http.NewRequest("GET", "https://"+route.Spec.Host, nil) + request, err := http.NewRequest("GET", "https://"+routeHost, nil) if err != nil { return fmt.Errorf("error creating canary HTTP request %v: %v", request, err) } @@ -71,7 +72,7 @@ func probeRouteEndpoint(route *routev1.Route) error { dnsErr := &net.DNSError{} if errors.As(err, &dnsErr) { // Handle DNS error - CanaryRouteDNSError.WithLabelValues(route.Spec.Host, dnsErr.Server).Inc() + CanaryRouteDNSError.WithLabelValues(routeHost, dnsErr.Server).Inc() return fmt.Errorf("error sending canary HTTP request: DNS error: %v", err) } // Check if err is a timeout error @@ -79,7 +80,7 @@ func probeRouteEndpoint(route *routev1.Route) error { // Handle timeout error return fmt.Errorf("error sending canary HTTP Request: Timeout: %v", err) } - return fmt.Errorf("error sending canary HTTP request to %q: %v", route.Spec.Host, err) + return fmt.Errorf("error sending canary HTTP request to %q: %v", routeHost, err) } // Close response body even if read fails @@ -121,7 +122,7 @@ func probeRouteEndpoint(route *routev1.Route) error { switch status := response.StatusCode; status { case http.StatusOK: // Register total time in metrics (use milliseconds) - CanaryRequestTime.WithLabelValues(route.Spec.Host).Observe(float64(totalTime.Milliseconds())) + CanaryRequestTime.WithLabelValues(routeHost).Observe(float64(totalTime.Milliseconds())) case http.StatusRequestTimeout: return fmt.Errorf("status code %d: request timed out", status) case http.StatusServiceUnavailable: diff --git a/pkg/operator/controller/canary/route.go b/pkg/operator/controller/canary/route.go index 532e70093f..da587e3037 100644 --- a/pkg/operator/controller/canary/route.go +++ b/pkg/operator/controller/canary/route.go @@ -101,6 +101,16 @@ func canaryRouteChanged(current, expected *routev1.Route) (bool, *routev1.Route) changed := false updated := current.DeepCopy() + if current.Spec.Host != expected.Spec.Host { + updated.Spec.Host = expected.Spec.Host + changed = true + } + + if current.Spec.Subdomain != expected.Spec.Subdomain { + updated.Spec.Subdomain = expected.Spec.Subdomain + changed = true + } + if !cmp.Equal(current.Spec.Port, expected.Spec.Port, cmpopts.EquateEmpty()) { updated.Spec.Port = expected.Spec.Port changed = true @@ -131,6 +141,7 @@ func desiredCanaryRoute(service *corev1.Service) (*routev1.Route, error) { route.Namespace = name.Namespace route.Name = name.Name + route.Spec.Subdomain = fmt.Sprintf("%s-%s", route.Name, route.Namespace) if service == nil { return route, fmt.Errorf("expected non-nil canary service for canary route %s/%s", route.Namespace, route.Name) @@ -175,3 +186,23 @@ func checkRouteAdmitted(route *routev1.Route) bool { return false } + +// getRouteHost returns the host name of the route for the default +// IngressController. If the default IngressController has not added its host +// name to the route's status, this function returns the empty string. For +// simplicity, this function does not check the "Admitted" status condition, so +// it will return the host name if it is set even if the IngressController has +// rejected the route. +func getRouteHost(route *routev1.Route) string { + if route == nil { + return "" + } + + for _, ingress := range route.Status.Ingress { + if ingress.RouterName == manifests.DefaultIngressControllerName { + return ingress.Host + } + } + + return "" +} diff --git a/pkg/operator/controller/canary/route_test.go b/pkg/operator/controller/canary/route_test.go index 23696e0a07..5a62010ada 100644 --- a/pkg/operator/controller/canary/route_test.go +++ b/pkg/operator/controller/canary/route_test.go @@ -1,9 +1,11 @@ package canary import ( + "fmt" "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" routev1 "github.com/openshift/api/route/v1" @@ -30,34 +32,21 @@ func Test_desiredCanaryRoute(t *testing.T) { Name: "canary", } - if !cmp.Equal(route.Name, expectedRouteName.Name) { - t.Errorf("expected route name to be %s, but got %s", expectedRouteName.Name, route.Name) - } - - if !cmp.Equal(route.Namespace, expectedRouteName.Namespace) { - t.Errorf("expected route namespace to be %s, but got %s", expectedRouteName.Namespace, route.Namespace) - } + assert.Equal(t, route.Name, expectedRouteName.Name, "unexpected route name") + assert.Equal(t, route.Namespace, expectedRouteName.Namespace, "unexpected route namespace") + assert.Equal(t, route.Spec.Subdomain, "canary-openshift-ingress-canary", "unexpected route spec.subdomain") expectedAnnotations := map[string]string{ "haproxy.router.openshift.io/balance": "roundrobin", } - - if !cmp.Equal(route.Annotations, expectedAnnotations) { - t.Errorf("expected route annotations to be %s, but got %s", expectedAnnotations, route.Annotations) - } + assert.Equal(t, route.Annotations, expectedAnnotations, "unexpected route annotations") expectedLabels := map[string]string{ manifests.OwningIngressCanaryCheckLabel: canaryControllerName, } + assert.Equal(t, route.Labels, expectedLabels, "unexpected route labels") - if !cmp.Equal(route.Labels, expectedLabels) { - t.Errorf("expected route labels to be %q, but got %q", expectedLabels, route.Labels) - } - - routeToName := route.Spec.To.Name - if !cmp.Equal(routeToName, service.Name) { - t.Errorf("expected route.Spec.To.Name to be %q, but got %q", service.Name, routeToName) - } + assert.Equal(t, route.Spec.To.Name, service.Name, "route's spec.to.name does not match service name") routeTarget := route.Spec.Port.TargetPort validTarget := false @@ -66,23 +55,17 @@ func Test_desiredCanaryRoute(t *testing.T) { validTarget = true } } - - if !validTarget { - t.Errorf("expected %v to be a port in the %v. Route targetPort not in service targetPort list", route.Spec.Port.TargetPort, service.Spec.Ports) - } + assert.True(t, validTarget, "route's target port does not match any of the service's target ports: expected %v to match some port in %v", route.Spec.Port.TargetPort, service.Spec.Ports) expectedOwnerRefs := []metav1.OwnerReference{daemonsetRef} - if !cmp.Equal(route.OwnerReferences, expectedOwnerRefs) { - t.Errorf("expected service owner references %#v, but got %#v", expectedOwnerRefs, route.OwnerReferences) - } + assert.Equal(t, route.OwnerReferences, expectedOwnerRefs, "unexpected route owner references") + assert.Equal(t, service.OwnerReferences, expectedOwnerRefs, "unexpected service owner references") expectedTLS := &routev1.TLSConfig{ Termination: routev1.TLSTerminationEdge, InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect, } - if !cmp.Equal(route.Spec.TLS, expectedTLS) { - t.Errorf("expected route TLS config to be %v, but got %v", route.Spec.TLS, expectedTLS) - } + assert.Equal(t, route.Spec.TLS, expectedTLS, "unexpected route TLS config") } func Test_canaryRouteChanged(t *testing.T) { @@ -96,6 +79,20 @@ func Test_canaryRouteChanged(t *testing.T) { mutate: func(_ *routev1.Route) {}, expect: false, }, + { + description: "if route spec.host is changes", + mutate: func(route *routev1.Route) { + route.Spec.Host = "test" + }, + expect: true, + }, + { + description: "if route spec.subdomain changes", + mutate: func(route *routev1.Route) { + route.Spec.Subdomain = "test" + }, + expect: true, + }, { description: "if route spec.To changes", mutate: func(route *routev1.Route) { @@ -144,3 +141,67 @@ func Test_canaryRouteChanged(t *testing.T) { }) } } + +// Test_getRouteHost verifies that getRouteHost returns the expected value for a +// route. +func Test_getRouteHost(t *testing.T) { + canaryRoute := func(ingresses []routev1.RouteIngress) *routev1.Route { + return &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "openshift-ingress-canary", + Name: "canary", + }, + Status: routev1.RouteStatus{ + Ingress: ingresses, + }, + } + } + admittedBy := func(names ...string) []routev1.RouteIngress { + ingresses := []routev1.RouteIngress{} + for _, name := range names { + ingress := routev1.RouteIngress{ + RouterName: name, + Host: fmt.Sprintf("%s.apps.example.xyz", name), + } + ingresses = append(ingresses, ingress) + } + return ingresses + } + testCases := []struct { + name string + route *routev1.Route + expect string + }{ + { + name: "nil route", + route: nil, + expect: "", + }, + { + name: "not admitted route", + route: canaryRoute(admittedBy()), + expect: "", + }, + { + name: "admitted by some other ingresscontroller", + route: canaryRoute(admittedBy("foo")), + expect: "", + }, + { + name: "admitted by default ingresscontroller", + route: canaryRoute(admittedBy("default")), + expect: "default.apps.example.xyz", + }, + { + name: "admitted by default and others", + route: canaryRoute(admittedBy("foo", "default", "bar")), + expect: "default.apps.example.xyz", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expect, getRouteHost(tc.route)) + }) + } +} diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go index 12619a1139..ae9160577b 100644 --- a/test/e2e/canary_test.go +++ b/test/e2e/canary_test.go @@ -68,13 +68,14 @@ func TestCanaryRoute(t *testing.T) { return true, nil }) - if err != nil { t.Fatalf("failed to observe canary route: %v", err) } + canaryRouteHost := getRouteHost(t, canaryRoute, defaultName.Name) + image := deployment.Spec.Template.Spec.Containers[0].Image - clientPod := buildCanaryCurlPod("canary-route-check", canaryRoute.Namespace, image, canaryRoute.Spec.Host) + clientPod := buildCanaryCurlPod("canary-route-check", canaryRoute.Namespace, image, canaryRouteHost) if err := kclient.Create(context.TODO(), clientPod); err != nil { t.Fatalf("failed to create pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) } diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index 0e4317fe88..16672f2246 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -138,6 +138,8 @@ func TestClientTLS(t *testing.T) { t.Fatalf("failed to observe route %q: %v", routeName, err) } + routeHost := getRouteHost(t, route, ic.Name) + // We need an IP address to which to send requests. The test client // runs inside the cluster, so we can use the custom router's internal // service address. @@ -260,7 +262,7 @@ func TestClientTLS(t *testing.T) { expectAllowed: false, }} for _, tc := range optionalPolicyTestCases { - _, err := curlGetStatusCode(t, clientPod, tc.cert, route.Spec.Host, service.Spec.ClusterIP, true) + _, err := curlGetStatusCode(t, clientPod, tc.cert, routeHost, service.Spec.ClusterIP, true) if err == nil && !tc.expectAllowed { t.Errorf("%q: expected error, got success", tc.description) } @@ -306,7 +308,7 @@ func TestClientTLS(t *testing.T) { expectAllowed: false, }} for _, tc := range requiredPolicyTestCases { - _, err := curlGetStatusCode(t, clientPod, tc.cert, route.Spec.Host, service.Spec.ClusterIP, true) + _, err := curlGetStatusCode(t, clientPod, tc.cert, routeHost, service.Spec.ClusterIP, true) if err == nil && !tc.expectAllowed { t.Errorf("%q: expected error, got success", tc.description) } @@ -1010,6 +1012,8 @@ func TestMTLSWithCRLs(t *testing.T) { t.Fatalf("failed to observe route %q: %v", routeName, err) } + routeHost := getRouteHost(t, route, ic.Name) + // If the canary route is used, normally the default ingress controller will handle the request, but by // using curl's --resolve flag, we can send an HTTP request intended for the canary pod directly to our // ingress controller instead. In order to do that, we need the ingress controller's service IP. @@ -1020,12 +1024,12 @@ func TestMTLSWithCRLs(t *testing.T) { } for certName := range tcCerts.ClientCerts.Accepted { - if _, err := curlGetStatusCode(t, clientPod, certName, route.Spec.Host, service.Spec.ClusterIP, false); err != nil { + if _, err := curlGetStatusCode(t, clientPod, certName, routeHost, service.Spec.ClusterIP, false); err != nil { t.Errorf("Failed to curl route with cert %q: %v", certName, err) } } for certName := range tcCerts.ClientCerts.Rejected { - if httpStatusCode, err := curlGetStatusCode(t, clientPod, certName, route.Spec.Host, service.Spec.ClusterIP, false); err != nil { + if httpStatusCode, err := curlGetStatusCode(t, clientPod, certName, routeHost, service.Spec.ClusterIP, false); err != nil { if httpStatusCode == 0 { // TLS/SSL verification failures result in a 0 http status code (no connection is made to the backend, so no http status code is returned). continue diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 7b073e444c..dd2b2063c8 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -2584,6 +2584,7 @@ func TestHTTPHeaderCapture(t *testing.T) { if err := kclient.Get(context.TODO(), routeName, route); err != nil { t.Fatalf("failed to get the console route: %v", err) } + routeHost := getRouteHost(t, route, ic.Name) clientPod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "headertest", @@ -2601,8 +2602,8 @@ func TestHTTPHeaderCapture(t *testing.T) { "-H", "x-test-header-1:foo", "-H", "x-test-header-2:bar", "--resolve", - route.Spec.Host + ":443:" + podList.Items[0].Status.PodIP, - "https://" + route.Spec.Host, + routeHost + ":443:" + podList.Items[0].Status.PodIP, + "https://" + routeHost, }, }, }, @@ -2724,6 +2725,7 @@ func TestHTTPCookieCapture(t *testing.T) { if err := kclient.Get(context.TODO(), routeName, route); err != nil { t.Fatalf("failed to get the console route: %v", err) } + routeHost := getRouteHost(t, route, ic.Name) clientPod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "cookietest", @@ -2742,8 +2744,8 @@ func TestHTTPCookieCapture(t *testing.T) { "-H", "cookie:foo=xyzzypop", "-H", "cookie:foobaz=abc", "--resolve", - route.Spec.Host + ":443:" + podList.Items[0].Status.PodIP, - "https://" + route.Spec.Host, + routeHost + ":443:" + podList.Items[0].Status.PodIP, + "https://" + routeHost, }, }, }, diff --git a/test/e2e/router_compression_test.go b/test/e2e/router_compression_test.go index c1c133d2de..b36d90518c 100644 --- a/test/e2e/router_compression_test.go +++ b/test/e2e/router_compression_test.go @@ -198,13 +198,15 @@ func TestRouterCompressionOperation(t *testing.T) { t.Fatalf("failed to get route: %v", err) } + routeHost := getRouteHost(t, r, ic.Name) + // curl to canary, without the Accept-Encoding header set to gzip - if err := testContentEncoding(t, client, r, false, ""); err != nil { + if err := testContentEncoding(t, client, routeHost, false, ""); err != nil { t.Error(err) } // curl to canary, WITH the Accept-Encoding header set to gzip - if err := testContentEncoding(t, client, r, true, "gzip"); err != nil { + if err := testContentEncoding(t, client, routeHost, true, "gzip"); err != nil { t.Error(err) } } @@ -212,24 +214,24 @@ func TestRouterCompressionOperation(t *testing.T) { // testContentEncoding makes a call to the provided route, adds a gzip content header if addHeader is true, and // compares the returned Content-Encoding header to the given expectedContentEncoding. If expectedContentEncoding // is the same as the returned Content-Encoding header, then the test succeeds. Otherwise it fails. -func testContentEncoding(t *testing.T, client *http.Client, route *routev1.Route, addHeader bool, expectedContentEncoding string) error { +func testContentEncoding(t *testing.T, client *http.Client, routeHost string, addHeader bool, expectedContentEncoding string) error { t.Helper() if err := wait.PollImmediate(2*time.Second, 5*time.Minute, func() (bool, error) { - header, code, err := getHttpHeaders(client, route, addHeader) + header, code, err := getHttpHeaders(client, routeHost, addHeader) if err != nil { - t.Logf("GET %s failed: %v, retrying...", route.Spec.Host, err) + t.Logf("GET %s failed: %v, retrying...", routeHost, err) return false, nil } if code != http.StatusOK { - t.Logf("GET %s failed: status %v, expected %v, retrying...", route.Spec.Host, code, http.StatusOK) + t.Logf("GET %s failed: status %v, expected %v, retrying...", routeHost, code, http.StatusOK) return false, nil // retry on 503 as pod/service may not be ready } contentEncoding := header.Get("Content-Encoding") if contentEncoding != expectedContentEncoding { - return false, fmt.Errorf("compression error: expected %q, got %q for %s route", expectedContentEncoding, contentEncoding, route.Name) + return false, fmt.Errorf("compression error: expected %q, got %q for %s route", expectedContentEncoding, contentEncoding, routeHost) } return true, nil }); err != nil { @@ -239,9 +241,9 @@ func testContentEncoding(t *testing.T, client *http.Client, route *routev1.Route } // getHttpHeaders returns the HTTP Headers, and first adds the request header "Accept-Encoding: gzip" if requested. -func getHttpHeaders(client *http.Client, route *routev1.Route, addHeader bool) (http.Header, int, error) { +func getHttpHeaders(client *http.Client, routeHost string, addHeader bool) (http.Header, int, error) { // Create the HTTPS request - request, err := http.NewRequest("GET", "https://"+route.Spec.Host, nil) + request, err := http.NewRequest("GET", "https://"+routeHost, nil) if err != nil { return nil, -1, fmt.Errorf("New request failed: %v", err) } @@ -253,7 +255,7 @@ func getHttpHeaders(client *http.Client, route *routev1.Route, addHeader bool) ( response, err := client.Do(request) if err != nil { - return nil, -1, fmt.Errorf("GET %s failed: %v", route.Spec.Host, err) + return nil, -1, fmt.Errorf("GET %s failed: %v", routeHost, err) } // Close response body defer response.Body.Close() diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index 145ca0584f..2a68b758f1 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -688,3 +688,27 @@ 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) } } + +// getRouteHost returns the host name of the given route for the named +// IngressController. If the IngressController has not added its host name to +// the route's status, this function causes a fatal error. For simplicity, this +// function does not check the "Admitted" status condition, so it will return +// the host name if it is set even if the IngressController has rejected the +// route. +func getRouteHost(t *testing.T, route *routev1.Route, router string) string { + t.Helper() + + if route == nil { + t.Fatal("route is nil") + return "" + } + + for _, ingress := range route.Status.Ingress { + if ingress.RouterName == router { + return ingress.Host + } + } + + t.Fatalf("failed to find host name for default router in route: %#v", route) + return "" +}