From ec6324409d6c0e9ddcda5d110ac570ca79fe9e38 Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:02:34 +0200 Subject: [PATCH] fix(s3): support instance-policy credentials in object storage tests (#10238) * fix(s3): cache ambient aws credentials and surface credential chain errors Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SCczeVBDhDnLXWqxPWw6od * fix(frontend): clarify that object storage connection tests run on the server Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SCczeVBDhDnLXWqxPWw6od * fix: render test connection tooltip in popup and harden cache test Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SCczeVBDhDnLXWqxPWw6od * better doc * fix(frontend): correct tooltip wording, tests run from the executing worker Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SCczeVBDhDnLXWqxPWw6od * fix(s3): single-flight ambient credential provider creation * fix(frontend): clarify object storage tests run on the server process * fix(frontend): make instance object storage test tooltip provider-neutral --------- Co-authored-by: Claude Fable 5 Co-authored-by: Ruben Fiszel --- backend/windmill-api-settings/src/lib.rs | 14 +- backend/windmill-object-store/src/lib.rs | 136 ++++++++++++++++-- .../src/lib/components/TestConnection.svelte | 48 ++++--- 3 files changed, 165 insertions(+), 33 deletions(-) diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 9d37233228..e02630b104 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -283,15 +283,15 @@ pub async fn test_s3_bucket( let mut list = client.list(Some( &windmill_object_store::object_store_reexports::Path::from("".to_string()), )); - let first_file = list.next().await; - if first_file.is_some() { - if let Err(e) = first_file.as_ref().unwrap() { + match list.next().await { + Some(Err(e)) => { tracing::error!("error listing bucket: {e:#}"); - error::Error::internal_err(format!("Failed to list files in blob storage: {e:#}")); + return Err(error::Error::internal_err(format!( + "Failed to list files in blob storage: {e:#}" + ))); } - tracing::info!("Listed files: {:?}", first_file.unwrap()); - } else { - tracing::info!("No files in blob storage"); + Some(Ok(first_file)) => tracing::info!("Listed files: {:?}", first_file), + None => tracing::info!("No files in blob storage"), } let path = windmill_object_store::object_store_reexports::Path::from(format!( diff --git a/backend/windmill-object-store/src/lib.rs b/backend/windmill-object-store/src/lib.rs index 553eb69917..9121f6183d 100644 --- a/backend/windmill-object-store/src/lib.rs +++ b/backend/windmill-object-store/src/lib.rs @@ -361,12 +361,7 @@ pub async fn build_s3_client(s3_resource_ref: &S3Resource) -> error::Result>, +} + +#[cfg(feature = "parquet")] +impl AmbientAwsCredentials { + // Credentials without an expiry (env vars, static profile) are still re-resolved + // periodically so runtime changes to the environment are eventually picked up. + const NO_EXPIRY_TTL: std::time::Duration = std::time::Duration::from_secs(300); + const EXPIRY_MARGIN: std::time::Duration = std::time::Duration::from_secs(120); + + fn still_valid(creds: &aws_sdk_sts::config::Credentials, age: std::time::Duration) -> bool { + match creds.expiry() { + Some(expiry) => std::time::SystemTime::now() + Self::EXPIRY_MARGIN < expiry, + None => age < Self::NO_EXPIRY_TTL, + } + } + + async fn get(&self) -> anyhow::Result { + if let Some((creds, fetched_at)) = self.cached.read().await.as_ref() { + if Self::still_valid(creds, fetched_at.elapsed()) { + return Ok(creds.clone()); + } + } + // The write lock is held across the chain resolution so concurrent requests don't all + // hit the metadata service at once. + let mut guard = self.cached.write().await; + if let Some((creds, fetched_at)) = guard.as_ref() { + if Self::still_valid(creds, fetched_at.elapsed()) { + return Ok(creds.clone()); + } + } + let creds = self.chain.provide_credentials().await.map_err(|e| { + anyhow::anyhow!( + "no S3 access key/secret key is configured and no ambient AWS credentials could \ + be loaded through the AWS SDK default chain (env vars, profile, ECS/EC2 instance \ + role): {cause}. If an EC2/ECS instance role is expected to be used, the instance \ + metadata service must be reachable from the process running Windmill — on EC2 the \ + AWS Rust SDK only supports IMDSv2, so when Windmill runs in a Docker container \ + the instance metadata hop limit (HttpPutResponseHopLimit) must be at least 2", + cause = format!("{:#}", anyhow::Error::new(e)) + ) + })?; + *guard = Some((creds.clone(), std::time::Instant::now())); + Ok(creds) + } +} + +#[cfg(feature = "parquet")] +lazy_static::lazy_static! { + static ref AMBIENT_AWS_CREDS_PROVIDERS: Cache> = + Cache::new(20); +} + +#[cfg(feature = "parquet")] +async fn ambient_aws_credentials_provider(region: &str) -> Arc { + // Single-flight: concurrent cold misses for the same region must share one provider, + // otherwise each gets its own instance and their per-instance refresh locks can't serialize + // the initial credential resolution — every caller would hit the metadata service. + match AMBIENT_AWS_CREDS_PROVIDERS + .get_value_or_guard_async(region) + .await + { + Ok(provider) => provider, + Err(guard) => { + let chain = DefaultCredentialsChain::builder() + .region(Region::new(region.to_string())) + .build() + .await; + let provider = Arc::new(AmbientAwsCredentials { chain, cached: RwLock::new(None) }); + let _ = guard.insert(provider.clone()); + provider + } + } +} + #[cfg(feature = "parquet")] #[derive(Debug)] struct AwsCredentialAdapter { - pub inner: DefaultCredentialsChain, + pub inner: Arc, } #[cfg(feature = "parquet")] @@ -775,9 +852,9 @@ struct AwsCredentialAdapter { impl CredentialProvider for AwsCredentialAdapter { type Credential = AwsCredential; async fn get_credential(&self) -> object_store::Result> { - let creds = self.inner.provide_credentials().await.map_err(|e| { - tracing::error!("Error getting credentials: {:?}", e); - object_store::Error::Generic { store: "AWS", source: Box::new(e) } + let creds = self.inner.get().await.map_err(|e| { + tracing::error!("Error getting AWS credentials: {e:#}"); + object_store::Error::Generic { store: "AWS", source: e.into() } })?; Ok(Arc::new(Self::Credential { key_id: creds.access_key_id().to_string(), @@ -1481,6 +1558,45 @@ pub async fn get_logs_from_store( mod tests { use super::*; + // --- ambient credentials cache tests --- + + #[cfg(feature = "parquet")] + #[test] + fn test_ambient_credentials_still_valid() { + use std::time::{Duration, SystemTime}; + + fn creds(expiry: Option) -> aws_sdk_sts::config::Credentials { + let mut builder = aws_sdk_sts::config::Credentials::builder() + .access_key_id("AK") + .secret_access_key("SK") + .provider_name("test"); + if let Some(expiry) = expiry { + builder = builder.expiry(expiry); + } + builder.build() + } + + // Expiry far in the future: valid regardless of fetch time + assert!(AmbientAwsCredentials::still_valid( + &creds(Some(SystemTime::now() + Duration::from_secs(3600))), + Duration::ZERO + )); + // Expiry within the refresh margin: must be re-fetched + assert!(!AmbientAwsCredentials::still_valid( + &creds(Some(SystemTime::now() + Duration::from_secs(30))), + Duration::ZERO + )); + // No expiry: valid while fresh, re-fetched after the TTL + assert!(AmbientAwsCredentials::still_valid( + &creds(None), + Duration::ZERO + )); + assert!(!AmbientAwsCredentials::still_valid( + &creds(None), + AmbientAwsCredentials::NO_EXPIRY_TTL + Duration::from_secs(1) + )); + } + // --- render_endpoint tests --- #[test] diff --git a/frontend/src/lib/components/TestConnection.svelte b/frontend/src/lib/components/TestConnection.svelte index 7c59a2a324..37e3e152db 100644 --- a/frontend/src/lib/components/TestConnection.svelte +++ b/frontend/src/lib/components/TestConnection.svelte @@ -3,15 +3,16 @@ import { Database, Loader2 } from 'lucide-svelte' import Button from './common/button/Button.svelte' + import Tooltip from './meltComponents/Tooltip.svelte' import { sendUserToast } from '$lib/toast' import { workspaceStore } from '$lib/stores' import { tryEvery } from '$lib/utils' interface Props { - workspaceOverride?: string | undefined; - resourceType: string | undefined; - args?: Record | any; - buttonTextOverride?: string | undefined; + workspaceOverride?: string | undefined + resourceType: string | undefined + args?: Record | any + buttonTextOverride?: string | undefined } let { @@ -19,13 +20,15 @@ resourceType, args = {}, buttonTextOverride = undefined - }: Props = $props(); + }: Props = $props() const scripts: { [key: string]: { code: string lang: string argName: string + // Shown as an info tooltip next to the button, e.g. to clarify where the test executes + tooltip?: string additionalCheck?: (testResult: CompletedJob) => CompletedJob } } = { @@ -97,7 +100,9 @@ export async function main(s3: S3) { } `, lang: 'bun', - argName: 's3' + argName: 's3', + tooltip: + 'The storage operations of this test run on the Windmill server (the API process), not on the worker. If no access key/secret key is set, the ambient AWS credentials of the server (environment variables, instance role) are used — scripts using this resource directly through an S3 SDK resolve credentials on the worker instead, so results may differ.' }, azure_blob: { code: ` @@ -125,7 +130,9 @@ export async function main(s3: S3) { } `, lang: 'bun', - argName: 's3' + argName: 's3', + tooltip: + 'The storage operations of this test run on the Windmill server (the API process), not on the worker.' }, graphql: { code: '{ __typename }', @@ -171,7 +178,9 @@ export async function main(bucket: any) { } `, lang: 'bun', - argName: 'bucket' + argName: 'bucket', + tooltip: + "The storage operations of this test run on the Windmill server (the API process). If no credentials are configured, the server's ambient credentials for the configured provider (environment variables, instance role) are used." } } @@ -236,13 +245,20 @@ export async function main(bucket: any) { } -{#if Object.keys(scripts).includes(resourceType || '')} - + {#if scripts[resourceType].tooltip} + + {#snippet text()}{scripts[resourceType].tooltip}{/snippet} + {/if} - {buttonTextOverride ?? 'Test connection'} - + {/if}