Merge pull request #861 from stlaz/further_split_oauth

OCPBUGS-29532: ouathclients: further split oidc and integrated oauth handling

Author: openshift-merge-bot[bot]

pkg/console/controllers/oauthclients/oauthclients.go

@@ -6,21 +6,15 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
-	apiexensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	apiexensionsv1informers "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/v1"
-	apiexensionsv1listers "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	corev1informers "k8s.io/client-go/informers/core/v1"
-	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
 
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
-	configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 	configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1"
 	configv1lister "github.com/openshift/client-go/config/listers/config/v1"
 	oauthclient "github.com/openshift/client-go/oauth/clientset/versioned"
@@ -32,80 +26,59 @@ import (
 	routev1listers "github.com/openshift/client-go/route/listers/route/v1"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
-	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
-	appsv1informers "k8s.io/client-go/informers/apps/v1"
-	appsv1listers "k8s.io/client-go/listers/apps/v1"
 
 	"github.com/openshift/console-operator/pkg/api"
 	"github.com/openshift/console-operator/pkg/console/controllers/util"
 	"github.com/openshift/console-operator/pkg/console/status"
-	deploymentsub "github.com/openshift/console-operator/pkg/console/subresource/deployment"
 	oauthsub "github.com/openshift/console-operator/pkg/console/subresource/oauthclient"
 	routesub "github.com/openshift/console-operator/pkg/console/subresource/route"
 	secretsub "github.com/openshift/console-operator/pkg/console/subresource/secret"
-	utilsub "github.com/openshift/console-operator/pkg/console/subresource/util"
-	"github.com/openshift/console-operator/pkg/crypto"
 )
 
+// oauthClientsController:
+//
+//	updates:
+//	- oauthclient.oauth.openshift.io/console (created by CVO)
+//	writes:
+//	- consoles.operator.openshift.io/cluster .status.conditions:
+//		- type=OAuthClientSyncProgressing
+//		- type=OAuthClientSyncDegraded
 type oauthClientsController struct {
 	oauthClient    oauthv1client.OAuthClientsGetter
 	operatorClient v1helpers.OperatorClient
-	secretsClient  corev1client.SecretsGetter
 
 	oauthClientLister           oauthv1lister.OAuthClientLister
 	oauthClientSwitchedInformer *util.InformerWithSwitch
-	authentication              configv1client.AuthenticationInterface
 	authnLister                 configv1lister.AuthenticationLister
 	consoleOperatorLister       operatorv1listers.ConsoleLister
-	crdLister                   apiexensionsv1listers.CustomResourceDefinitionLister
 	routesLister                routev1listers.RouteLister
 	ingressConfigLister         configv1lister.IngressLister
 	targetNSSecretsLister       corev1listers.SecretLister
-	configNSSecretsLister       corev1listers.SecretLister
-	targetNSDeploymentsLister   appsv1listers.DeploymentLister
-	targetNSConfigLister        corev1listers.ConfigMapLister
-
-	authStatusHandler status.AuthStatusHandler
 }
 
 func NewOAuthClientsController(
-	ctx context.Context,
 	operatorClient v1helpers.OperatorClient,
 	oauthClient oauthclient.Interface,
-	secretsClient corev1client.SecretsGetter,
-	crdInformer apiexensionsv1informers.CustomResourceDefinitionInformer,
-	authentication configv1client.AuthenticationInterface,
 	authnInformer configv1informers.AuthenticationInformer,
 	consoleOperatorInformer operatorv1informers.ConsoleInformer,
 	routeInformer routev1informers.RouteInformer,
 	ingressConfigInformer configv1informers.IngressInformer,
 	targetNSsecretsInformer corev1informers.SecretInformer,
-	configNSSecretsInformer corev1informers.SecretInformer,
-	targetNSConfigInformer corev1informers.ConfigMapInformer,
-	targetNSDeploymentsInformer appsv1informers.DeploymentInformer,
 	oauthClientSwitchedInformer *util.InformerWithSwitch,
 	recorder events.Recorder,
 ) factory.Controller {
 	c := oauthClientsController{
 		oauthClient:    oauthClient.OauthV1(),
 		operatorClient: operatorClient,
-		secretsClient:  secretsClient,
 
 		oauthClientLister:           oauthClientSwitchedInformer.Lister(),
 		oauthClientSwitchedInformer: oauthClientSwitchedInformer,
-		authentication:              authentication,
 		authnLister:                 authnInformer.Lister(),
 		consoleOperatorLister:       consoleOperatorInformer.Lister(),
 		routesLister:                routeInformer.Lister(),
 		ingressConfigLister:         ingressConfigInformer.Lister(),
 		targetNSSecretsLister:       targetNSsecretsInformer.Lister(),
-		configNSSecretsLister:       configNSSecretsInformer.Lister(),
-		targetNSConfigLister:        targetNSConfigInformer.Lister(),
-		targetNSDeploymentsLister:   targetNSDeploymentsInformer.Lister(),
-		crdLister:                   crdInformer.Lister(),
-
-		authStatusHandler: status.NewAuthStatusHandler(authentication, api.OpenShiftConsoleName, api.TargetNamespace, api.OpenShiftConsoleOperator),
 	}
 
 	return factory.New().
@@ -116,42 +89,44 @@ func NewOAuthClientsController(
 			routeInformer.Informer(),
 			ingressConfigInformer.Informer(),
 			targetNSsecretsInformer.Informer(),
-			configNSSecretsInformer.Informer(),
-			targetNSDeploymentsInformer.Informer(),
 		).
 		WithFilteredEventsInformers(
 			factory.NamesFilter(api.OAuthClientName),
 			oauthClientSwitchedInformer.Informer(),
 		).
-		WithFilteredEventsInformers(
-			factory.NamesFilter("authentications.config.openshift.io"),
-			crdInformer.Informer(),
-		).
 		WithSyncDegradedOnError(operatorClient).
 		ResyncEvery(wait.Jitter(time.Minute, 1.0)).
 		ToController("OAuthClientsController", recorder.WithComponentSuffix("oauth-clients-controller"))
 }
 
 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 {
 		return nil
 	}
 
-	operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName)
+	statusHandler := status.NewStatusHandler(c.operatorClient)
+
+	authnConfig, err := c.authnLister.Get(api.ConfigResourceName)
 	if err != nil {
 		return err
 	}
 
-	ingressConfig, err := c.ingressConfigLister.Get(api.ConfigResourceName)
+	switch authnConfig.Spec.Type {
+	case "", configv1.AuthenticationTypeIntegratedOAuth:
+	default:
+		// if we're not using integrated oauth, reset all degraded conditions
+		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSync", "", nil))
+		return statusHandler.FlushAndReturn(nil)
+	}
+
+	operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName)
 	if err != nil {
 		return err
 	}
 
-	authnConfig, err := c.authnLister.Get(api.ConfigResourceName)
+	ingressConfig, err := c.ingressConfigLister.Get(api.ConfigResourceName)
 	if err != nil {
 		return err
 	}
@@ -167,142 +142,24 @@ func (c *oauthClientsController) sync(ctx context.Context, controllerContext fac
 		return routeErr
 	}
 
-	var syncErr error
-	switch authnConfig.Spec.Type {
-	case "", configv1.AuthenticationTypeIntegratedOAuth:
-		waitCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
-		defer cancel()
-		if !cache.WaitForCacheSync(waitCtx.Done(), c.oauthClientSwitchedInformer.Informer().HasSynced) {
-			return statusHandler.FlushAndReturn(fmt.Errorf("timed out waiting for OAuthClients cache sync"))
-		}
-
-		clientSecret, secErr := c.syncSecret(ctx, operatorConfig, controllerContext.Recorder())
-		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSecretSync", "FailedApply", secErr))
-		if secErr != nil {
-			return statusHandler.FlushAndReturn(secErr)
-		}
-
-		oauthErrReason, oauthErr := c.syncOAuthClient(ctx, clientSecret, consoleURL.String())
-		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSync", oauthErrReason, oauthErr))
-		if oauthErr != nil {
-			return statusHandler.FlushAndReturn(oauthErr)
-		}
-
-	case configv1.AuthenticationTypeOIDC:
-		syncErr = c.syncAuthTypeOIDC(ctx, controllerContext, statusHandler, operatorConfig, authnConfig)
-		if syncErr != nil {
-			return statusHandler.FlushAndReturn(syncErr)
-		}
-	}
-
-	oidcClientsSchema, err := authnConfigHasOIDCFields(c.crdLister)
-	if err != nil {
-		return statusHandler.FlushAndReturn(err)
-	}
-
-	if oidcClientsSchema {
-		applyErr := c.authStatusHandler.Apply(ctx, authnConfig)
-		statusHandler.AddConditions(status.HandleProgressingOrDegraded("AuthStatusHandler", "FailedApply", applyErr))
-		if applyErr != nil {
-			return statusHandler.FlushAndReturn(applyErr)
-		}
-	}
-
-	return statusHandler.FlushAndReturn(nil)
-}
-
-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("")
-		c.authStatusHandler.Unavailable("OIDCClientConfig", "no OIDC client found")
-		return nil
-	}
-
-	if len(clientConfig.ClientID) == 0 {
-		err := fmt.Errorf("no ID set on OIDC client")
-		statusHandler.AddConditions(status.HandleProgressingOrDegraded("OIDCClientConfig", "MissingID", err))
-		return statusHandler.FlushAndReturn(err)
-	}
-	c.authStatusHandler.WithCurrentOIDCClient(clientConfig.ClientID)
-
-	if len(clientConfig.ClientSecret.Name) == 0 {
-		c.authStatusHandler.Degraded("OIDCClientMissingSecret", "no client secret in the OIDC client config")
-		return nil
+	waitCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
+	defer cancel()
+	if !cache.WaitForCacheSync(waitCtx.Done(), c.oauthClientSwitchedInformer.Informer().HasSynced) {
+		return statusHandler.FlushAndReturn(fmt.Errorf("timed out waiting for OAuthClients cache sync"))
 	}
 
-	clientSecret, err := c.configNSSecretsLister.Secrets(api.OpenShiftConfigNamespace).Get(clientConfig.ClientSecret.Name)
+	clientSecret, err := c.targetNSSecretsLister.Secrets(api.TargetNamespace).Get("console-oauth-config")
 	if err != nil {
-		c.authStatusHandler.Degraded("OIDCClientSecretGet", err.Error())
-		return err
-	}
-
-	secret, err := c.targetNSSecretsLister.Secrets(api.TargetNamespace).Get(secretsub.Stub().Name)
-	expectedClientSecret := secretsub.GetSecretString(clientSecret)
-	if apierrors.IsNotFound(err) || secretsub.GetSecretString(secret) != expectedClientSecret {
-		secret, _, err = resourceapply.ApplySecret(ctx, c.secretsClient, controllerContext.Recorder(), secretsub.DefaultSecret(operatorConfig, expectedClientSecret))
-		if err != nil {
-			statusHandler.AddConditions(status.HandleProgressingOrDegraded("OIDCClientSecretSync", "FailedApply", err))
-			return err
-		}
-	}
-
-	if valid, msg, err := c.checkClientConfigStatus(authnConfig, secret); err != nil {
-		c.authStatusHandler.Degraded("DeploymentOIDCConfig", err.Error())
 		return err
-
-	} else if !valid {
-		c.authStatusHandler.Progressing("DeploymentOIDCConfig", msg)
-		return nil
 	}
 
-	c.authStatusHandler.Available("OIDCConfigAvailable", "")
-	return nil
-}
-
-// checkClientConfigStatus checks whether the current client configuration is being currently in use,
-// by looking at the deployment status. It checks whether the deployment is available and updated,
-// and also whether the resource versions for the oauth secret and server CA trust configmap match
-// the deployment.
-func (c *oauthClientsController) checkClientConfigStatus(authnConfig *configv1.Authentication, clientSecret *corev1.Secret) (bool, string, error) {
-	depl, err := c.targetNSDeploymentsLister.Deployments(api.OpenShiftConsoleNamespace).Get(api.OpenShiftConsoleDeploymentName)
+	oauthErrReason, err := c.syncOAuthClient(ctx, clientSecret, consoleURL.String())
+	statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthClientSync", oauthErrReason, err))
 	if err != nil {
-		return false, "", err
-	}
-
-	deplAvailableUpdated := deploymentsub.IsAvailableAndUpdated(depl)
-	if !deplAvailableUpdated {
-		return false, "deployment unavailable or outdated", nil
-	}
-
-	if clientSecret.GetResourceVersion() != depl.ObjectMeta.Annotations["console.openshift.io/oauth-secret-version"] {
-		return false, "client secret version not up to date in current deployment", nil
-	}
-
-	if len(authnConfig.Spec.OIDCProviders) > 0 {
-		serverCAConfigName := authnConfig.Spec.OIDCProviders[0].Issuer.CertificateAuthority.Name
-		if len(serverCAConfigName) == 0 {
-			return deplAvailableUpdated, "", nil
-		}
-
-		serverCAConfig, err := c.targetNSConfigLister.ConfigMaps(api.OpenShiftConsoleNamespace).Get(serverCAConfigName)
-		if err != nil {
-			return false, "", err
-		}
-
-		if serverCAConfig.GetResourceVersion() != depl.ObjectMeta.Annotations["console.openshift.io/authn-ca-trust-config-version"] {
-			return false, "OIDC provider CA version not up to date in current deployment", nil
-		}
+		return statusHandler.FlushAndReturn(err)
 	}
 
-	return deplAvailableUpdated, "", nil
+	return statusHandler.FlushAndReturn(nil)
 }
 
 // handleStatus returns whether sync should happen and any error encountering
@@ -329,15 +186,6 @@ func (c *oauthClientsController) handleManaged(ctx context.Context) (bool, error
 	}
 }
 
-func (c *oauthClientsController) syncSecret(ctx context.Context, operatorConfig *operatorv1.Console, recorder events.Recorder) (*corev1.Secret, error) {
-	secret, err := c.targetNSSecretsLister.Secrets(api.TargetNamespace).Get(secretsub.Stub().Name)
-	if apierrors.IsNotFound(err) || secretsub.GetSecretString(secret) == "" {
-		secret, _, err = resourceapply.ApplySecret(ctx, c.secretsClient, recorder, secretsub.DefaultSecret(operatorConfig, crypto.Random256BitsString()))
-	}
-	// any error should be returned & kill the sync loop
-	return secret, err
-}
-
 // applies changes to the oauthclient
 // should not be called until route & secret dependencies are verified
 func (c *oauthClientsController) syncOAuthClient(
@@ -375,28 +223,3 @@ func (c *oauthClientsController) deregisterClient(ctx context.Context) error {
 	return err
 
 }
-
-func authnConfigHasOIDCFields(crdLister apiexensionsv1listers.CustomResourceDefinitionLister) (bool, error) {
-	authnCRD, err := crdLister.Get("authentications.config.openshift.io")
-	if err != nil {
-		return false, err
-	}
-
-	var authnV1Config *apiexensionsv1.CustomResourceDefinitionVersion
-	for _, version := range authnCRD.Spec.Versions {
-		if version.Name == "v1" && version.Served && version.Storage {
-			authnV1Config = &version
-			break
-		}
-	}
-
-	if authnV1Config == nil {
-		return false, fmt.Errorf("authentications.config.openshift.io is not served or stored as v1")
-	}
-
-	schema := authnV1Config.Schema.OpenAPIV3Schema
-	_, clientsExist := schema.Properties["status"].Properties["oidcClients"]
-
-	return clientsExist, nil
-
-}