zakariya-s commented on code in PR #2932:
URL: https://github.com/apache/iceberg-rust/pull/2932#discussion_r4185367533
##########
crates/iceberg/src/io/storage/mod.rs:
##########
@@ -139,4 +140,358 @@ pub trait StorageFactory: Debug + Send + Sync {
/// A `Result` containing an `Arc<dyn Storage>` on success, or an error
/// if the storage could not be created.
fn build(&self, config: &StorageConfig) -> Result<Arc<dyn Storage>>;
+
+ /// Build a new Storage instance, optionally supplying a credential
provider
+ /// that the backend can call to obtain and refresh short-lived
credentials.
+ ///
+ /// Backends that cannot use the provider ignore it and use the credentials
+ /// in `config`, as they would without one. The default does exactly that.
+ #[allow(unused_variables)]
+ fn build_with_credentials(
+ &self,
+ config: &StorageConfig,
+ credential_provider: Option<Arc<dyn StorageCredentialProvider>>,
+ ) -> Result<Arc<dyn Storage>> {
+ self.build(config)
+ }
+}
+
+/// Supplies fresh, backend-specific storage credentials on demand.
+///
+/// A catalog that vends temporary credentials implements this trait so that
+/// storage backends can re-fetch credentials as they approach expiry instead
+/// of failing once the initial token's TTL runs out.
+///
+/// # Caching
+///
+/// [`load_credential`](Self::load_credential) may be called very frequently.
+/// Implementations must cache internally and only re-fetch when the current
+/// credential is at or near expiry; otherwise every object-store request could
+/// trigger a call back to the catalog.
+#[async_trait]
+pub trait StorageCredentialProvider: Debug + Send + Sync {
+ /// Return whether this provider has refresh configuration for `path`.
+ ///
+ /// Backends use this before replacing their normal credential chain. The
+ /// default is `true` for single-backend providers; multi-backend providers
+ /// should return `false` for schemes they do not configure.
+ fn supports_path(&self, _path: &str) -> bool {
+ true
+ }
+
+ /// Load a fresh credential for the storage location identified by `path`.
+ ///
+ /// `path` is the absolute location being accessed (e.g.
+ /// `s3://bucket/warehouse/db/table/...`). Providers that vend distinct
+ /// credentials per location prefix use it to select the most specific
+ /// match. When the selected credential has a declared
+ /// [`StorageCredential::prefix`], it must
[cover](StorageCredential::covers) `path`.
+ async fn load_credential(&self, path: &str) -> Result<StorageCredential>;
+
+ /// Return a factory that rebuilds an equivalent provider in another
process.
+ ///
+ /// [`FileIO::serialize_all`](crate::io::FileIO::serialize_all) serializes
this
+ /// factory in place of the provider. The default reports that the provider
+ /// cannot be serialized.
+ fn factory(&self) -> Result<Arc<dyn StorageCredentialProviderFactory>> {
+ Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "storage credential provider cannot be serialized",
+ ))
+ }
+}
+
+/// Serializable recipe that rebuilds a [`StorageCredentialProvider`] after
+/// [`FileIO`](crate::io::FileIO) deserialization.
+///
+/// Factories are serialized through [`typetag`](https://docs.rs/typetag), so
+/// implementations must use `#[typetag::serde]`, and the receiving binary must
+/// link the concrete implementation.
+#[typetag::serde(tag = "type")]
+pub trait StorageCredentialProviderFactory: Debug + Send + Sync {
+ /// Build a provider for a `FileIO` with the given storage configuration.
+ fn build(&self, config: &StorageConfig) -> Result<Arc<dyn
StorageCredentialProvider>>;
+}
+
+/// A vended storage credential together with its scope and expiry.
+#[derive(Clone, Debug)]
+pub struct StorageCredential {
+ /// Storage-location prefix this credential is scoped to. `None`
represents a
+ /// credential without a declared scope, sourced from flat storage
properties.
+ prefix: Option<String>,
+ /// The backend-specific credential material.
+ kind: StorageCredentialKind,
+ /// When the credential expires, if known. `None` means non-expiring and
+ /// backends treat such a credential as always valid and never refresh it.
+ expires_at: Option<SystemTime>,
+}
+
+impl StorageCredential {
+ /// Create a storage credential with no declared scope or expiration.
+ pub fn new(kind: StorageCredentialKind) -> Self {
+ Self {
+ prefix: None,
+ kind,
+ expires_at: None,
+ }
+ }
+
+ /// Set the storage-location prefix this credential is scoped to.
+ pub fn with_prefix(mut self, prefix: impl Into<String>) -> Self {
+ self.prefix = Some(prefix.into());
+ self
+ }
+
+ /// Set when this credential expires.
+ pub fn with_expiration(mut self, expires_at: SystemTime) -> Self {
+ self.expires_at = Some(expires_at);
+ self
+ }
+
+ /// Return the storage-location prefix this credential is scoped to.
+ pub fn prefix(&self) -> Option<&str> {
+ self.prefix.as_deref()
+ }
+
+ /// Return whether this credential applies to `location`.
+ ///
+ /// A credential without a prefix covers every location. Otherwise the
+ /// prefix must match whole path segments of `location`, and scheme
+ /// aliases (`s3a`/`s3n` for `s3`, `gcs` for `gs`, and the plain-text
+ /// Azure schemes for their TLS variants) are treated as equal. A prefix
+ /// that is only a scheme, such as `s3`, covers every location with that
+ /// scheme.
+ pub fn covers(&self, location: &str) -> bool {
+ self.prefix
+ .as_deref()
+ .is_none_or(|prefix| storage_prefix_covers(prefix, location))
+ }
+
+ /// Return the backend-specific credential material.
+ pub fn kind(&self) -> &StorageCredentialKind {
+ &self.kind
+ }
+
+ /// Consume this credential and return its backend-specific material.
+ pub fn into_kind(self) -> StorageCredentialKind {
+ self.kind
+ }
+
+ /// Return when this credential expires.
+ pub fn expires_at(&self) -> Option<SystemTime> {
+ self.expires_at
+ }
+}
+
+/// Return whether the storage-location `prefix` covers `location`, with the
+/// matching rules of [`StorageCredential::covers`].
+pub fn storage_prefix_covers(prefix: &str, location: &str) -> bool {
+ let Some((location_scheme, location_rest)) = location.split_once("://")
else {
+ return false;
+ };
+ let Some((prefix_scheme, prefix_rest)) = prefix.split_once("://") else {
+ return !prefix.is_empty() && canonical_scheme(prefix) ==
canonical_scheme(location_scheme);
+ };
+
+ canonical_scheme(prefix_scheme) == canonical_scheme(location_scheme)
+ && location_rest
+ .strip_prefix(prefix_rest)
+ .is_some_and(|remainder| {
+ prefix_rest.is_empty()
+ || prefix_rest.ends_with('/')
+ || remainder.is_empty()
+ || remainder.starts_with('/')
+ })
+}
Review Comment:
This was intentional as whilst the spec doesn't mention segment boundaries,
I thought it was in the spirit of it. If we want to follow Java's semantics
(which is a good enough reason by itself), then I'd be happy to change this.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
