feat! Update on interval #22
Conversation
kjellkongsvik
force-pushed
the
update_interval
branch
from
15c0371
to
960bf11
Compare
kjellkongsvik
force-pushed
the
update_interval
branch
5 times, most recently
from
5a1f3df
to
99b8bd6
Compare
|
Hello ! |
axum-jwks/src/key_manager.rs
Outdated
| info!("Periodically updated jwks"); | ||
| *ks = new_ks | ||
| } | ||
| Err(e) => error!(?e, "Could not update jwks from: {}", url), |
There was a problem hiding this comment.
I think this error needs to be bubbled up to the caller instead of just being logged. As a consumer of the library, I would at least like the option to choose how to handle errors.
There was a problem hiding this comment.
Agreed, hard to handle inside of a spawned task. I remove this functionality. The users can write this themselves and use the update function that is now public. I also remove other usage of error!
axum-jwks/src/key_manager.rs
Outdated
| loop { | ||
| { | ||
| let mut ks = key_store.write().await; | ||
| match KeyStore::new(&client, &url, &audience).await { |
There was a problem hiding this comment.
Should we pull the new keystore before obtaining the write lock? This has the potential to take a while due to network delays, and my understanding is that with the keystore locked, no requests can read the keystore.
axum-jwks/src/key_store.rs
Outdated
| impl Default for KeyStore { | ||
| fn default() -> Self { | ||
| Self { | ||
| last_updated: Instant::now() - Duration::from_secs(3600 * 24 * 365), |
There was a problem hiding this comment.
I think making this an Option instead of faking times in the past makes more sense.
There was a problem hiding this comment.
Agreed, after a little tweaking I got the usage to be ok after all.
| client: reqwest::Client, | ||
| } | ||
|
|
||
| impl KeyManager { |
There was a problem hiding this comment.
Instead of constructing an empty keystore and then waiting for the first update interval, it might make more sense to employ a builder pattern, and to fetch the keys synchronously the first time. This should make it easier to expose errors on the first fetch. I think it's much more likely that the first fetch errors (maybe a configuration issue) rather than future ones, so this should make for a better developer experience.
There was a problem hiding this comment.
Considered that, wanted simplicity and being free to set it up more freely. I now agree with you. Better to make a more standard builder pattern. Users are free to create it as they wish without using the builder pattern
kjellkongsvik
force-pushed
the
update_interval
branch
3 times, most recently
from
42ba95a
to
871bda6
Compare
kjellkongsvik
force-pushed
the
update_interval
branch
from
871bda6
to
3208410
Compare
No description provided.