Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow to upgrade che 7.31 or later to upgrade installation that configured with namespace strategies other than "per user" #818

Merged
merged 5 commits into from
May 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions deploy/crds/org_v1_che_crd-v1beta1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -437,10 +437,11 @@ spec:
Eclipse Che in a restricted environment.
type: string
allowUserDefinedWorkspaceNamespaces:
description: Defines that a user is allowed to specify a Kubernetes
namespace, or an OpenShift project, which differs from the default.
It's NOT RECOMMENDED to set to `true` without OpenShift OAuth
configured. The OpenShift infrastructure also uses this property.
description: Deprecated. The value of this flag is ignored. Defines
that a user is allowed to specify a Kubernetes namespace, or an
OpenShift project, which differs from the default. It's NOT RECOMMENDED
to set to `true` without OpenShift OAuth configured. The OpenShift
infrastructure also uses this property.
type: boolean
cheClusterRoles:
description: A comma-separated list of ClusterRoles that will be
Expand Down
9 changes: 5 additions & 4 deletions deploy/crds/org_v1_che_crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -447,10 +447,11 @@ spec:
install Eclipse Che in a restricted environment.
type: string
allowUserDefinedWorkspaceNamespaces:
description: Defines that a user is allowed to specify a Kubernetes
namespace, or an OpenShift project, which differs from the default.
It's NOT RECOMMENDED to set to `true` without OpenShift OAuth
configured. The OpenShift infrastructure also uses this property.
description: Deprecated. The value of this flag is ignored. Defines
that a user is allowed to specify a Kubernetes namespace, or
an OpenShift project, which differs from the default. It's NOT
RECOMMENDED to set to `true` without OpenShift OAuth configured.
The OpenShift infrastructure also uses this property.
type: boolean
cheClusterRoles:
description: A comma-separated list of ClusterRoles that will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ metadata:
categories: Developer Tools
certified: "false"
containerImage: quay.io/eclipse/che-operator:nightly
createdAt: "2021-05-11T18:38:31Z"
createdAt: "2021-05-12T08:55:32Z"
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/che-operator
support: Eclipse Foundation
name: eclipse-che-preview-kubernetes.v7.30.0-176.nightly
name: eclipse-che-preview-kubernetes.v7.31.0-178.nightly
namespace: placeholder
spec:
apiservicedefinitions: {}
Expand Down Expand Up @@ -1132,4 +1132,4 @@ spec:
maturity: stable
provider:
name: Eclipse Foundation
version: 7.30.0-176.nightly
version: 7.31.0-178.nightly
Original file line number Diff line number Diff line change
Expand Up @@ -447,10 +447,11 @@ spec:
install Eclipse Che in a restricted environment.
type: string
allowUserDefinedWorkspaceNamespaces:
description: Defines that a user is allowed to specify a Kubernetes
namespace, or an OpenShift project, which differs from the default.
It's NOT RECOMMENDED to set to `true` without OpenShift OAuth
configured. The OpenShift infrastructure also uses this property.
description: Deprecated. The value of this flag is ignored. Defines
that a user is allowed to specify a Kubernetes namespace, or
an OpenShift project, which differs from the default. It's NOT
RECOMMENDED to set to `true` without OpenShift OAuth configured.
The OpenShift infrastructure also uses this property.
type: boolean
cheClusterRoles:
description: A comma-separated list of ClusterRoles that will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ metadata:
categories: Developer Tools, OpenShift Optional
certified: "false"
containerImage: quay.io/eclipse/che-operator:nightly
createdAt: "2021-05-11T18:38:45Z"
createdAt: "2021-05-12T08:55:42Z"
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/che-operator
support: Eclipse Foundation
name: eclipse-che-preview-openshift.v7.30.0-176.nightly
name: eclipse-che-preview-openshift.v7.31.0-178.nightly
namespace: placeholder
spec:
apiservicedefinitions: {}
Expand Down Expand Up @@ -1207,4 +1207,4 @@ spec:
maturity: stable
provider:
name: Eclipse Foundation
version: 7.30.0-176.nightly
version: 7.31.0-178.nightly
Original file line number Diff line number Diff line change
Expand Up @@ -452,11 +452,11 @@ spec:
useful to install Eclipse Che in a restricted environment.
type: string
allowUserDefinedWorkspaceNamespaces:
description: Defines that a user is allowed to specify a Kubernetes
namespace, or an OpenShift project, which differs from the
default. It's NOT RECOMMENDED to set to `true` without OpenShift
OAuth configured. The OpenShift infrastructure also uses this
property.
description: Deprecated. The value of this flag is ignored.
Defines that a user is allowed to specify a Kubernetes namespace,
or an OpenShift project, which differs from the default. It's
NOT RECOMMENDED to set to `true` without OpenShift OAuth configured.
The OpenShift infrastructure also uses this property.
type: boolean
cheClusterRoles:
description: A comma-separated list of ClusterRoles that will
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/org/v1/che_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ type CheClusterSpecServer struct {
// In that case, a new namespace will be created for each user or workspace.
// +optional
WorkspaceNamespaceDefault string `json:"workspaceNamespaceDefault,omitempty"`
// Deprecated. The value of this flag is ignored.
// Defines that a user is allowed to specify a Kubernetes namespace, or an OpenShift project, which differs from the default.
// It's NOT RECOMMENDED to set to `true` without OpenShift OAuth configured. The OpenShift infrastructure also uses this property.
// +optional
Expand Down
24 changes: 9 additions & 15 deletions pkg/controller/che/che_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,14 +361,16 @@ func (r *ReconcileChe) Reconcile(request reconcile.Request) (reconcile.Result, e
}

// Check Che CR correctness
if err := ValidateCheCR(instance, isOpenShift); err != nil {
// Che cannot be deployed with current configuration.
// Print error message in logs and wait until the configuration is changed.
logrus.Error(err)
if err := r.SetStatusDetails(instance, request, failedValidationReason, err.Error(), ""); err != nil {
return reconcile.Result{}, err
if !util.IsTestMode() {
if err := ValidateCheCR(instance); err != nil {
// Che cannot be deployed with current configuration.
// Print error message in logs and wait until the configuration is changed.
logrus.Error(err)
if err := r.SetStatusDetails(instance, request, failedValidationReason, err.Error(), ""); err != nil {
return reconcile.Result{}, err
}
return reconcile.Result{}, nil
}
return reconcile.Result{}, nil
}

if !util.IsTestMode() {
Expand Down Expand Up @@ -576,14 +578,6 @@ func (r *ReconcileChe) Reconcile(request reconcile.Request) (reconcile.Result, e
return reconcile.Result{RequeueAfter: time.Second}, err
}

done, err = r.checkWorkspacePermissions(deployContext)
if !done {
if err != nil {
logrus.Error(err)
}
return reconcile.Result{}, err
}

done, err = r.reconcileWorkspacePermissions(deployContext)
if !done {
if err != nil {
Expand Down
105 changes: 0 additions & 105 deletions pkg/controller/che/che_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -901,9 +901,6 @@ func TestCheController(t *testing.T) {
if err := cl.Get(context.TODO(), types.NamespacedName{Name: "che", Namespace: cheCR.Namespace}, cm); err != nil {
t.Errorf("ConfigMap %s not found: %s", cm.Name, err)
}
if cm.Data["CHE_INFRA_OPENSHIFT_PROJECT"] != "" {
t.Errorf("ConfigMap wasn't updated properly. Extecting empty string, got: '%s'", cm.Data["CHE_INFRA_OPENSHIFT_PROJECT"])
}

_, isOpenshiftv4, err := util.DetectOpenShift()
if err != nil {
Expand Down Expand Up @@ -1101,30 +1098,6 @@ func TestShouldDelegatePermissionsForCheWorkspaces(t *testing.T) {
crWsInAnotherNs3.Spec.Server.WorkspaceNamespaceDefault = "some-test-namespace"

testCases := []testCase{
{
name: "che-operator should delegate permission for workspaces in the same namespace with Che. WorkspaceNamespaceDefault=" + crWsInTheSameNs1.Namespace,
initObjects: []runtime.Object{},
clusterRole: false,
checluster: crWsInTheSameNs1,
},
{
name: "che-operator should delegate permission for workspaces in the same namespace with Che. WorkspaceNamespaceDefault=''",
initObjects: []runtime.Object{},
clusterRole: false,
checluster: crWsInTheSameNs2,
},
{
name: "che-operator should delegate permission for workspaces in the same namespace with Che. Property CHE_INFRA_KUBERNETES_NAMESPACE_DEFAULT=''",
initObjects: []runtime.Object{},
clusterRole: false,
checluster: crWsInTheSameNs3,
},
{
name: "che-operator should delegate permission for workspaces in the same namespace with Che. Property CHE_INFRA_KUBERNETES_NAMESPACE_DEFAULT=" + crWsInTheSameNs1.Namespace,
initObjects: []runtime.Object{},
clusterRole: false,
checluster: crWsInTheSameNs4,
},
{
name: "che-operator should delegate permission for workspaces in differ namespace than Che. WorkspaceNamespaceDefault = 'some-test-namespace'",
initObjects: []runtime.Object{},
Expand All @@ -1137,12 +1110,6 @@ func TestShouldDelegatePermissionsForCheWorkspaces(t *testing.T) {
clusterRole: true,
checluster: crWsInAnotherNs2,
},
{
name: "che-operator should delegate permission for workspaces in differ namespace than Che. Property CHE_INFRA_KUBERNETES_NAMESPACE_DEFAULT points to Che namespace with higher priority WorkspaceNamespaceDefault = 'some-test-namespace'.",
initObjects: []runtime.Object{},
clusterRole: false,
checluster: crWsInAnotherNs3,
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
Expand Down Expand Up @@ -1250,78 +1217,6 @@ func TestShouldDelegatePermissionsForCheWorkspaces(t *testing.T) {
}
}

func TestShouldFallBackWorspaceNamespaceDefaultBecauseNotEnoughtPermissions(t *testing.T) {
// the same namespace with Che
cr := InitCheWithSimpleCR().DeepCopy()
cr.Spec.Server.WorkspaceNamespaceDefault = "che-workspace-<username>"

logf.SetLogger(zap.LoggerTo(os.Stdout, true))

scheme := scheme.Scheme
orgv1.SchemeBuilder.AddToScheme(scheme)
scheme.AddKnownTypes(oauth.SchemeGroupVersion, oAuthClient)
scheme.AddKnownTypes(userv1.SchemeGroupVersion, &userv1.UserList{}, &userv1.User{})
scheme.AddKnownTypes(oauth_config.SchemeGroupVersion, &oauth_config.OAuth{})
scheme.AddKnownTypes(routev1.GroupVersion, route)

cr.Spec.Auth.OpenShiftoAuth = util.NewBoolPointer(false)

cli := fake.NewFakeClientWithScheme(scheme, cr)
nonCachedClient := fake.NewFakeClientWithScheme(scheme, cr)
clientSet := fakeclientset.NewSimpleClientset()
// todo do we need fake discovery
fakeDiscovery, ok := clientSet.Discovery().(*fakeDiscovery.FakeDiscovery)
fakeDiscovery.Fake.Resources = []*metav1.APIResourceList{}

if !ok {
t.Fatal("Error creating fake discovery client")
}

var m *mocks.MockPermissionChecker
ctrl := gomock.NewController(t)
m = mocks.NewMockPermissionChecker(ctrl)
m.EXPECT().GetNotPermittedPolicyRules(gomock.Any(), "").Return([]rbac.PolicyRule{
{
APIGroups: []string{""},
Resources: []string{"namespaces"},
Verbs: []string{"get", "create", "update"},
},
}, nil).MaxTimes(2)
defer ctrl.Finish()

r := &ReconcileChe{
client: cli,
nonCachedClient: nonCachedClient,
discoveryClient: fakeDiscovery,
scheme: scheme,
permissionChecker: m,
tests: true,
}
req := reconcile.Request{
NamespacedName: types.NamespacedName{
Name: name,
Namespace: namespace,
},
}

_, err := r.Reconcile(req)
if err != nil {
t.Fatalf("Error reconciling: %v", err)
}
_, err = r.Reconcile(req)
if err != nil {
t.Fatalf("Error reconciling: %v", err)
}

cheCluster := &orgv1.CheCluster{}
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, cheCluster); err != nil {
t.Errorf("Unable to get checluster")
}
if cheCluster.Spec.Server.WorkspaceNamespaceDefault != namespace {
t.Error("Failed fallback workspaceNamespaceDefault to execute workspaces in the same namespace with Che")
}
}

func Init() (client.Client, discovery.DiscoveryInterface, runtime.Scheme) {
objs, ds, scheme := createAPIObjects()

Expand Down
11 changes: 9 additions & 2 deletions pkg/controller/che/che_cr_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,28 @@ package che

import (
"fmt"
"strings"

orgv1 "github.com/eclipse-che/che-operator/pkg/apis/org/v1"
"github.com/eclipse-che/che-operator/pkg/util"
)

// ValidateCheCR checks Che CR configuration.
// It should detect:
// - configurations which miss required field(s) to deploy Che
// - self-contradictory configurations
// - configurations with which it is impossible to deploy Che
func ValidateCheCR(checluster *orgv1.CheCluster, isOpenshift bool) error {
if !isOpenshift {
func ValidateCheCR(checluster *orgv1.CheCluster) error {
if !util.IsOpenShift {
if checluster.Spec.K8s.IngressDomain == "" {
return fmt.Errorf("Required field \"spec.K8s.IngressDomain\" is not set")
}
}

workspaceNamespaceDefault := util.GetWorkspaceNamespaceDefault(checluster)
if strings.Index(workspaceNamespaceDefault, "<username>") == -1 && strings.Index(workspaceNamespaceDefault, "<userid>") == -1 {
return fmt.Errorf(`Namespace strategies other than 'per user' is not supported anymore. Using the <username> or <userid> placeholder is required in the 'spec.server.workspaceNamespaceDefault' field. The current value is: %s`, workspaceNamespaceDefault)
}

return nil
}
Loading