-
Notifications
You must be signed in to change notification settings - Fork 296
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
OIDC support #544
OIDC support #544
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @arbielsk! |
CNCF ICLA accepted. |
There seem to be non-deterministic tests in LeaderElectionTests.cs Example Tests finish successfully for Ubuntu, but fail for MacOS with same code. On my local machine (MacOS) the tests run successfully (3/3 runs, no test failures). Also without any code changes, the result for tests on the Ubuntu build differ between these two runs (I added an empty commit to re-trigger Github Actions): I don't believe this is in the scope of this PR as it should be independent of the LeaderElection feature if I am interpreting the code correctly. |
seems |
Yes it definitely was. However I am unsure what the correct resolution would be:
Due to my unfamiliarity with C# I am also unsure on the possible implications of this conflict and the gravity of the warning. |
Can you just use explicit full package names (e.g. |
you may want rebase master to get rid of flaky testcases i am fine with allow |
Co-authored-by: Boshi Lian <farmer1992@gmail.com>
Co-authored-by: Boshi Lian <farmer1992@gmail.com>
@brendandburns I've spent a little more time educating myself on the issue. As far as I understand the issue, the warning is produced because the type name I therefore understand that there are the following possible resolutions:
@tg123 @brendanburns Please let me know what your preferred way of solving this would be. I have a commit prepared to rename the type name to Thank you again for taking the time to review my PR! |
Accepting the warning for now is fine with me. If we want to make the change, we need to make it across a major version since it is a breaking change. |
please add CA1723 to kubernetes-client.ruleset to disable warning |
Thanks again, @brendanburns @tg123 for getting back to me so quickly. I've added CA1724 to the exceptions and added a comment explaining the situation. Thanks again and have a great night! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
/assign @brendandburns |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arbielsk, brendandburns, tg123 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 |
Thank you, @tg123, @brendandburns! |
Feature
Add support for OpenID Connect. This includes a new OidcTokenProvider which is able to refresh tokens. A new dependency is introduced in the form of IdentityModel.OidcClient.
While at it, I fixed a few typos in comments in src/KubernetesClient/KubernetesClientConfiguration.ConfigFile.cs
Motivation
At my company we are avid users of Bridge-To-Kubernetes which uses the C# K8S Client under the hood. We currently have multiple teams that are blocked from using Bridge-To-Kubernetes since the C# K8S client currently does not support OIDC. I therefore created this PR with the intent of adding OIDC support to C# K8S client so it can be integrated into Bridge-To-Kubernetes.
Disclaimer
I am not a C# developer. In fact this is the first C# code I have ever written. I therefore would like to ask for a thorough review; I am more than happy to update the PR to conform with any best practices, style guides and testing coverage as you see fit. I would be very grateful for any feedback!
I am also currently seeing the following warning and due to my unfamiliarity with C# I am unsure how to solve it optimally:
Thank you for taking the time to review and consider this PR!
My team is very much looking forward to seeing this feature downstream in Bridge-To-Kubernetes as soon as possible, as this will significantly reduce our development efforts on multiple projects.