Skip to content
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

KIALI-2689 Support OAuth logout in OpenShift 4 #1015

Merged
merged 1 commit into from May 3, 2019

Conversation

@mwringe
Copy link
Contributor

commented Apr 17, 2019

** Describe the change **

Adds in additional logout information needed when running on OpenShift 4. On OpenShift 4 we need to go to a logout page and include a specific redirect url to be used. On OpenShift 3 this not needed.

It will also delete the OpenShift token in the k8s backend so that it is marked as being invalid by OpenShift.

** Issue reference **

https://issues.jboss.org/browse/KIALI-2689

** Backwards incompatible? **

Yes, it will continue to work the same with OpenShift 3 and OpenShift 4. It will not affect what is happening when running in Kubernetes.

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Apr 17, 2019

Jenkins CI: kiali-core-pr-e2e-test #929

  • ✔️ run-kiali-e2e-tests #[1736]
@mwringe mwringe requested a review from mattmahoneyrh Apr 18, 2019
Copy link
Contributor

left a comment

Verified. Logout works fine.

@mwringe mwringe force-pushed the mwringe:KIALI-2689 branch from f3e72d4 to 126a953 May 2, 2019
@mwringe mwringe requested review from cfcosta and jshaughn May 2, 2019
@mwringe

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

We had the Kiali-UI PR for this merged (kiali/kiali-ui#1122) but the corresponding PR for backend didn't get merged.

QE has already verified the PR works (thanks @mattmahoneyrh ) We just need someone else to take a look.

I have an instance running if someone wants to take a look

Copy link
Contributor

left a comment

A couple of non-blocking comments, fixes optional.

redirectURL, err := getKialiRoutePath()
metadata = &OAuthMetadata{}

if version.Major == "1" && (strings.HasPrefix(version.Minor, "11") || strings.HasPrefix(version.Minor, "10")) {

This comment has been minimized.

Copy link
@jshaughn

jshaughn May 3, 2019

Contributor

I don't love this hardcoded version stuff with no comment about why it's special-cased. In the future I suggest a small function named like isMySpecialCase(version) or some such.

if claims, err := config.GetTokenClaimsIfValid(tokenString); err != nil {
log.Warningf("Token is invalid: %s", err.Error())
return err
} else {

This comment has been minimized.

Copy link
@jshaughn

jshaughn May 3, 2019

Contributor

unnecessary else

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

Jenkins CI: kiali-core-pr-e2e-test #1019
🔴 PRT Failed, Please Contact QE

@hhovsepy

This comment has been minimized.

Copy link

commented May 3, 2019

kialiqeci

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

Jenkins CI: kiali-core-pr-e2e-test #1021

  • ✔️ run-kiali-e2e-tests #[1840]
@mwringe mwringe merged commit db9dd09 into kiali:master May 3, 2019
3 checks passed
3 checks passed
Jenkins-CI Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - tests/e2e/requirements.txt (theute) No manifest changes detected
@kiali-workflow kiali-workflow bot added this to the v0.20.0 milestone May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.