diff --git a/pkg/operator/controller/canary/controller.go b/pkg/operator/controller/canary/controller.go index 22982bc482..caa8b29cc1 100644 --- a/pkg/operator/controller/canary/controller.go +++ b/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 +} diff --git a/pkg/operator/controller/canary/http.go b/pkg/operator/controller/canary/http.go index 2f3bf9e397..9df6601052 100644 --- a/pkg/operator/controller/canary/http.go +++ b/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) } diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index 74c26d9554..bb4dfa121a 100644 --- a/test/e2e/all_test.go +++ b/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) diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go index ae9160577b..63056ecdc1 100644 --- a/test/e2e/canary_test.go +++ b/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) + } +}