zakariya-s commented on code in PR #2976:
URL: https://github.com/apache/iceberg-rust/pull/2976#discussion_r4376716142
##########
crates/iceberg/src/io/file_io.rs:
##########
@@ -123,14 +146,38 @@ impl FileIO {
///
/// All storage configuration properties are included in the serialized
representation. These
/// properties may contain credentials or other sensitive values, so the
returned bytes must be
- /// protected in transit and at rest by the application embedding this
crate.
+ /// protected in transit and at rest by the application embedding this
crate. A serialized
+ /// credential provider may likewise carry catalog authentication and
vended credentials.
///
/// Storage factories are serialized through
[`typetag`](https://docs.rs/typetag). Third-party
/// factories must use `#[typetag::serde]` on their [`StorageFactory`]
implementation.
+ ///
+ /// A credential provider is serialized as the
+ /// [`StorageCredentialProviderFactory`] returned by
+ /// [`StorageCredentialProvider::factory`], and rebuilt on
deserialization. A provider that
+ /// cannot be rebuilt in another process is left out with a warning, as
with
+ /// [`FileIO::without_credential_provider`]: the deserialized `FileIO`
uses the credentials in
+ /// its configuration, which are not refreshed. Storage factories that
hold their own
+ /// credential sources, such as a custom AWS credential loader, still fail
to serialize, as
+ /// leaving them out would leave no credentials at all.
pub fn serialize_all(&self) -> Result<Vec<u8>> {
+ let credential_provider =
self.credential_provider.as_ref().and_then(|provider| {
+ provider
+ .factory()
+ .and_then(|factory| Ok(serde_json::to_value(factory)?))
+ .inspect_err(|error| {
+ static WARNED: Once = Once::new();
+ warn_once_without_provider(&WARNED, || {
+ format!("serializing FileIO without its credential
provider: {error}")
+ })
+ })
+ .ok()
Review Comment:
i'm okay with either behaviour, @mbutrovich suggested to warn in the
reference pr:
https://github.com/apache/iceberg-rust/pull/2932#discussion_r4136994726. i do
agree that this will eventually fail for a long task once the token expires,
but unlike java we don't have reflection so we can't load AuthManager
dynamically. we could maybe make them rebuildable if we use typetag like we
have done so for providers? that should probably be in a follow-up PR though.
for now should i make this fail?
--
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]
