Skip to content

feat(io): refresh vended storage credentials per prefix - #892

Merged
wgtmac merged 7 commits into
apache:mainfrom
plusplusjiajia:feat-vended-credential-refresh
Oct 17, 2026
Merged

wgtmac merged 7 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 can expire while an ArrowS3FileIO instance and its
file handles are still in use. This change refreshes them on demand and keeps
the refresh state scoped to each storage prefix.

The implementation:

  • Keeps one CredentialStore for credential snapshots, per-prefix slots,
    expiry state, refresh coordination, backoff, and generation checks.
  • Resolves each credential once into its normalized prefix, effective client
    properties, client settings, and AWS credentials.
  • Refreshes credentials independently by prefix. A response may update the
    prefixes that are still active; invalid, expired, missing, or incompatible
    entries keep their previous state.
  • Preserves S3-compatible schemes during routing. For example, s3:// and
    oss:// remain separate prefixes so credentials for one endpoint cannot be
    sent to another.
  • Lets existing AWS SDK clients pick up refreshed credentials through
    AWSCredentialsProvider, while keeping clients and handles safe when the
    FileIO or a credential generation is replaced.
  • Uses the REST client connection and request timeout properties for all HTTP
    methods. Transport failures during table creation or commit are reported as
    CommitStateUnknown because the server may already have applied the write.

The change removes the previous global expiry/layout bookkeeping and duplicate
credential-property parsing, making the refresh path easier to maintain and
extend while preserving the existing StorageCredentialProvider API.

Tests cover prefix routing, refresh timing and concurrency, failed and invalid
refreshes, reordered responses, expiry handling, REST client timeouts, and
real S3-compatible round trips.

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

Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
@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?

Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc Outdated
Comment thread src/iceberg/catalog/rest/http_client.cc Outdated
@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.

Comment thread src/iceberg/arrow/s3/arrow_s3_file_io.cc
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
Manage vended credentials independently by storage prefix, preserve scheme-specific routing, and simplify credential resolution and refresh state.\n\nApply REST client connection and request timeouts consistently, and report transport failures during table writes as unknown commit states.

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

Thanks for the continued optimization on this PR. I pushed one cleanup commit after another review of the implementation against the Java behavior and REST spec.
The goal of this change is to make the design simpler and easier to maintain. In particular, I:

  • consolidated credential resolution and per-prefix refresh state in CredentialStore;
  • removed the duplicated layout and global expiry tracking;
  • kept different S3-compatible schemes routed independently;
  • reused the REST client timeout settings for all HTTP methods;
  • handled transport failures during commits as CommitStateUnknown.

These changes are intended to preserve the current behavior while reducing duplicated state and making future extensions safer. I also cross-checked the design with several AI-assisted review suggestions, Java implementation details, and the REST spec. Thanks again for continuously refining the PR!

@wgtmac wgtmac changed the title feat(io): refresh vended storage credentials before they expire feat(io): refresh vended storage credentials per prefix Oct 17, 2026
@wgtmac
wgtmac merged commit 72fbb5a into apache:main Oct 17, 2026
15 checks passed
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