Skip to content

Commit fe472d1

Browse files
committed
(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 <[email protected]>
1 parent dbb03bb commit fe472d1

File tree

3 files changed

+44
-12
lines changed

3 files changed

+44
-12
lines changed

pkg/console/controllers/clioidcclientstatus/controller.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,16 @@ func (c *cliOIDCClientStatusController) HandleManaged(ctx context.Context) error
101101
}
102102

103103
if authnConfig.Spec.Type != configv1.AuthenticationTypeOIDC {
104+
// If the authentication type is not "OIDC", set the CurrentOIDCClient
105+
// on the authStatusHandler to an empty string. This is necessary during a
106+
// scenario where the authentication type goes from OIDC to non-OIDC because
107+
// the CurrentOIDCClient would have been set while the authentication type was OIDC.
108+
// If the CurrentOIDCClient value isn't reset on this transition the authStatusHandler
109+
// will think OIDC is still configured and attempt to update the OIDC client in the
110+
// status to have an empty providerName and issuerURL, violating the validations
111+
// on the Authentication CRD as seen in https://issues.redhat.com/browse/OCPBUGS-44953
112+
c.authStatusHandler.WithCurrentOIDCClient("")
113+
104114
applyErr := c.authStatusHandler.Apply(ctx, authnConfig)
105115
c.statusHandler.AddConditions(status.HandleProgressingOrDegraded("CLIAuthStatusHandler", "FailedApply", applyErr))
106116

pkg/console/controllers/oidcsetup/oidcsetup.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,16 @@ func (c *oidcSetupController) sync(ctx context.Context, syncCtx factory.SyncCont
138138
}
139139

140140
if authnConfig.Spec.Type != configv1.AuthenticationTypeOIDC {
141+
// If the authentication type is not "OIDC", set the CurrentOIDCClient
142+
// on the authStatusHandler to an empty string. This is necessary during a
143+
// scenario where the authentication type goes from OIDC to non-OIDC because
144+
// the CurrentOIDCClient would have been set while the authentication type was OIDC.
145+
// If the CurrentOIDCClient value isn't reset on this transition the authStatusHandler
146+
// will think OIDC is still configured and attempt to update the OIDC client in the
147+
// status to have an empty providerName and issuerURL, violating the validations
148+
// on the Authentication CRD as seen in https://issues.redhat.com/browse/OCPBUGS-44953
149+
c.authStatusHandler.WithCurrentOIDCClient("")
150+
141151
applyErr := c.authStatusHandler.Apply(ctx, authnConfig)
142152
statusHandler.AddConditions(status.HandleProgressingOrDegraded("AuthStatusHandler", "FailedApply", applyErr))
143153

@@ -209,7 +219,6 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig
209219
api.TargetNamespace, caCM.Name,
210220
sets.New[string]("ca-bundle.crt"),
211221
[]metav1.OwnerReference{*utilsub.OwnerRefFrom(operatorConfig)})
212-
213222
if err != nil {
214223
return fmt.Errorf("failed to sync the provider's CA configMap: %w", err)
215224
}

pkg/console/status/auth_status.go

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"k8s.io/apimachinery/pkg/api/equality"
88
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
99
applymetav1 "k8s.io/client-go/applyconfigurations/meta/v1"
10+
"k8s.io/klog/v2"
1011

1112
configv1 "github.com/openshift/api/config/v1"
1213
configv1ac "github.com/openshift/client-go/config/applyconfigurations/config/v1"
@@ -87,6 +88,7 @@ func (c *AuthStatusHandler) WithCurrentOIDCClient(currentClientID string) {
8788
}
8889

8990
func (c *AuthStatusHandler) Apply(ctx context.Context, authnConfig *configv1.Authentication) error {
91+
logger := klog.FromContext(ctx).WithName("AuthStatusHandler")
9092
defer func() {
9193
c.conditionsToApply = map[string]*applymetav1.ConditionApplyConfiguration{}
9294
}()
@@ -102,19 +104,30 @@ func (c *AuthStatusHandler) Apply(ctx context.Context, authnConfig *configv1.Aut
102104
}
103105

104106
if len(c.currentClientID) > 0 {
105-
var providerName, providerIssuerURL string
106107
if len(authnConfig.Spec.OIDCProviders) > 0 {
107-
providerName = authnConfig.Spec.OIDCProviders[0].Name
108-
providerIssuerURL = authnConfig.Spec.OIDCProviders[0].Issuer.URL
108+
providerName := authnConfig.Spec.OIDCProviders[0].Name
109+
providerIssuerURL := authnConfig.Spec.OIDCProviders[0].Issuer.URL
110+
111+
// It violates the Authentication CRD validations to set an empty
112+
// OIDCProviderName and IssuerURL value, so only ever add CurrentOIDCClients
113+
// to the OIDCClientStatus if there are OIDCProviders present in the spec.
114+
clientStatus.WithCurrentOIDCClients(
115+
&configv1ac.OIDCClientReferenceApplyConfiguration{
116+
OIDCProviderName: &providerName,
117+
IssuerURL: &providerIssuerURL,
118+
ClientID: &c.currentClientID,
119+
},
120+
)
121+
} else {
122+
// Generally, we should never get here because when the Authentication type
123+
// is changed from OIDC to something else the currentClientID field on the authStatusHandler
124+
// should be reset to an empty string. In the event that it isn't reset and it seems like
125+
// there are no OIDC providers configured, we should avoid trying to set the
126+
// OIDCClients information in the status and marking the clusteroperator as degraded.
127+
// Instead, log a warning that we see the currentClientID is set but there is no
128+
// evidence of OIDCProviders actually being configured.
129+
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)
109130
}
110-
111-
clientStatus.WithCurrentOIDCClients(
112-
&configv1ac.OIDCClientReferenceApplyConfiguration{
113-
OIDCProviderName: &providerName,
114-
IssuerURL: &providerIssuerURL,
115-
ClientID: &c.currentClientID,
116-
},
117-
)
118131
}
119132

120133
if authnConfig.Spec.Type == configv1.AuthenticationTypeOIDC {

0 commit comments

Comments
 (0)