Skip to content

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

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

plusplusjiajia wants to merge 4 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.
  • Each vended S3 client signs through a VendedCredentialsProvider (an Aws::Auth::AWSCredentialsProvider set as Arrow's S3Options::credentials_provider), which the AWS SDK asks before every request. It returns the keys the FileIO currently vends for the client's prefix and refreshes them once due, as Java's VendedCredentialsProvider does, so handles and streams opened before a refresh use the new keys. Only keys vended with the settings a client was built with reach it, so keys meant for another endpoint are never sent to an old one. A client asks only once its own keys are installed: the SDK also asks while building it, when the FileIO still holds the keys being replaced.
  • A refresh may only rotate keys: it must keep the S3 prefixes, their settings and which of them sign with keys. It therefore never builds clients, so a request can run one, or wait for one in flight once its keys have expired. (Arrow holds its client registry for the duration of a request, and building a client locks it exclusively.)
  • A refresh is installed only if it keeps that layout and every session token has a parseable expiry that has not passed. Failed or rejected refreshes keep the current credentials and back off for up to 30 seconds. Concurrent callers share one refresh; callers with expired credentials wait up to 10 seconds for one in flight. A concurrent credential install supersedes any older refresh result.
  • Once a prefix's credentials have expired and cannot be refreshed, creating a handle or deleting under it fails with TokenExpired; a batch delete resolves each file as a single delete does. A request on a handle opened earlier can only sign with the expired keys, which S3 rejects with ExpiredToken: the SDK gives a credentials provider no way to report an error, and sends a request with empty keys unsigned.
  • HttpClient::Get takes a timeout; the REST credentials request uses 10 seconds. A GET whose transfer fails after a successful status, such as a body that stalls past the timeout, is an IOError rather than a success. Writes keep their status, since one may have taken effect: failing a commit outright would clean up its files.
  • The bundle links aws-cpp-sdk-core when S3 is enabled. An installed Arrow does not export the SDK it bundles, so with one the system SDK is used.

Tests cover refresh timing, concurrency, unusable, failed and layout-changing replacements, reordered prefixes, expiry errors, provider initialization, HTTP timeouts and truncated responses, plus real S3 round trips: through REST FileIO and ResolvingFileIO, reading through a handle opened before a refresh (and not taking keys installed with other settings), reading through a handle after its FileIO is released, and a request that waits for a refresh under way once its keys have expired.

@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.

@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.

/// \brief Replace the credentials once they are close to expiring.
///
/// Called before each handle is created; a handle keeps the delegate it was
/// built from, so I/O on an open one is not re-checked. A failure keeps the

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.

Can we separate credential refresh from filesystem replacement? Existing handles and streams keep the old token, even after FileIO refreshes. A refreshable provider on the S3 client, as in Java, would cover those requests and avoid rebuilding clients for token-only changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. Each vended S3 client now signs through a VendedCredentialsProvider set as Arrow's S3Options::credentials_provider. The AWS SDK asks it before every request: it returns the keys the FileIO currently vends for the client's prefix and refreshes them once due, so existing handles and streams pick up new tokens. A token-only refresh keeps the existing clients; only a change of prefixes or settings builds new ones. One caveat: Arrow holds its client registry while a request runs, and building a client locks it exclusively, so a refresh started from a request that needs new clients is left to the next file operation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Update: a refresh now only rotates keys, so the caveat above no longer applies.

}
// Expired credentials leave nothing to proceed with, so wait instead.
if (!refresh_lock.try_lock_for(kExpiredCredentialWait)) {
return;

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.

If this wait times out, the caller still uses credentials we know are expired. A failed refresh does the same. Return an error after expiry instead of creating a handle with unusable credentials.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. Each prefix's delegate now tracks its own expiry. Once it has passed and the refresh failed or the wait timed out, creating a handle or deleting under that prefix returns TokenExpired. Locations outside vended prefixes keep the default credentials.


// `delegates`/`*refreshed` take the retired generation; both are declared
// before the lock, so it destructs only after the lock releases.
InstallCredentials(*refreshed, delegates);

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.

This installs a returned token even if it is already expired or its expiry is missing or invalid. It can replace a still-valid token and then suppress retries for 30 seconds. Validate temporary credentials before installing them and keep the old set on failure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. A refreshed set is installed only if every S3 credential is usable: a session token needs a parseable expiry that has not passed, and the set must contain an S3 prefix. Otherwise the current set is kept and the refresh backs off as for any failed refresh, never past the current expiry. A token without a usable expiry at the initial install still triggers an immediate refresh, but is no longer treated as expired.

// Outside `mutex_`: both are slow and must not block readers.
Status status;
DelegatesByPrefix delegates;
auto refreshed = provider->Load();

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.

The 10-second limit only covers waiting for the refresh lock. The thread running Load() can block indefinitely in the REST request, even while the old credentials are still valid. Add a timeout to the refresh request itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. HttpClient::Get takes an optional timeout, and RestStorageCredentialProvider::Load() uses 10 seconds, so a stalled catalog no longer blocks the refreshing operation indefinitely; a timeout counts as a failed refresh.

@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch from a1cf5ef to 6f8cfe1 Compare October 13, 2026 07:46
@wgtmac

wgtmac commented Oct 14, 2026

Copy link
Copy Markdown
Member

Could you rebase onto main and resolve the conflicts?

if (!refresh_lock.try_lock()) {
// Another operation is already fetching; normally just use what we have.
if (from_request) {
return;

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.

This also skips the refresh when the token has expired. Another request on an open stream then signs with that expired token. Can we wait for a request-safe credential refresh, or fail explicitly instead of returning expired keys?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. A refresh now only rotates keys and never builds clients, so a request with expired keys waits up to 10s for one in flight. If that fails, the provider can't fail the request (the SDK sends empty keys unsigned), so S3 rejects the expired keys with ExpiredToken; new handles and deletes fail with TokenExpired.

}
auto result = delegates;
for (size_t i = 0; i < result.size(); ++i) {
if (!result[i].vended_keys || !DiffersOnlyInKeys(*current_s3[i], *refreshed_s3[i])) {

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.

Can we match these credentials by prefix instead of array position? The server may return the same prefixes in a different order. That makes a key-only refresh look like a client change, so an existing stream keeps its old token.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. Refreshed credentials are now matched by canonical prefix, so a reordered response is a key-only refresh. One that adds or drops a prefix or changes a setting is rejected.

Comment thread src/iceberg/catalog/rest/http_client.cc Outdated
return {};
}
// No response to hand the error handler.
if (response.error.code == cpr::ErrorCode::OPERATION_TIMEDOUT) {

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.

Check the transport error before accepting the HTTP status. A server can send 200 headers and then stall during the body. CPR keeps status 200 when the transfer times out, so this returns success instead of an IOError.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Done. Get checks transport errors first, so a timeout or a broken transfer after a 2xx is an IOError. I left POST as is: a commit may already have succeeded, and an error would clean up its files.

@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch from 6f8cfe1 to b6990f9 Compare October 14, 2026 11:00
@plusplusjiajia

Copy link
Copy Markdown
Member Author

@wgtmac Thanks! Rebased onto main. The first commit is the previous one rebased; the second addresses this round. Since #783 made Get virtual, the timeout is a parameter without a default, so callers and the two mocks pass it. A refresh now only rotates keys, which keeps it simple and safe to run from a request.

std::lock_guard lock(mutex_);
if (current.has_value()) {
credentials_ = std::move(current).value();
}

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.

The AWS signer calls this provider while the new client is being built, before SetStorageCredentials installs the new keys. This replaces the initial keys with the old ones. If the FileIO is released before the handle's first request, that handle keeps the old keys; can we prevent this cache rollback?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Fixed. A client now asks the FileIO for keys only after its own install, so the signer's warm-up while building it keeps the keys it was built with. Added HandleKeepsItsKeysOnceTheFileIOIsGone, which reads through such a handle after the FileIO is released.

Vended storage credentials expire, so new file operations on a reused
table could eventually fail. ArrowS3FileIO now refreshes them through
the StorageCredentialProvider installed by InitializeStorageCredentials,
starting five minutes before the earliest applicable
s3.session-token-expires-at-ms. Later SetStorageCredentials calls keep
the provider.

Each vended S3 client signs through a VendedCredentialsProvider, which
the AWS SDK asks before every request: it refreshes once due and returns
the keys the FileIO currently vends for the client's prefix, as Java's
VendedCredentialsProvider does. Handles and streams opened before a
refresh therefore use the new keys, including those on clients a later
refresh replaced. A refresh that changes only keys keeps the existing
clients.

A request cannot build clients, since Arrow holds its client registry
for the request and building one locks it exclusively. A refresh it
starts that needs new clients is left to the next file operation, and
it never waits on another refresh.

A refresh is installed only if every S3 credential is usable: a session
token must carry a parseable expiry that has not passed. Otherwise, or
if loading fails, the current set is kept and the refresh backs off for
up to 30 seconds. Concurrent operations share one refresh, and
operations with expired credentials wait up to 10 seconds for one in
flight. A concurrent credential install supersedes any older refresh
result.

Once a delegate's credentials have expired and could not be refreshed,
creating a handle or deleting under its prefix fails with TokenExpired
instead of handing out credentials known to be unusable.

The LoadCredentials request had no deadline, so a stalled catalog could
block a file operation indefinitely. HttpClient::Get takes an optional
timeout, after which the request fails with an IOError, and
RestStorageCredentialProvider uses 10 seconds.

The bundle now links aws-cpp-sdk-core when S3 is enabled.
A refresh may now only rotate keys: refreshed credentials must keep the
S3 prefixes, matched by canonical prefix in any order, their settings
and which of them sign with keys. It therefore never builds S3 clients,
and a request whose keys have expired waits up to 10 seconds for a
refresh in flight, as file operations do, instead of signing with the
expired keys. A response that changes that layout is rejected like any
unusable refresh, which drops the deferred client rebuild.

Only keys vended with the settings a client was built with reach it, so
keys meant for another endpoint are never sent to an old one. Keys,
settings and token expiries come from the vended config merged over the
FileIO's properties, and only a token that signs requests is tracked.
For a repeated prefix, the first credential is used throughout.

Expiries are tracked per prefix from the installed credentials, and a
batch delete resolves each file as a single delete does, so a long
batch follows refreshes.

HttpClient::Get checks the transfer before the status: a timeout, or a
transport error after a successful status, is an IOError. Writes keep
their status, since one may have taken effect: failing a commit
outright would clean up its files.

An installed Arrow does not export the AWS SDK it bundles, so with one
the system SDK is used for the S3 FileIO's credentials provider.
A response keeps its curl handle, which is attached to the client's
connection pool. Destroying the client first left that handle pointing
at a freed pool, so the POST test crashed when the response went away.
The AWS signer asks the credentials provider while the client is built,
before SetStorageCredentials installs its keys, so the provider cached
the keys being replaced. A handle whose FileIO was released before its
first request then signed with them. A client now asks the FileIO for
keys only once its own are installed.
@plusplusjiajia
plusplusjiajia force-pushed the feat-vended-credential-refresh branch from 13e8736 to 6972755 Compare October 15, 2026 09:25

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