Repository navigation
[feature](be) Native Azure credentials, SAS expiry and client lifetime in S3ClientFactory - #68117
morningman wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
…e in S3ClientFactory Part 1 of the apache#67470 split tracked in apache#68103: the BE consumer side of the native Azure protocol. - AzureCredentialOptions models SharedKey / SAS / OAuth2 credential groups and S3ClientConf carries it; equality, hash and to_string are per provider. - S3ClientFactory accepts the native AZURE_* map and the legacy AWS_* SharedKey map (convert_legacy_azure_properties), validates the data URI against the binding, and keeps Azure clients in a separate bounded cache that is pruned by SAS expiry and re-validated on every access. - AzureAuthFactory builds clients for all three credential types and derives the effective SAS expiry from the token and AZURE_SAS_EXPIRY_MS. - S3URI parses abfs[s]:// and wasb[s]:// authorities and Azure HTTP(S) hosts; ABFS/WASB object names stay literal. - SAS tokens are redacted from SDK logs and client error messages; generate_presigned_url signs only under SharedKey and the error-log path falls back to the local file when no URL can be signed; abort_multipart_upload no longer deletes the blob. Issue: apache#68103 Related PR: apache#67470 Depends on: nothing Co-authored-by: xylaaaaa <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
67d8ae5 to
5f2f1c5
Compare
|
run buildall |
TPC-H: Total hot run time: 27689 ms |
TPC-DS: Total hot run time: 153835 ms |
ClickBench: Total hot run time: 24.07 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Requesting changes for two legacy Azure endpoint compatibility regressions. Existing SharedKey property, protobuf, and Thrift configurations can now be redirected when an endpoint is an unschemed single-label host, and schemed custom proxy base paths are newly rewritten by full URI normalization.
Review checkpoints
- Goal and scope: The credential/auth/cache/URI foundation is otherwise coherent with the staged Azure migration, and all 21 changed files were reviewed.
- Correctness and compatibility: The two inline findings are current old-FE/new-BE regressions. Adjacent scheme, port, IP, DFS/Blob, encoded-path, and standard custom-endpoint cases did not produce a distinct third issue.
- Concurrency and lifecycle: Azure cache locking, expiry publication, eviction, and active shared ownership are sound. Holder reset ordering and unlocked configuration concerns exist in the merge base and are not introduced by this change.
- Errors, security, and observability: Credential-group validation fails closed; examined SDK/status/log paths redact SAS queries; presign and abort callers preserve their documented failure and visibility semantics. No in-scope security vulnerability was substantiated under the repository threat model.
- Tests and build boundaries: Focused coverage is broad, but legacy property/PB/Thrift fixtures are missing for the two endpoint regressions. BUILD_AZURE selection and header boundaries are statically consistent.
- Persistence, configuration, and performance: No storage-format, EditLog, dynamic-variable, transaction, or material resource regression was found beyond the persisted endpoint compatibility changes called out inline.
- User focus: No additional review focus was supplied.
Verification was static-only as required: no compilation, unit tests, or regression tests were run. The pure-text build-hygiene gate passed.
| host.ends_with(".dfs.core.cloudapi.de")); | ||
| if (official_dfs) { | ||
| endpoint.replace(authority_begin + dfs_pos, 5, ".blob."); | ||
| } else if (!has_scheme && authority.find('.') == std::string::npos && |
There was a problem hiding this comment.
[P1] Preserve single-label hosts for legacy Azure endpoints
This shorthand rule also runs after convert_legacy_azure_properties() and both persisted storage-vault adapters. An existing legacy config with AWS_ENDPOINT=storage-proxy used to create https://storage-proxy/container; after this change it targets https://storage-proxy.blob.core.windows.net/container. The legacy endpoint contract does not require a scheme or dotted host, so internal proxies/emulators using one-label DNS names stop working on upgrade (and requests are redirected to an unrelated public origin). Please keep account-name suffix inference native-only, or otherwise distinguish legacy literal endpoints, and cover the property/PB/Thrift paths with an unschemed single-label endpoint.
There was a problem hiding this comment.
Fixed in 573f12c. Account-name suffix inference now only runs where the native protocol is parsed (convert_native_azure_properties); _create_azure_client() applies just the old factory's transport rule (default scheme + endpoint/container join boundary), so AWS_ENDPOINT=storage-proxy still targets https://storage-proxy/container. S3ClientFactoryTest.LegacyAzureEndpointsKeepSingleLabelHosts covers the property / ObjectStoreInfoPB / TS3StorageParam producers with unschemed single-label hosts (with and without port) and asserts the container base the SDK signs against.
| void import_legacy_azure_shared_key(S3ClientConf* conf) { | ||
| // Keep the established SharedKey endpoint normalization at the legacy | ||
| // boundary. Native endpoints and object keys retain their internal slashes. | ||
| conf->endpoint = normalize_http_uri(conf->endpoint); |
There was a problem hiding this comment.
[P2] Keep legacy custom endpoint paths byte-preserving
Calling normalize_http_uri() here changes existing legacy endpoints, not just their credential representation. For example, current FE maps preserve https://proxy.example/gateway//tenant; the old factory passed https://proxy.example/gateway//tenant/container to the SDK, while this import rewrites it to /gateway/tenant/container. Repeated or trailing separators can be meaningful reverse-proxy routes, so those persisted endpoints silently target a different path after upgrade. Please normalize only the endpoint/container join boundary (or limit full normalization to the native protocol) and add property/PB/Thrift coverage for an exact custom base path.
There was a problem hiding this comment.
Fixed in 573f12c. import_legacy_azure_shared_key() no longer calls normalize_http_uri(); the persisted endpoint stays byte-for-byte and only the join boundary with the container is normalized, so https://proxy.example/gateway//tenant still becomes https://proxy.example/gateway//tenant/container. S3ClientFactoryTest.LegacyAzureEndpointsKeepCustomBasePathsBytePreserving covers all three producers with repeated separators, a trailing slash and an encoded base path.
Review follow-up for apache#68117. Legacy SharedKey producers (old FE property maps, ObjectStoreInfoPB, TS3StorageParam) hand the endpoint over literally and the old factory only defaulted the scheme. Two rewrites had leaked into that path: - normalize_azure_endpoint()'s bare account-name inference also ran for legacy endpoints, so an unschemed single-label host such as AWS_ENDPOINT=storage-proxy was redirected to https://storage-proxy.blob.core.windows.net. - import_legacy_azure_shared_key() ran normalize_http_uri(), collapsing repeated path separators in custom reverse-proxy base paths. The native protocol keeps its full normalization where it is parsed (convert_native_azure_properties). _create_azure_client() now only applies the transport rule the old factory had: default the scheme and normalize the endpoint/container join boundary. The persisted endpoint is no longer rewritten. Tests cover all three legacy producers with unschemed single-label hosts and exact custom base paths, asserting both the stored endpoint and the container base the SDK signs against. Issue: apache#68103 Related PR: apache#67470 Co-authored-by: xylaaaaa <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Follow-up for apache#68117. azure_transport_endpoint() was added in the anonymous namespace without a guard, but its only caller is the USE_AZURE branch of S3ClientFactory::_create_azure_client(). The Performance pipeline builds with DISABLE_BUILD_AZURE=ON, so gcc rejected the file with -Werror=unused-function: be/src/util/s3_util.cpp:333:13: error: 'std::string doris::{anonymous}::azure_transport_endpoint(std::string)' defined but not used [-Werror=unused-function] Every other pipeline builds with Azure on, which is why only the Performance job failed. Move the helper under the same USE_AZURE guard as the other _create_azure_client()-only helpers. Verified locally with -fsyntax-only for s3_util.cpp, s3_uri.cpp, s3_file_system.cpp and runtime_state.cpp both with and without -DUSE_AZURE. Issue: apache#68103 Related PR: apache#67470 Co-authored-by: xylaaaaa <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
run buildall |
TPC-H: Total hot run time: 28359 ms |
TPC-DS: Total hot run time: 154071 ms |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
ClickBench: Total hot run time: 24.01 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Static review of exact head 3eb12a821b362b8f216ad6941b396e6587202e52 converged after three bounded rounds. I am requesting changes for three P2 correctness issues in the new native Azure URI contract: OneLake authorities with explicit ports bypass the Hadoop-only fence, ABFS/WASB paths lose literal ?/# object-name characters, and documented Azure -secondary endpoints are rejected. The two earlier legacy-endpoint discussions were treated as hard duplicate fences and their fixes are present at this head.
Critical checkpoint conclusions:
- Goal and proof: the PR largely establishes the intended BE-1 native SharedKey/SAS/OAuth2 credential, URI, cache, SDK, redaction, presign, and multipart-lifecycle foundation while retaining legacy SharedKey inputs. The added unit coverage is broad, but the three missing negative/compatibility cases above mean the goal is not yet fully met.
- Scope/minimality: the production changes are cohesive for BE-1. Provider-aware access-path parsing and per-open SAS validation are explicit BE-2 prerequisites, with native FE emission staged in FE-4; those dormant gaps were investigated and not duplicated as findings on this PR.
- Concurrency: the factory mutex consistently protects cache, LRU, pruning, and publication state; SDK construction and rate-wrapper creation remain outside it; concurrent publication rechecks identity/expiry. No race, long critical section, lock-order, or deadlock defect was substantiated.
- Lifecycle/static initialization: cache eviction preserves active clients through shared ownership, holder replacement is synchronized, the Azure cache is bounded, and no ownership cycle, leak, unsafe destruction dependency, or cross-TU initialization issue was found.
- Configuration: no mutable Doris configuration item is added; the cache bound is internal, so dynamic-update behavior is not applicable.
- Compatibility: legacy property/PB/Thrift SharedKey adapters, dual-field rolling-upgrade behavior, sovereign clouds, custom transports, and
USE_AZURE-off guards were traced. The accepted OneLake and secondary-endpoint issues are the remaining compatibility failures. - Parallel paths: ABFS/WASB/HTTP(S), FileFactory, S3FileSystem, snapshot/result-writer/connectivity paths, all three auth modes, rate-limited delegation, presigning, and multipart operations were checked. No fourth distinct issue survived.
- Conditional checks: credential groups generally fail closed and the endpoint/account checks are documented, but the OneLake and secondary-account conditions are incomplete as described inline.
- Test coverage: URI, factory, legacy-adapter, cache, auth, real-SDK fake-transport, redaction, presign, and lifecycle tests were reviewed. Add deterministic cases for the three inline failures; no regression
.outresult applies to these C++ unit tests. - Executed results: this review was static-only as required; I did not build or run tests. The applicable exact-head CI checks visible at submission time are successful, which is CI evidence rather than independent execution.
- Observability: existing statuses/logs retain useful endpoint/account context while redacting credential-bearing URLs; no additional metric or logging requirement was identified.
- Persistence/transactions: no EditLog, replay, transaction-visibility, or storage-format contract changes. PB/Thrift edits preserve the legacy SharedKey representation.
- Data writes/crash behavior: per-writer block IDs and completion ownership are coherent, and Azure abort being non-destructive avoids deleting committed data or another writer's blocks. No additional partial-write or crash-recovery defect was found.
- FE/BE variables: no new Thrift/session variable is introduced here. The native property producer is intentionally staged in FE-4 behind the BE-1/BE-2 prerequisites.
- Performance/memory/error handling: parsing is linear in short strings, cache scans are bounded to 256 entries, expensive construction stays outside the mutex, statuses/exceptions are propagated and redacted, and no nullable-column or tracked-allocation concern applies.
- User focus: no additional focus point was supplied; the complete PR was reviewed.
Review status: complete and converged for this exact head; all candidates were submitted, dismissed with concrete staging/code evidence, or fenced as existing discussion.
| } | ||
| const auto dot = _endpoint.find('.'); | ||
| _account = dot == std::string::npos ? _endpoint : _endpoint.substr(0, dot); | ||
| _key = authority_split.size() == 1 ? "/" : authority_split[1]; |
There was a problem hiding this comment.
[P2] Keep ? and # literal in ABFS/WASB object paths
This new branch captures the complete ADLS suffix here, but the common parsing block below still truncates _key at ? and #. The pinned Iceberg ADLSLocation treats everything after the authority as the literal object path, and Azure client libraries escape reserved blob-name characters. Consequently, abfss://[email protected]/path/a?b#c.parquet is currently opened as path/a, which can fail or read a different blob. Skip query/fragment stripping for the ABFS/WASB schemes and add direct-parser plus FileFactory coverage for both characters.
There was a problem hiding this comment.
Fixed in 41567d7. S3URI::parse() no longer strips a query or fragment for the abfs/abfss/wasb/wasbs schemes: ADLSLocation (^(abfss?|wasbs?)://([^/?#]+)(.*)?$) and Hadoop's Path ("query & fragment not supported") both take everything after the authority as the object path, so abfss://[email protected]/path/a?b#c.parquet now yields the key path/a?b#c.parquet, with and without the provider flag. S3 and HTTP(S) locations keep stripping (an HTTP(S) Azure location may carry a SAS query, and %3F/%23 remain the encoded spelling there). S3URITest.QueryAndFragmentAreLiteralOnlyInAdlsPaths pins both sides and IcebergAdlsPathsPreserveLiteralPercentSequences gained ?/# object names for all four schemes.
FileFactory is not part of this PR; the reader/writer-open coverage for these names goes into BE-2 (#68103), the PR that changes file_factory.cpp and adds the provider-aware second parse there.
| return Status::OK(); // Old SharedKey file-system paths use the S3 wire spelling. | ||
| } | ||
| const auto uri_host = to_lower(uri.get_endpoint()); | ||
| if (uri_host.ends_with(".dfs.fabric.microsoft.com") || |
There was a problem hiding this comment.
[P2] Canonicalize the OneLake host before applying this fence
S3URI retains an explicit port in get_endpoint(), so abfss://[email protected]:443/file makes both suffix checks false. With a matching native endpoint (or a custom transport), the later authority checks accept it and route a genuine Fabric location through the native Azure client even though this branch requires OneLake to stay on its Hadoop binding; the HTTPS Blob spelling has the same issue. Extract the host without an optional port (and account for a DNS trailing dot) before the Fabric suffix check, and cover both ABFS and HTTPS OneLake authorities with :443.
There was a problem hiding this comment.
Fixed in 41567d7. The fence now matches on the DNS host rather than the raw authority: authority_host() lower-cases it and drops an explicit port and a trailing root dot before the Fabric suffix check, so abfss://[email protected]:443/..., the https://onelake.blob.fabric.microsoft.com:443/... spelling and ONELAKE.dfs.fabric.microsoft.com. all return NotSupported before any endpoint or account comparison runs. S3ClientFactoryTest.OneLakeFenceIgnoresPortAndDnsRootDot covers the ABFS and HTTPS forms with :443 against both a Fabric endpoint and a custom transport endpoint (the two paths that previously let it through).
| if (http_uri && !endpoint.starts_with(uri.get_scheme() + "://")) { | ||
| return Status::InvalidArgument("Azure URI scheme conflicts with the storage endpoint"); | ||
| } | ||
| if (S3URI::is_azure_endpoint(uri.get_endpoint()) && |
There was a problem hiding this comment.
[P2] Allow the documented Azure secondary account endpoint
For a native binding with AZURE_ACCOUNT_NAME=myaccount and AZURE_ENDPOINT=https://myaccount-secondary.blob.core.windows.net, this compares the URI label myaccount-secondary with the credential identity myaccount and rejects the request. Azure's RA-GRS/RA-GZRS secondary endpoint deliberately appends -secondary while using the same primary account identity and keys, so this prevents valid secondary reads. Canonicalize that documented suffix only for recognized Azure service authorities before this comparison, and add a secondary Blob-endpoint test.
There was a problem hiding this comment.
Fixed in 41567d7. On official Azure service hosts the -secondary account label is now folded to the primary account (azure_primary_authority()), and that folded identity is what the ABFS/WASB logical-account comparison against the configured endpoint and the credential account check both use. That matches how the SDK signs secondary reads: SharedKeyPolicy builds the canonicalized resource from m_credential->AccountName, and StorageSwitchToSecondaryPolicy only swaps the host, so the credential must keep the primary name. With AZURE_ENDPOINT=https://myaccount-secondary.blob.core.windows.net and AZURE_ACCOUNT_NAME=myaccount the factory now accepts abfss://[email protected]/..., the HTTPS secondary spelling, and abfss://[email protected]/... (table metadata written against the primary host, which is the usual RA-GRS read case and was rejected one check earlier by the same root cause).
HTTP(S) locations still have to match the configured transport origin exactly, and the fold is limited to recognized Azure authorities: other-secondary, an account literally named secondary, and myaccount-secondary-secondary stay rejected. Covered by AllowsDocumentedAzureSecondaryEndpoints and SecondaryFoldingKeepsOtherAccountsApart.
…idate_azure_uri Review follow-up for apache#68117, three defects in the new native Azure URI contract: - S3URI::parse() stripped '?' and '#' from ABFS/WASB object paths. Iceberg's ADLSLocation ("^(abfss?|wasbs?)://([^/?#]+)(.*)?$") and Hadoop's Path both take everything after the authority as the object path, so an object named path/a?b#c.parquet was opened as path/a. Query and fragment are now only stripped for the S3 and HTTP(S) schemes. - The OneLake fence compared the raw authority, so an explicit port (onelake.dfs.fabric.microsoft.com:443) or a trailing DNS root dot made a genuine Fabric location pass the later checks and reach the native client. The fence now matches on the DNS host (case-folded, no port, no root dot). - Azure RA-GRS/RA-GZRS secondary endpoints append "-secondary" to the account label while keeping the primary account identity and keys (the SDK signs with the credential's account name and only swaps the host). The account check compared that label with the credential and rejected valid secondary reads. On official Azure hosts the suffix now folds to the primary account for the ABFS/WASB endpoint identity and for the credential account check; HTTP(S) transport matching stays exact. Tests: S3URITest.QueryAndFragmentAreLiteralOnlyInAdlsPaths (plus '?'/'#' names in IcebergAdlsPathsPreserveLiteralPercentSequences), S3ClientFactoryTest.OneLakeFenceIgnoresPortAndDnsRootDot, AllowsDocumentedAzureSecondaryEndpoints and SecondaryFoldingKeepsOtherAccountsApart. Issue: apache#68103 Related PR: apache#67470 Co-authored-by: xylaaaaa <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Addressed the three round-3 findings in 41567d7 (all three were introduced by this PR, so they are fixed here rather than deferred):
Unit coverage: |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 27805 ms |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-DS: Total hot run time: 153131 ms |
ClickBench: Total hot run time: 24.14 s |
What problem does this PR solve?
Issue Number: #68103
Related PR: #67470
Original author: @xylaaaaa (all code in this PR is carved out of #67470; the commit carries
Co-authored-by: xylaaaaa)Problem Summary:
This is part 1 of the series tracked in #68103, which splits #67470 (route Azure data access
through native Azure credentials, original implementation by @xylaaaaa) into independently
reviewable PRs. Depends on nothing.
Today the BE knows only one way to talk to Azure Blob Storage: a SharedKey pair carried in the
AWS-shaped
AWS_ACCESS_KEY/AWS_SECRET_KEYfields, withprovider=azureas the only hint.There is no way to pass a SAS token or an OAuth2 client credential,
S3URIcannot parseabfs[s]:///wasb[s]://locations, and a SAS token appended to a URL ends up in logs anderror messages. This PR is the BE consumer side of the native Azure protocol. The FE producer
side comes later in the series (FE-4), so nothing changes for existing deployments until a new
FE starts emitting the native fields.
What this PR does:
AzureCredentialOptions(new headercommon/cpp/obj-client/auth/azure_credential_options.h)is the provider-owned credential model:
SHARED_KEY/SAS/OAUTH2, withvalidate()rejecting mixed credential groups. It has no Azure SDK dependency so
S3ClientConfcan carryit in builds with
BUILD_AZURE=OFF.S3ClientConfgains anazure_credentialsmember.operator==,get_hash()andto_string()are now provider-aware: for Azure the client identity is the Azure credentialgroup plus endpoint / bucket / timeouts, and the AWS fields (region, role, signing settings)
no longer participate.
to_string()never prints a SAS token.S3ClientFactory::convert_properties_to_s3_confdispatches onprovider=azure:AZURE_AUTH_TYPEis the native protocol (AZURE_ENDPOINT,AZURE_ACCOUNT_NAME,AZURE_ACCOUNT_KEY,AZURE_SAS_TOKEN,AZURE_SAS_EXPIRY_MS,AZURE_CLIENT_ID,AZURE_CLIENT_SECRET,AZURE_TENANT_ID,AZURE_OAUTH_SERVER_URI,AZURE_CONTAINER);AWS_ENDPOINT/AWS_ACCESS_KEY/AWS_SECRET_KEY), handled byconvert_legacy_azure_propertieswith unchanged behavior;AWS_*fields may also be present (rolling upgrade) andmust agree with the
AZURE_*fields.validate_azure_urichecks that the data URI's container, account and host agree with thebinding. OneLake hosts (
*.fabric.microsoft.com) are refused by the native client and stayon their Hadoop binding.
S3Conf::get_s3_conf(ObjectStoreInfoPB)/get_s3_conf(TS3StorageParam)(storage vault path)move ak/sk into
azure_credentialsfor the Azure provider, so the vault path keeps workingunchanged.
AzureAuthFactory::createbuilds the container client for all three credential types:SharedKey (
StorageSharedKeyCredential), SAS (token appended to the container URL) andOAuth2 (
Azure::Identity::ClientSecretCredential; the tenant comes fromAZURE_TENANT_IDoris derived from the OAuth server URI).
AzureAuthFactory::validateparses the SASse=field and combines it with
AZURE_SAS_EXPIRY_MSinto the earliest effective expiry.Azure clients live in a dedicated bounded cache (
_azure_cache: capacity 256, the leastrecently accessed entry is evicted, expired SAS entries are pruned on every access). Expiry
is re-checked on every
create(), so an expired SAS is refused before a new reader or writeris opened. The existing
_cachefor non-Azure providers is untouched.S3ClientFactory::validate_credentials_for_accessis the entry point the next PR (BE-2)uses at reader/writer open time.
ObjClientHolder::resetnow copiesazure_credentials, so astorage-vault credential rotation replaces the Azure client instead of being ignored.
S3URIparsesabfs[s]://[email protected]/keyandwasb[s]://..., recognizes Azure HTTP(S) hosts (*.blob.core.windows.net,*.dfs.core.windows.netand the China / US Gov / Germany cloud suffixes), exposesget_endpoint()/get_account()/get_scheme()/is_azure(), and percent-decodes onlyHTTP(S) Azure paths (ABFS/WASB object names are literal, matching Iceberg
ADLSLocation).Parse errors for Azure locations no longer echo the location.
SAS redaction: the Azure SDK logger listener and every
AzureObjStorageClienterror pathstrip the query component of URLs before the message is logged or returned.
AzureObjStorageClient::generate_presigned_urlsigns only with a SharedKey credential andreturns an empty string for SAS/OAuth2;
RuntimeState::get_error_log_file_paththen keepsthe local error log path instead of publishing an empty URL. The signed URL is now built from
the SDK blob client URL, so a literal
%2Fin the object name signs the right blob.AzureObjStorageClient::abort_multipart_uploadbecomes a no-op instead of deleting the blob.Behavior / compatibility:
AWS_ENDPOINT/AWS_ACCESS_KEY/AWS_SECRET_KEY+provider=azurekeepsworking through
convert_legacy_azure_properties; Azure storage vault configs are unchanged.abfs://...,https://x.blob.core.windows.net/...) withoutprovider=azureis now rejected instead of being treated as S3.abort_multipart_uploadon Azure no longer deletes the target blob. Azure has no server-sidemultipart session; blocks are isolated per upload UUID, and deleting the blob could remove
previously committed data or another writer's staged blocks.
otherwise be republished), and the error-log path falls back to the local file.
Azure::Identity::ClientSecretCredentialfromAZURE_CLIENT_ID/AZURE_CLIENT_SECRET/AZURE_TENANT_ID(or the tenant inAZURE_OAUTH_SERVER_URI). The description of [fix](iceberg) Route Azure data files through native credentials #67470 said native OAuth2 "fails closed"; thisPR describes what the code does. User-delegation SAS signing for OAuth2 is not implemented, so
presigned URLs are unavailable in that mode.
Rolling upgrade: a new FE emits both the native
AZURE_*fields and the matching legacyAWS_*group for SharedKey, so old BEs keep working; new BEs verify that the two groups agree. SAS and
OAuth2 have no legacy wire form: upgrade all BEs before enabling native SAS/OAuth2 from a new FE,
and stop native SAS/OAuth2 use before rolling back.
Review focus:
S3ClientConf::operator==/get_hash()are now per-provider._azure_cache(capacity 256, access-ordered eviction, expiry pruning) coexists with_cache.abort_multipart_uploadsemantic change (no-op instead of delete).generate_presigned_urlsigns only under SharedKey.convert_properties_to_s3_confrejects Azure host URIs withoutprovider=azure.Release note
None
Check List (For Author)
Test
AzureAuthFactoryTest.*:AzureObjStorageClient*Test.*:AzureClientFactoryCacheTest.*:S3ClientFactoryTest.*:S3URITest.*:S3FileSystemTest.*: 121 tests, 84 passed, 37 skipped (34S3FileSystemTestneed a live bucket, 3 need Azurite), 0 failed*S3*:*Azure*:*Rowset*(guards everyS3ClientConfdesignated-initializer site): 341 tests, 296 passed, 44 skipped, 1 failed. The failure (S3ObjStorageClientMockTest.test_ca_cert) and an exit-time abort fromS3FileWriterTestare pre-existing macOS-only issues in files this PR does not touch (Linux CA bundle paths; a leaked staticS3FileSystemdestroyed afterAws::ShutdownAPI)build-support/check-format.shclean; the fourcommon/cppfiles clang-format 16 cleanBehavior changed:
abort_multipart_uploadno longer deletes the blob; presigned URLs are only generated for SharedKey; Azure host URIs requireprovider=azure.Does this need documentation?
Check List (For Reviewer who merge this PR)