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

Fix the logout button to work with the recent version of oidc-authservice #6609

Merged
merged 1 commit into from Feb 6, 2023

Conversation

alembiewski
Copy link
Member

Required as a part of kubeflow/manifests#2150

Fixes the logout button in the Central Dashboard to work with the recent versions of the oidc-authservice (tested with gcr.io/arrikto/kubeflow/oidc-authservice:e236439 image version)

Changes:

  • Pin alpine repository version
  • Introduce a new LogoutButton component

@kimwnasptd
Copy link
Member

@alembiewski as discussed in kubeflow/manifests#2150 (comment) we'll need to slightly update this PR.

Specifically, the new AuthService will expect the logout URL to be /authservice/logout. But since we can't hardcode this value, since not everyone might be using AuthService, we'll need to have a dynamic mechanism for learning this URL.

I'd propose to

  1. have a new ENV Var LOGOUT_URL in the Deployment of the app
  2. The backend will add this for the /api/workgroup/env-info route handler https://github.com/kubeflow/kubeflow/blob/master/components/centraldashboard/app/api_workgroup.ts
  3. The frontend will dynamically use this value from the backend

First off we'll need to rebase this work on top of the latest branch. Would you have the cycles for this or should we assign this task to someone else?

* Pin alpine repository version

* Introduce a new LogoutButton component
@alembiewski
Copy link
Member Author

@kimwnasptd I rebased the PR on top of the latest branch.

Specifically, the new AuthService will expect the logout URL to be /authservice/logout

Yes, good catch - the way we resolved that is by adding a new rule to a VirualService, e.g.:

  - name: logout
    match:
    - uri:
        prefix: /logout
    rewrite:
      uri: /authservice/logout

What you suggesting is definitely a better approach to handle this.

Unfortunately, I don't have enough capacity to work on the dynamic mechanism for learning the logout URL, so I would propose we merge the existing PR and ask someone else to address #6940 separately. WDYT?

@kimwnasptd
Copy link
Member

Thanks for confirming @alembiewski!

Yes, we'll merge this PR and handle the fixup in a follow up. @orfeas-k could you take a stab at it?

@alembiewski thank you very much for your time, and apologies for the late review on this that resulted in a bad timing.

@kimwnasptd
Copy link
Member

The PR changes look good as well, so going forward with merging this. We'll also need to cherry-pick this to the release branch cc @DomFleischmann

/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 74f020e into kubeflow:master Feb 6, 2023
kimwnasptd pushed a commit to arrikto/kubeflow that referenced this pull request Feb 15, 2023
…rvice` (kubeflow#6609)

* Pin alpine repository version

* Introduce a new LogoutButton component
kimwnasptd pushed a commit to arrikto/kubeflow that referenced this pull request Feb 15, 2023
…rvice` (kubeflow#6609)

* Pin alpine repository version

* Introduce a new LogoutButton component
@skuchipu
Copy link

@alembiewski @kimwnasptd This change is redirecting to a blank page , should it not redirect to login page like earlier versions ?

@divyank000
Copy link

the change of LOGOUT_URL to /authservice/logout leads to an invalid_page.

Not a good User Experience, thought it doesn't break the functionality.

@juliusvonkohout
Copy link
Member

Create a new issue including screenshots and detailed explanation if necessary.

@rahu984
Copy link

rahu984 commented Sep 4, 2023

@kimwnasptd I rebased the PR on top of the latest branch.

Specifically, the new AuthService will expect the logout URL to be /authservice/logout

Yes, good catch - the way we resolved that is by adding a new rule to a VirualService, e.g.:

  - name: logout
    match:
    - uri:
        prefix: /logout
    rewrite:
      uri: /authservice/logout

What you suggesting is definitely a better approach to handle this.

Unfortunately, I don't have enough capacity to work on the dynamic mechanism for learning the logout URL, so I would propose we merge the existing PR and ask someone else to address #6940 separately. WDYT?

But where do we implement this virtual service.

@ajinkya933
Copy link

@kimwnasptd I rebased the PR on top of the latest branch.

Specifically, the new AuthService will expect the logout URL to be /authservice/logout

Yes, good catch - the way we resolved that is by adding a new rule to a VirualService, e.g.:

  - name: logout
    match:
    - uri:
        prefix: /logout
    rewrite:
      uri: /authservice/logout

What you suggesting is definitely a better approach to handle this.
Unfortunately, I don't have enough capacity to work on the dynamic mechanism for learning the logout URL, so I would propose we merge the existing PR and ask someone else to address #6940 separately. WDYT?

But where do we implement this virtual service.

can you help me out @kimwnasptd @alembiewski Im also stuck on this, where is this virtual service, which needs to be changed ?

@juliusvonkohout
Copy link
Member

@ajinkya933 You can join our Kubeflow manifest wg meeting or book consulting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants