From a6b80f3ce1c877727bcc1960bb57f9be99aee380 Mon Sep 17 00:00:00 2001 From: Ryan Fredette Date: Mon, 3 Jul 2023 14:50:28 -0400 Subject: [PATCH 1/2] If mTLS is required and the canary receives a "certificate required" TLS error, consider the canary probe successful. Without an API to provide the canary with a valid client certificate, the canary probes are guaranteed to fail when mTLS is required by the default ingress controller. However, if the failure is determined to be because the canary is missing a client certificate, it's reasonable to assume that the ingress controller is functional. This change checks specifically for the TLS alert "certificate required", and if the default ingress controller also has mTLS required, the probe function returns nil, indicating a successful probe. --- pkg/operator/controller/canary/controller.go | 15 ++++++++- pkg/operator/controller/canary/http.go | 32 +++++++++++++++++--- 2 files changed, 41 insertions(+), 6 deletions(-) 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) } From 4dc90073eae9958d284a40b633904ae93bfc64d5 Mon Sep 17 00:00:00 2001 From: Ryan Fredette Date: Mon, 24 Jul 2023 14:32:36 -0400 Subject: [PATCH 2/2] Add a test to verify that enabling MTLS doesn't make canary degraded --- test/e2e/all_test.go | 1 + test/e2e/canary_test.go | 98 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) 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) + } +}