Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using
the respective domain of any shard that exposes the route.

Before this commit, the canary route specified neither spec.subdomain nor
spec.host, and so the API server would set a default value for spec.host
using the cluster ingress domain.  If the cluster ingress domain didn't
match the default IngressController's domain, then this would cause canary
checks to fail.

For example, the cluster-admin could end up in this situation by first
using the cluster ingress config's appsDomain option to set a custom domain
different from the domain of the default IngressController and then
deleting the canary route so that it would be recreated with the custom
domain.  This commit ensures that the canary route continues to work in
this example.

This commit fixes OCPBUGS-16089.

https://issues.redhat.com/browse/OCPBUGS-16089

* pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):
* pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new
getRouteHost helper function.
* pkg/operator/controller/canary/route.go (canaryRouteChanged): Check
whether spec.host or spec.subdomain have changed, and update them if they
have.
(desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function.  Return the host name of the route for the
default IngressController.
* pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute):
Verify that the route has the expected value for spec.subdomain.
(Test_canaryRouteChanged): Verify that canaryRouteChanged checks and
updates the spec.host and spec.subdomain fields.
(Test_getRouteHost): New test.  Verify that getRouteHost behaves correctly.
* test/e2e/canary_test.go (TestCanaryRoute):
* test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):
* test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):
* test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test
helper function.
* test/e2e/util_test.go (getRouteHost): New test helper function.  Return
the host name of the route for the named IngressController.

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,6 +1,7 @@
 package canary
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -33,6 +34,7 @@ func Test_desiredCanaryRoute(t *testing.T) {
 
 	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",
@@ -77,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) {
@@ -125,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,
 					},
 				},
 			},

test/e2e/router_compression_test.go

@@ -198,38 +198,40 @@ 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)
 	}
 }
 
 // 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()