feat: Delegate only needed roles to user in his namespace #355

tolusha · 2022-09-14T14:01:35Z

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

What does this PR do?

feat: Delegate only needed roles to user in his namespace
related che-operator PR: eclipse-che/che-operator#1532

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#21695

How to test this PR?

Deploy Eclipse Che
chectl server:deploy --platform=openshift --cheimage docker.io/abazko/che-server:21695 --che-operator-image=docker.io/abazko/operator:21695
Log into Eclipse Che
Check rolebindings in a user namespace, it must contain only 2 rolebindings for the current user and it mustn't contain admin ClusterRoleBinding

oc get rolebindings -n <USER_NAMESPACE> -o custom-columns=NAME:metadata.name,ROLE:roleRef.name,SUBJECT:subjects\[0\].name  | grep <USERNAME>
NAME                                                 ROLE                                                 SUBJECT
eclipse-che-cheworkspaces-clusterrole                eclipse-che-cheworkspaces-clusterrole                <USERNAME>
eclipse-che-cheworkspaces-devworkspace-clusterrole   eclipse-che-cheworkspaces-devworkspace-clusterrole   <USERNAME>
...

Create and start a workspace

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

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

vinokurig · 2022-09-15T07:21:02Z

...t/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenShiftInfraModule.java

-import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.configurator.SshKeysConfigurator;
-import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.configurator.UserPreferencesConfigurator;
-import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.configurator.UserProfileConfigurator;
+import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.configurator.*;


AFAIK we avoid wildcard imports.

ibuziuk · 2022-09-15T08:52:09Z

.../java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java

-      namespace = create(name, client);
+      create(name, client);
    }
-    label(namespace, labels);
-    annotate(namespace, annotations);
+    label(client.namespaces().withName(name).get(), labels);
+    annotate(client.namespaces().withName(name).get(), annotations);


is it just refactoring? I'm not sure which benefits it provides e.g. client.namespaces().withName(name).get() is called twice. I like the original version more

To label and to annotation, we need a new fresh namespace object.
Otherwise adding annotation removes previously added labels.

sorry, for not following
I looked at the label method and it seems to merge the existing and new labels - https://github.com/eclipse-che/che-server/pull/355/files#diff-2e663cdd99b84ca2ecf99c9d2df6edc6f1036866f87a175a2a7d8ae05f9aad7bR196

Could you clarify how this change is relevant to the PR?

ibuziuk · 2022-09-15T08:58:03Z

...t/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenShiftInfraModule.java

@@ -107,6 +101,7 @@ protected void configure() {

    Multibinder<NamespaceConfigurator> namespaceConfigurators =
        Multibinder.newSetBinder(binder(), NamespaceConfigurator.class);
+    namespaceConfigurators.addBinding().to(UserPermissionConfigurator.class);


wondering why this configurator was not applied before? from what I see it falls back on che.infra.kubernetes.user_cluster_roles property. In the context of https://issues.redhat.com/browse/CRW-3328 it is not clear for me how this would help

To be honest I don't know as well.

so, do we really need this change? e.g. why would you like to bind this configurator as part of this PR?

ibuziuk · 2022-09-15T09:14:50Z

...c/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java

-        OpenShiftClient openshiftClient = cheServerOpenshiftClientFactory.createOC();
-        create(projectName, openshiftClient);
-        waitDefaultServiceAccount(projectName, openshiftClient);
-        openshiftClient
-            .roleBindings()
-            .inNamespace(projectName)
-            .createOrReplace(
-                new RoleBindingBuilder()
-                    .withNewMetadata()
-                    .withName("admin")
-                    .endMetadata()
-                    .addToUserNames(osClient.currentUser().getMetadata().getName())
-                    .withNewRoleRef()
-                    .withApiVersion("rbac.authorization.k8s.io")
-                    .withKind("RoleBinding")
-                    .withName("admin")
-                    .endRoleRef()
-                    .build());
-      } else {


This looks like the main change relevant to the project creation and role provisioning, but I'm not sure if this what we would like to achieve. e.g. by design user gets admin permissions in the namespace and this should be just fine.

That's legacy code.
Basically I make OpenShift Infrastructure project creation codebase similar to Kuberntes infrastructure.

ibuziuk

@tolusha my understanding was that we would restrict CR creation via webhook - eclipse-che/che-operator#1517
This change looks risky for me, and changes the default behaviour where admin rights are provisioned to user by default

l0rd · 2022-09-16T08:39:57Z

restricting the privileges of the developer to just what's needed to run a dev environment is good in general: admins are happy, developers are not affected.
webhook are a more robust solution to the root problem we want to solve: no one, not even an admin, can create multiple CheClusters by mistake
in the future we may get the request to support multiple CheClusters in the same cluster (i.e. one Che operator but multiple Che instances in one cluster) but that's not something we need to worry right now.

So I think both approach brings value although using a webhook is probably more effective and more urgent now so +1 to prioritize that.

l0rd · 2022-09-16T08:43:14Z

@ibuziuk why do you think that the changes in this PR are risky? Why do we require that the developer is an admin in his namespace? (this is a question I already got from customers)

ibuziuk · 2022-09-16T08:56:15Z

why do you think that the changes in this PR are risky?

Because I do not know why admin was provided in the first place when namespaces are provisioned using the SA and what impact it could have on the user flow. Would be nice to get the review from @skabashnyuk @metlos @sparkoo

skabashnyuk

I do not know why admin was provided in the first place when namespaces are provisioned using the SA and what impact it could have on the user flow.

I don't remember either That is quite an old piece of code.
In general, pr looks good to me.

amisevsk · 2022-09-19T15:56:55Z

I believe the reason admin was chosen for the binding was to reduce the differences between Kubernetes and OpenShift. On OpenShift, if a user uses oc new-project the namespace is created with a rolebinding to the admin clusterrole for that user. Using something different for Kubernetes would potentially introduce bugs (though it may be unlikely). The previous issue here was eclipse-che/che#21171 (which should likely be closed seeing as we're having the issue of users having too many permissions 🙂)

ibuziuk

LGTM, verified against crc, after the change user can not remove the *-che project provisioned during the signup, the only suggestion would be to remove the wildcard imports.

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

feat: Delegate only needed roles to user in his namespace

4994821

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

tolusha requested review from skabashnyuk, amisevsk, metlos and nickboldt as code owners September 14, 2022 14:01

tolusha requested review from ibuziuk and vinokurig September 14, 2022 14:05

vinokurig reviewed Sep 15, 2022

View reviewed changes

ibuziuk reviewed Sep 15, 2022

View reviewed changes

ibuziuk requested changes Sep 15, 2022

View reviewed changes

skabashnyuk approved these changes Sep 19, 2022

View reviewed changes

tolusha mentioned this pull request Oct 4, 2022

chore: Add missed user roles eclipse-che/che-operator#1532

Merged

11 tasks

ibuziuk approved these changes Oct 5, 2022

View reviewed changes

tolusha mentioned this pull request Oct 5, 2022

chore: Add missed user roles eclipse-che/che-operator#1533

Closed

11 tasks

Fix import

dfe5b37

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

tolusha merged commit d4e29df into main Oct 6, 2022

tolusha deleted the CRW-3328 branch October 6, 2022 08:54

che-bot added this to the 7.55 milestone Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Delegate only needed roles to user in his namespace #355

feat: Delegate only needed roles to user in his namespace #355

tolusha commented Sep 14, 2022 •

edited

Loading

vinokurig Sep 15, 2022

ibuziuk Sep 15, 2022

tolusha Sep 16, 2022

ibuziuk Oct 5, 2022

ibuziuk Sep 15, 2022

tolusha Sep 16, 2022

ibuziuk Oct 5, 2022

ibuziuk Sep 15, 2022

tolusha Sep 19, 2022

ibuziuk left a comment •

edited

Loading

l0rd commented Sep 16, 2022

l0rd commented Sep 16, 2022

ibuziuk commented Sep 16, 2022

skabashnyuk left a comment

amisevsk commented Sep 19, 2022

ibuziuk left a comment

feat: Delegate only needed roles to user in his namespace #355

feat: Delegate only needed roles to user in his namespace #355

Conversation

tolusha commented Sep 14, 2022 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk left a comment • edited Loading

Choose a reason for hiding this comment

l0rd commented Sep 16, 2022

l0rd commented Sep 16, 2022

ibuziuk commented Sep 16, 2022

skabashnyuk left a comment

Choose a reason for hiding this comment

amisevsk commented Sep 19, 2022

ibuziuk left a comment

Choose a reason for hiding this comment

tolusha commented Sep 14, 2022 •

edited

Loading

ibuziuk left a comment •

edited

Loading