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(*): Adds automatic token refreshing #234

Merged
merged 3 commits into from
Apr 27, 2020

Conversation

thomastaylor312
Copy link
Contributor

@thomastaylor312 thomastaylor312 commented Apr 24, 2020

This is by no means elegant, but seems to be functional. Setting the
authorization headers now occurs at request time instead of during
initial configuration. It will only try to refresh a token if the
configured expiration time has passed.

The second commit on this introduces what I think is probably a more easily readable version of the auth using an enum. If we don't like it, we can pop off that commit

Closes #72
Closes #224

Copy link

@owenthereal owenthereal left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

left some comments as i have been trying to unpack it.

it's a really good refactor that helps out a ton, but wonder if perhaps the lock from the Client -> Config can be done a little more elegantly. Perhaps by lifting the lock into the client?

kube/src/client/mod.rs Show resolved Hide resolved
kube/src/config/mod.rs Show resolved Hide resolved
kube/src/config/mod.rs Show resolved Hide resolved
kube/src/config/mod.rs Show resolved Hide resolved
kube/src/config/mod.rs Show resolved Hide resolved
kube/src/config/mod.rs Show resolved Hide resolved
@thomastaylor312
Copy link
Contributor Author

So I thought about trying to lock at the Client level over the Config level and I settled on this approach. The main reason is that we don't have a need to lock the whole client just to check for a token refresh. Right now we are only locking the Config when it is a refresh token and otherwise we don't even use a lock. If we lifted it to the Client level, we'd have to lock no matter what on the in memory Config unless we wanted to move all token refresh logic to the Client.

I am ok either way! Just want to explain the reasoning behind this current design

@clux
Copy link
Member

clux commented Apr 27, 2020

So I thought about trying to lock at the Client level over the Config level and I settled on this approach. The main reason is that we don't have a need to lock the whole client just to check for a token refresh. Right now we are only locking the Config when it is a refresh token and otherwise we don't even use a lock. If we lifted it to the Client level, we'd have to lock no matter what on the in memory Config unless we wanted to move all token refresh logic to the Client.

I am ok either way! Just want to explain the reasoning behind this current design

I appreciate the comments. The design makes sense and probably only really benefits from minor aesthetic tweaks at this point. Left 2 small comments for docs and arc unpacking, but beyond that, happy to merge.

Thanks a lot for this pr!

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I think this PR is the right approach 😀

This is by no means elegant, but seems to be functional. Setting the
authorization headers now occurs at request time instead of during
initial configuration. It will only try to refresh a token if the
configured expiration time has passed.

Closes kube-rs#72
@thomastaylor312
Copy link
Contributor Author

@clux I have updated my branch and resolved all comments that I know are complete. Just a few outstanding small follow ups and this should be good to go!

@thomastaylor312
Copy link
Contributor Author

Everything is now updated and addressed!

@clux clux merged commit 250d380 into kube-rs:master Apr 27, 2020
@clux
Copy link
Member

clux commented Apr 27, 2020

Released in kube 0.33.0. Thanks again!

@thomastaylor312 thomastaylor312 deleted the feat/token_refresh branch April 28, 2020 15:58
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.

Reauthentication of expired token refresh id tokens for auth-providers
4 participants