Skip to content

SS-369 Refresh catalog-vended storage credentials - #38475

Merged
patrickwwbutler merged 2 commits into
patrick/iceberg-access-delegationfrom
patrick/iceberg-vended-creds-refresh
Aug 27, 2026
Merged

patrickwwbutler merged 2 commits into
patrick/iceberg-access-delegationfrom
patrick/iceberg-vended-creds-refresh

Conversation

@patrickwwbutler

Copy link
Copy Markdown
Contributor

Motivation

Catalog-vended credentials expire, typically within the hour, and nothing
refreshed them. loadTable hands their access keys to the FileIO once and
OpenDAL keeps signing with them until they stop working, at which point the
sink fails and only recovers by restarting the dataflow. Any sink running
longer than one credential lifetime hits this.

Description

Adds VendedCredentialLoader, a ProvideCredential implementation that
re-fetches from the catalog's loadCredentials endpoint and hands the result
to OpenDAL's S3 credential chain.

The loader caches with its own deadline rather than relying on reqsign's
cache. OpenDAL rebuilds its Operator for every file operation, so the
Signer that holds reqsign's cached credential never survives a single call;
without an internal cache this would be one catalog round trip per parquet
write and per metadata read. The deadline comes from
s3.session-token-expires-at-ms where the catalog reports one, refreshing
ahead of expiry, and from a short fixed interval where it does not. That
interval is a constant for now, with a TODO to make it a dyncfg once we know
what real catalogs report.

On a 401 or 403 the loader invalidates the catalog token so the next attempt
mints a fresh one, since nothing else on the storage path re-mints it.

Two supporting changes:

  • Materialize now constructs the OAuth2 provider itself instead of passing a
    credential catalog property, so a single token object serves both catalog
    requests and credential refreshes. The two cannot coexist: the catalog
    client rejects a custom authenticator combined with that property.
  • connect takes the table the handle will be used against, because the
    credentials endpoint is table-scoped. Callers that only prove reachability
    (connection validation, sink purification) pass None.

Installing a loader hands it sole responsibility for S3 credentials, since
OpenDAL replaces its entire provider chain and discards the static keys
parsed from the vended properties. It is therefore installed only when the
connection asked for delegation and a table is known; every other case keeps
the existing static-property path.

Verification

cargo check and cargo clippy are clean. Not yet exercised against a live
catalog: the refresh path needs a sink run long enough to cross a credential
lifetime, and which branch of the deadline logic applies depends on whether
the catalog reports an expiry.

@patrickwwbutler
patrickwwbutler force-pushed the patrick/iceberg-vended-creds-refresh branch 2 times, most recently from b0c5679 to cedc5d4 Compare August 25, 2026 20:16
@patrickwwbutler
patrickwwbutler marked this pull request as ready for review August 25, 2026 20:32
@patrickwwbutler
patrickwwbutler requested review from a team as code owners August 25, 2026 20:32
@patrickwwbutler patrickwwbutler changed the title storage: Refresh catalog-vended storage credentials SS-369 Refresh catalog-vended storage credentials Aug 25, 2026
@linear-code

linear-code Bot commented Aug 25, 2026

Copy link
Copy Markdown

SS-369

@def-

def- commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- Credential refresh ignores the catalog's URI override

src/storage-types/src/connections.rs:1422

The credential endpoint is always built from self.uri, even when the REST config response overrides uri. Such a catalog sends normal loadTable requests to the resolved runtime URI, but the first S3 operation sends loadCredentials to the bootstrap URI instead and fails if that host only serves discovery.

Details

iceberg-rust treats overrides.uri specially when it merges the server configuration, replacing the base URI before constructing table endpoints. table_credentials_endpoint parses the same response but extracts only prefix, then starts again from self.uri at line 1428. Because the new custom credential loader starts with an empty cache and replaces the static credentials returned by loadTable, this is not limited to a later refresh: a valid catalog configuration with a bootstrap URI and a runtime URI can no longer perform its first metadata or data-file operation. The endpoint should be based on the resolved URI from overrides when present, ideally by reusing the catalog client's resolved runtime configuration rather than independently implementing part of its merge logic.

2. MEDIUM -- One prefix-scoped credential is used for every table path

src/storage-types/src/connections.rs:262

When loadCredentials returns multiple entries, the loader chooses the globally longest prefix and supplies that credential for every S3 operation. A table whose metadata and data files are under different credential prefixes will therefore sign one of those locations with the wrong key and halt on access errors.

Details

The longest-prefix rule applies after matching credentials against the path being accessed. The existing loadTable path preserves every returned StorageCredential as a separate prefixed FileIO configuration, but installing customized_credential_load replaces the provider chain for all of those configurations. Since this loader caches only the one entry selected by max_by_key and ProvideCredential receives no path, every operator receives the same credential regardless of the prefix that selected its FileIO configuration. The loader needs to retain the full prefix mapping and bind the matching provider when the storage operator is created for a concrete path, or otherwise preserve one refreshable cache per prefixed configuration.

3. MEDIUM -- A transient proactive-refresh failure discards a still-valid credential

src/storage-types/src/connections.rs:343

Once the early refresh deadline is reached, provide_credential returns any catalog refresh error instead of falling back to the cached credential, even though a credential with a reported expiry is still valid for another 120 seconds. A brief credential-endpoint outage can therefore halt the sink while its existing S3 credentials remain usable.

Details

refresh_deadline deliberately schedules the fetch VENDED_CREDENTIAL_REFRESH_BUFFER before actual expiry, but the self.fetch().await? path never consults the cached credential after a failed fetch. OpenDAL's default retry layer only retries the credential-loading failure for a short exponential-backoff window, and the upload error then propagates out of write_data_files as a halting health status. Preserve the old cache entry and return it when refresh fails and its expires_in still covers the current operation; only surface the refresh error after the cached credential is actually unusable.

@ublubu ublubu left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Logic looks about right. 👍


We need some tests for this. If your Unity Catalog is always up, you should be able to point Nightly testdrives at it.

Something like what I did for GCP here (but without the materialize/i2 changes for now): a57699d

IIRC, I set the variables/secrets from i2, so you'll need to ask Dennis (or another test env owner?) to set them for you instead.

Comment thread src/storage-types/src/connections.rs Outdated
Comment thread src/storage-types/src/connections.rs Outdated
Comment thread src/storage-types/src/connections.rs Outdated
Comment thread src/storage-types/src/connections.rs Outdated
Comment thread src/storage-types/src/connections.rs Outdated
/// others). `iceberg-rust` resolves the same value when it builds the catalog but keeps it
/// private, so this asks the server for it directly.
async fn table_credentials_endpoint(
&self,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

table_credentials_endpoint only uses self.uri.

This function and VendedCredentialLoader could go in a new file 👀

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, there's enough happening in this function that we should test it. Maybe the easiest way is to split out the pure components and unit test those.

Comment thread src/storage-types/src/connections.rs Outdated
Comment thread src/storage-types/src/connections.rs
@patrickwwbutler
patrickwwbutler force-pushed the patrick/iceberg-vended-creds-refresh branch from cedc5d4 to c4f1530 Compare August 27, 2026 20:41
@patrickwwbutler
patrickwwbutler force-pushed the patrick/iceberg-vended-creds-refresh branch from c4f1530 to 166be2a Compare August 27, 2026 21:17
@patrickwwbutler
patrickwwbutler force-pushed the patrick/iceberg-vended-creds-refresh branch from 166be2a to e400f51 Compare August 27, 2026 21:25
@def-

def- commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- A vended credential lifetime at or under the 15-minute buffer disables the cache entirely

src/storage-types/src/connections/iceberg_credentials.rs:248

refresh_deadline subtracts the 900-second buffer from the credential's remaining lifetime, so a catalog that vends credentials expiring in 15 minutes or less produces a deadline of now and every provide_credential call re-fetches. Because a fresh OpenDAL Operator is built per storage operation, the sink then pays one serialized loadCredentials round trip per parquet write and per metadata read rather than one per credential lifetime.

Details

remaining.saturating_sub(VENDED_CREDENTIAL_REFRESH_BUFFER) is zero for any reported expiry within 900s, which makes the Instant::now() < *refresh_at check at line 223 false on the very next call. The mutex is deliberately held across the fetch, so concurrent S3 operations queue behind one HTTP round trip each instead of overlapping, and the request rate against the catalog scales with file count rather than with time.

900s is not a pathological value: it is exactly the AWS STS minimum session duration and the minimum credential duration Apache Polaris accepts, so a catalog configured for short-lived vended credentials lands on it. The degradation is also gradual and silent above that point, and it inverts the two constants: a catalog that reports no expiry at all is refreshed every VENDED_CREDENTIAL_DEFAULT_TTL (300s), while one that honestly reports a 900s expiry is refreshed on every single S3 operation.

A floor on the computed deadline keeps the long-lifetime behavior the buffer was chosen for while bounding the short-lifetime case, for example refreshing at max(remaining - BUFFER, min(remaining / 2, VENDED_CREDENTIAL_DEFAULT_TTL)) so the buffer can never consume the whole lifetime.

Vended credentials expire, and nothing refreshed them: `loadTable` hands
their access keys to the FileIO once and OpenDAL keeps signing with them
until the dataflow restarts.

Adds `VendedCredentialLoader`, a `ProvideCredential` implementation that
re-fetches from the catalog's `loadCredentials` endpoint. OpenDAL rebuilds
its `Operator` for every file operation, so reqsign's own credential cache
never survives one call; the loader therefore caches with its own expiry
deadline, taken from `s3.session-token-expires-at-ms` where the catalog
reports one and a short interval where it does not. On a 401 or 403 it
invalidates the catalog token so the next attempt mints a fresh one.

Materialize now builds the OAuth2 provider itself rather than passing a
`credential` catalog property, so one token object serves both catalog
requests and credential refreshes. The catalog client rejects a custom
authenticator combined with that property, so the two cannot coexist.

`connect` grows a table argument because the credentials endpoint is
table-scoped. Installing a loader also means it alone supplies S3
credentials, since OpenDAL replaces its whole provider chain, so one is
installed only when the connection asked for delegation.
@patrickwwbutler
patrickwwbutler force-pushed the patrick/iceberg-vended-creds-refresh branch from 4c59283 to 1340185 Compare August 27, 2026 22:28
@patrickwwbutler
patrickwwbutler merged commit f4ed781 into main Aug 27, 2026
84 checks passed
@patrickwwbutler
patrickwwbutler deleted the patrick/iceberg-vended-creds-refresh branch August 27, 2026 23:24
ggevay added a commit to ggevay/materialize that referenced this pull request Sep 25, 2026
The fuzz crates build in their own workspace, test/cargo-fuzz, which
carries a copy of the root's `[patch.crates-io]` table. Its iceberg-rust
entries were still pinned to the 0.9.0 fork revision after the root moved
to the 0.10.1 revision in MaterializeInc#38471. A patch that no longer satisfies the
version requirement is not an error to Cargo: it lands in
`[[patch.unused]]` and the crate resolves from crates.io, which lacks the
fork API that MaterializeInc#38475 started using, so every fuzz target reaching
mz-storage-types has failed to build since 2026-08-28. The stale
launchdarkly-server-sdk patch in the same table had drifted the same way.

Repin the iceberg entries to the root's current revision, drop the
stale one, and add a bin/lint-cargo check that fails when the fuzz
workspace's patch table carries an entry the root lacks or one that
differs from the root's, so the next root patch bump fails lint instead
of the nightly.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ggevay added a commit that referenced this pull request Sep 25, 2026
…ot (#38700)

### Motivation

The `:rust: cargo-fuzz` job has failed on every release-qualification
run since 2026-08-28 (v26.40.0-rc.1) with `build FAILED for
src/transform/fuzz`: `mz-storage-types` fails to compile with unresolved
imports of `TokenProvider`, `OAuth2TokenProvider`,
`RequestAuthenticator` and `BearerTokenAuthenticator` from
`iceberg_catalog_rest`. Every release from v26.40 to v26.44 shipped
without fuzz coverage.

The fuzz crates build in their own workspace, `test/cargo-fuzz`, which
carries a copy of the root's `[patch.crates-io]` table. Its three
iceberg-rust entries were still pinned to the fork revision for 0.9.0
after #38471 moved the root to the 0.10.1 revision, and the root has
moved the revision twice more since (#38733, #38893). A patch that no
longer satisfies the version requirement is not an error to Cargo: it
lands in `[[patch.unused]]` with a warning and the crate resolves from
crates.io, which lacks the fork API that #38475 started using two
seconds later. The `launchdarkly-server-sdk` patch in the same table had
drifted the same way after the root dropped it in #37026.

### Description

- `test/cargo-fuzz/Cargo.toml`: repin the three iceberg entries to the
root's current revision, add the missing `chrono-tz` fork entry, and
drop the stale LaunchDarkly patch.
- `bin/lint-cargo` (run by CI's lint step): a new check that fails when
the fuzz workspace's patch table carries an entry the root does not
have, one that differs from the root's, or lacks a root entry. It fails
on the pre-fix manifest naming all five drifted or missing entries, and
passes after. It also caught the two later root bumps: rebased onto
current main before the repin, it failed naming exactly the three
iceberg entries. A missing entry gets no Cargo warning at all: the
root's `chrono-tz` fork (#38735) was absent, so the fuzz targets
reaching `mz-pgtz` built against crates.io's older tzdata, and the check
now fails naming it on the previous manifest. Root entries outside the
fuzz dependency graph (`duckdb`, `postgres_array`) are listed in the
check as omitted.

Alternatives considered: per-crate fuzz workspaces (more duplication),
symlinking or generating the manifest (Cargo has no include mechanism; a
generated file for a table that changes a few times a year is not worth
the tooling), repinning without the lint (guarantees the same outage on
the next root patch bump).

### Verification

`cargo check` passes for `src/transform/fuzz` and for the whole
`test/cargo-fuzz` workspace (17 crates), resolving iceberg and
`chrono-tz` from the forks; `cargo metadata` shows zero
`[[patch.unused]]` entries. `bin/lint-cargo` exits 1 on the old manifest
and 0 on the new one; black, ruff and pyright are clean. Not run
locally: the release build with sanitizer-coverage flags and `cargo fuzz
build` itself. The errors were unresolved imports, which `cargo check`
exercises fully; the release-qualification `cargo-fuzz` step is the
end-to-end check.

Closes: [QAR-200](https://linear.app/materializeinc/issue/QAR-200)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.

3 participants