Skip to content

Use edit role for Che SA#11177

Merged
1 commit merged intomasterfrom
che_sa_edit_role
Sep 13, 2018
Merged

Use edit role for Che SA#11177
1 commit merged intomasterfrom
che_sa_edit_role

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 12, 2018

What does this PR do?

It looks like Che service account may have edit role to be able to create workspaces in the same namespace with Che server pod

What issues does this PR fix or reference?

#11142

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 12, 2018

ci-test

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Sep 12, 2018
Copy link
Copy Markdown
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Please change PR title to make clear that it affects OpenShift deployment only

name: che
roleRef:
name: admin
name: edit
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it makes describe in the documentation that CHE_INFRA_OPENSHIFT_PROJECT must be set to:

  1. the same with Che Server namespace
  2. empty value then oauth provider must be used
  3. any value different from Che Server namespace then cluster admin roles should be bound to Che service account (not sure that we need this way of configuration)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently Che service account does not have cluster wide privileges so it is a general documentation improvement not really related to this particular PR. But good point.

metadata:
name: che
roleRef:
name: admin
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we use the same role to create namespaces for workspaces?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, a cluster admin should grant Che SA cluster-admin privileges:

oc adm policy add-cluster-role-to-user self-provisioner system:serviceaccount:eclipse-che:che

For k8s we do that out of the box: https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/cluster-role-binding.yaml#L16

And this makes it possible to set this env to empty string by default and things work out of the box.

So, to sum it up - on OpenShift we NEVER granted Che SA cluster-admin role while it was true for Kubernetes Helm charts. Hence, if we do the same change for k8s, then, https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/values.yaml#L47 should be namespace where Che SA has been created. And if a cluster admin wants Che to create workspaces in individual namespaces, Che SA is granted a cluster-admin role and https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/values.yaml#L47 becomes an empty string.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we don't have right now an OS deployment option where Che creates namespaces?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garagatyi this option should be supported when Che is configured to use users tokens. Giving cluster-admin privileges to Che is something that we should not support: you can do it at your own risk :-)

@riuvshin
Copy link
Copy Markdown
Contributor

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 12, 2018

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

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

@ghost ghost merged commit cb17ad3 into master Sep 13, 2018
@ghost ghost deleted the che_sa_edit_role branch September 13, 2018 07:15
@benoitf 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 18, 2018
@benoitf benoitf added this to the 6.12.0 milestone Sep 18, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement A feature request - must adhere to the feature request template.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants