Make OpenShift infrastructure create a special service account for workspaces #11199

sleshchenko · 2018-09-13T08:17:06Z

What does this PR do?

Improve OpenShift infrastructure to create a special service account for workspaces if there is no predefined project configured (it means that each workspace will be created in a new project which should be create by Che Server). Che Server makes sure that all needed objects (service account, roles, role bindings) exist each time before a workspace start. It is done in this way to provide backward compatibility with existing installation where some of workspaces' projects exist.

What issues does this PR fix or reference?

#10991

Release Notes

N/A

Docs PR

Will be provided further

sleshchenko · 2018-09-14T11:45:08Z

ci-test

riuvshin · 2018-09-14T12:15:02Z

ci-test build report:
Build details
Test report
docker image: eclipseche/che-server:11199
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-09-14T12:34:34Z

Tests failed because of error in che-plugin-java-debugger-server which is not changed into this PR. I rebased the PR with master branch and I'm going to restart ci tests.

sleshchenko · 2018-09-14T12:34:42Z

ci-test

riuvshin · 2018-09-14T13:07:57Z

ci-test build report:
Build details
Test report
docker image: eclipseche/che-server:11199
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-09-14T13:15:02Z

@riuvshin @tolusha @tsmaeder Guys, as I see all last ci-test executions failed because of build error in che-plugin-java-debugger-server. Could you please take a look?

tolusha · 2018-09-14T13:21:24Z

To be honest I have no idea.
Could you reproduce that locally?

16:04:49 [ERROR] DOCKER> Error occurred during container startup, shutting down...
16:04:49 [ERROR] DOCKER> I/O Error [Unable to start container id [fa3ea942f31a] : OCI runtime create failed: container_linux.go:348: starting container process caused "process_linux.go:301: running exec setns process for init caused \"exit status 40\"": unknown (Internal Server Error: 500)]

sleshchenko · 2018-09-14T13:22:16Z

@tolusha I saw it) Do you think it happens because of CI infrastructures issue?

tolusha · 2018-09-14T13:22:53Z

I think so.

riuvshin · 2018-09-14T13:52:26Z

looks like docker issue, try rerun

sleshchenko · 2018-09-14T14:02:26Z

ci-test

riuvshin · 2018-09-14T14:33:47Z

ci-test build report:
Build details
Test report
docker image: eclipseche/che-server:11199
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi

Overall looks good. Can you take a look at inlined comments?

...java/org/eclipse/che/workspace/infrastructure/openshift/project/WorkspaceServiceAccount.java

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

garagatyi · 2018-09-14T15:01:51Z

...java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java

   */
  public OpenShiftProject create(String workspaceId) throws InfrastructureException {
-    final String projectName = isNullOrEmpty(this.projectName) ? workspaceId : this.projectName;
+    boolean isPredefined = !isNullOrEmpty(this.projectName);


Looks like this line can be moved to the constructor

Yeap, it even can be moved to the constructor of parent class KubernetesNamespaceFactory.

So, really thank you for this comment. Because of that, I believe that I found an issue in KubernetesNamespaceFactory. isPredefined method returns true if the configured namespace isNullOrEmpty and false otherwise, but it should do vice versa.

It may be a cause of appearing a lot of error messages in ci-jobs tests about failed PVC clean up jobs.

I'll check it and will consider adding unit test for KubernetesNamespaceFactory.

Done.

Also, I added dummy test class for OpenShiftProjectFactory and will fix it before merge.

l0rd · 2018-09-14T16:16:01Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties

 # Defines Kubernetes Service Account name which should be specified to be bound to all workspaces pods.
-# Note that Che Server won't create the service account and it should exist.
+# Note that Kubernetes Infrastructure won't create the service account and it should exist.
+# OpenShift infrastructure will check if project is predefined:


when you say "if project is predefined" you mean "if che.infra.openshift.project is set" right?

Right, I'll add the corresponding comment here.

sleshchenko · 2018-09-17T06:12:47Z

ci-test

riuvshin · 2018-09-17T07:09:12Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11199
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-09-17T07:20:04Z

Tests failed because of error in Che Dockerfiles :: Theia module that is not modified in this PR. Going to restart tests

sleshchenko · 2018-09-17T07:20:07Z

ci-test

riuvshin · 2018-09-17T07:55:49Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11199
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

amisevsk

Looks good, just a few (not very important) quesitons.

amisevsk · 2018-09-17T13:38:02Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties


 # Defines Kubernetes Service Account name which should be specified to be bound to all workspaces pods.
-# Note that Che Server won't create the service account and it should exist.
+# Note that Kubernetes Infrastructure won't create the service account and it should exist.


This line is a little unclear to me in related to KubernetesServiceAccount.java -- we're creating a SA there if necessary right?

I believe this is the limitation of the current version of k8s client - rolebindings can only be created via OpenShiftClient, not KubernetesClient

This line is a little unclear to me in related to KubernetesServiceAccount.java -- we're creating a SA there if necessary right?

Actually, we don't create but we wait for default SA. We are able to create a new one but because of limitation that Illya mentioned we are not able to create rolebinding and role, so it will be implemented for K8s too after upgrading to facric8-client 4.0.4.

amisevsk · 2018-09-17T13:41:50Z

...java/org/eclipse/che/workspace/infrastructure/openshift/project/WorkspaceServiceAccount.java

+    }
+  }
+
+  private void createWorkspaceServiceAccount(OpenShiftClient osClient) {


Just to double check: does creating a SA / rolebindings require different privileges from what the standard Che SA might be given currently (esp. on public clusters)?

If workspaces are configured to be created in the same namespace then Che SA needs admin rights in the current namespace (for OpenShift, now we create SA with edit rights in its project).
If workspaces are configured to be created in different namespaces then Che SA needs cluster-admin right or OAuth provider should be used, then a user should be able to create Projects and objects there.

So, I would say that service-account should be used only if:

Workspaces are created in the same namespace and service account is created during deployment.

Oauth provider is configured.
Otherwise, extra privileges should be provided for Che SA. Hope it will be helpful.

amisevsk · 2018-09-17T13:43:54Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties

+# Note that Kubernetes Infrastructure won't create the service account and it should exist.
+# OpenShift infrastructure will check if project is predefined(if `che.infra.openshift.project` is not empty):
+#  - if it is predefined then service account must exist there
+#  - if it is 'NULL' or empty string then infrastructure will create new OpenShift project per workspace


This is for single-user only, right? I'm not clear on how this would work in the multi-user case.

This is for both. But out-of-the-box it will work only with OAuth provider configured, without it Che SA needs extra permissions for single or multi user mode.

garagatyi · 2018-09-18T08:05:24Z

@eivantsov FYI

sleshchenko · 2018-09-20T08:34:37Z

Tests failed again because of error in Che Dockerfiles :: Theia module that is not modified in this PR =(
Going to restart tests

sleshchenko · 2018-09-20T08:34:44Z

ci-test

…needed

riuvshin · 2018-09-20T12:03:59Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11199
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Sep 13, 2018

sleshchenko self-assigned this Sep 13, 2018

sleshchenko requested review from a user, garagatyi, ibuziuk and l0rd September 13, 2018 08:17

sleshchenko requested review from riuvshin and skabashnyuk as code owners September 13, 2018 08:17

sleshchenko mentioned this pull request Sep 13, 2018

Provide an access token for terminal and exec sidecar #10991

Closed

3 tasks

sleshchenko force-pushed the workspaceServiceAccountCreation branch from 9adc196 to 86aaa3b Compare September 14, 2018 12:32

garagatyi reviewed Sep 14, 2018

View reviewed changes

l0rd reviewed Sep 14, 2018

View reviewed changes

l0rd approved these changes Sep 14, 2018

View reviewed changes

garagatyi approved these changes Sep 17, 2018

View reviewed changes

amisevsk reviewed Sep 17, 2018

View reviewed changes

ibuziuk approved these changes Sep 18, 2018

View reviewed changes

skabashnyuk approved these changes Sep 19, 2018

View reviewed changes

Make OpenShift infrastructure to create workspace service account if …

fede1b2

…needed

sleshchenko force-pushed the workspaceServiceAccountCreation branch from 926a2bf to fede1b2 Compare September 20, 2018 08:36

sleshchenko merged commit 41b14c7 into eclipse-che:master Sep 21, 2018

sleshchenko deleted the workspaceServiceAccountCreation branch September 21, 2018 05:50

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 21, 2018

benoitf added this to the 6.12.0 milestone Sep 21, 2018

This was referenced Sep 25, 2018

Make Kubernetes infrastructure create a special service account for workspaces #11349

Closed

Fixed exec, terminal sidecars #11432

Merged

Make OpenShift infrastructure create a special service account for workspaces #11199

Make OpenShift infrastructure create a special service account for workspaces #11199

Uh oh!

Conversation

sleshchenko commented Sep 13, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Uh oh!

sleshchenko commented Sep 14, 2018

Uh oh!

riuvshin commented Sep 14, 2018

Uh oh!

sleshchenko commented Sep 14, 2018

Uh oh!

sleshchenko commented Sep 14, 2018

Uh oh!

riuvshin commented Sep 14, 2018

Uh oh!

sleshchenko commented Sep 14, 2018

Uh oh!

tolusha commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sleshchenko commented Sep 14, 2018

Uh oh!

tolusha commented Sep 14, 2018

Uh oh!

riuvshin commented Sep 14, 2018

Uh oh!

sleshchenko commented Sep 14, 2018

Uh oh!

riuvshin commented Sep 14, 2018

Uh oh!

garagatyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleshchenko commented Sep 17, 2018

Uh oh!

riuvshin commented Sep 17, 2018

Uh oh!

sleshchenko commented Sep 17, 2018

Uh oh!

sleshchenko commented Sep 17, 2018

Uh oh!

riuvshin commented Sep 17, 2018

Uh oh!

amisevsk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuziuk Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolusha commented Sep 14, 2018 •

edited

Loading

ibuziuk Sep 18, 2018 •

edited

Loading