Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/console/controllers/clioidcclientstatus/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
11 changes: 10 additions & 1 deletion pkg/console/controllers/oidcsetup/oidcsetup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down Expand Up @@ -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)
}
Expand Down
35 changes: 24 additions & 11 deletions pkg/console/status/auth_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}
}()
Expand All @@ -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 {
Expand Down