alamb commented on code in PR #781:
URL:
https://github.com/apache/arrow-rs-object-store/pull/781#discussion_r3547829728
##########
src/azure/client.rs:
##########
@@ -662,16 +663,47 @@ async fn parse_blob_batch_delete_body(
Ok(results)
}
+/// How long a freshly fetched user delegation key is requested to remain
valid.
+///
+/// The SAS tokens we sign with it stay short-lived; this only bounds how often
+/// we call `GetUserDelegationKey`. Azure caps the key lifetime at 7 days.
Review Comment:
Can you add a URL that with some evidence for this claim? It seems this
could be a good one
https://learn.microsoft.com/en-us/rest/api/storageservices/get-user-delegation-key#request-body
> Start Required. The start time for the user delegation SAS, in ISO date
format. It must be a valid date and time within seven days of the current date.
##########
src/azure/client.rs:
##########
@@ -662,16 +663,47 @@ async fn parse_blob_batch_delete_body(
Ok(results)
}
+/// How long a freshly fetched user delegation key is requested to remain
valid.
+///
+/// The SAS tokens we sign with it stay short-lived; this only bounds how often
Review Comment:
I think it would help to define SAS at least once (I realize it is used
elsewhere in this module without that)
```suggestion
/// The shared access signature (SAS) tokens we sign with it stay
short-lived; this only bounds how often
```
##########
src/azure/client.rs:
##########
@@ -1043,6 +1075,50 @@ impl AzureClient {
}
}
+ /// Return a user delegation key valid for a SAS over `[sas_start,
sas_expiry]`.
+ ///
+ /// `GetUserDelegationKey` is a network round-trip that Azure throttles
(HTTP
+ /// 503) under load, so a long-lived key is cached and reused to sign many
+ /// short-lived SAS URLs.
+ ///
+ /// The cache only returns a key with more than [`DELEGATION_KEY_MIN_TTL`]
+ /// remaining, so any SAS no longer than that is guaranteed to expire
before
+ /// its key. The (rare) longer-lived SAS get a dedicated key instead.
+ async fn user_delegation_key(
+ &self,
+ sas_start: DateTime<Utc>,
+ sas_expiry: DateTime<Utc>,
+ expires_in: Duration,
+ ) -> Result<UserDelegationKey> {
+ if expires_in <= DELEGATION_KEY_MIN_TTL {
+ self.delegation_key_cache
+ .get_or_insert_with(||
self.fetch_delegation_key(DELEGATION_KEY_VALIDITY))
+ .await
+ } else {
+ self.get_user_delegation_key(&sas_start, &sas_expiry).await
+ }
+ }
+
+ /// Fetch a user delegation key valid for `validity` and wrap it as a
+ /// [`TemporaryToken`] so [`TokenCache`] can expire it.
+ async fn fetch_delegation_key(
Review Comment:
I found the naming somewhat confusing here (as it wasn't clear to me what
the relationship between `get_user_delegation_key` and
`fetch_user_delegation_key` was (`get` and `fetch` are very similar). I wa sall
the more confused because `get` actually does the network call, when I would
normally exepct `fetch` to do that
Something like this might be easier to understand
* fetch_delegation_key --> get_delegation_key
* (current) get_delegation_key --> get_delegation_key_inner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
