Merge pull request #848 from deads2k/fix-status-conditin

OCPBUGS-25484: make it impossible double set conditions in a single loop

Author: openshift-merge-bot[bot]

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/openshift/api v0.0.0-20231218131639-7a5aa77cc72d
 	github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d
 	github.com/openshift/client-go v0.0.0-20231218140158-47f6d749b9d9
-	github.com/openshift/library-go v0.0.0-20240115112243-470c096a1ca9
+	github.com/openshift/library-go v0.0.0-20240124134907-4dfbf6bc7b11
 	github.com/spf13/cobra v1.7.0
 	golang.org/x/exp v0.0.0-20230713183714-613f0c0eb8a1
 	gopkg.in/yaml.v2 v2.4.0

go.sum

@@ -63,7 +63,6 @@ type oauthClientsController struct {
 	targetNSConfigLister        corev1listers.ConfigMapLister
 	featureGatesLister          configv1lister.FeatureGateLister
 
-	statusHandler     status.StatusHandler
 	authStatusHandler status.AuthStatusHandler
 }
 
@@ -103,7 +102,6 @@ func NewOAuthClientsController(
 		targetNSDeploymentsLister:   targetNSDeploymentsInformer.Lister(),
 		featureGatesLister:          featureGatesInformer.Lister(),
 
-		statusHandler:     status.NewStatusHandler(operatorClient),
 		authStatusHandler: status.NewAuthStatusHandler(authentication, api.OpenShiftConsoleName, api.TargetNamespace, api.OpenShiftConsoleOperator),
 	}
 
@@ -128,6 +126,8 @@ func NewOAuthClientsController(
 }
 
 func (c *oauthClientsController) sync(ctx context.Context, controllerContext factory.SyncContext) error {
+	statusHandler := status.NewStatusHandler(c.operatorClient)
+
 	if shouldSync, err := c.handleManaged(ctx); err != nil {
 		return err
 	} else if !shouldSync {
@@ -176,36 +176,43 @@ func (c *oauthClientsController) sync(ctx context.Context, controllerContext fac
 		}
 
 		clientSecret, secErr := c.syncSecret(ctx, operatorConfig, controllerContext.Recorder())
-		c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSecretSync", "FailedApply", secErr))
+		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSecretSync", "FailedApply", secErr))
 		if secErr != nil {
 			syncErr = secErr
 			break
 		}
 
 		oauthErrReason, oauthErr := c.syncOAuthClient(ctx, clientSecret, consoleURL.String())
-		c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSync", oauthErrReason, oauthErr))
+		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSync", oauthErrReason, oauthErr))
 		if oauthErr != nil {
 			syncErr = oauthErr
 			break
 		}
 
 	case configv1.AuthenticationTypeOIDC:
-		syncErr = c.syncAuthTypeOIDC(ctx, controllerContext, operatorConfig, authnConfig)
+		syncErr = c.syncAuthTypeOIDC(ctx, controllerContext, statusHandler, operatorConfig, authnConfig)
 	}
 
 	// AuthStatusHandler manages fields that are behind the CustomNoUpgrade and TechPreviewNoUpgrade featuregate sets
 	// call Apply() only if they are enabled, otherwise server-side apply will complain
 	if featureGates.Spec.FeatureSet == configv1.TechPreviewNoUpgrade || featureGates.Spec.FeatureSet == configv1.CustomNoUpgrade {
 		if err := c.authStatusHandler.Apply(ctx, authnConfig); err != nil {
-			c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("AuthStatusHandler", "FailedApply", err))
-			return c.statusHandler.FlushAndReturn(err)
+			statusHandler.AddConditions(status.HandleProgressingOrDegraded("AuthStatusHandler", "FailedApply", err))
+			return statusHandler.FlushAndReturn(err)
 		}
 	}
 
-	return c.statusHandler.FlushAndReturn(syncErr)
+	return statusHandler.FlushAndReturn(syncErr)
 }
 
-func (c *oauthClientsController) syncAuthTypeOIDC(ctx context.Context, controllerContext factory.SyncContext, operatorConfig *operatorv1.Console, authnConfig *configv1.Authentication) error {
+func (c *oauthClientsController) syncAuthTypeOIDC(
+	ctx context.Context,
+	controllerContext factory.SyncContext,
+	statusHandler status.StatusHandler,
+	operatorConfig *operatorv1.Console,
+	authnConfig *configv1.Authentication,
+) error {
+
 	clientConfig := utilsub.GetOIDCClientConfig(authnConfig)
 	if clientConfig == nil {
 		c.authStatusHandler.WithCurrentOIDCClient("")
@@ -215,8 +222,8 @@ func (c *oauthClientsController) syncAuthTypeOIDC(ctx context.Context, controlle
 
 	if len(clientConfig.ClientID) == 0 {
 		err := fmt.Errorf("no ID set on OIDC client")
-		c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("OIDCClientConfig", "MissingID", err))
-		return c.statusHandler.FlushAndReturn(err)
+		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OIDCClientConfig", "MissingID", err))
+		return statusHandler.FlushAndReturn(err)
 	}
 	c.authStatusHandler.WithCurrentOIDCClient(clientConfig.ClientID)
 
@@ -236,7 +243,7 @@ func (c *oauthClientsController) syncAuthTypeOIDC(ctx context.Context, controlle
 	if apierrors.IsNotFound(err) || secretsub.GetSecretString(secret) != expectedClientSecret {
 		secret, _, err = resourceapply.ApplySecret(ctx, c.secretsClient, controllerContext.Recorder(), secretsub.DefaultSecret(operatorConfig, expectedClientSecret))
 		if err != nil {
-			c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("OIDCClientSecretSync", "FailedApply", err))
+			statusHandler.AddConditions(status.HandleProgressingOrDegraded("OIDCClientSecretSync", "FailedApply", err))
 			return err
 		}
 	}

pkg/console/controllers/oauthclients/oauthclients.go

@@ -27,6 +27,9 @@ type AuthStatusHandler struct {
 	currentClientID    string
 }
 
+// NewAuthStatusHandler creates a handler for updating the Authentication.config.openshift.io
+// status with information about the expected and currently used OIDC client.
+// Not thread safe, only use in controllers with a single worker!
 func NewAuthStatusHandler(authnClient configv1client.AuthenticationInterface, componentName, componentNamespace, fieldManager string) AuthStatusHandler {
 	return AuthStatusHandler{
 		client:             authnClient,
@@ -83,6 +86,10 @@ func (c *AuthStatusHandler) WithCurrentOIDCClient(currentClientID string) {
 }
 
 func (c *AuthStatusHandler) Apply(ctx context.Context, authnConfig *configv1.Authentication) error {
+	defer func() {
+		c.conditionsToApply = map[string]*metav1.Condition{}
+	}()
+
 	applyConfig, err := configv1ac.ExtractAuthenticationStatus(authnConfig, c.fieldManager)
 	if err != nil {
 		return err

pkg/console/status/auth_status.go

@@ -40,32 +40,44 @@ import (
 //     Message: error string value is used as message
 //
 // all degraded suffix conditions will be aggregated into a final "Degraded" status that will be set on the console ClusterOperator
-func HandleDegraded(typePrefix string, reason string, err error) v1helpers.UpdateStatusFunc {
+func HandleDegraded(typePrefix string, reason string, err error) ConditionUpdate {
 	conditionType := typePrefix + operatorsv1.OperatorStatusTypeDegraded
 	condition := handleCondition(conditionType, reason, err)
-	return v1helpers.UpdateConditionFn(condition)
+	return ConditionUpdate{
+		ConditionType:  conditionType,
+		StatusUpdateFn: v1helpers.UpdateConditionFn(condition),
+	}
 }
 
-func HandleProgressing(typePrefix string, reason string, err error) v1helpers.UpdateStatusFunc {
+func HandleProgressing(typePrefix string, reason string, err error) ConditionUpdate {
 	conditionType := typePrefix + operatorsv1.OperatorStatusTypeProgressing
 	condition := handleCondition(conditionType, reason, err)
-	return v1helpers.UpdateConditionFn(condition)
+	return ConditionUpdate{
+		ConditionType:  conditionType,
+		StatusUpdateFn: v1helpers.UpdateConditionFn(condition),
+	}
 }
 
-func HandleAvailable(typePrefix string, reason string, err error) v1helpers.UpdateStatusFunc {
+func HandleAvailable(typePrefix string, reason string, err error) ConditionUpdate {
 	conditionType := typePrefix + operatorsv1.OperatorStatusTypeAvailable
 	condition := handleCondition(conditionType, reason, err)
-	return v1helpers.UpdateConditionFn(condition)
+	return ConditionUpdate{
+		ConditionType:  conditionType,
+		StatusUpdateFn: v1helpers.UpdateConditionFn(condition),
+	}
 }
 
-func HandleUpgradable(typePrefix string, reason string, err error) v1helpers.UpdateStatusFunc {
+func HandleUpgradable(typePrefix string, reason string, err error) ConditionUpdate {
 	conditionType := typePrefix + operatorsv1.OperatorStatusTypeUpgradeable
 	condition := handleCondition(conditionType, reason, err)
-	return v1helpers.UpdateConditionFn(condition)
+	return ConditionUpdate{
+		ConditionType:  conditionType,
+		StatusUpdateFn: v1helpers.UpdateConditionFn(condition),
+	}
 }
 
-func (c *StatusHandler) ResetConditions(conditions []operatorsv1.OperatorCondition) []v1helpers.UpdateStatusFunc {
-	updateStatusFuncs := []v1helpers.UpdateStatusFunc{}
+func (c *StatusHandler) ResetConditions(conditions []operatorsv1.OperatorCondition) []ConditionUpdate {
+	updateStatusFuncs := []ConditionUpdate{}
 	for _, condition := range conditions {
 		klog.V(2).Info("\nresetting condition: ", condition.Type)
 		if strings.HasSuffix(condition.Type, operatorsv1.OperatorStatusTypeDegraded) {
@@ -101,8 +113,8 @@ func (c *StatusHandler) ResetConditions(conditions []operatorsv1.OperatorConditi
 // - Type suffix will be set to Degraded
 // TODO: when we eliminate the special case SyncError, this helper can go away.
 // When we do that, however, we must make sure to register deprecated conditions with NewRemoveStaleConditions()
-func HandleProgressingOrDegraded(typePrefix string, reason string, err error) []v1helpers.UpdateStatusFunc {
-	updateStatusFuncs := []v1helpers.UpdateStatusFunc{}
+func HandleProgressingOrDegraded(typePrefix string, reason string, err error) []ConditionUpdate {
+	updateStatusFuncs := []ConditionUpdate{}
 	if errors.IsSyncError(err) {
 		updateStatusFuncs = append(updateStatusFuncs, HandleDegraded(typePrefix, reason, nil))
 		updateStatusFuncs = append(updateStatusFuncs, HandleProgressing(typePrefix, reason, err))
@@ -144,22 +156,39 @@ func setConditionValue(conditionType string, err error) operatorsv1.ConditionSta
 }
 
 type StatusHandler struct {
-	client      v1helpers.OperatorClient
+	client v1helpers.OperatorClient
+	// conditionUpdates are keyed by condition type so that we always choose the latest as authoritative
+	conditionUpdates map[string]v1helpers.UpdateStatusFunc
+
 	statusFuncs []v1helpers.UpdateStatusFunc
 }
 
-func (c *StatusHandler) AddCondition(newStatusFunc v1helpers.UpdateStatusFunc) {
-	c.statusFuncs = append(c.statusFuncs, newStatusFunc)
+type ConditionUpdate struct {
+	ConditionType  string
+	StatusUpdateFn v1helpers.UpdateStatusFunc
 }
 
-func (c *StatusHandler) AddConditions(newStatusFuncs []v1helpers.UpdateStatusFunc) {
-	for _, newStatusFunc := range newStatusFuncs {
-		c.statusFuncs = append(c.statusFuncs, newStatusFunc)
+func (c *StatusHandler) AddCondition(conditionUpdate ConditionUpdate) {
+	c.conditionUpdates[conditionUpdate.ConditionType] = conditionUpdate.StatusUpdateFn
+}
+
+func (c *StatusHandler) AddConditions(conditionUpdates []ConditionUpdate) {
+	for i := range conditionUpdates {
+		conditionUpdate := conditionUpdates[i]
+		c.conditionUpdates[conditionUpdate.ConditionType] = conditionUpdate.StatusUpdateFn
 	}
 }
 
 func (c *StatusHandler) FlushAndReturn(returnErr error) error {
-	if _, _, updateErr := v1helpers.UpdateStatus(context.TODO(), c.client, c.statusFuncs...); updateErr != nil {
+	allStatusFns := []v1helpers.UpdateStatusFunc{}
+	for i := range c.statusFuncs {
+		allStatusFns = append(allStatusFns, c.statusFuncs[i])
+	}
+	for k := range c.conditionUpdates {
+		allStatusFns = append(allStatusFns, c.conditionUpdates[k])
+	}
+
+	if _, _, updateErr := v1helpers.UpdateStatus(context.TODO(), c.client, allStatusFns...); updateErr != nil {
 		return updateErr
 	}
 	return returnErr
@@ -191,7 +220,8 @@ func (c *StatusHandler) UpdateDeploymentGeneration(actualDeployment *appsv1.Depl
 
 func NewStatusHandler(client v1helpers.OperatorClient) StatusHandler {
 	return StatusHandler{
-		client: client,
+		client:           client,
+		conditionUpdates: map[string]v1helpers.UpdateStatusFunc{},
 	}
 }