Merge pull request #142 from enj/enj/i/handle_degraded_conditions

Bug 1725935: Split handling of degraded across multiple conditions

Author: openshift-merge-robot

pkg/operator2/oauth.go

@@ -82,9 +82,7 @@ func (c *authOperator) handleOAuthConfig(
 			},
 		)
 	}
-	if err := v1helpers.NewMultiLineAggregate(errsIDP); err != nil {
-		setDegradedTrue(operatorConfig, "IdentityProviderConfigError", err.Error())
-	}
+	handleDegraded(operatorConfig, "IdentityProviderConfig", v1helpers.NewMultiLineAggregate(errsIDP))
 
 	assetPublicURL, corsAllowedOrigins := consoleToDeploymentData(consoleConfig)
 

pkg/operator2/operator.go

@@ -252,13 +252,13 @@ func (c *authOperator) Sync(obj metav1.Object) error {
 
 	operatorConfigCopy := operatorConfig.DeepCopy()
 
-	// clear degraded status
-	setDegradedFalse(operatorConfigCopy)
-
 	syncErr := c.handleSync(operatorConfigCopy)
-	if syncErr != nil {
-		setDegradedTrue(operatorConfigCopy, "OperatorSyncLoopError", syncErr.Error())
+	// this is a catch all degraded state that we only set when we are otherwise not degraded
+	globalDegradedErr := syncErr
+	if isDegraded(operatorConfigCopy) {
+		globalDegradedErr = nil // unset because we are already degraded for some other reason
 	}
+	handleDegraded(operatorConfigCopy, "OperatorSync", globalDegradedErr)
 
 	if _, _, err := v1helpers.UpdateStatus(c.authOperatorConfigClient, func(status *operatorv1.OperatorStatus) error {
 		// store a copy of our starting conditions, we need to preserve last transition time
@@ -313,6 +313,7 @@ func (c *authOperator) handleSync(operatorConfig *operatorv1.Authentication) err
 	resourceVersions = append(resourceVersions, ingress.GetResourceVersion())
 
 	route, routerSecret, err := c.handleRoute(ingress)
+	handleDegraded(operatorConfig, "RouteStatus", err)
 	if err != nil {
 		return fmt.Errorf("failed handling the route: %v", err)
 	}
@@ -451,6 +452,7 @@ func (c *authOperator) handleVersion(
 	// but we do NOT want to go to the next version until all OAuth server pods are at that version
 
 	routeReady, routeMsg, err := c.checkRouteHealthy(route, routerSecret, ingress)
+	handleDegraded(operatorConfig, "RouteHealth", err)
 	if err != nil {
 		return fmt.Errorf("unable to check route health: %v", err)
 	}
@@ -460,6 +462,7 @@ func (c *authOperator) handleVersion(
 	}
 
 	wellknownReady, wellknownMsg, err := c.checkWellknownEndpointsReady(authConfig, route)
+	handleDegraded(operatorConfig, "WellKnownEndpoint", err)
 	if err != nil {
 		return fmt.Errorf("unable to check the .well-known endpoint: %v", err)
 	}
@@ -469,6 +472,7 @@ func (c *authOperator) handleVersion(
 	}
 
 	oauthClientsReady, oauthClientsMsg, err := c.oauthClientsReady(route)
+	handleDegraded(operatorConfig, "OAuthClients", err)
 	if err != nil {
 		return fmt.Errorf("unable to check OAuth clients' readiness: %v", err)
 	}

pkg/operator2/route.go

@@ -23,17 +23,20 @@ func (c *authOperator) handleRoute(ingress *configv1.Ingress) (*routev1.Route, *
 		return nil, nil, err
 	}
 
+	host := getCanonicalHost(route, ingress)
+	if len(host) == 0 {
+		// be careful not to print route.spec as it many contain secrets
+		return nil, nil, fmt.Errorf("route is not available at canonical host %s: %+v", ingressToHost(ingress), route.Status.Ingress)
+	}
+
 	// assume it is unsafe to mutate route in case we go to a shared informer in the future
 	// this way everything else can just assume route.Spec.Host is correct
 	// note that we are not updating route.Spec.Host in the API - that value is nonsense to us
 	route = route.DeepCopy()
-	route.Spec.Host = getCanonicalHost(route, ingress)
-
-	if len(route.Spec.Host) == 0 {
-		return nil, nil, fmt.Errorf("route has no host: %#v", route)
-	}
+	route.Spec.Host = host
 
 	if err := isValidRoute(route, ingress); err != nil {
+		// TODO remove this delete so that we do not lose the early creation timestamp of our route
 		// delete the route so that it is replaced with the proper one in next reconcile loop
 		klog.Infof("deleting invalid route: %#v", route)
 		opts := &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &route.UID}}
@@ -48,7 +51,8 @@ func (c *authOperator) handleRoute(ingress *configv1.Ingress) (*routev1.Route, *
 		return nil, nil, err
 	}
 	if len(routerSecret.Data) == 0 {
-		return nil, nil, fmt.Errorf("router secret is empty: %#v", routerSecret)
+		// be careful not to print the routerSecret even when it is empty
+		return nil, nil, fmt.Errorf("router secret %s/%s is empty", routerSecret.Namespace, routerSecret.Name)
 	}
 
 	return route, routerSecret, nil

pkg/operator2/status.go

@@ -1,26 +1,38 @@
 package operator2
 
 import (
+	"strings"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 )
 
-func setDegradedTrue(operatorConfig *operatorv1.Authentication, reason, message string) {
+func handleDegraded(operatorConfig *operatorv1.Authentication, prefix string, err error) {
+	if err != nil {
+		v1helpers.SetOperatorCondition(&operatorConfig.Status.Conditions,
+			operatorv1.OperatorCondition{
+				Type:    prefix + operatorv1.OperatorStatusTypeDegraded,
+				Status:  operatorv1.ConditionTrue,
+				Reason:  "Error",
+				Message: err.Error(),
+			})
+		return
+	}
 	v1helpers.SetOperatorCondition(&operatorConfig.Status.Conditions,
 		operatorv1.OperatorCondition{
-			Type:    operatorv1.OperatorStatusTypeDegraded,
-			Status:  operatorv1.ConditionTrue,
-			Reason:  reason,
-			Message: message,
+			Type:   prefix + operatorv1.OperatorStatusTypeDegraded,
+			Status: operatorv1.ConditionFalse,
 		})
 }
 
-func setDegradedFalse(operatorConfig *operatorv1.Authentication) {
-	v1helpers.SetOperatorCondition(&operatorConfig.Status.Conditions,
-		operatorv1.OperatorCondition{
-			Type:   operatorv1.OperatorStatusTypeDegraded,
-			Status: operatorv1.ConditionFalse,
-		})
+func isDegraded(operatorConfig *operatorv1.Authentication) bool {
+	for _, condition := range operatorConfig.Status.Conditions {
+		if strings.HasSuffix(condition.Type, operatorv1.OperatorStatusTypeDegraded) &&
+			condition.Status == operatorv1.ConditionTrue {
+			return true
+		}
+	}
+	return false
 }
 
 func setProgressingTrue(operatorConfig *operatorv1.Authentication, reason, message string) {