Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Add oidc auth #48

Merged
merged 2 commits into from
Mar 20, 2018
Merged

Conversation

ltamaster
Copy link
Contributor

No description provided.

@dmyerscough
Copy link

Hi @ltamaster you will want to run the following against the files you modified:-

$ pip install pycodestyle
$ pycodestyle ...

The Travis CI is failing due to coding style errors.

@ltamaster
Copy link
Contributor Author

@dmyerscough thanks, I will try

@mcalmer
Copy link

mcalmer commented Feb 27, 2018

JFYI: I tried this patch and it works fine against our kubernetes cluster. Seems that only one unit test is failing. Maybe the last thing what prevents this PR from being merged.

@codecov-io
Copy link

codecov-io commented Mar 1, 2018

Codecov Report

Merging #48 into master will decrease coverage by 0.35%.
The diff coverage is 86.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   93.64%   93.29%   -0.36%     
==========================================
  Files          11       11              
  Lines         866      939      +73     
==========================================
+ Hits          811      876      +65     
- Misses         55       63       +8
Impacted Files Coverage Δ
config/kube_config_test.py 93.1% <100%> (+0.58%) ⬆️
config/kube_config.py 89.69% <79.16%> (-2.37%) ⬇️
config/dateutil.py 93.33% <0%> (+4.44%) ⬆️

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 11da619...5731554. Read the comment docs.

@dmyerscough
Copy link

Nice work @ltamaster, I just saw you message now and see you applied a fix to the unit test.

@ltamaster
Copy link
Contributor Author

thanks @dmyerscough, finally I found the issue !!!

@dmyerscough dmyerscough mentioned this pull request Mar 9, 2018
@mvle
Copy link
Contributor

mvle commented Mar 9, 2018

Re-posting comment here from closed PR #31. Seems to affect this new PR as well.

I apologize in advance if this is already supported, but from looking at the PR, I think there needs to be a way to refresh the token not only at configuration initialization time but also during invocation of the K8 API calls. The token might expire right before a call thus causing the call to fail with an authentication failure. This will be needed for services that run a long time and rely on being able to reach the K8 Api Server (also short services that happen to be unlucky).

I think this can be done with a check to 'exp' field in the id-token right before a call_api or catch a 401 return status and perform a refresh token operation and update the configuration object with the new id_token and refresh_token values from the OAuth server.

@mcalmer
Copy link

mcalmer commented Mar 11, 2018

@roycaihw - what prevents this pr to get merged and added to 6.0.0?

@roycaihw
Copy link
Member

I think this can be done with a check to 'exp' field in the id-token right before a call_api or catch a 401 return status and perform a refresh token operation and update the configuration object with the new id_token and refresh_token values from the OAuth server.

@mvle I agree that we need to ensure id_token (and also access_token for gcp) is valid and refresh as needed. However, currently swagger-codegen doesn't support refreshing token of OAuth2. Without upstream change, the generated client's support for refreshing token would be hacky. cc @yliaog for more insight here

what prevents this pr to get merged and added to 6.0.0?

@mcalmer Sorry I didn't get a chance to look at this yet. client-python is currently 6.0.0b1. This pr can still get in 6.0.0 which will get released after k8s 1.10 rolls out.

@yliaog
Copy link
Contributor

yliaog commented Mar 15, 2018

i think the suggestion for refreshing token is fine, but can be done in a separate PR.

For this PR, I see many commits, @ltamaster could you please squash the commits?

@ltamaster
Copy link
Contributor Author

Hi @yliaog I can do that, but I understand that I need a new PR for that right?

@yliaog
Copy link
Contributor

yliaog commented Mar 15, 2018

i think you can use the same PR for squashing commits, so all the history can still be kept.

Fix for the `TypeError: Incorrect padding` error
Adding test with "mocked" variables

Persist the new token (refresh token) and add a not-ssl-verification for the refresh token call (i didn't find a way to pass the certificate to OAuth2Session

fixing the refresh-token problem (ssl certificate) and saving returning the new refresh-token

Fix test

fixing  coding style errors

Fixing test update-pep8

Fix test_oidc_with_refresh error
@yliaog
Copy link
Contributor

yliaog commented Mar 20, 2018

not sure how you did it, did you try something like what's described here (https://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit)

@ltamaster
Copy link
Contributor Author

Hi @yliaog, finally I squashed the commits, please take a look.
Thanks
Luis

@yliaog yliaog merged commit 2010e2d into kubernetes-client:master Mar 20, 2018
@yliaog
Copy link
Contributor

yliaog commented Mar 20, 2018

thanks @ltamaster, this PR is merged. The remaining issues are tracked separately.

@yliaog
Copy link
Contributor

yliaog commented Mar 20, 2018

@mvle could you please file another issue to track the feature request for refreshing token?

@ltamaster
Copy link
Contributor Author

Hi @yliaog

I noticed this PR is merged, but it hasn't been merged on the client repo.

Are you waiting for the other issues or do I need something else?

Thanks
Luis

@roycaihw
Copy link
Member

roycaihw commented Apr 5, 2018

@ltamaster I was planning on updating the base submodule when we do the 6.0.0 release. PR is also welcome to do the update :) (e.g. kubernetes-client/python#475)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants