feat(rest): Support refreshing vended storage credentials - #2932
feat(rest): Support refreshing vended storage credentials#2932zakariya-s wants to merge 2 commits into
Conversation
| /// Disable header redaction in error logs (defaults to false for security) | ||
| pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str = "disable-header-redaction"; | ||
| /// Identifier for a server-side scan plan associated with credential requests. | ||
| pub const REST_CATALOG_PROP_SCAN_PLAN_ID: &str = "rest.scan.plan-id"; |
There was a problem hiding this comment.
Server-side planning isn't supported yet so this will always be empty anyway. I don't think it's a problem to keep it until it eventually is supported
| let config = response | ||
| .config | ||
| .into_iter() | ||
| .chain(self.user_config.props.clone()) | ||
| .collect(); | ||
| let file_io = self | ||
| .load_file_io(Some(metadata_location), Some(config)) | ||
| .await?; |
There was a problem hiding this comment.
Drive-by fix since this is inconsistent with create_table() and load_table()
| [ | ||
| "auth", | ||
| "token", | ||
| "secret", | ||
| "key", | ||
| "password", | ||
| "cookie", | ||
| "credential", | ||
| ] |
There was a problem hiding this comment.
Not sure if we want to do it like this since I guess there could technically be other headers that would be made sensitive. The main change I wanted to do was also make x-client-secret and x-client-credential sensitive. I think Java redacts every header anyway, so I'm not sure if this really matters
| http = { workspace = true } | ||
| iceberg = { workspace = true } | ||
| itertools = { workspace = true } | ||
| rand = { workspace = true } |
There was a problem hiding this comment.
Not sure if we're okay with adding a new dep. It's only added for the jittering functionality when retrying
| /// only re-fetch when the current credential is at or near expiry; otherwise | ||
| /// every object-store request would trigger a call back to the catalog. | ||
| #[async_trait] | ||
| pub trait StorageCredentialProvider: Debug + Send + Sync { |
There was a problem hiding this comment.
This is probably one of the most important additions in the PR but it doesn't really follow Java since Java doesn't have an interface over vended credential providers. I think it's probably fine and nicer like this, especially since OpenDAL abstracts everything away quite nicely
| if let Some(no_auth) = m.remove(GCS_NO_AUTH) | ||
| && is_truthy(no_auth.to_lowercase().as_str()) | ||
| { |
There was a problem hiding this comment.
Drive-by fix since this looked pretty bad. AWS did this correctly, but GCS would disable this even if gcs.no-auth was set to true
| let url = url::Url::parse(path).map_err(|e| { | ||
| Error::new( | ||
| ErrorKind::DataInvalid, | ||
| format!("Invalid gcs url: {path}: {e}"), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
S3 had this validation before above but GCS didn't, so another drive-by fix
| if self.uses_dynamic_credentials(&path) { | ||
| let (op, relative_path) = self.create_operator(&path)?; | ||
| op.delete(relative_path).await.map_err(from_opendal_error)?; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This will probably need to be looked at again later since we forgo batching for paths with a vended credential provider. We would probably have to batch by path-prefix to make this work later. Happy to do this later in this PR or in a new PR
| /// `reqsign` [`Timestamp`](reqsign_core::time::Timestamp) used on backend | ||
| /// credential types (e.g. `AwsCredential::expires_in`, `google::Token::expires_at`). | ||
| #[cfg(any(feature = "opendal-s3", feature = "opendal-gcs"))] | ||
| pub(crate) fn system_time_to_timestamp( |
There was a problem hiding this comment.
Didn't want to rely on directly on reqsign's Timestamp, but I'm happy to hear different opinions
| /// It contains the location schemes it backs, the property keys it is configured | ||
| /// with, and how to parse its credential. The generic provider stays free of | ||
| /// any per-cloud knowledge. | ||
| struct CloudRefresh { |
There was a problem hiding this comment.
This could also be made into a trait I guess, and we could split aws and gcp support into different modules, but I thought it was small enough to keep it as a struct and make consts for the different cloud providers
| schemes: &["gs", "gcs"], | ||
| endpoint_key: GCS_REFRESH_CREDENTIALS_ENDPOINT, | ||
| enabled_key: GCS_REFRESH_CREDENTIALS_ENABLED, | ||
| jitter_prefetch: false, |
There was a problem hiding this comment.
GCP doesn't have jitter for Java as it's SDK dependent, which only AWS does it appears. I'm happy to discuss more about the retry strategy here in order to make it consistent (which Java doesn't have at the moment)
| } | ||
|
|
||
| /// Resolve a possibly-relative refresh endpoint against the catalog base URI. | ||
| fn resolve_endpoint(base_uri: &str, endpoint: &str) -> String { |
There was a problem hiding this comment.
The behaviour here was done to match Java
1 participant
Which issue does this PR close?
What changes are included in this PR?
This PR adds support for refreshing short-lived storage credentials vended by REST catalogs:
StorageCredentialProviderinterface toFileIOheader.*properties for refresh requestsDebugoutputAzure credential refresh is not included because the current OpenDAL Azure backend does not expose the credential-provider and expiry hooks required for safe refresh.
Are these changes tested?
Yes