From fe472d16096335dda422f3416afa67d770a9b8c3 Mon Sep 17 00:00:00 2001 From: everettraven Date: Thu, 5 Dec 2024 14:10:08 -0500 Subject: [PATCH] (bugfix): don't set current clients when no OIDC providers are configured by resetting the AuthStatusHandler.currentClientID to an empty string when the Authentication.Spec.Type is not set to OIDC. Additionally, protect against a degraded state when AuthStatusHandler.currentClientID is set but there is no evidence of OIDC providers being configured in the spec by only adding current clients when both the currentClientID is set and there is evidence of OIDC providers being configured. Add a warning log when we notice the currentClientID is set but there is no evidence of OIDC providers being configured. Fixes [OCPBUGS-44953](https://issues.redhat.com/browse/OCPBUGS-44953) Signed-off-by: everettraven --- .../clioidcclientstatus/controller.go | 10 ++++++ .../controllers/oidcsetup/oidcsetup.go | 11 +++++- pkg/console/status/auth_status.go | 35 +++++++++++++------ 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/pkg/console/controllers/clioidcclientstatus/controller.go b/pkg/console/controllers/clioidcclientstatus/controller.go index 460740448..7711fac09 100644 --- a/pkg/console/controllers/clioidcclientstatus/controller.go +++ b/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)) diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index 54bd5d049..ef0b506a6 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/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) } diff --git a/pkg/console/status/auth_status.go b/pkg/console/status/auth_status.go index fea65f593..6ca0191b8 100644 --- a/pkg/console/status/auth_status.go +++ b/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 {