Skip to content

[1/6] feat: add refreshable storage credential providers - #2976

Open
zakariya-s wants to merge 1 commit into
apache:mainfrom
zakariya-s:feat/storage-credential-provider
Open

[1/6] feat: add refreshable storage credential providers#2976
zakariya-s wants to merge 1 commit into
apache:mainfrom
zakariya-s:feat/storage-credential-provider

Conversation

@zakariya-s

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

This PR introduces the core API for supplying refreshable storage credentials:

  • Adds StorageCredentialProvider and backend-specific credential types for S3 and GCS.
  • Propagates an optional credential provider through FileIOBuilder and FileIO.
  • Adds StorageFactory::build_with_credentials.
  • Adds the standard S3 and GCS credential refresh property constants.

Backend integration and REST credential refreshing will be added in subsequent PRs.

Are these changes tested?

Yes.

AI Disclosure

AI helped splitting this from the reference PR.

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! I'm not a committer but I did recently reinvent basically this to get credential refresh working for our catalog and this API would make our lives much easier.

I don't have any notes here...this entrypoint work work for our code because at the point we construct the FileIO we also have access to our client object that can refresh credentials. The credential values are redacted in the Debug implementation and they're not serializable. The PR obviously needs a few things to go from good idea -> useful but I also think the attempt to minimize the scope is great too.

@zakariya-s

Copy link
Copy Markdown
Contributor Author

Hey @paleolimbot, great to hear this will be useful! We also have to do workarounds at the moment to get this working for our DataFusion clients, and I've also heard that others are doing similar stuff in their internal forks of iceberg-rust.

Like you mention, this PR doesn't quite implement the entire E2E refreshing, but I do plan to split it into a few PRs to ease the review burden. Feel free to take a look at the full draft PR (#2932) which does implement it E2E and gives an indicator of how it should roughly look like when done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants