Skip to content

SS-471 Add support for vending (and refreshing) credentials for Iceberg catalogs backed by Azure Data Lake Storage (V2) - #38893

Merged
patrickwwbutler merged 5 commits into
MaterializeInc:mainfrom
patrickwwbutler:patrick/adls-refresh
Sep 16, 2026
Merged

patrickwwbutler merged 5 commits into
MaterializeInc:mainfrom
patrickwwbutler:patrick/adls-refresh

Conversation

@patrickwwbutler

@patrickwwbutler patrickwwbutler commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Implements the VendedCredential trait for the newly created CustomAzdlsCredentialLoader added to the MZ iceberg-rust fork in MaterializeInc/iceberg-rust#9

Then plugs in a CustomAzdlsCredentialLoader with the previously created VendedCredentialLoader using this trait implementation into the OpenDalStorageFactory::Azdls constructor, allowing opendal to call it for fresh vended credentials from the catalog.

Tested manually in our Azure Databricks workspace, with optimistic plans to add CI testing later on.

Also closes SS-173

@patrickwwbutler
patrickwwbutler requested a review from a team as a code owner September 16, 2026 15:55
@def-

def- commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- ADLS from_vended rejects catalogs that alias one SAS token under several adls.sas-token.* keys

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

Vending catalogs routinely publish the same SAS token under more than one adls.sas-token.<suffix> key for client compatibility. The new loop treats the second such key as a fatal ambiguity, so against those catalogs the loader never yields a credential and every ADLS storage request fails — a regression, because before this PR the same response worked through the static props.

Details

Apache Polaris AzureCredentialsStorageIntegration.handleAzureCredential emits, for a single ADLS location, all of:

  • adls.sas-token.<account>.dfs.core.windows.net → token
  • adls.sas-token.<account> → the same token (its comment: "Iceberg 1.7.x may expect the credential key to not be suffixed with endpoint")
  • adls.sas-token (bare) → the same token
  • adls.sas-token-expires-at-ms.<account>.dfs.core.windows.net

The first two both satisfy prop.starts_with("adls.sas-token."), so from_vended returns credential_invalid on the second one it happens to see. reqsign_core::ProvideCredentialChain::provide_credential swallows provider errors and moves on (reqsign-core-3.3.x api.rs:494), and the fork's azdls_config_build replaces the whole chain when a loader is installed rather than prepending to it, so there is no fallback: the chain returns None and the operator fails with a generic "signing credential" error. The real cause only shows up in the warn! inside VendedCredentialLoader::provide_credential.

Note also that the bare adls.sas-token key (no trailing dot) is what iceberg-rust's azdls_config_parse reads (ADLS_SAS_TOKEN = "adls.sas-token"), and the REST catalog merges cred.config into the per-prefix FileIO props. That is why Polaris + ADLS worked pre-PR and stops working now.

Databricks/Unity Catalog appears to emit only the host-suffixed key, which is consistent with the manual testing reported in the PR description.

Suggested fix: only the aliases disagreeing on the token value are genuinely ambiguous, and the bare key is worth honoring as a fallback.

let mut sas_token: Option<&String> = None;
for (prop, val) in vended.config.iter() {
    if prop.starts_with("adls.sas-token.") {
        // Catalogs alias one token under several keys (host-suffixed, account-suffixed,
        // bare) for client compatibility. Only differing values are ambiguous.
        if sas_token.is_some_and(|seen| seen != val) {
            return Err(reqsign_core::Error::credential_invalid(format!(
                "vended Iceberg storage credential for prefix {} has conflicting \
                 adls.sas-token.<account> properties",
                vended.prefix
            )));
        }
        sas_token = Some(val);
    }
    // ... expiry handling unchanged
}
let sas_token = sas_token.or_else(|| vended.config.get("adls.sas-token"));

A unit test alongside test_aws_credential_from_vended / test_gcs_credential_from_vended covering the Polaris key set would pin this down; there is currently no test for the ADLS impl.

2. LOW -- TODO(SS-449) says ADLS has no loader, directly above the code that installs one

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

The comment states that "vended credentials here still expire without one" and that wiring one up "needs a VendedCredential mapping from the catalog's adls.sas-token.<account> prop onto Credential::SasToken" — which is exactly what this PR adds two lines below and in iceberg_credentials.rs:126. Left as is, the next reader will believe ADLS refresh is unimplemented.

3. LOW -- the iceberg-rust rev bump is not a fast-forward and silently reverts fork PR #7

Cargo.toml:679

eafda25 is not a descendant of the currently pinned 985929c (their merge base is upstream 2cf6128), so the bump drops fork commit 5c5c189 "make Transaction::commit internals public" along with picking up the ADLS work. iceberg::transaction::TransactionAction, RowDeltaAction and TableCommit's public builder go back to pub(crate) — visible in the crates/iceberg/public-api.txt delta.

Details

Nothing in this tree names those items today (src/storage/src/sink/iceberg.rs:108 only uses ApplyTransactionAction and Transaction, both still public), so there is no functional impact. Flagging because a rev bump shows up in the diff as one hex string and the revert is otherwise invisible: worth confirming it was intentional rather than a rebase casualty.

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

aside from the comment, the rest looks good! thanks @patrickwwbutler

Comment thread src/storage-types/src/connections.rs Outdated
@patrickwwbutler
patrickwwbutler merged commit 6e55add into MaterializeInc:main Sep 16, 2026
82 checks passed
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