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

Imperative api pls #434

Merged
merged 6 commits into from
Jan 28, 2025
Merged

Conversation

joshmeranda
Copy link
Contributor

rancher/rancher#47540

Replaces #316

Implements the default authentication.

This was original part of #287 but was split up to make it easier to review.

@joshmeranda joshmeranda requested a review from a team as a code owner January 8, 2025 16:09
@tomleb tomleb requested review from bigkevmcd and tomleb January 8, 2025 18:29
@tomleb
Copy link
Contributor

tomleb commented Jan 8, 2025

@bigkevmcd adding you since you had reviewed the other one previously.

tomleb
tomleb previously approved these changes Jan 8, 2025
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

One thing I'd like to have as test in the future is running the API server with the APIService configured so that we can verify a few things when going through the main kubeapiserver (eg: ensuring user, groups and extras headers are properly propagated). This would require running a pod as an integration test (or potentially we can do that when RDP work is done).

Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crobby crobby self-requested a review January 27, 2025 19:37
// NewUnionAuthenticator creates a [UnionAuthenticator].
//
// The authenticators will be tried one by one, in the order they are given, until
// one succeed or all fails.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"one succeeds or all fail" ??

Copy link
Contributor

@crobby crobby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I like the examples included in the comments.

@joshmeranda joshmeranda merged commit 5cdbd29 into rancher:main Jan 28, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants