diff --git a/pkg/deploy/gateway/oauth_proxy.go b/pkg/deploy/gateway/oauth_proxy.go index 7bbea41c4..fd0de4384 100644 --- a/pkg/deploy/gateway/oauth_proxy.go +++ b/pkg/deploy/gateway/oauth_proxy.go @@ -61,7 +61,7 @@ func openshiftOauthProxyConfig(ctx *chetypes.DeployContext, cookieSecret string) oauthSecret := "" oauthClientName := "" - oauthClient, _ := identityprovider.FindOAuthClient(ctx) + oauthClient, _ := identityprovider.GetOAuthClient(ctx) if oauthClient == nil { logrus.Error("oauth client not found") } else { diff --git a/pkg/deploy/identity-provider/identity_provider_reconciler.go b/pkg/deploy/identity-provider/identity_provider_reconciler.go index f49ec5802..5adc816e5 100644 --- a/pkg/deploy/identity-provider/identity_provider_reconciler.go +++ b/pkg/deploy/identity-provider/identity_provider_reconciler.go @@ -12,9 +12,8 @@ package identityprovider import ( - "strings" - "github.com/eclipse-che/che-operator/pkg/common/utils" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/eclipse-che/che-operator/pkg/common/chetypes" @@ -46,57 +45,38 @@ func (ip *IdentityProviderReconciler) Reconcile(ctx *chetypes.DeployContext) (re } func (ip *IdentityProviderReconciler) Finalize(ctx *chetypes.DeployContext) bool { - oauthClients, err := FindAllEclipseCheOAuthClients(ctx) + oauthClient, err := GetOAuthClient(ctx) if err != nil { logrus.Errorf("Error getting OAuthClients: %v", err) return false } - for _, oauthClient := range oauthClients { - if _, err := deploy.DeleteClusterObject(ctx, oauthClient.Name, &oauth.OAuthClient{}); err != nil { + if oauthClient != nil { + if err := deploy.DeleteObjectWithFinalizer(ctx, types.NamespacedName{Name: oauthClient.Name}, &oauth.OAuthClient{}, OAuthFinalizerName); err != nil { logrus.Errorf("Error deleting OAuthClient: %v", err) return false } } - if err := deploy.DeleteFinalizer(ctx, OAuthFinalizerName); err != nil { - logrus.Errorf("Error deleting finalizer: %v", err) - return false - } - return true } func syncOAuthClient(ctx *chetypes.DeployContext) (bool, error) { - oauthClientName := ctx.CheCluster.Spec.Networking.Auth.OAuthClientName - oauthSecret := ctx.CheCluster.Spec.Networking.Auth.OAuthSecret + var oauthClientName, oauthSecret string - if oauthClientName == "" { - oauthClient, err := FindOAuthClient(ctx) - if err != nil { - logrus.Errorf("Error getting OAuthClients: %v", err) - return false, err - } - - if oauthClient != nil { - oauthClientName = oauthClient.Name - if oauthSecret == "" { - oauthSecret = oauthClient.Secret - } - } - } else { - oauthClient := &oauth.OAuthClient{} - exists, _ := deploy.GetClusterObject(ctx, oauthClientName, oauthClient) - if exists { - if oauthSecret == "" { - oauthSecret = oauthClient.Secret - } - } + oauthClient, err := GetOAuthClient(ctx) + if err != nil { + logrus.Errorf("Error getting OAuthClients: %v", err) + return false, err } - // Generate secret and name - oauthSecret = utils.GetValue(oauthSecret, utils.GeneratePassword(12)) - oauthClientName = utils.GetValue(oauthClientName, ctx.CheCluster.Name+"-openshift-identity-provider-"+strings.ToLower(utils.GeneratePassword(6))) + if oauthClient != nil { + oauthClientName = oauthClient.Name + oauthSecret = utils.GetValue(ctx.CheCluster.Spec.Networking.Auth.OAuthSecret, oauthClient.Secret) + } else { + oauthClientName = GetOAuthClientName(ctx) + oauthSecret = utils.GetValue(ctx.CheCluster.Spec.Networking.Auth.OAuthSecret, utils.GeneratePassword(12)) + } redirectURIs := []string{"https://" + ctx.CheHost + "/oauth/callback"} oauthClientSpec := GetOAuthClientSpec( diff --git a/pkg/deploy/identity-provider/identity_provider_reconciler_test.go b/pkg/deploy/identity-provider/identity_provider_reconciler_test.go index 524e0ec2f..e0b0607f5 100644 --- a/pkg/deploy/identity-provider/identity_provider_reconciler_test.go +++ b/pkg/deploy/identity-provider/identity_provider_reconciler_test.go @@ -28,11 +28,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" ) -func TestFinalize(t *testing.T) { - oauthClient1 := GetOAuthClientSpec("test1", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) - oauthClient2 := GetOAuthClientSpec("test2", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) - oauthClient3 := GetOAuthClientSpec("test3", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) - oauthClient3.ObjectMeta.Labels = map[string]string{} +func TestFinalizeDefaultOAuthClientName(t *testing.T) { + oauthClient := GetOAuthClientSpec("eclipse-che-client", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) + oauthClient.ObjectMeta.Labels = map[string]string{} checluster := &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -42,21 +40,72 @@ func TestFinalize(t *testing.T) { }, } - ctx := test.GetDeployContext(checluster, []runtime.Object{oauthClient1, oauthClient2, oauthClient3}) + ctx := test.GetDeployContext(checluster, []runtime.Object{oauthClient}) - assert.True(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test1"}, &oauthv1.OAuthClient{})) - assert.True(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test2"}, &oauthv1.OAuthClient{})) - assert.True(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test3"}, &oauthv1.OAuthClient{})) + assert.True(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "eclipse-che-client"}, &oauthv1.OAuthClient{})) identityProviderReconciler := NewIdentityProviderReconciler() done := identityProviderReconciler.Finalize(ctx) assert.True(t, done) - assert.False(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test1"}, &oauthv1.OAuthClient{})) - assert.False(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test2"}, &oauthv1.OAuthClient{})) - assert.True(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test3"}, &oauthv1.OAuthClient{})) + assert.False(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "eclipse-che-client"}, &oauthv1.OAuthClient{})) assert.Equal(t, 0, len(checluster.Finalizers)) } +func TestFinalizeOAuthClient(t *testing.T) { + oauthClient := GetOAuthClientSpec("test", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) + oauthClient.ObjectMeta.Labels = map[string]string{} + + checluster := &chev2.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "eclipse-che", + Namespace: "eclipse-che", + Finalizers: []string{OAuthFinalizerName}, + }, + Spec: chev2.CheClusterSpec{ + Networking: chev2.CheClusterSpecNetworking{ + Auth: chev2.Auth{ + OAuthClientName: "test", + }, + }, + }, + } + + ctx := test.GetDeployContext(checluster, []runtime.Object{oauthClient}) + + assert.True(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test"}, &oauthv1.OAuthClient{})) + + identityProviderReconciler := NewIdentityProviderReconciler() + done := identityProviderReconciler.Finalize(ctx) + assert.True(t, done) + assert.False(t, test.IsObjectExists(ctx.ClusterAPI.Client, types.NamespacedName{Name: "test"}, &oauthv1.OAuthClient{})) + assert.Equal(t, 0, len(checluster.Finalizers)) +} + +func TestShouldFindSingleOAuthClient(t *testing.T) { + oauthClient1 := GetOAuthClientSpec("test1", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) + oauthClient2 := GetOAuthClientSpec("test2", "secret", []string{"https://che-host/oauth/callback"}, nil, nil) + + checluster := &chev2.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "eclipse-che", + Namespace: "eclipse-che", + }, + Spec: chev2.CheClusterSpec{ + Networking: chev2.CheClusterSpecNetworking{ + Auth: chev2.Auth{ + OAuthClientName: "test1", + }, + }, + }, + } + + ctx := test.GetDeployContext(checluster, []runtime.Object{oauthClient1, oauthClient2}) + oauthClient, err := GetOAuthClient(ctx) + assert.Nil(t, err) + assert.NotNil(t, oauthClient) + assert.Equal(t, "test1", oauthClient.Name) +} + func TestSyncOAuthClientShouldSyncTokenTimeout(t *testing.T) { checluster := &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -78,10 +127,11 @@ func TestSyncOAuthClientShouldSyncTokenTimeout(t *testing.T) { assert.True(t, done) assert.Nil(t, err) - oauthClients, err := FindAllEclipseCheOAuthClients(ctx) + oauthClient, err := GetOAuthClient(ctx) assert.Nil(t, err) - assert.Equal(t, int32(10), *oauthClients[0].AccessTokenInactivityTimeoutSeconds) - assert.Equal(t, int32(20), *oauthClients[0].AccessTokenMaxAgeSeconds) + assert.NotNil(t, oauthClient) + assert.Equal(t, int32(10), *oauthClient.AccessTokenInactivityTimeoutSeconds) + assert.Equal(t, int32(20), *oauthClient.AccessTokenMaxAgeSeconds) } func TestSyncOAuthClient(t *testing.T) { @@ -113,13 +163,14 @@ func TestSyncOAuthClient(t *testing.T) { expectedSecret: "secret", }, { - name: "Sync OAuthClient, generate name and secret", + name: "Sync OAuthClient, generate secret, default name", cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "eclipse-che", Namespace: "eclipse-che", }, }, + expectedName: "eclipse-che-client", }, { name: "Sync OAuthClient, generate secret", @@ -139,7 +190,7 @@ func TestSyncOAuthClient(t *testing.T) { expectedName: "test", }, { - name: "Sync OAuthClient, generate name", + name: "Sync OAuthClient, default name", cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "eclipse-che", @@ -154,6 +205,7 @@ func TestSyncOAuthClient(t *testing.T) { }, }, expectedSecret: "secret", + expectedName: "eclipse-che-client", }, } @@ -165,21 +217,22 @@ func TestSyncOAuthClient(t *testing.T) { _, err := syncOAuthClient(ctx) assert.Nil(t, err) - oauthClients, err := FindAllEclipseCheOAuthClients(ctx) + oauthClient, err := GetOAuthClient(ctx) assert.Nil(t, err) - assert.Equal(t, 1, len(oauthClients)) + assert.NotNil(t, oauthClient) if testCase.expectedName != "" { - assert.Equal(t, testCase.expectedName, oauthClients[0].Name) + assert.Equal(t, testCase.expectedName, oauthClient.Name) } if testCase.expectedSecret != "" { - assert.Equal(t, testCase.expectedSecret, oauthClients[0].Secret) + assert.Equal(t, testCase.expectedSecret, oauthClient.Secret) } }) } } func TestSyncExistedOAuthClient(t *testing.T) { - oauthClient := GetOAuthClientSpec("test", "secret", []string{}, nil, nil) + oauthClient1 := GetOAuthClientSpec("test", "secret", []string{}, nil, nil) + oauthClient2 := GetOAuthClientSpec("eclipse-che-client", "secret", []string{}, nil, nil) type testCase struct { name string @@ -190,7 +243,7 @@ func TestSyncExistedOAuthClient(t *testing.T) { testCases := []testCase{ { - name: "Sync existed OAuthClient", + name: "Sync existed OAuthClient with the default name", cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "eclipse-che", @@ -198,29 +251,10 @@ func TestSyncExistedOAuthClient(t *testing.T) { }, }, expectedSecret: "secret", - expectedName: "test", + expectedName: "eclipse-che-client", }, { - name: "Sync existed OAuthClient, OAuthSecret and OAuthClientName are defined", - cheCluster: &chev2.CheCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "eclipse-che", - Namespace: "eclipse-che", - }, - Spec: chev2.CheClusterSpec{ - Networking: chev2.CheClusterSpecNetworking{ - Auth: chev2.Auth{ - OAuthSecret: "secret", - OAuthClientName: "test", - }, - }, - }, - }, - expectedSecret: "secret", - expectedName: "test", - }, - { - name: "Sync existed OAuthClient, OAuthClientName is defined", + name: "Sync existed OAuthClient with a custom name", cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "eclipse-che", @@ -238,43 +272,7 @@ func TestSyncExistedOAuthClient(t *testing.T) { expectedName: "test", }, { - name: "Sync existed OAuthClient, OAuthSecret is defined", - cheCluster: &chev2.CheCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "eclipse-che", - Namespace: "eclipse-che", - }, - Spec: chev2.CheClusterSpec{ - Networking: chev2.CheClusterSpecNetworking{ - Auth: chev2.Auth{ - OAuthSecret: "secret", - }, - }, - }, - }, - expectedSecret: "secret", - expectedName: "test", - }, - { - name: "Sync existed OAuthClient, update secret, usecase #1", - cheCluster: &chev2.CheCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "eclipse-che", - Namespace: "eclipse-che", - }, - Spec: chev2.CheClusterSpec{ - Networking: chev2.CheClusterSpecNetworking{ - Auth: chev2.Auth{ - OAuthSecret: "new-secret", - }, - }, - }, - }, - expectedSecret: "new-secret", - expectedName: "test", - }, - { - name: "Sync existed OAuthClient, update secret, usecase #2", + name: "Sync existed OAuthClient with a custom name, update secret", cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "eclipse-che", @@ -292,24 +290,42 @@ func TestSyncExistedOAuthClient(t *testing.T) { expectedSecret: "new-secret", expectedName: "test", }, + { + name: "Sync existed OAuthClient with the default name, update secret", + cheCluster: &chev2.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "eclipse-che", + Namespace: "eclipse-che", + }, + Spec: chev2.CheClusterSpec{ + Networking: chev2.CheClusterSpecNetworking{ + Auth: chev2.Auth{ + OAuthSecret: "new-secret", + }, + }, + }, + }, + expectedSecret: "new-secret", + expectedName: "eclipse-che-client", + }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { logf.SetLogger(zap.New(zap.WriteTo(os.Stdout), zap.UseDevMode(true))) - ctx := test.GetDeployContext(testCase.cheCluster, []runtime.Object{oauthClient}) + ctx := test.GetDeployContext(testCase.cheCluster, []runtime.Object{oauthClient1, oauthClient2}) _, err := syncOAuthClient(ctx) assert.Nil(t, err) - oauthClients, err := FindAllEclipseCheOAuthClients(ctx) + oauthClient, err := GetOAuthClient(ctx) assert.Nil(t, err) - assert.Equal(t, 1, len(oauthClients)) + assert.NotNil(t, oauthClient) if testCase.expectedName != "" { - assert.Equal(t, testCase.expectedName, oauthClients[0].Name) + assert.Equal(t, testCase.expectedName, oauthClient.Name) } if testCase.expectedSecret != "" { - assert.Equal(t, testCase.expectedSecret, oauthClients[0].Secret) + assert.Equal(t, testCase.expectedSecret, oauthClient.Secret) } }) } diff --git a/pkg/deploy/identity-provider/identity_provider_util.go b/pkg/deploy/identity-provider/identity_provider_util.go index cc1b0d5c0..f4fc24f17 100644 --- a/pkg/deploy/identity-provider/identity_provider_util.go +++ b/pkg/deploy/identity-provider/identity_provider_util.go @@ -12,15 +12,11 @@ package identityprovider import ( - "context" - "fmt" - "github.com/eclipse-che/che-operator/pkg/common/chetypes" "github.com/eclipse-che/che-operator/pkg/common/constants" + "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( @@ -29,6 +25,7 @@ func GetOAuthClientSpec( redirectURIs []string, accessTokenInactivityTimeoutSeconds *int32, accessTokenMaxAgeSeconds *int32) *oauth.OAuthClient { + return &oauth.OAuthClient{ TypeMeta: metav1.TypeMeta{ Kind: "OAuthClient", @@ -47,31 +44,22 @@ func GetOAuthClientSpec( } } -func FindOAuthClient(ctx *chetypes.DeployContext) (*oauth.OAuthClient, error) { - oauthClients, err := FindAllEclipseCheOAuthClients(ctx) - if err != nil { +func GetOAuthClient(ctx *chetypes.DeployContext) (*oauth.OAuthClient, error) { + oAuthClientName := GetOAuthClientName(ctx) + + oauthClient := &oauth.OAuthClient{} + exists, err := deploy.GetClusterObject(ctx, oAuthClientName, oauthClient) + if !exists { return nil, err } - switch len(oauthClients) { - case 0: - return nil, nil - case 1: - return &oauthClients[0], nil - default: - return nil, fmt.Errorf("more than one OAuthClient found with '%s:%s' labels", constants.KubernetesPartOfLabelKey, constants.CheEclipseOrg) - } + return oauthClient, nil } -func FindAllEclipseCheOAuthClients(ctx *chetypes.DeployContext) ([]oauth.OAuthClient, error) { - oauthClients := &oauth.OAuthClientList{} - listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(map[string]string{constants.KubernetesPartOfLabelKey: constants.CheEclipseOrg})} - - if err := ctx.ClusterAPI.Client.List( - context.TODO(), - oauthClients, - listOptions); err != nil { - return nil, err +func GetOAuthClientName(ctx *chetypes.DeployContext) string { + if ctx.CheCluster.Spec.Networking.Auth.OAuthClientName != "" { + return ctx.CheCluster.Spec.Networking.Auth.OAuthClientName } - return oauthClients.Items, nil + + return ctx.CheCluster.Namespace + "-client" }