OCPBUGS-9037: If mTLS is required and the canary receives a "certificate required" TLS error, consider the canary probe successful

pkg/operator/controller/canary/controller.go

@@ -277,7 +277,7 @@ func (r *reconciler) startCanaryRoutePolling(stop <-chan struct{}) error {
 			return
 		}
 
-		err = probeRouteEndpoint(route)
+		err = r.probeRouteEndpoint(route)
 		if err != nil {
 			log.Error(err, "error performing canary route check")
 			SetCanaryRouteReachableMetric(getRouteHost(route), false)
@@ -428,3 +428,16 @@ func cycleServicePort(service *corev1.Service, route *routev1.Route) (*routev1.R
 
 	return updated, nil
 }
+
+func (r *reconciler) isMTLSRequired() (bool, error) {
+	ic := &operatorv1.IngressController{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      manifests.DefaultIngressControllerName,
+			Namespace: r.config.Namespace,
+		},
+	}
+	if err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: ic.Namespace, Name: ic.Name}, ic); err != nil {
+		return false, fmt.Errorf("failed to get ingress controller %s: %v", ic.Name, err)
+	}
+	return ic.Spec.ClientTLS.ClientCertificatePolicy == operatorv1.ClientCertificatePolicyRequired, nil
+}

pkg/operator/controller/canary/http.go

@@ -22,7 +22,7 @@ const (
 
 // probeRouteEndpoint probes the given route's host
 // and returns an error when applicable.
-func probeRouteEndpoint(route *routev1.Route) error {
+func (r *reconciler) probeRouteEndpoint(route *routev1.Route) error {
 	routeHost := getRouteHost(route)
 	if len(routeHost) == 0 {
 		return fmt.Errorf("route host is empty, cannot test route")
@@ -48,7 +48,7 @@ func probeRouteEndpoint(route *routev1.Route) error {
 	request = request.WithContext(ctx)
 
 	// Send the HTTP request
-	timeout, _ := time.ParseDuration("10s")
+	timeout := 10 * time.Second
 	client := &http.Client{
 		Timeout: timeout,
 		// The canary route uses edge termination and the
@@ -68,18 +68,40 @@ func probeRouteEndpoint(route *routev1.Route) error {
 	response, err := client.Do(request)
 
 	if err != nil {
-		// Check if err is a DNS error
+		// Check if err is a DNS error.
 		dnsErr := &net.DNSError{}
 		if errors.As(err, &dnsErr) {
 			// Handle DNS error
 			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
+		// Check if err is a timeout error.
 		if os.IsTimeout(err) {
-			// Handle timeout error
+			// Handle timeout error.
 			return fmt.Errorf("error sending canary HTTP Request: Timeout: %v", err)
 		}
+		// Check if err is a TLS alert.
+		opErr := &net.OpError{}
+		if errors.As(err, &opErr) {
+			if opErr.Op == "remote error" {
+				// If the operation is "remote error," it may be a TLS alert, as described in RFC 8446 section 6.
+				// crypto/tls doesn't expose its TLS alert type, but we can check the error string. If the alert is
+				// "certificate required", it means the canary lacks the client certificate necessary to complete its
+				// check. In that case, verify that the router is configured to require mTLS, and if so, assume the
+				// router is working as intended.
+				if opErr.Err.Error() == "tls: certificate required" {
+					if mtlsRequired, mtlsErr := r.isMTLSRequired(); mtlsErr != nil {
+						// Log the error from isMTLSRequired(), but continue the function so the actual request error is
+						// returned.
+						log.Error(mtlsErr, "Failed to verify mTLS status of default ingress controller")
+					} else if mtlsRequired {
+						// Since mTLS is required in the ingress config and our probe was rejected due to a missing
+						// certificate, consider this a successful probe.
+						return nil
+					}
+				}
+			}
+		}
 		return fmt.Errorf("error sending canary HTTP request to %q: %v", routeHost, err)
 	}
 

test/e2e/all_test.go

@@ -108,6 +108,7 @@ func TestAll(t *testing.T) {
 		t.Run("TestRouterCompressionOperation", TestRouterCompressionOperation)
 		t.Run("TestUpdateDefaultIngressControllerSecret", TestUpdateDefaultIngressControllerSecret)
 		t.Run("TestCanaryRoute", TestCanaryRoute)
+		t.Run("TestCanaryWithMTLS", TestCanaryWithMTLS)
 		t.Run("TestRouteHTTP2EnableAndDisableIngressConfig", TestRouteHTTP2EnableAndDisableIngressConfig)
 		t.Run("TestRouteHardStopAfterEnableOnIngressConfig", TestRouteHardStopAfterEnableOnIngressConfig)
 		t.Run("TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig", TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig)

test/e2e/canary_test.go

@@ -6,15 +6,18 @@ package e2e
 import (
 	"bufio"
 	"context"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
 
+	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	routev1 "github.com/openshift/api/route/v1"
 
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 	canarycontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/canary"
+	ingresscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress"
 
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 
@@ -160,3 +163,98 @@ func buildCanaryCurlPod(name, namespace, image, host string) *corev1.Pod {
 		},
 	}
 }
+
+func TestCanaryWithMTLS(t *testing.T) {
+	defaultIC := &operatorv1.IngressController{}
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, defaultAvailableConditions...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	caCert := MustCreateTLSKeyCert("root-ca", time.Now(), time.Now().Add(24*time.Hour), true, nil, nil)
+
+	clientCAConfigmap := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "canary-with-mtls-client-ca",
+			Namespace: "openshift-config",
+		},
+		Data: map[string]string{
+			"ca-bundle.pem": caCert.CertPem,
+		},
+	}
+
+	if err := kclient.Create(context.TODO(), clientCAConfigmap); err != nil {
+		t.Fatalf("Failed to create configmap %s: %v", clientCAConfigmap.Name, err)
+	}
+
+	defer assertDeleted(t, kclient, clientCAConfigmap)
+
+	if err := kclient.Get(context.TODO(), defaultName, defaultIC); err != nil {
+		t.Fatalf("failed to get default ingresscontroller: %v", err)
+	}
+
+	// Save the current clientTLS config so it can be restored at the end of the test.
+	originalClientTLS := defaultIC.Spec.ClientTLS
+
+	// Set client TLS (mTLS) to required, and use clientCAConfigmap as the CA bundle.
+	defaultIC.Spec.ClientTLS = operatorv1.ClientTLS{
+		ClientCertificatePolicy: operatorv1.ClientCertificatePolicyRequired,
+		ClientCA: configv1.ConfigMapNameReference{
+			Name: clientCAConfigmap.Name,
+		},
+	}
+	if err := kclient.Update(context.TODO(), defaultIC); err != nil {
+		t.Fatalf("Failed to enable mTLS on default ingress controller: %v", err)
+	}
+
+	defer func() {
+		// Reset client TLS.
+		if err := kclient.Get(context.TODO(), defaultName, defaultIC); err != nil {
+			panic(fmt.Errorf("failed to get default ingresscontroller: %v", err))
+		}
+		defaultIC.Spec.ClientTLS = originalClientTLS
+		if err := kclient.Update(context.TODO(), defaultIC); err != nil {
+			panic(fmt.Errorf("Failed to restore default ingress configuration: %w", err))
+		}
+	}()
+
+	// Wait for IC update to completely roll out.
+	routerDeployment := &appsv1.Deployment{}
+	routerDeploymentName := controller.RouterDeploymentName(defaultIC)
+	if err := kclient.Get(context.TODO(), routerDeploymentName, routerDeployment); err != nil {
+		t.Fatalf("Failed to get router deployment for ingresscontroller %s: %v", defaultIC.Name, err)
+	}
+
+	if err := waitForDeploymentComplete(t, kclient, routerDeployment, 3*time.Minute); err != nil {
+		t.Fatalf("Timed out waiting for ingress update to complete: %v", err)
+	}
+
+	// Verify that canary does not fail.
+	err := wait.PollImmediate(10*time.Second, 6*time.Minute, func() (bool, error) {
+		ic := &operatorv1.IngressController{}
+		if err := kclient.Get(context.TODO(), defaultName, ic); err != nil {
+			return false, err
+		}
+		foundCondition := false
+		for _, condition := range ic.Status.Conditions {
+			if condition.Type == ingresscontroller.IngressControllerCanaryCheckSuccessConditionType {
+				foundCondition = true
+				if condition.Status == operatorv1.ConditionFalse {
+					t.Errorf("Canary failed: %s", condition.Reason)
+					return true, nil
+				}
+				break
+			}
+		}
+		if !foundCondition {
+			t.Errorf("Canary status condition not found")
+			return false, nil
+		}
+		return false, nil
+	})
+
+	// Timeout means the canary succeeded for the entire polling interval, so only consider non-timeout errors a
+	// failure.
+	if err != nil && err != wait.ErrWaitTimeout {
+		t.Fatalf("Got unexpected error %v", err)
+	}
+}