Merge pull request #945 from everettraven/bugfix/ocpbugs-44953

OCPBUGS-44953: don't set current clients when no OIDC providers are configured

pkg/console/controllers/clioidcclientstatus/controller.go

@@ -101,6 +101,16 @@ func (c *cliOIDCClientStatusController) HandleManaged(ctx context.Context) error
 	}
 
 	if authnConfig.Spec.Type != configv1.AuthenticationTypeOIDC {
+		// If the authentication type is not "OIDC", set the CurrentOIDCClient
+		// on the authStatusHandler to an empty string. This is necessary during a
+		// scenario where the authentication type goes from OIDC to non-OIDC because
+		// the CurrentOIDCClient would have been set while the authentication type was OIDC.
+		// If the CurrentOIDCClient value isn't reset on this transition the authStatusHandler
+		// will think OIDC is still configured and attempt to update the OIDC client in the
+		// status to have an empty providerName and issuerURL, violating the validations
+		// on the Authentication CRD as seen in https://issues.redhat.com/browse/OCPBUGS-44953
+		c.authStatusHandler.WithCurrentOIDCClient("")
+
 		applyErr := c.authStatusHandler.Apply(ctx, authnConfig)
 		c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("CLIAuthStatusHandler", "FailedApply", applyErr))
 

pkg/console/controllers/oidcsetup/oidcsetup.go

@@ -138,6 +138,16 @@ func (c *oidcSetupController) sync(ctx context.Context, syncCtx factory.SyncCont
 	}
 
 	if authnConfig.Spec.Type != configv1.AuthenticationTypeOIDC {
+		// If the authentication type is not "OIDC", set the CurrentOIDCClient
+		// on the authStatusHandler to an empty string. This is necessary during a
+		// scenario where the authentication type goes from OIDC to non-OIDC because
+		// the CurrentOIDCClient would have been set while the authentication type was OIDC.
+		// If the CurrentOIDCClient value isn't reset on this transition the authStatusHandler
+		// will think OIDC is still configured and attempt to update the OIDC client in the
+		// status to have an empty providerName and issuerURL, violating the validations
+		// on the Authentication CRD as seen in https://issues.redhat.com/browse/OCPBUGS-44953
+		c.authStatusHandler.WithCurrentOIDCClient("")
+
 		applyErr := c.authStatusHandler.Apply(ctx, authnConfig)
 		statusHandler.AddConditions(status.HandleProgressingOrDegraded("AuthStatusHandler", "FailedApply", applyErr))
 
@@ -209,7 +219,6 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig
 			api.TargetNamespace, caCM.Name,
 			sets.New[string]("ca-bundle.crt"),
 			[]metav1.OwnerReference{*utilsub.OwnerRefFrom(operatorConfig)})
-
 		if err != nil {
 			return fmt.Errorf("failed to sync the provider's CA configMap: %w", err)
 		}

pkg/console/status/auth_status.go

@@ -7,6 +7,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	applymetav1 "k8s.io/client-go/applyconfigurations/meta/v1"
+	"k8s.io/klog/v2"
 
 	configv1 "github.com/openshift/api/config/v1"
 	configv1ac "github.com/openshift/client-go/config/applyconfigurations/config/v1"
@@ -87,6 +88,7 @@ func (c *AuthStatusHandler) WithCurrentOIDCClient(currentClientID string) {
 }
 
 func (c *AuthStatusHandler) Apply(ctx context.Context, authnConfig *configv1.Authentication) error {
+	logger := klog.FromContext(ctx).WithName("AuthStatusHandler")
 	defer func() {
 		c.conditionsToApply = map[string]*applymetav1.ConditionApplyConfiguration{}
 	}()
@@ -102,19 +104,30 @@ func (c *AuthStatusHandler) Apply(ctx context.Context, authnConfig *configv1.Aut
 	}
 
 	if len(c.currentClientID) > 0 {
-		var providerName, providerIssuerURL string
 		if len(authnConfig.Spec.OIDCProviders) > 0 {
-			providerName = authnConfig.Spec.OIDCProviders[0].Name
-			providerIssuerURL = authnConfig.Spec.OIDCProviders[0].Issuer.URL
+			providerName := authnConfig.Spec.OIDCProviders[0].Name
+			providerIssuerURL := authnConfig.Spec.OIDCProviders[0].Issuer.URL
+
+			// It violates the Authentication CRD validations to set an empty
+			// OIDCProviderName and IssuerURL value, so only ever add CurrentOIDCClients
+			// to the OIDCClientStatus if there are OIDCProviders present in the spec.
+			clientStatus.WithCurrentOIDCClients(
+				&configv1ac.OIDCClientReferenceApplyConfiguration{
+					OIDCProviderName: &providerName,
+					IssuerURL:        &providerIssuerURL,
+					ClientID:         &c.currentClientID,
+				},
+			)
+		} else {
+			// Generally, we should never get here because when the Authentication type
+			// is changed from OIDC to something else the currentClientID field on the authStatusHandler
+			// should be reset to an empty string. In the event that it isn't reset and it seems like
+			// there are no OIDC providers configured, we should avoid trying to set the
+			// OIDCClients information in the status and marking the clusteroperator as degraded.
+			// Instead, log a warning that we see the currentClientID is set but there is no
+			// evidence of OIDCProviders actually being configured.
+			logger.V(2).Info("WARNING: currentClientID is set but the Authentication resource doesn't seem to have any OIDC providers configured, not adding OIDC clients information to status.", "currentClientID", c.currentClientID)
 		}
-
-		clientStatus.WithCurrentOIDCClients(
-			&configv1ac.OIDCClientReferenceApplyConfiguration{
-				OIDCProviderName: &providerName,
-				IssuerURL:        &providerIssuerURL,
-				ClientID:         &c.currentClientID,
-			},
-		)
 	}
 
 	if authnConfig.Spec.Type == configv1.AuthenticationTypeOIDC {