-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Apply OpenShift OAuth provider #15963
Conversation
Can one of the admins verify this patch? |
...he-core-api-auth-openshift/src/main/java/org/eclipse/che/security/oauth/OpenShiftModule.java
Outdated
Show resolved
Hide resolved
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
E2E tests of Eclipse Che Multiuser on OCP has been successful:
|
assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
Outdated
Show resolved
Hide resolved
CC @tolusha |
...he-core-api-auth-openshift/src/main/java/org/eclipse/che/security/oauth/OpenShiftModule.java
Outdated
Show resolved
Hide resolved
...auth-openshift/src/main/java/org/eclipse/che/security/oauth/OpenShiftOAuthAuthenticator.java
Outdated
Show resolved
Hide resolved
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
@skabashnyuk |
E2E tests of Eclipse Che Multiuser on OCP has failed:
|
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
Outdated
Show resolved
Hide resolved
E2E tests of Eclipse Che Multiuser on OCP has failed: Use command [ci-test] to rerun the test. |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
Not sure I followed the whole story about this issue, but does this mean that now, by default, in order to have Openshift integration, users will have to manually create an additional OAuth client in Openshift in addition to the one already automatically created by the Che operator when Openshift OAuth is enabled (to support Keycloak-Openshift identity provider) ? If it's the case I'm a bit worried about the fact that it might be a problem in some situations, such as deployment on OSD for example, where it would expected that any required change is done by the operator itself afaik, and no manual step is expected, apart the creation of a In such a case I would suggest that, on the operator side, when OpenShift OAuth is enabled, the same OpenShift OAuth Client would be used to configure the Che server (of this PR) as the one already used for the Keycloak OpenShift identity provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you answer the question I asked here: #15963 (comment) ?
Thanks !
@davidfestal |
GitHub is a bit different since, afaik, you cannot automate the creation of the GitHub OAuth client easily, and the GitHub OAuth client is somehow something expected to be owned by the Che admin user in a self-service mode. OTOH setting up an OpenShift OAuth client requires cluster admin rights into the cluster, and the installer user of Che/CRW on OpenShift doesn't necessarily own the cluster on which it installs Che (think of OSD addon for example). So this should be done by the Che operator, and is already done to support Keycloak Openshift identity provider setup.
Afaik the operator-based installation is the official installation mode for OpenShift, either through But it seems that requiring a manual setup of an additional OAuth client inside the OpenShift cluster before being able to use the OpenShift integration tools (OpenShift plugin ?) will be a no-go in a number of contexts we aim to support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing with @vinokurig I consider that the user experience doesn't get worse with this PR. If we merge this PR Che will still use OpenShift OAuth as identity provider even if the administrator doesn't create the extra OpenShift OAuth client. But the administrator would have a new (manual) option to enable the OpenShift OAuth provider for the workspaces.
For this reason I consider it as a good first step and the PR can be merged as it is (and even cherry picked to 7.9.x if we have time cc @nickboldt). But as mentioned by @davidfestal there are some important improvements that need to be introduced (update the operator to automatically create the client for instance). Such improvements can be done in a second PR.
@l0rd @davidfestal Created sub-issue: #16199 |
crw-ci-test |
[ci-test] |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
E2E tests of Eclipse Che Multiuser on OCP has been successful: |
crw-ci-test |
Thanks @l0rd for providing more context.
Thanks @vinokurig Let me approve this PR then. |
crw-ci-test |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
crw-ci-test |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
crw-ci-test |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
crw-ci-test |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
What does this PR do?
Add OpenShift OAuth provider to be able to retrieve openshift token.
What issues does this PR fix or reference?
fixes #15670
Release Notes
Docs PR