-
Notifications
You must be signed in to change notification settings - Fork 891
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
Conversation
a6c4063
to
42925b5
Compare
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. |
standardize oath is really a great idea. 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,
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 -- base on the user of kubeflow platform :) |
@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 :-) |
If you need , I can also raise a PR to provide my solution as a overlay under oauth2 proxy :) |
@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. |
A short general educational overlay example for the configuration directly inline in the documentation is enough for now :-) Please run your text through a sophisticated spell checker and formatter because there seem to be some typographical errors etc. |
@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. |
Yes, the oauth2-proxy readme is probably a better place, but I definitely want to document it. |
@cybernagle please also retest / rebase against master / 1.9.1 https://github.com/kubeflow/manifests/releases/tag/v1.9.1-rc.2 |
42925b5
to
41c39d8
Compare
Hi @thesuperzapper, By means "significant changes" do you mean this PR ? If upon understanding is correct, I have two more points to mention. First PointI checked the overlays, there is no conflict. base on what your PR described, M2M for off cluster is using kubectl + port forward. But , in many situation, we need access Kubeflow out side of cluster without kubectl. Second PointBase on the PR, implementation still remain the same arch, The solution I've suggest still remain the same: Also, this solution can be implement very easy and generic. as long as we using Istio & OAuth2 proxy , it won't be incorrect. |
|
/retest |
@cybernagle: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/cc @juliusvonkohout Are there any updates or any blocker under this PR ? |
/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 <[email protected]>
Signed-off-by: Cyber Nagle <[email protected]>
Signed-off-by: Cyber Nagle <[email protected]>
3b90ed9
to
98af702
Compare
Signed-off-by: juliusvonkohout <[email protected]>
Signed-off-by: juliusvonkohout <[email protected]>
Signed-off-by: juliusvonkohout <[email protected]>
/lgtm since it is good enough to be an improvement |
[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 |
Pull Request Template for Kubeflow manifests Issues
✏️ A brief description of the changes
📦 List any dependencies that are required for this change
🐛 If this PR is related to an issue, please put the link to the issue here.
✅ Contributor checklist
DCO
check)