-
-
Notifications
You must be signed in to change notification settings - Fork 326
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
Conversation
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 👍
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.
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?
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! |
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.
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
b228b1c
to
e61554b
Compare
@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! |
9151669
to
ec7c839
Compare
Everything is now updated and addressed! |
Released in kube 0.33.0. Thanks again! |
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 commitCloses #72
Closes #224