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

feat: add doc to describe use oauth2proxy directly. #2884

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

cybernagle
Copy link
Contributor

Pull Request Template for Kubeflow manifests Issues

✏️ A brief description of the changes

I add doc to descirbe how to use oauth2 proxy directly to do authentication. as I was mentioned in slack channel:
https://cloud-native.slack.com/archives/C073N7BMLB1/p1726067178084329?thread_ts=1723629094.049759&cid=C073N7BMLB1

📦 List any dependencies that are required for this change

My PR depends on #

🐛 If this PR is related to an issue, please put the link to the issue here.

The following issues are related, because ...

✅ Contributor checklist


You can join the CNCF Slack and access our meetings at the Kubeflow Community website. Our channel on the CNCF Slack is here #kubeflow-platform.

@thesuperzapper
Copy link
Member

I think it's better if we standardize on the auth proposed in #2864.

Nothing stops a distribution or advanced user from removing dex, but the raw manifests need to avoid trying to document every possible use case, especially for changes that make auth behave significantly differently.

My suggestion is to create a blog post about what you did for users who want to do the same thing.

@cybernagle
Copy link
Contributor Author

standardize oath is really a great idea.
As a user, I used to inquery the best practice do auth with kubeflow platform by search on google, and query all the post on kubeflow site. but got no lucky.

I must say, kubeflow has a very good design, but lack doc to describe how it works, and how to do the infrastructure level customization.

That's why this PR is create under manifest README/installaction section,

  1. it shows people what current auth infrastructure is and may save people to read the source code.
  2. it tells people how to customize auth in a detailed way. since kubeflow are used world wide, and it need to be integrate with so many systems; you never know how people need integrate with there own system.

for example: this doc https://deploy-preview-3795--competent-brattain-de2d6d.netlify.app/docs/components/pipelines/user-guides/core-functions/connect-api/#kubeflow-platform---outside-the-cluster is really a good code, but it still not work in the platform I managed.

by doing this, people will get to know how kubeflow auth works, and they may able to do other types of customization to fit their own needs.

base on upon , I don't think create a blog post is a good idea, because get to know what is the infra how kubeflow platform auth works & how to do customization of kubeflow oauth is the basic needs of kubeflow platform from user side.

-- base on the user of kubeflow platform :)

@juliusvonkohout
Copy link
Member

@cybernagle thank you for the PR. Yes that is an often requested feature and for many Kubeflow needs to work without Dex as well. Dex is definitely not mandatory. We should definitely provide the rough general details and an example for advanced users. I will review this PR as soon as i find the time :-)

@cybernagle
Copy link
Contributor Author

cybernagle commented Sep 29, 2024

If you need , I can also raise a PR to provide my solution as a overlay under oauth2 proxy :)

@thesuperzapper
Copy link
Member

@juliusvonkohout please be careful about including too many variations of the manifests in the official repo.

Everything we add to the repo people assume we're going to support.

@juliusvonkohout
Copy link
Member

If you need , I can also raise a PR to provide my solution as a overlay under oauth2 proxy :)

A short general educational overlay example for the configuration directly inline in the documentation is enough for now :-)
You can just add it here in this PR as markdown.

Please run your text through a sophisticated spell checker and formatter because there seem to be some typographical errors etc.

@thesuperzapper
Copy link
Member

@juliusvonkohout I am not sure all this info needs to be in the root README, perhaps under one of the oauth2-proxy folders and a link in the main README is better?

Either way, I am still against describing non-standard deployment methods in the manifests, it's just not sustainable for the project. For example, we have already made significant changes to how the auth works, making some of the info in this PR incorrect.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 2, 2024

Yes, the oauth2-proxy readme is probably a better place, but I definitely want to document it.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 3, 2024

@cybernagle please also retest / rebase against master / 1.9.1 https://github.com/kubeflow/manifests/releases/tag/v1.9.1-rc.2

@cybernagle
Copy link
Contributor Author

@juliusvonkohout I am not sure all this info needs to be in the root README, perhaps under one of the oauth2-proxy folders and a link in the main README is better?

Either way, I am still against describing non-standard deployment methods in the manifests, it's just not sustainable for the project. For example, we have already made significant changes to how the auth works, making some of the info in this PR incorrect.

Hi @thesuperzapper,

By means "significant changes" do you mean this PR ?

If upon understanding is correct, I have two more points to mention.

First Point

I checked the overlays, there is no conflict. base on what your PR described, M2M for off cluster is using kubectl + port forward.

image

But , in many situation, we need access Kubeflow out side of cluster without kubectl.
To be more specific , In my situation, I need access Kubeflow from GCP cluster, and the Kubeflow platform in in our own IDC.
So the doc described , is access kubeflow without kubectl, and directly consume the Kubeflow Platform external link;

Second Point

Base on the PR, implementation still remain the same arch,
Istio Ingress Gateway -> Virtual Service -> OAuth2 Proxy -> Dex -> IdP

The solution I've suggest still remain the same:
Istio Ingress Gateway -> Virtual Service -> OAuth2 proxy -> IdP
But just without Dex.

Also, this solution can be implement very easy and generic. as long as we using Istio & OAuth2 proxy , it won't be incorrect.

@cybernagle
Copy link
Contributor Author

@cybernagle please also retest / rebase against master / 1.9.1 https://github.com/kubeflow/manifests/releases/tag/v1.9.1-rc.2

Hi @juliusvonkohout

  1. typo all fixed ,thanks :)
  2. I've rebased master/1.9.1 to my branch, does this meet our requirments? (CI seems failed, checked it, not related to the README )

@cybernagle
Copy link
Contributor Author

/retest

Copy link

@cybernagle: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cybernagle
Copy link
Contributor Author

/cc @juliusvonkohout

Are there any updates or any blocker under this PR ?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 21, 2024

/ok-to-test

@cybernagle i will fix the spelling and grammar then, rebase to master and push to the same branch.

Signed-off-by: Cyber Nagle <nagle.zhang@qq.com>
Signed-off-by: Cyber Nagle <nagle.zhang@qq.com>
Signed-off-by: Cyber Nagle <nagle.zhang@qq.com>
Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
@google-oss-prow google-oss-prow bot removed the size/M label Oct 21, 2024
@juliusvonkohout juliusvonkohout self-assigned this Oct 21, 2024
@juliusvonkohout
Copy link
Member

/lgtm
/approve

since it is good enough to be an improvement

@google-oss-prow google-oss-prow bot added the lgtm label Oct 21, 2024
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout

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 a56df7f into kubeflow:master Oct 21, 2024
12 checks passed
@cybernagle cybernagle deleted the oauth2-proxy-doc branch October 22, 2024 03:07
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.

3 participants