-
Notifications
You must be signed in to change notification settings - Fork 57
rh-che #541: Login to user project using oc CLI in workspace containers #555
Conversation
openshift/rh-che.config.yaml
Outdated
@@ -20,6 +20,7 @@ data: | |||
workspaces-memory-limit: "2400" | |||
workspaces-memory-limit-max: "2400mb" | |||
enable-workspaces-autostart: "false" | |||
keycloak-oso-endpoint: "https://sso.prod-preview.openshift.io/auth/realms/fabric8/broker/openshift-v3/token" |
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.
Can't you just reuse che-keycloak-auth-server-url
instead of having a new config map?
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.
2 points here:
- the
che-keycloak-auth-server-url
will disappear with the switch tofabric8_auth
- It seems that
fabric8_auth
doesn't provide an endpoint to get the OSO token, as it does for GitHub
So the current way implemented in this PR works for now, but we will probably have to ask the Auth team about the best way to get the OSO token for such a need (which from their POV should not exist anymore because of the OSO proxy)
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.
@l0rd I can replace it, but I used the approach OSIO-che team implemented previously. I thought you evaluated it as a better approach. Do you this it is better to change the approach?
@davidfestal there is an endpoint for that I believe. Here are swagger of it http://swagger.goa.design/?url=github.com%2Ffabric8-services%2Ffabric8-auth%2Fdesign#!/token/token_Retrieve
The idea is to call something like GET https://auth.prod-preview.openshift.io/api/token?for=https://api.starter-us-east-2.openshift.com
where https://api.starter-us-east-2.openshift.com
should be retrieved from cluster field of UserCheTenantData.
One thing to mention is that Aslak said to me that we should not call this API. I don't know the reason but will write an email to figure it out.
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.
You can’t use sso.openshift.io to get OSO tokens anymore.
It’s depricated. It doesn’t support multi cluster and it doesn’t have tokens for users provisioned since January.
You have to use auth.openshift.io to get the tokens.
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.
To be clear. We still support login via sso (but working on moving it to Auth) but token management has already moved to auth completely.
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.
@davidfestal @l0rd @alexeykazakov I reworked PR to get OSO token from Auth service.
3756a01
to
656b6e4
Compare
I reworked PR to get OSO token from Auth service. Please, review my changes. |
Can one of the admins verify this patch? |
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
7258d00
to
ef159a5
Compare
…ects registry Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
I updated PR to fetch the token from Auth service. I checked it - it works. |
@garagatyi Jus send PR to openshiftio-cico-jobs repo adding your gh username to admin list: https://github.com/openshiftio/openshiftio-cico-jobs/blob/master/devtools-ci-index.yaml#L12 |
@rhopp thank you, Radim! |
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.
LGTM
try { | ||
return httpJsonRequestFactory |
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.
Why don't you use an httpJsonRequestFactory
finally ?
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.
Because response contains custom content type header which is not supported by this class
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.
LGTM
What does this PR do?
Injects environment variable to log in to a project on OSO where a user has edit rights.
What issues does this PR fix or reference?
Fixes #541
How have you tested this PR?