Skip to content

feat(io): refresh vended storage credentials before they expire - #892

Open
plusplusjiajia wants to merge 3 commits into
apache:mainfrom
plusplusjiajia:feat-vended-credential-refresh
Open

plusplusjiajia wants to merge 3 commits into
apache:mainfrom
plusplusjiajia:feat-vended-credential-refresh

Conversation

@plusplusjiajia

@plusplusjiajia plusplusjiajia commented Aug 19, 2026 •

Copy link
Copy Markdown
Member

Vended storage credentials expire, so new file operations on a reused table can eventually fail. ArrowS3FileIO now refreshes them on demand, starting five minutes before the earliest applicable s3.session-token-expires-at-ms.

Builds on the StorageCredentialProvider API merged in #899:

  • InitializeStorageCredentials() installs the initial credentials and provider before first use. Later SetStorageCredentials() calls retain the provider.
  • File operations call StorageCredentialProvider::Load() when refresh is due. REST fetching stays in the REST provider; no background thread is added.
  • Failed or unusable replacements keep the current credentials and back off for up to 30 seconds. Concurrent operations share one refresh; operations with expired credentials wait up to 10 seconds for an in-flight refresh.
  • S3 clients are rebuilt outside the credential lock. A concurrent credential install supersedes any older refresh result.

Tests cover refresh timing, concurrency, failed replacements, and provider initialization, including a real S3 read/write round trip through REST FileIO and ResolvingFileIO.

@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch 4 times, most recently from d92ef4d to 7088aed Compare August 21, 2026 15:56
@wgtmac

wgtmac commented Aug 23, 2026

Copy link
Copy Markdown
Member

Thanks for adding this feature! I think it is too large to review which may incur a long delay. Perhaps let's split it into smaller ones so we can review it one by one?

@plusplusjiajia

plusplusjiajia commented Aug 24, 2026 •

Copy link
Copy Markdown
Member Author

Thanks for adding this feature! I think it is too large to review which may incur a long delay. Perhaps let's split it into smaller ones so we can review it one by one?

@wgtmac Thanks — split into a stack, smallest first:

  1. test(io): stop probing the EC2 metadata service in S3 tests #897 — S3 tests stop probing the EC2 metadata service (54s → 0.3s).
  2. refactor(io): hand out S3 delegates by shared_ptr and build them off the lock #898 — ArrowS3FileIO gets refactor(io): make FileIO resolution registry-driven #889's concurrency treatment. No behavior change.
  3. feat(rest): load table credentials from the LoadCredentials endpoint #899 (draft until refactor(io): hand out S3 delegates by shared_ptr and build them off the lock #898 merges) — LoadCredentials plumbing, not yet invoked.
  4. This PR shrinks to just the refresh policy.

Each builds and passes the full suite standalone; the diff here narrows as each one merges.

@plusplusjiajia
plusplusjiajia marked this pull request as draft August 24, 2026 09:56
@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch from 7088aed to b06186d Compare August 24, 2026 10:49
@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch 2 times, most recently from afe009b to c15c6e3 Compare September 15, 2026 06:37
@wgtmac

wgtmac commented Sep 24, 2026

Copy link
Copy Markdown
Member

Is this ready for review?

@plusplusjiajia

Copy link
Copy Markdown
Member Author

Is this ready for review?

@wgtmac Thanks! Not yet. #899 is ready for review now, and this one is stacked on it, so I'll mark it ready once #899 lands.

Adapt the refresh policy to the provider API merged in apache#899. Install
initial credentials and the provider together, and keep the provider
across later credential updates.

Migrate the refresh tests and cover failed initialization and the
REST-to-S3 refresh path with real object storage.

AI-Model: gpt-6
AI-Contributed/Feature: 40/40
AI-Contributed/UT: 248/248
@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch from f0f0f83 to a1cf5ef Compare October 9, 2026 05:29
@plusplusjiajia
plusplusjiajia marked this pull request as ready for review October 9, 2026 05:29
@plusplusjiajia

Copy link
Copy Markdown
Member Author

@wgtmac Thanks for the provider refactor in #899! Rebased on main and adapted the refresh policy and tests to StorageCredentialProvider and InitializeStorageCredentials. Ready for review.

This branch has not been deployed

No deployments
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