From 1fb898e2d2f9da3b66eae94cc848045d7682cf3f Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Tue, 9 Feb 2021 23:12:57 +0200 Subject: [PATCH] Try to handle remaining feedback. Signed-off-by: Oleksandr Andriienko --- local-debug.sh | 1 + .../oauth_initial_htpasswd_provider_mock.go | 17 ++++---- pkg/controller/che/che_controller.go | 4 +- pkg/controller/che/che_controller_test.go | 2 +- pkg/controller/che/create.go | 13 ++++-- .../che/oauth_initial_htpasswd_provider.go | 40 +++++++------------ .../oauth_initial_htpasswd_provider_test.go | 32 +++++++++++++-- pkg/deploy/secret.go | 40 ++++++------------- 8 files changed, 77 insertions(+), 72 deletions(-) diff --git a/local-debug.sh b/local-debug.sh index 313a8ccce..e98de32e3 100755 --- a/local-debug.sh +++ b/local-debug.sh @@ -60,6 +60,7 @@ kubectl apply -f "${CR}" -n $CHE_NAMESPACE cp templates/keycloak-provision.sh /tmp/keycloak-provision.sh cp templates/delete-identity-provider.sh /tmp/delete-identity-provider.sh cp templates/create-github-identity-provider.sh /tmp/create-github-identity-provider.sh +cp templates/oauth-provision.sh /tmp/oauth-provision.sh ENV_FILE=/tmp/che-operator-debug.env rm -rf "${ENV_FILE}" diff --git a/mocks/pkg/controller/che/oauth_initial_htpasswd_provider_mock.go b/mocks/pkg/controller/che/oauth_initial_htpasswd_provider_mock.go index 2bd1f04a2..af19b465f 100644 --- a/mocks/pkg/controller/che/oauth_initial_htpasswd_provider_mock.go +++ b/mocks/pkg/controller/che/oauth_initial_htpasswd_provider_mock.go @@ -5,6 +5,7 @@ package mock_che import ( + deploy "github.com/eclipse/che-operator/pkg/deploy" gomock "github.com/golang/mock/gomock" v1 "github.com/openshift/api/config/v1" reflect "reflect" @@ -34,29 +35,29 @@ func (m *MockOpenShiftOAuthUserHandler) EXPECT() *MockOpenShiftOAuthUserHandlerM } // CreateOAuthInitialUser mocks base method -func (m *MockOpenShiftOAuthUserHandler) CreateOAuthInitialUser(userNamePrefix, crNamespace string, openshiftOAuth *v1.OAuth) error { +func (m *MockOpenShiftOAuthUserHandler) CreateOAuthInitialUser(openshiftOAuth *v1.OAuth, deployContext *deploy.DeployContext) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOAuthInitialUser", userNamePrefix, crNamespace, openshiftOAuth) + ret := m.ctrl.Call(m, "CreateOAuthInitialUser", openshiftOAuth, deployContext) ret0, _ := ret[0].(error) return ret0 } // CreateOAuthInitialUser indicates an expected call of CreateOAuthInitialUser -func (mr *MockOpenShiftOAuthUserHandlerMockRecorder) CreateOAuthInitialUser(userNamePrefix, crNamespace, openshiftOAuth interface{}) *gomock.Call { +func (mr *MockOpenShiftOAuthUserHandlerMockRecorder) CreateOAuthInitialUser(openshiftOAuth, deployContext interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOAuthInitialUser", reflect.TypeOf((*MockOpenShiftOAuthUserHandler)(nil).CreateOAuthInitialUser), userNamePrefix, crNamespace, openshiftOAuth) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOAuthInitialUser", reflect.TypeOf((*MockOpenShiftOAuthUserHandler)(nil).CreateOAuthInitialUser), openshiftOAuth, deployContext) } // DeleteOAuthInitialUser mocks base method -func (m *MockOpenShiftOAuthUserHandler) DeleteOAuthInitialUser(userNamePrefix, crNamespace string) error { +func (m *MockOpenShiftOAuthUserHandler) DeleteOAuthInitialUser(deployContext *deploy.DeployContext) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteOAuthInitialUser", userNamePrefix, crNamespace) + ret := m.ctrl.Call(m, "DeleteOAuthInitialUser", deployContext) ret0, _ := ret[0].(error) return ret0 } // DeleteOAuthInitialUser indicates an expected call of DeleteOAuthInitialUser -func (mr *MockOpenShiftOAuthUserHandlerMockRecorder) DeleteOAuthInitialUser(userNamePrefix, crNamespace interface{}) *gomock.Call { +func (mr *MockOpenShiftOAuthUserHandlerMockRecorder) DeleteOAuthInitialUser(deployContext interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuthInitialUser", reflect.TypeOf((*MockOpenShiftOAuthUserHandler)(nil).DeleteOAuthInitialUser), userNamePrefix, crNamespace) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuthInitialUser", reflect.TypeOf((*MockOpenShiftOAuthUserHandler)(nil).DeleteOAuthInitialUser), deployContext) } diff --git a/pkg/controller/che/che_controller.go b/pkg/controller/che/che_controller.go index a5a581c5a..c4e8f6407 100644 --- a/pkg/controller/che/che_controller.go +++ b/pkg/controller/che/che_controller.go @@ -385,7 +385,7 @@ func (r *ReconcileChe) Reconcile(request reconcile.Request) (reconcile.Result, e } if isOpenShift4 && instance.Spec.Auth.InitialOpenShiftOAuthUser != nil && !*instance.Spec.Auth.InitialOpenShiftOAuthUser && instance.Status.OpenShiftOAuthUserCredentialsSecret != "" { - if err := r.userHandler.DeleteOAuthInitialUser(instance.Namespace, deploy.DefaultCheFlavor(instance)); err != nil { + if err := r.userHandler.DeleteOAuthInitialUser(deployContext); err != nil { logrus.Errorf("Unable to delete initial user from cluster. Cause: %s", err.Error()) instance.Spec.Auth.InitialOpenShiftOAuthUser = nil if err := r.UpdateCheCRSpec(instance, "InitialOpenShiftOAuthUser", "nil"); err != nil { @@ -1137,7 +1137,7 @@ func (r *ReconcileChe) autoEnableOAuth(deployContext *deploy.DeployContext, requ if len(openshitOAuth.Spec.IdentityProviders) > 0 { oauth = true } else if util.IsInitialOpenShiftOAuthUserEnabled(cr) { - if err := r.userHandler.CreateOAuthInitialUser(deploy.DefaultCheFlavor(cr), cr.Namespace, openshitOAuth); err != nil { + if err := r.userHandler.CreateOAuthInitialUser(openshitOAuth, deployContext); err != nil { message = warningNoIdentityProvidersMessage + " Operator tried to create initial OpenShift OAuth user for HTPasswd identity provider, but failed. Cause: " + err.Error() logrus.Error(message) logrus.Info("To enable OpenShift OAuth, please add identity provider first: " + howToAddIdentityProviderLinkOS4) diff --git a/pkg/controller/che/che_controller_test.go b/pkg/controller/che/che_controller_test.go index 492b49bd1..4bda9c28e 100644 --- a/pkg/controller/che/che_controller_test.go +++ b/pkg/controller/che/che_controller_test.go @@ -296,7 +296,7 @@ func TestCaseAutoDetectOAuth(t *testing.T) { initialOpenShiftOAuthUserEnabled: util.NewBoolPointer(true), mockFunction: func(ctrl *gomock.Controller, crNamespace string, userNamePrefix string) *che_mocks.MockOpenShiftOAuthUserHandler { m := che_mocks.NewMockOpenShiftOAuthUserHandler(ctrl) - m.EXPECT().CreateOAuthInitialUser(userNamePrefix, crNamespace, gomock.Any()) + m.EXPECT().CreateOAuthInitialUser(gomock.Any(), gomock.Any()) return m }, OpenShiftOAuthUserCredentialsSecret: "openshift-oauth-user-credentials", diff --git a/pkg/controller/che/create.go b/pkg/controller/che/create.go index 36d78b818..4df609b4b 100644 --- a/pkg/controller/che/create.go +++ b/pkg/controller/che/create.go @@ -19,6 +19,7 @@ import ( func (r *ReconcileChe) GenerateAndSaveFields(deployContext *deploy.DeployContext, request reconcile.Request) (err error) { cheFlavor := deploy.DefaultCheFlavor(deployContext.CheCluster) + cheNamespace := deployContext.CheCluster.Namespace if len(deployContext.CheCluster.Spec.Server.CheFlavor) < 1 { deployContext.CheCluster.Spec.Server.CheFlavor = cheFlavor if err := r.UpdateCheCRSpec(deployContext.CheCluster, "installation flavor", cheFlavor); err != nil { @@ -31,7 +32,10 @@ func (r *ReconcileChe) GenerateAndSaveFields(deployContext *deploy.DeployContext if len(deployContext.CheCluster.Spec.Database.ChePostgresSecret) < 1 { if len(deployContext.CheCluster.Spec.Database.ChePostgresUser) < 1 || len(deployContext.CheCluster.Spec.Database.ChePostgresPassword) < 1 { chePostgresSecret := deploy.DefaultChePostgresSecret() - deploy.SyncSecretToCluster(deployContext, chePostgresSecret, map[string][]byte{"user": []byte(deploy.DefaultChePostgresUser), "password": []byte(util.GeneratePasswd(12))}) + _, err := deploy.SyncSecret(deployContext, chePostgresSecret, cheNamespace, map[string][]byte{"user": []byte(deploy.DefaultChePostgresUser), "password": []byte(util.GeneratePasswd(12))}) + if err != nil { + return err + } deployContext.CheCluster.Spec.Database.ChePostgresSecret = chePostgresSecret if err := r.UpdateCheCRSpec(deployContext.CheCluster, "Postgres Secret", chePostgresSecret); err != nil { return err @@ -60,7 +64,10 @@ func (r *ReconcileChe) GenerateAndSaveFields(deployContext *deploy.DeployContext if len(deployContext.CheCluster.Spec.Auth.IdentityProviderPostgresPassword) < 1 { identityPostgresSecret := deploy.DefaultCheIdentityPostgresSecret() - deploy.SyncSecretToCluster(deployContext, identityPostgresSecret, map[string][]byte{"password": []byte(keycloakPostgresPassword)}) + _, err := deploy.SyncSecret(deployContext, identityPostgresSecret, cheNamespace, map[string][]byte{"password": []byte(keycloakPostgresPassword)}) + if err != nil { + return err + } deployContext.CheCluster.Spec.Auth.IdentityProviderPostgresSecret = identityPostgresSecret if err := r.UpdateCheCRSpec(deployContext.CheCluster, "Identity Provider Postgres Secret", identityPostgresSecret); err != nil { return err @@ -80,7 +87,7 @@ func (r *ReconcileChe) GenerateAndSaveFields(deployContext *deploy.DeployContext if len(deployContext.CheCluster.Spec.Auth.IdentityProviderAdminUserName) < 1 || len(deployContext.CheCluster.Spec.Auth.IdentityProviderPassword) < 1 { identityProviderSecret := deploy.DefaultCheIdentitySecret() - _, err = deploy.SyncSecretToCluster(deployContext, identityProviderSecret, map[string][]byte{"user": []byte(keycloakAdminUserName), "password": []byte(keycloakAdminPassword)}) + _, err = deploy.SyncSecret(deployContext, identityProviderSecret, cheNamespace, map[string][]byte{"user": []byte(keycloakAdminUserName), "password": []byte(keycloakAdminPassword)}) if err != nil { return err } diff --git a/pkg/controller/che/oauth_initial_htpasswd_provider.go b/pkg/controller/che/oauth_initial_htpasswd_provider.go index e63cb1738..51f2d6f8e 100644 --- a/pkg/controller/che/oauth_initial_htpasswd_provider.go +++ b/pkg/controller/che/oauth_initial_htpasswd_provider.go @@ -23,7 +23,6 @@ import ( oauthv1 "github.com/openshift/api/config/v1" userv1 "github.com/openshift/api/user/v1" "github.com/sirupsen/logrus" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -39,8 +38,8 @@ const ( // OpenShiftOAuthUserHandler - handler to create or delete new Openshift oAuth user. type OpenShiftOAuthUserHandler interface { - CreateOAuthInitialUser(userName string, crNamespace string, openshiftOAuth *oauthv1.OAuth) error - DeleteOAuthInitialUser(userName string, crNamespace string) error + CreateOAuthInitialUser(openshiftOAuth *oauthv1.OAuth, deployContext *deploy.DeployContext) error + DeleteOAuthInitialUser(deployContext *deploy.DeployContext) error } // OpenShiftOAuthUserOperatorHandler - implementation OpenShiftOAuthUserHandler. @@ -64,24 +63,18 @@ func NewOpenShiftOAuthUserHandler(runtimeClient client.Client) OpenShiftOAuthUse // It usefull for good first user expirience. // User can't use kube:admin or system:admin user in the Openshift oAuth. That's why we provide // initial user for good first meeting with Eclipse Che. -func (iuh *OpenShiftOAuthUserOperatorHandler) CreateOAuthInitialUser(userName string, crNamespace string, openshiftOAuth *oauthv1.OAuth) error { +func (iuh *OpenShiftOAuthUserOperatorHandler) CreateOAuthInitialUser(openshiftOAuth *oauthv1.OAuth, deployContext *deploy.DeployContext) error { password := util.GeneratePasswd(6) + cr := deployContext.CheCluster + userName := deploy.DefaultCheFlavor(cr) htpasswdFileContent, err := iuh.generateHtPasswdUserInfo(userName, password) if err != nil { return err } - secret := &corev1.Secret{} - nsName := types.NamespacedName{Name: htpasswdSecretName, Namespace: ocConfigNamespace} - if err := iuh.runtimeClient.Get(context.TODO(), nsName, secret); err != nil { - if errors.IsNotFound(err) { - htpasswdFileSecretData := map[string][]byte{"htpasswd": []byte(htpasswdFileContent)} - if err = deploy.CreateSecret(htpasswdFileSecretData, htpasswdSecretName, ocConfigNamespace, iuh.runtimeClient); err != nil { - return err - } - } else { - return err - } + htpasswdFileSecretData := map[string][]byte{"htpasswd": []byte(htpasswdFileContent)} + if _, err := deploy.SyncSecret(deployContext, htpasswdSecretName, ocConfigNamespace, htpasswdFileSecretData); err != nil { + return err } if err := appendIdentityProvider(openshiftOAuth, iuh.runtimeClient); err != nil { @@ -90,7 +83,7 @@ func (iuh *OpenShiftOAuthUserOperatorHandler) CreateOAuthInitialUser(userName st initialUserSecretData := map[string][]byte{"user": []byte(userName), "password": []byte(password)} logrus.Info("Create initial user secret for che-operator") - if err := deploy.CreateSecret(initialUserSecretData, openShiftOAuthUserCredentialsSecret, crNamespace, iuh.runtimeClient); err != nil { + if _, err := deploy.SyncSecret(deployContext, openShiftOAuthUserCredentialsSecret, cr.Namespace, initialUserSecretData); err != nil { return err } @@ -98,25 +91,20 @@ func (iuh *OpenShiftOAuthUserOperatorHandler) CreateOAuthInitialUser(userName st } // DeleteOAuthInitialUser - removes initial user, htpasswd provider, htpasswd secret and Che secret with username and password. -func (iuh *OpenShiftOAuthUserOperatorHandler) DeleteOAuthInitialUser(userName string, crNamespace string) error { +func (iuh *OpenShiftOAuthUserOperatorHandler) DeleteOAuthInitialUser(deployContext *deploy.DeployContext) error { oAuth, err := GetOpenshiftOAuth(iuh.runtimeClient) if err != nil { return err } - var identityProviderExists bool - for _, ip := range oAuth.Spec.IdentityProviders { - if ip.Name == htpasswdIdentityProviderName { - identityProviderExists = true - break - } - } - if identityProviderExists { + if identityProviderExists(htpasswdIdentityProviderName, oAuth) { + cr := deployContext.CheCluster + userName := deploy.DefaultCheFlavor(cr) if err := deploy.DeleteSecret(htpasswdSecretName, ocConfigNamespace, iuh.runtimeClient); err != nil { return err } - if err := deploy.DeleteSecret(openShiftOAuthUserCredentialsSecret, crNamespace, iuh.runtimeClient); err != nil { + if err := deploy.DeleteSecret(openShiftOAuthUserCredentialsSecret, cr.Namespace, iuh.runtimeClient); err != nil { return err } diff --git a/pkg/controller/che/oauth_initial_htpasswd_provider_test.go b/pkg/controller/che/oauth_initial_htpasswd_provider_test.go index c39c09caa..d0bed9ca9 100644 --- a/pkg/controller/che/oauth_initial_htpasswd_provider_test.go +++ b/pkg/controller/che/oauth_initial_htpasswd_provider_test.go @@ -14,8 +14,12 @@ package che import ( "context" + "os" + "testing" + util_mocks "github.com/eclipse/che-operator/mocks/pkg/util" orgv1 "github.com/eclipse/che-operator/pkg/apis/org/v1" + "github.com/eclipse/che-operator/pkg/deploy" "github.com/golang/mock/gomock" oauth_config "github.com/openshift/api/config/v1" userv1 "github.com/openshift/api/user/v1" @@ -26,11 +30,9 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" - "os" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" - "testing" ) const ( @@ -38,6 +40,20 @@ const ( testUserName = "test" ) +var ( + testCR = &orgv1.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "eclipse-che", + Namespace: testNamespace, + }, + Spec: orgv1.CheClusterSpec{ + Server: orgv1.CheClusterSpecServer{ + CheFlavor: testUserName, + }, + }, + } +) + func TestCreateInitialUser(t *testing.T) { type testCase struct { name string @@ -72,7 +88,11 @@ func TestCreateInitialUser(t *testing.T) { runtimeClient: runtimeClient, runnable: m, } - err := initialUserHandler.CreateOAuthInitialUser(testUserName, testNamespace, oAuth) + dc := &deploy.DeployContext{ + CheCluster: testCR, + ClusterAPI: deploy.ClusterAPI{Client: runtimeClient, NonCachedClient: runtimeClient, DiscoveryClient: nil, Scheme: scheme}, + } + err := initialUserHandler.CreateOAuthInitialUser(oAuth, dc) if err != nil { t.Errorf("Failed to create user: %s", err.Error()) } @@ -152,7 +172,11 @@ func TestDeleteInitialUser(t *testing.T) { runtimeClient: runtimeClient, } - if err := initialUserHandler.DeleteOAuthInitialUser(testUserName, testNamespace); err != nil { + dc := &deploy.DeployContext{ + CheCluster: testCR, + ClusterAPI: deploy.ClusterAPI{Client: runtimeClient, NonCachedClient: runtimeClient, DiscoveryClient: nil, Scheme: scheme}, + } + if err := initialUserHandler.DeleteOAuthInitialUser(dc); err != nil { t.Errorf("Unable to delete initial user: %s", err.Error()) } diff --git a/pkg/deploy/secret.go b/pkg/deploy/secret.go index 402d39cea..842cfce6e 100644 --- a/pkg/deploy/secret.go +++ b/pkg/deploy/secret.go @@ -31,13 +31,14 @@ var secretDiffOpts = cmp.Options{ cmpopts.IgnoreFields(corev1.Secret{}, "TypeMeta", "ObjectMeta"), } -// SyncSecretToCluster applies secret into cluster -func SyncSecretToCluster( +// SyncSecret applies secret into cluster or external namespace +func SyncSecret( deployContext *DeployContext, name string, + namespace string, data map[string][]byte) (*corev1.Secret, error) { - specSecret, err := GetSpecSecret(deployContext, name, data) + specSecret, err := GetSpecSecret(deployContext, name, namespace, data) if err != nil { return nil, err } @@ -90,7 +91,7 @@ func GetClusterSecret(name string, namespace string, clusterAPI ClusterAPI) (*co } // GetSpecSecret return default secret config for given data -func GetSpecSecret(deployContext *DeployContext, name string, data map[string][]byte) (*corev1.Secret, error) { +func GetSpecSecret(deployContext *DeployContext, name string, namespace string, data map[string][]byte) (*corev1.Secret, error) { labels := GetLabels(deployContext.CheCluster, DefaultCheFlavor(deployContext.CheCluster)) secret := &corev1.Secret{ TypeMeta: metav1.TypeMeta{ @@ -99,15 +100,17 @@ func GetSpecSecret(deployContext *DeployContext, name string, data map[string][] }, ObjectMeta: metav1.ObjectMeta{ Name: name, - Namespace: deployContext.CheCluster.Namespace, + Namespace: namespace, Labels: labels, }, Data: data, } - err := controllerutil.SetControllerReference(deployContext.CheCluster, secret, deployContext.ClusterAPI.Scheme) - if err != nil { - return nil, err + if deployContext.CheCluster.Namespace == namespace { + err := controllerutil.SetControllerReference(deployContext.CheCluster, secret, deployContext.ClusterAPI.Scheme) + if err != nil { + return nil, err + } } return secret, nil @@ -125,7 +128,7 @@ func CreateTLSSecretFromEndpoint(deployContext *DeployContext, url string, name return err } - secret, err = SyncSecretToCluster(deployContext, name, map[string][]byte{"ca.crt": crtBytes}) + secret, err = SyncSecret(deployContext, name, deployContext.CheCluster.Namespace, map[string][]byte{"ca.crt": crtBytes}) if err != nil { return err } @@ -154,22 +157,3 @@ func DeleteSecret(secretName string, namespace string, runtimeClient client.Clie return nil } - -// CreateSecret - create secret by name and namespace -func CreateSecret(content map[string][]byte, secretName string, namespace string, runtimeClient client.Client) error { - secret := &corev1.Secret{ - TypeMeta: metav1.TypeMeta{ - Kind: "Secret", - APIVersion: "v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: namespace, - }, - Data: content, - } - if err := runtimeClient.Create(context.TODO(), secret); err != nil { - return err - } - return nil -}