chore: Don't update CheCluster CR spec to set OAuthClientName and OAu… (#1393)

* chore: Don't update CheCluster CR spec to set OAuthClientName and OAuthClientSecret fields

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

controllers/che/checluster_controller.go

@@ -102,7 +102,9 @@ func NewReconciler(
 	// resources since che host is used for dashboard deployment and che config map
 	reconcileManager.RegisterReconciler(server.NewCheHostReconciler())
 	reconcileManager.RegisterReconciler(postgres.NewPostgresReconciler())
-	reconcileManager.RegisterReconciler(identityprovider.NewIdentityProviderReconciler())
+	if util.IsOpenShift {
+		reconcileManager.RegisterReconciler(identityprovider.NewIdentityProviderReconciler())
+	}
 	reconcileManager.RegisterReconciler(devfileregistry.NewDevfileRegistryReconciler())
 	reconcileManager.RegisterReconciler(pluginregistry.NewPluginRegistryReconciler())
 	reconcileManager.RegisterReconciler(dashboard.NewDashboardReconciler())

main.go

@@ -49,7 +49,7 @@ import (
 	"github.com/go-logr/logr"
 	configv1 "github.com/openshift/api/config/v1"
 	consolev1 "github.com/openshift/api/console/v1"
-	oauth "github.com/openshift/api/oauth/v1"
+	oauthv1 "github.com/openshift/api/oauth/v1"
 
 	orgv1 "github.com/eclipse-che/che-operator/api/v1"
 	checontroller "github.com/eclipse-che/che-operator/controllers/che"
@@ -142,7 +142,7 @@ func init() {
 
 	if util.IsOpenShift {
 		utilruntime.Must(routev1.AddToScheme(scheme))
-		utilruntime.Must(oauth.AddToScheme(scheme))
+		utilruntime.Must(oauthv1.AddToScheme(scheme))
 		utilruntime.Must(userv1.AddToScheme(scheme))
 		utilruntime.Must(configv1.AddToScheme(scheme))
 		utilruntime.Must(corev1.AddToScheme(scheme))
@@ -344,6 +344,7 @@ func getCacheFunc() (cache.NewCacheFunc, error) {
 	logrus.Infof("Limit cache by selector: %s", partOfCheObjectSelector.String())
 
 	routeKey := &routev1.Route{}
+	oauthKey := &oauthv1.OAuthClient{}
 	selectors := cache.SelectorsByObject{
 		&appsv1.Deployment{}: {
 			Label: partOfCheObjectSelector,
@@ -387,10 +388,14 @@ func getCacheFunc() (cache.NewCacheFunc, error) {
 		&corev1.PersistentVolumeClaim{}: {
 			Label: partOfCheObjectSelector,
 		},
+		oauthKey: {
+			Label: partOfCheObjectSelector,
+		},
 	}
 
 	if !util.IsOpenShift {
 		delete(selectors, routeKey)
+		delete(selectors, oauthKey)
 	}
 
 	return cache.BuilderWithOptions(cache.Options{

pkg/deploy/gateway/gateway_test.go

@@ -142,14 +142,12 @@ func TestOauthProxyConfigUnauthorizedPaths(t *testing.T) {
 	util.IsOpenShift4 = true
 
 	t.Run("no skip auth", func(t *testing.T) {
-		ctx := &deploy.DeployContext{
-			CheCluster: &orgv1.CheCluster{
-				Spec: orgv1.CheClusterSpec{
-					Server: orgv1.CheClusterSpecServer{
-						ExternalDevfileRegistry: true,
-						ExternalPluginRegistry:  true,
-					}}},
-		}
+		ctx := deploy.GetTestDeployContext(&orgv1.CheCluster{
+			Spec: orgv1.CheClusterSpec{
+				Server: orgv1.CheClusterSpecServer{
+					ExternalDevfileRegistry: true,
+					ExternalPluginRegistry:  true,
+				}}}, nil)
 
 		configmap := getGatewayOauthProxyConfigSpec(ctx, "blabol")
 		config := configmap.Data["oauth-proxy.cfg"]
@@ -159,14 +157,12 @@ func TestOauthProxyConfigUnauthorizedPaths(t *testing.T) {
 	})
 
 	t.Run("no devfile-registry auth", func(t *testing.T) {
-		ctx := &deploy.DeployContext{
-			CheCluster: &orgv1.CheCluster{
-				Spec: orgv1.CheClusterSpec{
-					Server: orgv1.CheClusterSpecServer{
-						ExternalDevfileRegistry: false,
-						ExternalPluginRegistry:  true,
-					}}},
-		}
+		ctx := deploy.GetTestDeployContext(&orgv1.CheCluster{
+			Spec: orgv1.CheClusterSpec{
+				Server: orgv1.CheClusterSpecServer{
+					ExternalDevfileRegistry: false,
+					ExternalPluginRegistry:  true,
+				}}}, nil)
 
 		configmap := getGatewayOauthProxyConfigSpec(ctx, "blabol")
 		config := configmap.Data["oauth-proxy.cfg"]
@@ -176,14 +172,12 @@ func TestOauthProxyConfigUnauthorizedPaths(t *testing.T) {
 	})
 
 	t.Run("skip plugin-registry auth", func(t *testing.T) {
-		ctx := &deploy.DeployContext{
-			CheCluster: &orgv1.CheCluster{
-				Spec: orgv1.CheClusterSpec{
-					Server: orgv1.CheClusterSpecServer{
-						ExternalDevfileRegistry: true,
-						ExternalPluginRegistry:  false,
-					}}},
-		}
+		ctx := deploy.GetTestDeployContext(&orgv1.CheCluster{
+			Spec: orgv1.CheClusterSpec{
+				Server: orgv1.CheClusterSpecServer{
+					ExternalDevfileRegistry: true,
+					ExternalPluginRegistry:  false,
+				}}}, nil)
 
 		configmap := getGatewayOauthProxyConfigSpec(ctx, "blabol")
 		config := configmap.Data["oauth-proxy.cfg"]
@@ -193,14 +187,12 @@ func TestOauthProxyConfigUnauthorizedPaths(t *testing.T) {
 	})
 
 	t.Run("skip both registries auth", func(t *testing.T) {
-		ctx := &deploy.DeployContext{
-			CheCluster: &orgv1.CheCluster{
-				Spec: orgv1.CheClusterSpec{
-					Server: orgv1.CheClusterSpecServer{
-						ExternalDevfileRegistry: false,
-						ExternalPluginRegistry:  false,
-					}}},
-		}
+		ctx := deploy.GetTestDeployContext(&orgv1.CheCluster{
+			Spec: orgv1.CheClusterSpec{
+				Server: orgv1.CheClusterSpecServer{
+					ExternalDevfileRegistry: false,
+					ExternalPluginRegistry:  false,
+				}}}, nil)
 
 		configmap := getGatewayOauthProxyConfigSpec(ctx, "blabol")
 		config := configmap.Data["oauth-proxy.cfg"]
@@ -210,10 +202,9 @@ func TestOauthProxyConfigUnauthorizedPaths(t *testing.T) {
 	})
 
 	t.Run("skip '/healthz' path", func(t *testing.T) {
-		ctx := &deploy.DeployContext{
-			CheCluster: &orgv1.CheCluster{
-				Spec: orgv1.CheClusterSpec{}},
-		}
+		ctx := deploy.GetTestDeployContext(&orgv1.CheCluster{
+			Spec: orgv1.CheClusterSpec{
+				Server: orgv1.CheClusterSpecServer{}}}, nil)
 
 		configmap := getGatewayOauthProxyConfigSpec(ctx, "blabol")
 		config := configmap.Data["oauth-proxy.cfg"]

pkg/deploy/gateway/oauth_proxy.go

@@ -16,6 +16,9 @@ import (
 	"fmt"
 	"strings"
 
+	identityprovider "github.com/eclipse-che/che-operator/pkg/deploy/identity-provider"
+	"github.com/sirupsen/logrus"
+
 	"k8s.io/apimachinery/pkg/api/resource"
 
 	orgv1 "github.com/eclipse-che/che-operator/api/v1"
@@ -32,7 +35,7 @@ func getGatewayOauthProxyConfigSpec(ctx *deploy.DeployContext, cookieSecret stri
 	if util.IsOpenShift {
 		config = openshiftOauthProxyConfig(ctx, cookieSecret)
 	} else {
-		config = kubernetesOauthProxyconfig(ctx, cookieSecret)
+		config = kubernetesOauthProxyConfig(ctx, cookieSecret)
 	}
 	return corev1.ConfigMap{
 		TypeMeta: metav1.TypeMeta{
@@ -51,6 +54,17 @@ func getGatewayOauthProxyConfigSpec(ctx *deploy.DeployContext, cookieSecret stri
 }
 
 func openshiftOauthProxyConfig(ctx *deploy.DeployContext, cookieSecret string) string {
+	oauthSecret := ""
+	oauthClientName := ""
+
+	oauthClient, _ := identityprovider.FindOAuthClient(ctx)
+	if oauthClient == nil {
+		logrus.Error("oauth client not found")
+	} else {
+		oauthSecret = oauthClient.Secret
+		oauthClientName = oauthClient.Name
+	}
+
 	return fmt.Sprintf(`
 http_address = ":%d"
 https_address = ""
@@ -72,14 +86,14 @@ skip_provider_button = false
 %s
 `, GatewayServicePort,
 		ctx.CheCluster.GetCheHost(),
-		ctx.CheCluster.Spec.Auth.OAuthClientName,
-		ctx.CheCluster.Spec.Auth.OAuthSecret,
+		oauthClientName,
+		oauthSecret,
 		GatewayServiceName,
 		cookieSecret,
 		skipAuthConfig(ctx.CheCluster))
 }
 
-func kubernetesOauthProxyconfig(ctx *deploy.DeployContext, cookieSecret string) string {
+func kubernetesOauthProxyConfig(ctx *deploy.DeployContext, cookieSecret string) string {
 	return fmt.Sprintf(`
 proxy_prefix = "/oauth"
 http_address = ":%d"

pkg/deploy/identity-provider/identity_provider_reconciler.go

@@ -42,18 +42,18 @@ func NewIdentityProviderReconciler() *IdentityProviderReconciler {
 
 func (ip *IdentityProviderReconciler) Reconcile(ctx *deploy.DeployContext) (reconcile.Result, bool, error) {
 	done, err := syncNativeIdentityProviderItems(ctx)
-	if !done {
-		return reconcile.Result{Requeue: true}, false, err
-	}
-	return reconcile.Result{}, true, nil
+	return reconcile.Result{Requeue: !done}, done, err
 }
 
 func (ip *IdentityProviderReconciler) Finalize(ctx *deploy.DeployContext) bool {
-	var err error
+	oauthClient, err := FindOAuthClient(ctx)
+	if err != nil {
+		logrus.Errorf("Error deleting finalizer: %v", err)
+		return false
+	}
 
-	oAuthClientName := ctx.CheCluster.Spec.Auth.OAuthClientName
-	if oAuthClientName != "" {
-		err = deploy.DeleteObjectWithFinalizer(ctx, types.NamespacedName{Name: oAuthClientName}, &oauth.OAuthClient{}, OAuthFinalizerName)
+	if oauthClient != nil {
+		err = deploy.DeleteObjectWithFinalizer(ctx, types.NamespacedName{Name: oauthClient.Name}, &oauth.OAuthClient{}, OAuthFinalizerName)
 	} else {
 		err = deploy.DeleteFinalizer(ctx, OAuthFinalizerName)
 	}
@@ -65,55 +65,31 @@ func (ip *IdentityProviderReconciler) Finalize(ctx *deploy.DeployContext) bool {
 	return true
 }
 
-func syncNativeIdentityProviderItems(deployContext *deploy.DeployContext) (bool, error) {
-	cr := deployContext.CheCluster
+func syncNativeIdentityProviderItems(ctx *deploy.DeployContext) (bool, error) {
+	oauthSecret := util.GeneratePasswd(12)
+	oauthClientName := ctx.CheCluster.Name + "-openshift-identity-provider-" + strings.ToLower(util.GeneratePasswd(6))
 
-	if err := resolveOpenshiftOAuthClientName(deployContext); err != nil {
-		return false, err
-	}
-	if err := resolveOpenshiftOAuthClientSecret(deployContext); err != nil {
+	oauthClient, err := FindOAuthClient(ctx)
+	if err != nil {
 		return false, err
 	}
 
-	if util.IsOpenShift {
-		redirectURIs := []string{"https://" + deployContext.CheCluster.GetCheHost() + "/oauth/callback"}
-		oAuthClient := getOAuthClientSpec(cr.Spec.Auth.OAuthClientName, cr.Spec.Auth.OAuthSecret, redirectURIs)
-		done, err := deploy.Sync(deployContext, oAuthClient, oAuthClientDiffOpts)
-		if !done {
-			return false, err
-		}
+	if oauthClient != nil {
+		oauthSecret = oauthClient.Secret
+		oauthClientName = oauthClient.Name
+	}
 
-		err = deploy.AppendFinalizer(deployContext, OAuthFinalizerName)
-		if err != nil {
-			return false, err
-		}
+	redirectURIs := []string{"https://" + ctx.CheCluster.GetCheHost() + "/oauth/callback"}
+	oauthClientSpec := getOAuthClientSpec(oauthClientName, oauthSecret, redirectURIs)
+	done, err := deploy.Sync(ctx, oauthClientSpec, oAuthClientDiffOpts)
+	if !done {
+		return false, err
+	}
+
+	err = deploy.AppendFinalizer(ctx, OAuthFinalizerName)
+	if err != nil {
+		return false, err
 	}
 
 	return true, nil
 }
-
-func resolveOpenshiftOAuthClientName(deployContext *deploy.DeployContext) error {
-	cr := deployContext.CheCluster
-	oAuthClientName := cr.Spec.Auth.OAuthClientName
-	if len(oAuthClientName) < 1 {
-		oAuthClientName = cr.Name + "-openshift-identity-provider-" + strings.ToLower(util.GeneratePasswd(6))
-		cr.Spec.Auth.OAuthClientName = oAuthClientName
-		if err := deploy.UpdateCheCRSpec(deployContext, "oAuthClient name", oAuthClientName); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func resolveOpenshiftOAuthClientSecret(deployContext *deploy.DeployContext) error {
-	cr := deployContext.CheCluster
-	oauthSecret := cr.Spec.Auth.OAuthSecret
-	if len(oauthSecret) < 1 {
-		oauthSecret = util.GeneratePasswd(12)
-		cr.Spec.Auth.OAuthSecret = oauthSecret
-		if err := deploy.UpdateCheCRSpec(deployContext, "oAuth secret name", oauthSecret); err != nil {
-			return err
-		}
-	}
-	return nil
-}

pkg/deploy/identity-provider/identity_provider_util.go

@@ -12,19 +12,25 @@
 package identityprovider
 
 import (
+	"context"
+	"fmt"
+
+	"github.com/eclipse-che/che-operator/pkg/deploy"
 	oauth "github.com/openshift/api/oauth/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func getOAuthClientSpec(name string, oauthSecret string, redirectURIs []string) *oauth.OAuthClient {
 	return &oauth.OAuthClient{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "OAuthClient",
-			APIVersion: oauth.SchemeGroupVersion.String(),
+			APIVersion: oauth.GroupVersion.String(),
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name:   name,
-			Labels: map[string]string{"app": "che"},
+			Labels: map[string]string{deploy.KubernetesPartOfLabelKey: deploy.CheEclipseOrg},
 		},
 
 		Secret:       oauthSecret,
@@ -32,3 +38,24 @@ func getOAuthClientSpec(name string, oauthSecret string, redirectURIs []string)
 		GrantMethod:  oauth.GrantHandlerPrompt,
 	}
 }
+
+func FindOAuthClient(ctx *deploy.DeployContext) (*oauth.OAuthClient, error) {
+	oauthClients := &oauth.OAuthClientList{}
+	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(map[string]string{deploy.KubernetesPartOfLabelKey: deploy.CheEclipseOrg})}
+
+	if err := ctx.ClusterAPI.Client.List(
+		context.TODO(),
+		oauthClients,
+		listOptions); err != nil {
+		return nil, err
+	}
+
+	switch len(oauthClients.Items) {
+	case 0:
+		return nil, nil
+	case 1:
+		return &oauthClients.Items[0], nil
+	default:
+		return nil, fmt.Errorf("more than one OAuthClient found with '%s:%s' labels", deploy.KubernetesPartOfLabelKey, deploy.CheEclipseOrg)
+	}
+}

pkg/deploy/migration/on-reconcile-one-time-migration.go

@@ -17,6 +17,8 @@ import (
 	"fmt"
 	"time"
 
+	oauthv1 "github.com/openshift/api/oauth/v1"
+
 	"github.com/eclipse-che/che-operator/pkg/deploy"
 	"github.com/eclipse-che/che-operator/pkg/util"
 	routev1 "github.com/openshift/api/route/v1"
@@ -231,6 +233,7 @@ func addPartOfCheLabelForObjectsWithLabel(ctx *deploy.DeployContext, labelKey st
 	}
 	if util.IsOpenShift {
 		kindsToMigrate = append(kindsToMigrate, &routev1.RouteList{})
+		kindsToMigrate = append(kindsToMigrate, &oauthv1.OAuthClientList{})
 	} else {
 		kindsToMigrate = append(kindsToMigrate, &networkingv1.IngressList{})
 	}