Add a role allowing che SA to stop workspaces #16532

tomgeorge · 2020-04-02T15:47:46Z

What does this PR do?

Adds another stop-workspace role in a user namespace that gives the che serviceaccount to correctly stop workspaces.

What issues does this PR fix or reference?

#15906

Release Notes

Docs PR

che-bot · 2020-04-02T15:47:47Z

Can one of the admins verify this patch?

che-bot · 2020-04-02T15:47:47Z

Can one of the admins verify this patch?

che-bot · 2020-04-02T16:05:55Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

metlos

If I'm not mistaken, #15906 suggests to also add creation privs for the SA - see #15906 (comment).

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java

metlos · 2020-04-02T16:24:19Z

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java

+            new ObjectReferenceBuilder()
+                .withKind("ServiceAccount")
+                .withName("che")
+                .withNamespace("che")


Namespace is configurable. Use the projectName field.

projectName and serviceAccountName refer to the user namespace and the user serviceaccount. The che ServiceAccount in the che namespace need to be able to manage resources in a user namespace.

yeah, here we need to provide a namespace where che-server is deployed. Not sure if the SA name is configurable though cc: @davidfestal

it looks like we need to create a @Singleton that would be bound in the OpenShiftInfraModule / KubernetesInfraModule and expose information about the namespace / sa.

My observations are that che SA name is hardcoded and not configurable and namespace name can be obtained via System.getenv (Need to check if this will work in all the cases e.g.:

the operator uses KUBERNETES_NAMESPACE env var

helm POD_NAMESPACE

tomgeorge · 2020-04-02T17:16:28Z

If I'm not mistaken, #15906 suggests to also add creation privs for the SA - see #15906 (comment).

The user who is creating the workspace has enough permissions to create the workspace resources, and the che SA stops the workspace in the case of idling. It doesn't seem like the che SA strictly needs create privileges in a user service account, and it's a good idea to assign the minimal level of access.

tomgeorge · 2020-04-02T17:22:30Z

in regards to not hard-coding the service account names/namespaces, is there a way to reference the che service account and che namespace? projectName and serviceAccountName refer to the user user namespace/SA I believe.

ibuziuk · 2020-04-07T10:52:44Z

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java

@@ -142,6 +151,37 @@ private void createViewRole(OpenShiftClient osClient, String name) {
    osClient.roles().inNamespace(projectName).create(viewRole);
  }

+  private void createStopWorkspacesRole(OpenShiftClient osClient, String name) {


I would probably move the logic of granting the rights on the level above to the OpenShiftProjectFactory / getOrCreate + extract the logic of the role / rb to a separate class smth like OpenShiftStopWorkspaceRoleProvisioner with docs and tests

che-bot · 2020-04-09T20:14:15Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java

ibuziuk · 2020-04-10T07:05:37Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+import javax.inject.Singleton;
+
+@Singleton
+public class OpenShiftCheInstallationLocation {


nice, IMO in future we need to standardize the env var name and always use KUBERNETES_NAMESPACE that is defined in the operator

Adding docs would be also really nice to have

ibuziuk · 2020-04-10T07:13:03Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+    String kubernetesNamespace = System.getenv("KUBERNETES_NAMESPACE");
+    String podNamespace = System.getenv("POD_NAMESPACE");


Probably we can move the logic of obtaining the env vars to @PostConstruct to avoid reading env vars on every call

can you inject them as env. KUBERNETES_NAMESPACE in the constructor?

is this env vars are set by k8s or che-operator?

the problem is that helm & operator are using different env vars (POD_NAMESPACE - helm / KUBERNETES_NAMESPACE - operator)

Is it possible to change the helm chart to also call this KUBERNETES_NAMESPACE or does helm define this dynamically?

I'm also voting for this to be somehow injected in a constructor or at least having a no-arg ctor determining the value from the env vars and another ctor initializing the namespace from a provided argument.

Also we should probably throw an exception if we're not able to determine the namespace. There's little point in continuing to deploy che if it won't be able to correctly handle workspaces..

+1 for Helm update - https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/deployment.yaml#L38
-1 for throwing the exception (there might be some cases when the namespace can not be obtained for any reason)

for this particular issue let's just do nothing if the namepsace can not be obtained - https://github.com/eclipse/che/pull/16532/files#r406635675

Moved to @PostConstruct

why not injection in the constructor?

ibuziuk · 2020-04-10T07:16:05Z

.../che/workspace/infrastructure/openshift/provision/OpenShiftStopWorkspaceRoleProvisioner.java

+    this.installationLocation = installationLocation;
+  }
+
+  public void provision(String projectName) throws InfrastructureException {


technically speaking installationLocation can be null based on implementation, so in this case can we log error and avoid doing anything in the provision method?

@tomgeorge can we check getInstallationLocationNamespace here and do nothing (log error) if null?

.../che/workspace/infrastructure/openshift/provision/OpenShiftStopWorkspaceRoleProvisioner.java

skabashnyuk · 2020-04-10T18:54:00Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+    String kubernetesNamespace = System.getenv("KUBERNETES_NAMESPACE");
+    String podNamespace = System.getenv("POD_NAMESPACE");


can you inject them as env. KUBERNETES_NAMESPACE in the constructor?

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

skabashnyuk · 2020-04-10T18:54:46Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+    String kubernetesNamespace = System.getenv("KUBERNETES_NAMESPACE");
+    String podNamespace = System.getenv("POD_NAMESPACE");


is this env vars are set by k8s or che-operator?

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java

.../che/workspace/infrastructure/openshift/provision/OpenShiftStopWorkspaceRoleProvisioner.java

ibuziuk

LGTM

che-bot · 2020-04-16T17:15:06Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

ibuziuk · 2020-04-17T13:24:48Z

@metlos @skabashnyuk @sleshchenko folks, are you ok to merge this PR in the current state or some changes are still expected to be done before merging? Please, let us know since we do expect the fix in 7.12.0

che-bot · 2020-04-17T14:39:13Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

skabashnyuk · 2020-04-17T15:28:22Z

Take a look plz https://github.com/eclipse/che/pull/16532/files#r410016501

dmytro-ndp · 2020-04-20T07:06:01Z

crw-ci-test

ibuziuk · 2020-04-20T16:09:27Z

thanks, @tomgeorge (left minor comments in review) + looks like there is a conflict now after 06a34e3 :/

…t to stop a workspace in another namespace Signed-off-by: Tom George <tgeorge@redhat.com>

…nd OpenShiftStopWorkspaceRoleProvisioner to provision necessary permissions for stopping workspaces Signed-off-by: Tom George <tgeorge@redhat.com>

amisevsk

Generally LGTM, a few comments.

amisevsk · 2020-04-20T16:17:23Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+ * @author Tom George
+ */
+@Singleton
+public class OpenShiftCheInstallationLocation {


While this PR is to address an issue with OpenShift, it's a little strange to me that this class is in the openshift infra rather than k8s. Not a huge issue though.

good point, the problem is that we do need this PR in 7.12.0 and I'm not sure if @tomgeorge has time for further refactoring

amisevsk · 2020-04-20T16:18:57Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+      LOG.info(
+          "Neither KUBERNETES_NAMESPACE nor POD_NAMESPACE is defined.  Unable to determine Che installation location");
+    }
+    return kubernetesNamespace == null ? podNamespace : kubernetesNamespace;


Maybe a warn if both podNamespace and kubernetesNamespace are defined but not matching would also make sense? I don't know if this can occur naturally but might be a hard-to-debug issue.

in general, we do need to standardize the env var name (use only KUBERNETES_NAMESPACE) and remove usage of POD_NAMESPACE e.g. in helm chart https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/deployment.yaml#L38

But we should do it in a separate PRs I beleive after 7.12.0

amisevsk · 2020-04-20T16:21:57Z

.../che/workspace/infrastructure/openshift/provision/OpenShiftStopWorkspaceRoleProvisioner.java

+  public OpenShiftStopWorkspaceRoleProvisioner(
+      OpenShiftClientFactory clientFactory, OpenShiftCheInstallationLocation installationLocation) {
+    this.clientFactory = clientFactory;
+    this.installationLocation = installationLocation;


I think it makes more sense to inject OpenShiftCheInstallationLocation but store OpenShiftCheInstallationLocation.getInstallationLocationNamespace() as a String in the class. It shouldn't change while Che is running, right?

…ronment variables into OpenShiftCheInstallationLocation Signed-off-by: Tom George <tgeorge@redhat.com>

tomgeorge · 2020-04-20T16:48:32Z

The conflict should be resolved, and made the change to OpenShiftCheInstallationLocation that @amisevsk suggested.

che-bot · 2020-04-20T16:52:41Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

tomgeorge · 2020-04-20T16:57:10Z

crw-ci-test

che-bot · 2020-04-20T17:04:22Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

tomgeorge · 2020-04-20T17:40:04Z

crw-ci-test

che-bot · 2020-04-20T18:03:05Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

tomgeorge · 2020-04-20T20:50:43Z

ci-build

skabashnyuk · 2020-04-21T05:59:03Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+ *
+ * @author Tom George
+ */
+@Singleton


Can we have test for this class?

skabashnyuk · 2020-04-21T05:59:50Z

...pse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java

+  private static final Logger LOG = LoggerFactory.getLogger(OpenShiftCheInstallationLocation.class);
+
+  @Inject(optional = true)
+  @Named("env.KUBERNETES_NAMESPACE")


I suggest using constructor injection. It would be much easier to test.

And javax.inject instead of google inject.

@skabashnyuk I believe we need to refactor this class based on the valid comments in the review e.g. move to different location + remove completely the usage of env.POD_NAMESPACE. wondering if this will be ok do in a separate PR though, since we really need to have a fix in 7.12.0

@skabashnyuk unfortunately Guice does not support constructor injection with optional dependencies[1]. We need to have these dependencies be optional for now because each installation method uses a different environment variable to specify the installation location. Perhaps when we refactor the installation methods to use the same environment variable, we can revisit this?

https://github.com/google/guice/wiki/FrequentlyAskedQuestions#how-can-i-inject-optional-parameters-into-a-constructor

what about import org.eclipse.che.commons.annotation.Nullable?

I assume it's not working this way

@Inject public OpenShiftCheInstallationLocation( @Named("env.KUBERNETES_NAMESPACE") String kubernetesNamespace, @Named("env.POD_NAMESPACE") String podNamespace) { this.kubernetesNamespace = kubernetesNamespace; this.podNamespace = podNamespace; }

but should work

@Inject public OpenShiftCheInstallationLocation( @Nullable @Named("env.KUBERNETES_NAMESPACE") String kubernetesNamespace, @Nullable @Named("env.POD_NAMESPACE") String podNamespace) { this.kubernetesNamespace = kubernetesNamespace; this.podNamespace = podNamespace; }

No?

https://github.com/eclipse/che/blob/master/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java#L56-L61

No, neither one worked. They both complained about not being able to find a binding for env.POD_NAMESPACE when it was not defined in the container, even though it was defined as @Nullable

I think those work because they are defined in che.properties, either null or with a value. POD_NAMESPACE and KUBERNETES_NAMESPACE are not defined in those files, and @Nullable @Named("env.POD_NAMESPACE") String podNamespace fails to create the injector because there is no implementation bound to it.

you right. Probably the first variant without injection was good too.

ibuziuk · 2020-04-21T07:33:47Z

ci-build

Please take a look on comments

…ty in order to have a possibility to disable the 'OpenShiftStopWorkspaceRoleProvisioner' Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2020-04-21T10:49:01Z

@tomgeorge it occurred to me that we will need to disable the provisioner for Hosted-Che (multicluster) environment. Please, consider applying this PR to your brarch which adds che.workspace.stop.role.enabled property - https://github.com/tomgeorge/che/pull/1/files

che eclipse-che#15906 Adding 'che.workspace.stop.role.enabled' property in order to have a possibility to disable the 'OpenShiftStopWorkspaceRoleProvisioner'

che-bot · 2020-04-21T12:28:31Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

ibuziuk · 2020-04-21T12:33:00Z

crw-ci-test

che-bot · 2020-04-21T12:53:08Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:16532

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

ibuziuk · 2020-04-21T13:25:38Z

[ci-build]

ibuziuk · 2020-04-21T13:58:10Z

[ci-build]

tomgeorge requested review from amisevsk, l0rd, metlos, nickboldt, rhopp and sleshchenko as code owners April 2, 2020 15:47

che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Apr 2, 2020

metlos reviewed Apr 2, 2020

View reviewed changes

ibuziuk reviewed Apr 7, 2020

View reviewed changes

ibuziuk reviewed Apr 10, 2020

View reviewed changes

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java Outdated Show resolved Hide resolved

ibuziuk reviewed Apr 10, 2020

View reviewed changes

...eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java Outdated Show resolved Hide resolved

ibuziuk reviewed Apr 10, 2020

View reviewed changes

.../che/workspace/infrastructure/openshift/provision/OpenShiftStopWorkspaceRoleProvisioner.java Show resolved Hide resolved

skabashnyuk reviewed Apr 10, 2020

View reviewed changes

metlos mentioned this pull request Apr 14, 2020

'rolebindings.authorization.openshift.io "exec" not found' error when start workspace with OS credentials on minishift #16534

Closed

23 tasks

ibuziuk approved these changes Apr 16, 2020

View reviewed changes

Tom George added 2 commits April 20, 2020 11:25

Add a workspace-stop role and rolebinding to allow che service accoun…

60e17b3

…t to stop a workspace in another namespace Signed-off-by: Tom George <tgeorge@redhat.com>

Add OpenShiftCheInstallationLocation to get ns of Che installation, a…

3fa9757

…nd OpenShiftStopWorkspaceRoleProvisioner to provision necessary permissions for stopping workspaces Signed-off-by: Tom George <tgeorge@redhat.com>

amisevsk approved these changes Apr 20, 2020

View reviewed changes

Do nothing if installation location cannot be determined, inject envi…

650f205

…ronment variables into OpenShiftCheInstallationLocation Signed-off-by: Tom George <tgeorge@redhat.com>

tomgeorge force-pushed the che-15906 branch from 2a489d3 to 650f205 Compare April 20, 2020 16:47

skabashnyuk previously approved these changes Apr 21, 2020

View reviewed changes

skabashnyuk self-requested a review April 21, 2020 08:28

che eclipse-che#15906 Adding 'che.workspace.stop.role.enabled' proper…

32e2cc9

…ty in order to have a possibility to disable the 'OpenShiftStopWorkspaceRoleProvisioner' Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

Merge pull request #1 from ibuziuk/che-15906

e187145

che eclipse-che#15906 Adding 'che.workspace.stop.role.enabled' property in order to have a possibility to disable the 'OpenShiftStopWorkspaceRoleProvisioner'

nickboldt mentioned this pull request Apr 21, 2020

Che 7.12.0 release #16690

Closed

16 tasks

tomgeorge merged commit 2241525 into eclipse-che:master Apr 21, 2020

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 21, 2020

This was referenced Apr 22, 2020

[OpenShift OAuth] Idling doesn't have enough permissions to stop the workspace. #15906

Closed

Cannot start workspace #16724

Closed

		String kubernetesNamespace = System.getenv("KUBERNETES_NAMESPACE");
		String podNamespace = System.getenv("POD_NAMESPACE");

Add a role allowing che SA to stop workspaces #16532

Add a role allowing che SA to stop workspaces #16532

Conversation

tomgeorge commented Apr 2, 2020

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

che-bot commented Apr 2, 2020

che-bot commented Apr 2, 2020

che-bot commented Apr 2, 2020

metlos left a 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

tomgeorge commented Apr 2, 2020

tomgeorge commented Apr 2, 2020

Choose a reason for hiding this comment

che-bot commented Apr 9, 2020

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

metlos Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

ibuziuk Apr 14, 2020 • edited Loading

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

Choose a reason for hiding this comment

che-bot commented Apr 16, 2020

ibuziuk commented Apr 17, 2020

che-bot commented Apr 17, 2020

skabashnyuk commented Apr 17, 2020

dmytro-ndp commented Apr 20, 2020

ibuziuk commented Apr 20, 2020

amisevsk left a 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

tomgeorge commented Apr 20, 2020

che-bot commented Apr 20, 2020

tomgeorge commented Apr 20, 2020

che-bot commented Apr 20, 2020

tomgeorge commented Apr 20, 2020

che-bot commented Apr 20, 2020

tomgeorge commented Apr 20, 2020

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

tomgeorge Apr 21, 2020 • edited Loading

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 commented Apr 21, 2020

ibuziuk commented Apr 21, 2020

che-bot commented Apr 21, 2020

ibuziuk commented Apr 21, 2020

che-bot commented Apr 21, 2020

ibuziuk commented Apr 21, 2020

ibuziuk commented Apr 21, 2020

metlos Apr 14, 2020 •

edited

Loading

ibuziuk Apr 14, 2020 •

edited

Loading

tomgeorge Apr 21, 2020 •

edited

Loading