Handle code review feedback.

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
pull/597/head
Oleksandr Andriienko 2021-02-11 19:05:50 +02:00
parent 9986117efb
commit b7f9c2e89b
12 changed files with 100 additions and 61 deletions

View File

@ -181,12 +181,12 @@ spec:
initialOpenShiftOAuthUser:
description: For operating with the OpenShift OAuth authentication,
create a new user account since the kubeadmin can not be used.
- If the value is true, then a new OpenShift OAuth user will be
created for the HTPasswd identity provider. - If the value is
false and the user has already been created, then it will be removed.
- If value is an empty, then do nothing. The user's credentials
are stored in the openshift-oauth-user-credentials secret by Operator.
Note that this solution is Openshift 4 platform-specific.
If the value is true, then a new OpenShift OAuth user will be
created for the HTPasswd identity provider. If the value is false
and the user has already been created, then it will be removed.
If value is an empty, then do nothing. The user's credentials
are stored in the `openshift-oauth-user-credentials` secret by
Operator. Note that this solution is Openshift 4 platform-specific.
type: boolean
oAuthClientName:
description: Name of the OpenShift `OAuthClient` resource used to
@ -782,9 +782,9 @@ spec:
description: Current installed Che version.
type: string
dbProvisioned:
description: Indicates that a Postgres instance has been correctly provisioned
or not. Indicates that a PosgreSQL instance has been correctly provisioned
or not.
description: Indicates that a PostgreSQL instance has been correctly
provisioned or not. Indicates that a PosgreSQL instance has been correctly
provisioned or not.
type: boolean
devfileRegistryURL:
description: Public URL to the devfile registry.

View File

@ -85,13 +85,13 @@ metadata:
categories: Developer Tools
certified: "false"
containerImage: quay.io/eclipse/che-operator:nightly
createdAt: "2021-02-10T18:00:39Z"
createdAt: "2021-02-11T17:05:13Z"
description: A Kube-native development solution that delivers portable and collaborative
developer workspaces.
operatorframework.io/suggested-namespace: eclipse-che
repository: https://github.com/eclipse/che-operator
support: Eclipse Foundation
name: eclipse-che-preview-kubernetes.v7.26.0-92.nightly
name: eclipse-che-preview-kubernetes.v7.26.0-94.nightly
namespace: placeholder
spec:
apiservicedefinitions: {}
@ -685,4 +685,4 @@ spec:
maturity: stable
provider:
name: Eclipse Foundation
version: 7.26.0-92.nightly
version: 7.26.0-94.nightly

View File

@ -181,12 +181,12 @@ spec:
initialOpenShiftOAuthUser:
description: For operating with the OpenShift OAuth authentication,
create a new user account since the kubeadmin can not be used.
- If the value is true, then a new OpenShift OAuth user will be
created for the HTPasswd identity provider. - If the value is
false and the user has already been created, then it will be removed.
- If value is an empty, then do nothing. The user's credentials
are stored in the openshift-oauth-user-credentials secret by Operator.
Note that this solution is Openshift 4 platform-specific.
If the value is true, then a new OpenShift OAuth user will be
created for the HTPasswd identity provider. If the value is false
and the user has already been created, then it will be removed.
If value is an empty, then do nothing. The user's credentials
are stored in the `openshift-oauth-user-credentials` secret by
Operator. Note that this solution is Openshift 4 platform-specific.
type: boolean
oAuthClientName:
description: Name of the OpenShift `OAuthClient` resource used to
@ -782,9 +782,9 @@ spec:
description: Current installed Che version.
type: string
dbProvisioned:
description: Indicates that a Postgres instance has been correctly provisioned
or not. Indicates that a PosgreSQL instance has been correctly provisioned
or not.
description: Indicates that a PostgreSQL instance has been correctly
provisioned or not. Indicates that a PosgreSQL instance has been correctly
provisioned or not.
type: boolean
devfileRegistryURL:
description: Public URL to the devfile registry.

View File

@ -76,13 +76,13 @@ metadata:
categories: Developer Tools, OpenShift Optional
certified: "false"
containerImage: quay.io/eclipse/che-operator:nightly
createdAt: "2021-02-10T18:00:50Z"
createdAt: "2021-02-11T17:05:23Z"
description: A Kube-native development solution that delivers portable and collaborative
developer workspaces in OpenShift.
operatorframework.io/suggested-namespace: eclipse-che
repository: https://github.com/eclipse/che-operator
support: Eclipse Foundation
name: eclipse-che-preview-openshift.v7.26.0-92.nightly
name: eclipse-che-preview-openshift.v7.26.0-94.nightly
namespace: placeholder
spec:
apiservicedefinitions: {}
@ -757,4 +757,4 @@ spec:
maturity: stable
provider:
name: Eclipse Foundation
version: 7.26.0-92.nightly
version: 7.26.0-94.nightly

View File

@ -182,12 +182,12 @@ spec:
initialOpenShiftOAuthUser:
description: For operating with the OpenShift OAuth authentication,
create a new user account since the kubeadmin can not be used.
- If the value is true, then a new OpenShift OAuth user will be
created for the HTPasswd identity provider. - If the value is
false and the user has already been created, then it will be removed.
- If value is an empty, then do nothing. The user's credentials
are stored in the openshift-oauth-user-credentials secret by Operator.
Note that this solution is Openshift 4 platform-specific.
If the value is true, then a new OpenShift OAuth user will be
created for the HTPasswd identity provider. If the value is false
and the user has already been created, then it will be removed.
If value is an empty, then do nothing. The user's credentials
are stored in the `openshift-oauth-user-credentials` secret by
Operator. Note that this solution is Openshift 4 platform-specific.
type: boolean
oAuthClientName:
description: Name of the OpenShift `OAuthClient` resource used to
@ -784,9 +784,9 @@ spec:
description: Current installed Che version.
type: string
dbProvisioned:
description: Indicates that a Postgres instance has been correctly provisioned
or not. Indicates that a PosgreSQL instance has been correctly provisioned
or not.
description: Indicates that a PostgreSQL instance has been correctly
provisioned or not. Indicates that a PosgreSQL instance has been correctly
provisioned or not.
type: boolean
devfileRegistryURL:
description: Public URL to the devfile registry.

View File

@ -34,18 +34,19 @@ func (m *MockOpenShiftOAuthUserHandler) EXPECT() *MockOpenShiftOAuthUserHandlerM
return m.recorder
}
// CreateOAuthInitialUser mocks base method
func (m *MockOpenShiftOAuthUserHandler) CreateOAuthInitialUser(openshiftOAuth *v1.OAuth, deployContext *deploy.DeployContext) error {
// SyncOAuthInitialUser mocks base method
func (m *MockOpenShiftOAuthUserHandler) SyncOAuthInitialUser(openshiftOAuth *v1.OAuth, deployContext *deploy.DeployContext) (bool, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "CreateOAuthInitialUser", openshiftOAuth, deployContext)
ret0, _ := ret[0].(error)
return ret0
ret := m.ctrl.Call(m, "SyncOAuthInitialUser", openshiftOAuth, deployContext)
ret0, _ := ret[0].(bool)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// CreateOAuthInitialUser indicates an expected call of CreateOAuthInitialUser
func (mr *MockOpenShiftOAuthUserHandlerMockRecorder) CreateOAuthInitialUser(openshiftOAuth, deployContext interface{}) *gomock.Call {
// SyncOAuthInitialUser indicates an expected call of SyncOAuthInitialUser
func (mr *MockOpenShiftOAuthUserHandlerMockRecorder) SyncOAuthInitialUser(openshiftOAuth, deployContext interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOAuthInitialUser", reflect.TypeOf((*MockOpenShiftOAuthUserHandler)(nil).CreateOAuthInitialUser), openshiftOAuth, deployContext)
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SyncOAuthInitialUser", reflect.TypeOf((*MockOpenShiftOAuthUserHandler)(nil).SyncOAuthInitialUser), openshiftOAuth, deployContext)
}
// DeleteOAuthInitialUser mocks base method

View File

@ -383,17 +383,31 @@ func (r *ReconcileChe) Reconcile(request reconcile.Request) (reconcile.Result, e
}
}
if isOpenShift4 && util.IsDeleteOAuthInitialUser(instance) {
if isOpenShift4 && instance.Spec.Auth.InitialOpenShiftOAuthUser != nil && util.IsDeleteOAuthInitialUser(instance) {
if err := r.userHandler.DeleteOAuthInitialUser(deployContext); err != nil {
logrus.Errorf("Unable to delete initial OpenShift OAuth user from a cluster. Cause: %s", err.Error())
instance.Spec.Auth.InitialOpenShiftOAuthUser = nil
err := r.UpdateCheCRSpec(instance, "InitialOpenShiftOAuthUser", "nil")
return reconcile.Result{RequeueAfter: time.Second * 1}, err
return reconcile.Result{}, err
}
instance.Spec.Auth.OpenShiftoAuth = nil
if err := r.UpdateCheCRSpec(instance, "OpenShiftoAuth", "nil"); err != nil {
return reconcile.Result{}, err
}
instance.Spec.Auth.InitialOpenShiftOAuthUser = nil
err := r.UpdateCheCRSpec(instance, "InitialOpenShiftOAuthUser", "nil")
if err != nil {
return reconcile.Result{}, err
}
instance.Status.OpenShiftOAuthUserCredentialsSecret = ""
if err := r.UpdateCheCRStatus(instance, "openShiftOAuthUserCredentialsSecret", openShiftOAuthUserCredentialsSecret); err != nil {
return reconcile.Result{}, err
}
return reconcile.Result{}, nil
}
if isOpenShift && instance.Spec.Auth.OpenShiftoAuth == nil {
@ -1134,7 +1148,8 @@ 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(openshitOAuth, deployContext); err != nil {
provisioned, err := r.userHandler.SyncOAuthInitialUser(openshitOAuth, deployContext);
if 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)
@ -1154,6 +1169,9 @@ func (r *ReconcileChe) autoEnableOAuth(deployContext *deploy.DeployContext, requ
}
}
}
if !provisioned {
return reconcile.Result{}, err
}
}
}
} else { // Openshift 3

View File

@ -306,7 +306,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(gomock.Any(), gomock.Any())
m.EXPECT().SyncOAuthInitialUser(gomock.Any(), gomock.Any()).Return(true, nil)
return m
},
OpenShiftOAuthUserCredentialsSecret: openShiftOAuthUserCredentialsSecret,

View File

@ -36,9 +36,14 @@ const (
openShiftOAuthUserCredentialsSecret = "openshift-oauth-user-credentials"
)
var (
password = util.GeneratePasswd(6)
htpasswdFileContent string
)
// OpenShiftOAuthUserHandler - handler to create or delete new Openshift oAuth user.
type OpenShiftOAuthUserHandler interface {
CreateOAuthInitialUser(openshiftOAuth *oauthv1.OAuth, deployContext *deploy.DeployContext) error
SyncOAuthInitialUser(openshiftOAuth *oauthv1.OAuth, deployContext *deploy.DeployContext) (bool, error)
DeleteOAuthInitialUser(deployContext *deploy.DeployContext) error
}
@ -58,35 +63,46 @@ func NewOpenShiftOAuthUserHandler(runtimeClient client.Client) OpenShiftOAuthUse
}
}
// CreateOAuthInitialUser - creates new htpasswd provider with inital user with Che flavor name
// SyncOAuthInitialUser - creates new htpasswd provider with inital user with Che flavor name
// if Openshift cluster hasn't got identity providers, otherwise do nothing.
// 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(openshiftOAuth *oauthv1.OAuth, deployContext *deploy.DeployContext) error {
password := util.GeneratePasswd(6)
func (iuh *OpenShiftOAuthUserOperatorHandler) SyncOAuthInitialUser(openshiftOAuth *oauthv1.OAuth, deployContext *deploy.DeployContext) (bool, error) {
cr := deployContext.CheCluster
userName := deploy.DefaultCheFlavor(cr)
htpasswdFileContent, err := iuh.generateHtPasswdUserInfo(userName, password)
var err error
if htpasswdFileContent == "" {
htpasswdFileContent, err = iuh.generateHtPasswdUserInfo(userName, password)
if err != nil {
return false, err
}
}
initialUserSecretData := map[string][]byte{"user": []byte(userName), "password": []byte(password)}
credentionalSecret, err := deploy.SyncSecret(deployContext, openShiftOAuthUserCredentialsSecret, cr.Namespace, initialUserSecretData)
if err != nil {
return err
return false, err
}
storedPassword := string(credentionalSecret.Data["password"])
if password != storedPassword {
password = storedPassword
// todo remove this log info
logrus.Info("Detected stored password", storedPassword)
}
htpasswdFileSecretData := map[string][]byte{"htpasswd": []byte(htpasswdFileContent)}
if _, err := deploy.SyncSecret(deployContext, htpasswdSecretName, ocConfigNamespace, htpasswdFileSecretData); err != nil {
return err
return false, err
}
if err := appendIdentityProvider(openshiftOAuth, iuh.runtimeClient); err != nil {
return err
return false, err
}
initialUserSecretData := map[string][]byte{"user": []byte(userName), "password": []byte(password)}
if _, err := deploy.SyncSecret(deployContext, openShiftOAuthUserCredentialsSecret, cr.Namespace, initialUserSecretData); err != nil {
return err
}
return nil
return true, nil
}
// DeleteOAuthInitialUser - removes initial user, htpasswd provider, htpasswd secret and Che secret with username and password.

View File

@ -92,10 +92,13 @@ func TestCreateInitialUser(t *testing.T) {
CheCluster: testCR,
ClusterAPI: deploy.ClusterAPI{Client: runtimeClient, NonCachedClient: runtimeClient, DiscoveryClient: nil, Scheme: scheme},
}
err := initialUserHandler.CreateOAuthInitialUser(oAuth, dc)
provisined, err := initialUserHandler.SyncOAuthInitialUser(oAuth, dc)
if err != nil {
t.Errorf("Failed to create user: %s", err.Error())
}
if !provisined {
t.Error("Unexpected error")
}
// Check created objects
expectedCheSecret := &corev1.Secret{}

View File

@ -283,6 +283,7 @@ func TestMountSecret(t *testing.T) {
},
ClusterAPI: ClusterAPI{
Client: cli,
NonCachedClient: cli,
Scheme: scheme.Scheme,
},
}

View File

@ -80,7 +80,7 @@ func GetSecret(name string, namespace string, clusterAPI ClusterAPI) (*corev1.Se
Namespace: namespace,
Name: name,
}
err := clusterAPI.Client.Get(context.TODO(), namespacedName, secret)
err := clusterAPI.NonCachedClient.Get(context.TODO(), namespacedName, secret)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil