Skip to content

Commit 38ae414

Browse files
Initialize S3 signer once (#17092)
## Summary Right now, we initialize the signer many times concurrently.
1 parent 6de869c commit 38ae414

File tree

2 files changed

+53
-21
lines changed

2 files changed

+53
-21
lines changed

crates/uv-auth/src/middleware.rs

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,15 @@ enum TokenState {
129129
Initialized(Option<AccessToken>),
130130
}
131131

132+
#[derive(Clone)]
133+
enum S3CredentialState {
134+
/// The S3 credential state has not yet been initialized.
135+
Uninitialized,
136+
/// The S3 credential state has been initialized, with either a signer or `None` if
137+
/// no S3 endpoint is configured.
138+
Initialized(Option<Arc<Authentication>>),
139+
}
140+
132141
/// A middleware that adds basic authentication to requests.
133142
///
134143
/// Uses a cache to propagate credentials from previously seen requests and
@@ -150,6 +159,8 @@ pub struct AuthMiddleware {
150159
pyx_token_store: Option<PyxTokenStore>,
151160
/// Tokens to use for persistent credentials.
152161
pyx_token_state: Mutex<TokenState>,
162+
/// Cached S3 credentials to avoid running the credential helper multiple times.
163+
s3_credential_state: Mutex<S3CredentialState>,
153164
preview: Preview,
154165
}
155166

@@ -172,6 +183,7 @@ impl AuthMiddleware {
172183
base_client: None,
173184
pyx_token_store: None,
174185
pyx_token_state: Mutex::new(TokenState::Uninitialized),
186+
s3_credential_state: Mutex::new(S3CredentialState::Uninitialized),
175187
preview: Preview::default(),
176188
}
177189
}
@@ -678,13 +690,26 @@ impl AuthMiddleware {
678690
return Some(credentials);
679691
}
680692

681-
if let Some(credentials) = S3EndpointProvider::credentials_for(url, self.preview)
682-
.map(Authentication::from)
683-
.map(Arc::new)
684-
{
685-
debug!("Found S3 credentials for {url}");
686-
self.cache().fetches.done(key, Some(credentials.clone()));
687-
return Some(credentials);
693+
if S3EndpointProvider::is_s3_endpoint(url, self.preview) {
694+
let mut s3_state = self.s3_credential_state.lock().await;
695+
696+
// If the S3 credential state is uninitialized, initialize it.
697+
let credentials = match &*s3_state {
698+
S3CredentialState::Uninitialized => {
699+
trace!("Initializing S3 credentials for {url}");
700+
let signer = S3EndpointProvider::create_signer();
701+
let credentials = Arc::new(Authentication::from(signer));
702+
*s3_state = S3CredentialState::Initialized(Some(credentials.clone()));
703+
Some(credentials)
704+
}
705+
S3CredentialState::Initialized(credentials) => credentials.clone(),
706+
};
707+
708+
if let Some(credentials) = credentials {
709+
debug!("Found S3 credentials for {url}");
710+
self.cache().fetches.done(key, Some(credentials.clone()));
711+
return Some(credentials);
712+
}
688713
}
689714

690715
// If this is a known URL, authenticate it via the token store.

crates/uv-auth/src/providers.rs

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,8 @@ static S3_ENDPOINT_REALM: LazyLock<Option<Realm>> = LazyLock::new(|| {
6666
pub(crate) struct S3EndpointProvider;
6767

6868
impl S3EndpointProvider {
69-
/// Returns the credentials for the S3 endpoint, if available.
70-
pub(crate) fn credentials_for(url: &Url, preview: Preview) -> Option<DefaultSigner> {
69+
/// Returns `true` if the URL matches the configured S3 endpoint.
70+
pub(crate) fn is_s3_endpoint(url: &Url, preview: Preview) -> bool {
7171
if let Some(s3_endpoint_realm) = S3_ENDPOINT_REALM.as_ref().map(RealmRef::from) {
7272
if !preview.is_enabled(PreviewFeatures::S3_ENDPOINT) {
7373
warn_user_once!(
@@ -79,19 +79,26 @@ impl S3EndpointProvider {
7979
// Treat any URL on the same domain or subdomain as available for S3 signing.
8080
let realm = RealmRef::from(url);
8181
if realm == s3_endpoint_realm || realm.is_subdomain_of(s3_endpoint_realm) {
82-
// TODO(charlie): Can `reqsign` infer the region for us? Profiles, for example,
83-
// often have a region set already.
84-
let region = std::env::var(EnvVars::AWS_REGION)
85-
.map(Cow::Owned)
86-
.unwrap_or_else(|_| {
87-
std::env::var(EnvVars::AWS_DEFAULT_REGION)
88-
.map(Cow::Owned)
89-
.unwrap_or_else(|_| Cow::Borrowed("us-east-1"))
90-
});
91-
let signer = reqsign::aws::default_signer("s3", &region);
92-
return Some(signer);
82+
return true;
9383
}
9484
}
95-
None
85+
false
86+
}
87+
88+
/// Creates a new S3 signer with the configured region.
89+
///
90+
/// This is potentially expensive as it may invoke credential helpers, so the result
91+
/// should be cached.
92+
pub(crate) fn create_signer() -> DefaultSigner {
93+
// TODO(charlie): Can `reqsign` infer the region for us? Profiles, for example,
94+
// often have a region set already.
95+
let region = std::env::var(EnvVars::AWS_REGION)
96+
.map(Cow::Owned)
97+
.unwrap_or_else(|_| {
98+
std::env::var(EnvVars::AWS_DEFAULT_REGION)
99+
.map(Cow::Owned)
100+
.unwrap_or_else(|_| Cow::Borrowed("us-east-1"))
101+
});
102+
reqsign::aws::default_signer("s3", &region)
96103
}
97104
}

0 commit comments

Comments
 (0)