Refactor auth - add re-auth mechanism #111
Conversation
aviramha
requested review from
thomastaylor312,
bacongobbler and
flavio
as code owners
aviramha
force-pushed
the
re-auth
branch
2 times, most recently
from
f85329c to
9675b12
Compare
|
@aviramha can you please look into the unit tests that are failing? I can reproduce the failures locally, and they are a regression introduced by this PR |
…token gen] mechanism when needed Signed-off-by: Aviram Hassan <[email protected]>
Signed-off-by: Aviram Hassan <[email protected]>
Signed-off-by: Aviram Hassan <[email protected]>
|
Sure! I had an assumption those tests require auth. Fixed now. |
| @@ -502,6 +534,38 @@ impl Client { | |||
| authentication: &RegistryAuth, | |||
| operation: RegistryOperation, | |||
| ) -> Result<Option<String>> { | |||
| self.store_auth_if_needed(image.resolve_registry(), authentication) | |||
| .await; | |||
| // preserve old caching behavior | |||
There was a problem hiding this comment.
Do we actually need to preserve the old behavior? I think a call to get_auth_token should do the same logic of fetching the token and doing the auth step, right?
There was a problem hiding this comment.
I didn't want to break API, and that could be that user wants to do auth and it might not really call auth underneath (because cache) feels a bit vile.
There was a problem hiding this comment.
Ok, that makes sense
Signed-off-by: Aviram Hassan <[email protected]>
There was a problem hiding this comment.
I think we're good to go! @flavio leaving merging to you since you started reviewing/conversation on this one
== What's Changed * client is now clonable and can be used in concurrent manner by @aviramha in oras-project#109 * fix(*): Fixes our use of deprecated functions by @thomastaylor312 in oras-project#113 * Pull alternative urls by @seanyoung in oras-project#108 * Refactor auth - add re-auth mechanism by @aviramha in oras-project#111 * Implement Client::pull_manifest_raw by @MathiasPius in oras-project#115 * chore(deps): Bump actions/checkout from 4.1.1 to 4.1.2 by @dependabot in oras-project#117 * chore(deps): update reqwest and http by @flavio in oras-project#119 == New Contributors * @aviramha made their first contribution in oras-project#109 * @thomastaylor312 made their first contribution in oras-project#113 * @seanyoung made their first contribution in oras-project#108 * @MathiasPius made their first contribution in oras-project#115 **Full Changelog**: oras-project/rust-oci-client@v0.10.0...v0.11.0 Signed-off-by: Flavio Castelli <[email protected]>
Sorry about the spam!
This builds upon the other 2 PRs #109 #110
Just out of comfort assuming if merged I'll work on it serially.
This adds logic to enable token re-creation when needed, and also making the auth API more fluent.
I had a situation where my token got expired mid-upload and there was no place to renew, and if I could break the API I'd have structured it a bit different, but I think is ok.