OCPBUGS-16089: Set spec.subdomain on the canary route

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")
 		}

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,15 +72,15 @@ 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
 		if os.IsTimeout(err) {
 			// 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:

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 ""
+}

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))
+		})
+	}
+}

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)
 	}

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

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,
 					},
 				},
 			},