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

Add support for open id connect token auth #36

Merged
merged 2 commits into from
Aug 21, 2018

Conversation

bpicolo
Copy link
Contributor

@bpicolo bpicolo commented Aug 1, 2018

Fixes #35.

The one think I wasn't yet able check the actual functionality of is idp-certificate-authority-data, but my org will add that sometime soon and then I can give that a shot to see if it works as intended in practice. (It essentially matches what kubectl tries at the moment, though)

Lmk what other tests you'd like to see here. It's based off the kubernetes/python-base version, but I cleaned up a bunch of the code and also fixed a number of bugs. I'm able to successfully auth / run commands against the kubernetes cluster in question with this patch.

Thanks!

@bpicolo bpicolo force-pushed the 35_oidc_auth_support branch from 63245e9 to ffc31d5 Compare August 1, 2018 18:52
@codecov-io
Copy link

codecov-io commented Aug 1, 2018

Codecov Report

Merging #36 into master will increase coverage by 0.55%.
The diff coverage is 97.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   93.51%   94.07%   +0.55%     
==========================================
  Files          16       18       +2     
  Lines        1003     1181     +178     
==========================================
+ Hits          938     1111     +173     
- Misses         65       70       +5
Impacted Files Coverage Δ
kubernetes_asyncio/config/openid.py 100% <100%> (ø)
kubernetes_asyncio/config/openid_test.py 100% <100%> (ø)
kubernetes_asyncio/config/kube_config.py 93.15% <91.11%> (-0.43%) ⬇️
kubernetes_asyncio/config/kube_config_test.py 93.5% <97.43%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72221b2...59967f7. Read the comment docs.

@bpicolo bpicolo force-pushed the 35_oidc_auth_support branch 2 times, most recently from 1718314 to a4b7a8a Compare August 1, 2018 19:18
@bpicolo
Copy link
Contributor Author

bpicolo commented Aug 1, 2018

screen shot 2018-08-01 at 3 29 20 pm

That should count, right? 😉

Copy link
Owner

@tomplus tomplus left a comment

Choose a reason for hiding this comment

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

Please try to use asynchronous calls to manage OAuth sessions. Thanks.

self._perform_oauth_refresh(provider, client, well_known_data)

def _perform_oauth_refresh(self, provider, client, well_known_data):
# It's not optimal that this isn't async, but there's no existing well-supported
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, and it was a reason why I didn't copy this from the official library. The main goal of this library is to use only asynchronous calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏼 Fair enough. I'll write the subset we need off of aio

@bpicolo bpicolo force-pushed the 35_oidc_auth_support branch 2 times, most recently from cea5c1a to 5267788 Compare August 3, 2018 15:33
@bpicolo bpicolo force-pushed the 35_oidc_auth_support branch from 5267788 to 6ced258 Compare August 3, 2018 15:42
@bpicolo
Copy link
Contributor Author

bpicolo commented Aug 3, 2018

@tomplus There's a functional asyncio version for ya ❤️

@bpicolo bpicolo force-pushed the 35_oidc_auth_support branch 2 times, most recently from 05bc021 to 3947abf Compare August 3, 2018 18:40
@bpicolo bpicolo force-pushed the 35_oidc_auth_support branch from 3947abf to 59967f7 Compare August 3, 2018 18:54
@bpicolo
Copy link
Contributor Author

bpicolo commented Aug 8, 2018

@tomplus this should be ready for re-review when you have a chance

@tomplus tomplus self-assigned this Aug 13, 2018
@tomplus
Copy link
Owner

tomplus commented Aug 13, 2018

@bpicolo Thanks a lot! I'll check how it works soon...

@tomplus
Copy link
Owner

tomplus commented Aug 21, 2018

LGTM

Great job! Thanks for your contribution.

@tomplus tomplus merged commit 76c111e into tomplus:master Aug 21, 2018
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.

Add OIDC auth support
3 participants