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 oidc's auth provider #3663

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Conversation

snappyyouth
Copy link

@snappyyouth snappyyouth commented Aug 7, 2024

#3662

What is the problem you're trying to solve
I will like to add support to scheduler such that also clusters that use oidc authentication will work.
webhook-manager belike:
image
controller-manager belike:
image
scheduler belike:
image

Describe the solution you'd like
add oidc's auth provider in the /cmd/scheduler/main.go /cmd/webhook-manager/main.go /cmd/controller-manager/main.go

_ "k8s.io/client-go/plugin/pkg/client/auth"

Arfter adding, all the comment running expectedly.

controller-manager belike:
image

scheduler belike:
image

@volcano-sh-bot
Copy link
Contributor

Welcome @snappyyouth!

It looks like this is your first PR to volcano-sh/volcano.

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 7, 2024
@lowang-bh
Copy link
Member

/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 8, 2024
@Monokaix
Copy link
Member

Monokaix commented Aug 8, 2024

The image in description is missing: )

@snappyyouth
Copy link
Author

The image in description is missing: )

thx for the reminder.I has supplemented some images in the description .

@Monokaix
Copy link
Member

Monokaix commented Aug 9, 2024

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2024
@hwdef
Copy link
Member

hwdef commented Aug 9, 2024

Will it have any impact on the current usage, or output more error logs without using oidc?

@snappyyouth
Copy link
Author

snappyyouth commented Aug 9, 2024

Will it have any impact on the current usage, or output more error logs without using oidc?

Sorry, I don't quite understand what you mean?
It have nothing impact on the current usage.
In current production, we must configure the config file through oidc auditing. Without oidc, all volcano components cannot run.
In addition, with the continuous practice of K8S, auditing operations is a trend, and I think it is necessary to add oidc authentication.

@hwdef
Copy link
Member

hwdef commented Aug 9, 2024

Will it have any impact on the current usage, or output more error logs without using oidc?

Sorry, I don't quite understand what you mean? In current production, we must configure the config file through oidc auditing. Without oidc, all volcano components cannot run. In addition, with the continuous practice of K8S, auditing operations is a trend, and I think it is necessary to add oidc authentication.

I mean if i don't use oidc. Will this change cause any confusion for me?
I briefly checked the code and found that if init fails, an error will be print.

https://github.com/kubernetes/client-go/blob/71959c526d543a5e4c3ca6fb808f535c2726483f/plugin/pkg/client/auth/oidc/oidc.go#L52

@snappyyouth
Copy link
Author

Will it have any impact on the current usage, or output more error logs without using oidc?

Sorry, I don't quite understand what you mean? In current production, we must configure the config file through oidc auditing. Without oidc, all volcano components cannot run. In addition, with the continuous practice of K8S, auditing operations is a trend, and I think it is necessary to add oidc authentication.

I mean if i don't use oidc. Will this change cause any confusion for me? I briefly checked the code and found that if init fails, an error will be print.

https://github.com/kubernetes/client-go/blob/71959c526d543a5e4c3ca6fb808f535c2726483f/plugin/pkg/client/auth/oidc/oidc.go#L52

if you don't use oidc. This change will't cause any confusion for you.

Errors only occur when registering repeatedly.
https://github.com/kubernetes/client-go/blob/71959c526d543a5e4c3ca6fb808f535c2726483f/rest/plugin.go#L66

And when your config didn't use oidc, AuthProvider is nil. The program will skip.
https://github.com/kubernetes/client-go/blob/71959c526d543a5e4c3ca6fb808f535c2726483f/rest/transport.go#L139

All in all, it's completely safe!

@hwdef
Copy link
Member

hwdef commented Aug 9, 2024

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hwdef

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

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2024
@volcano-sh-bot volcano-sh-bot merged commit e811170 into volcano-sh:master Aug 9, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants