Repository navigation
Conversation
### What problem does this PR solve? Problem Summary: GCS access currently depends on HMAC credentials or unsigned requests. A deployment using Application Default Credentials or a Compute Engine service account cannot consistently use the same identity for object reads and writes, Iceberg metadata operations, storage vaults, and backend error-log downloads. Add native GCP OAuth authentication across FE, BE, and Cloud using `gs.credential_provider_type=DEFAULT` or `COMPUTE_ENGINE`, with optional `gs.impersonation_service_account`. GCP storage without explicit authentication defaults to Application Default Credentials. Public-bucket access requires explicit `ANONYMOUS`; existing HMAC configuration remains supported. Reject conflicting native, HMAC, and AWS-only authentication properties before creating clients. The implementation follows the current master architecture: - Keep credential resolution in the filesystem API, OAuth client construction in the GCS plugin, and Iceberg client integration in the Iceberg connector. Use the native Hadoop GCS connector for namespace operations and bearer-authenticated S3 XML clients for object operations. - Carry typed credentials through additive Thrift and Protobuf fields, Cloud object information, and the shared C++ S3 client factory. Preserve AWS authentication, client caching, and rate limiting. - Normalize GCS aliases for resources, serialize resource ALTER operations, and validate effective properties before publication. Preserve omitted vault credential fields during partial ALTER, support clearing impersonation explicitly, and reject anonymous storage vaults. Apply vault changes to candidate metadata so a failed update does not persist a partial rename. - Support `gs://` in EXPORT and OUTFILE while preserving HTTP paths. Generate backend GCS V4 error-log URLs through IAM `signBlob`, falling back to the local error-log path if signing fails. - Add unit coverage and IAM regression suites for catalogs, TVFs, load, export, outfile, backup/restore, resources, vaults, property validation, and anonymous access. Export tests read back the written data and isolate cleanup by run directory. Example native authentication properties: ```properties provider=GCP gs.credential_provider_type=DEFAULT gs.impersonation_service_account=target@project.iam.gserviceaccount.com ``` The impersonation property is optional. Use `COMPUTE_ENGINE` to select the attached VM identity explicitly, or `ANONYMOUS` for public object access. Compatibility and deployment: - Existing AWS/HMAC protocol fields and authentication paths remain available. Native GCP credentials require updated consumers; upgrade MetaService and the relevant FE/BE processes before enabling the feature during a rolling upgrade. - Package google-cloud-cpp OAuth libraries for BE/Cloud and the shaded Hadoop GCS connector for FE/Iceberg. - FE presigned uploads with native OAuth remain unsupported and fail explicitly. Backend error-log downloads use the dedicated GCS V4 signing path. - Native OAuth uses the existing configurable object-store endpoint, so this remains an outbound-request/SSRF surface. Catalog creation requires catalog CREATE privilege; vault creation requires global ADMIN privilege. Google token and signing operations use the credential library and Google IAM APIs. No remote code-loading path is introduced. ### Release note Add native GCP IAM authentication with Application Default Credentials, Compute Engine identities, and optional service-account impersonation for GCS-backed storage. GCP configurations without HMAC or an explicit authentication mode now use ADC; public access must select `ANONYMOUS`. Improve resource/vault credential updates and support GCS error-log signing. ### Validation Passed locally: - GCS filesystem reactor unit tests, including 88 filesystem API tests, 152 S3-base tests, and 36 GCS tests; zero failures, errors, or skips in these modules. - FE Core and Iceberg Checkstyle, plus the GCS reactor's style checks. - clang-format 16 checks for all 33 changed C++ files, BE build hygiene, connector import boundaries, and FE metadata-funnel checks. - Parsing of all 41 changed Java files; Protobuf descriptor generation and Thrift C++ generation for the changed definitions. - `git diff --check`. Not completed locally: - Full FE unit tests: the standard runner requires JDK 17; the available test runtime is JDK 20. - Iceberg reactor tests: generation stops because the installed Thrift compiler is 0.16.0 while master requires 0.24.0. The standalone Thrift syntax check above does not replace this build. - BE/Cloud compilation, unit tests, and clang-tidy: the installed thirdparty bundle lacks the required google-cloud-cpp libraries. - Live IAM regression suites: no configured Doris cluster or GCP test environment was available. ### Check List (For Author) - Test: - [x] Unit tests added; filesystem API/S3/GCS tests executed successfully. - [x] Regression tests added. - [ ] Full FE, Iceberg, BE/Cloud, and live IAM tests completed. - Behavior changed: - [x] Yes: native GCP authentication, ADC default, explicit anonymous mode, and validated partial credential updates as described above. - Does this need documentation? - [x] Yes: document credential properties, ADC deployment, impersonation permissions, explicit anonymous access, and upgrade requirements.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…ures ### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Native GCP Hadoop access ignored the configured endpoint and HadoopCatalog selected S3A for s3/s3a warehouse aliases. Propagate the endpoint to the Google connector and normalize native HadoopCatalog warehouses to gs. The C++ client also sent anonymous object requests when token acquisition failed. Acquire bearer tokens before each object-storage SDK operation and return a non-retryable authentication error on missing or empty tokens. Add endpoint, warehouse alias, and authentication-failure coverage. ### Release note Native GCP catalogs honor custom endpoints and compatible warehouse schemes. Missing GCP tokens now fail locally instead of issuing anonymous requests. ### Check List (For Author) - Test: GCS module and upstream unit tests passed (37 GCS tests); FE Checkstyle, clang-format 16 on changed C++ files, build hygiene, and git diff --check passed. Iceberg tests stopped at the Thrift 0.16/0.24 mismatch; BE test setup stopped at the JDK 17 requirement. Further compilation and tests skipped at user request. Live GCP regression tests were not run. - Behavior changed: Yes, native Hadoop endpoint/scheme routing and token failure handling. - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Resources saved before GCS alias normalization can retain old gs.* properties. Normalizing only after merging an ALTER lets an old alias override a new s3.* value, silently ignoring credential rotation or restoring a session token that the user cleared. Normalize stored properties and the patch separately before merging, so the patch takes precedence while preserving gs.* priority within the same input map. Update the regression unit test to expect the new access/secret keys and the cleared session token. ### Release note GCP resource ALTER updates now override persisted gs.* aliases correctly. ### Check List (For Author) - Test: FE Core Checkstyle and git diff --check passed. Updated unit coverage for credential replacement, token clearing, unchanged properties, and same-input alias precedence. Compilation and tests skipped at user request. - Behavior changed: Yes, explicit resource updates override old alias values. - Does this need documentation: No
sollhui
marked this pull request as ready for review
September 28, 2026 07:38
sollhui
requested review from
924060929,
CalvinKirs,
Gabriel39,
englefly,
gavinchou,
liaoxin01,
luwei16,
morningman,
morrySnow and
starocean999
as code owners
September 28, 2026 07:38
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: five inline findings remain after two static review rounds. Three affect native GCP identity across Cloud stage retrieval, COPY INTO scan planning, and legacy instance AK/SK updates; two break IAM regression suites. The stage findings are independent: repairing the stage response alone does not repair COPY's separately built broker map. There was no additional user-provided review focus.
Critical checkpoints (code-review skill 1.3):
- Goal and proof: Native GCP OAuth is wired through FE, BE, Cloud, Iceberg, and error-log signing, but the three credential handoffs above prevent consistent identity use. Added builder and validation tests do not cover these end-to-end paths.
- Scope and clarity: The broad cross-module change generally reuses shared credential adapters. The Syncer helper removal also breaks an unchanged AWS suite.
- Concurrency: FE resource ALTER serializes snapshot and publication; the C++ token provider protects its cached generator. I found no separate lock-order, thread-context, or atomic-order defect.
- Lifecycle: Cached clients include the credential configuration and release the signing callback with the client. No cross-translation-unit static initialization or ownership-cycle issue was found.
- Configuration: The new GCP mode and impersonation properties are carried as persisted or transport fields. Vault partial ALTER preserves omitted fields; legacy instance key updates retain the native envelope incorrectly (inline finding). No new dynamic configuration knob needs a refresh path.
- Compatibility: The new Thrift and Protobuf fields are optional, and the PR declares that updated consumers are required before enabling native mode. The removed regression helper is an incompatible test-framework symbol (inline finding).
- Parallel paths and conditionals: MetaService's stage response and COPY's broker map each lose credential settings; both instance AK/SK update entry points retain native auth. Other auth conflict and anonymous-mode checks matched their documented behavior on static trace.
- Test coverage and results: Unit tests cover direct adapters, token/header behavior, and vault patching; no stage RPC/COPY scan or native-to-AK/SK instance transition test covers the findings. Two regression suites have deterministic failures under their supported AWS configurations. Builds, tests, and live IAM requests were prohibited by the review prompt, so this review makes no runtime validation claim; author-reported checks were not independently rerun.
- Observability and errors: Token and IAM signing failures are propagated or logged, with a local error-log fallback. I found no separate ignored Status, exception-boundary, or metric defect; wrong identity selection in the inline findings can still surface only downstream.
- Persistence, writes, and crashes: Vault ALTER validates a candidate and commits vault/instance metadata together. Legacy instance key updates persist a mixed auth state (inline finding). No other new write-atomicity or crash-recovery defect was substantiated.
- FE/BE propagation: Optional credential fields reach the regular adapters, but COPY's manually built scan map drops GCP mode and impersonation. The provider-less resource concern raised during review was dismissed after tracing GcpCredentialAdapter's downstream Thrift setter.
- Performance, memory, and BE nullable handling: No material new hot-path cost or memory ownership/reservation issue was substantiated. Vectorized ColumnNullable and MoW/versioned-read code are untouched by this PR.
Review complete after the final changed-file and duplicate sweep. Five distinct issues are left inline; no unresolved candidate remains. Static inspection only.
gavinchou
previously approved these changes
Sep 30, 2026
This was referenced Sep 30, 2026
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Clearing s3.role_arn previously erased static keys and native GCP credentials before persisting the vault. Only replace other credentials when selecting a nonempty AWS role or an explicit AWS credential provider. Cover encrypted static credentials, native GCP credentials, and repeated clears at the RPC/persistence boundary. Also use TableNameInfo in ExportCommandTest and restore the role repository helper still used by the legacy AWS suite. ### Release note Clearing an unused AWS role preserves the vault's active static or GCP credentials. ### Check List (For Author) - Test: clang-format 16 check, build hygiene, Java Checkstyle (JDK 20), git diff --check, and Groovy helper smoke checks passed. Added cloud RPC/persistence regression coverage. FE UT and cloud build could not start because the configured JDK 17 lacks lib/modules; cloud third-party build dependencies and a live Doris/AWS environment are also unavailable. Full unit/integration tests were not run. - Behavior changed: Yes, clearing an unused role preserves existing credentials. - Does this need documentation: No
…keys ### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Legacy instance key updates retained native GCP credentials, so clients kept using OAuth after a successful AK/SK update. Clear the native envelope in both instance update helpers, including both user-id branches, and allow unchanged keys to repair an already persisted mixed auth state. Reject empty replacement keys before removing native authentication. Add RPC/persistence coverage for native-to-key transitions, mixed-state repair, and rejection without mutation. Guard optional AWS external-ID assertions so an omitted external ID does not fail the resource ALTER regression suite. ### Release note Replacing native GCP authentication with instance access keys now makes the new keys effective. AWS IAM resource regression tests accept an omitted external ID. ### Check List (For Author) - Test: Added regression coverage; compilation and test execution skipped at user request. Applied repository clang-format 16; git diff --check passed. - Behavior changed: Yes, instance key updates remove native authentication. - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Merge apache/doris master at 7ac0eb2 into the GCP IAM branch. Resolve concurrent test additions in OutFileTest and ExternalFileTableValuedFunctionTest by retaining both the GCP credential coverage and upstream UUID coverage as separate methods. Preserve the previous credential-transition and regression fixes. ### Release note None ### Check List (For Author) - Test: Targeted Java Checkstyle passed with zero violations. Verified both parents' test methods remain, no unresolved merge entries, and prior repair patches still apply in reverse. Conflict-file whitespace checks passed; upstream generated .out whitespace was retained. Compilation and test execution skipped at user request. - Behavior changed: No additional behavior change beyond merging master. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review outcome: request changes. This exact-head static review found six distinct new issues in the inline comments: two P1 and four P2. Existing P1 comments 4119930342 (internal-stage credential lost in get_stage) and 4119930347 (COPY INTO broker properties omit native GCP credentials) still apply and are carried as blockers without reposting. The other three existing threads were rechecked and their issues are fixed at this head.
Goal and test evidence: The change adds native GCP OAuth across FE, BE, Cloud object storage, Iceberg, export, and signed URLs. The implementation does not yet meet that goal on the token-boundary and Cloud mutation paths identified inline. Added BE, Cloud, FE, and regression test sources were inspected; builds and tests were prohibited by this review contract and were not run, so their results are not independently verified.
Scope, configuration, and compatibility: The change mostly uses the existing storage adapters and factories, and no unrelated edit was substantiated as an issue. The inspected FE-to-BE and FE-to-Cloud mappings retain native credential fields for supported Resource, Vault, Iceberg, LOAD, TVF, OUTFILE, EXPORT, and recycler paths. The role-only ADD_S3_VAULT RPC loses its previous default-provider compatibility. No new dynamically reloaded process setting was identified. Native credential protocol compatibility during rolling upgrades remains dependent on updated consumers as described in the PR; this static review cannot validate a mixed-version deployment.
Concurrency and lifecycle: Client cache misses construct outside the factory lock, and the new GCP consumers read the shared CA path without its lock. Token refresh ownership and synchronization, presigning callbacks, release paths, and cross-translation-unit initialization were inspected; no other substantiated lifecycle issue remained. Heavy client construction occurs outside the cache lock, and no new lock-order issue was found.
Validation, errors, and observability: The auth-mode predicates and caller-selected endpoint boundary were traced through request construction, including the concrete path-style SQL case. Cloud admission conditions are inconsistent for native ADD_OBJ_INFO and role-only vault ADD. Signed URL error handling, IAM response parsing, and existing logging were inspected; no further distinct status-propagation or observability issue was substantiated.
Persistence, writes, and parallel paths: Cloud root and derived credential changes use separate transactions; a derived failure followed by the same request cannot finish the cascade. The two existing stage/COPY paths still drop native credentials. Other inspected vault persistence, recycler reads, and FE/BE transport paths preserve the selected identity. Table visible-version, MoW delete bitmap, and rowset publication logic are not changed by this PR. No additional confirmed memory-accounting or hot-path performance issue was found; final glibc 2.17 binary compatibility cannot be established without a build artifact.
Completion: All 107 changed paths, the prior inline threads, and the unresolved-candidate list were swept after two review rounds; all round-two agents returned NO_NEW_VALUABLE_FINDINGS. The user focus file specified no additional focus.
Existing P0/P1 findings confirmed for this head: #68540 (comment), #68540 (comment)
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Native GCP clients attached process bearer tokens to arbitrary storage endpoints. Require Google Cloud Storage HTTPS endpoints on port 443 in FE properties and request interception, and in the shared BE/Cloud client before fetching tokens. Keep TLS verification enabled, disable native-client redirects, and reject AWS access-point ARNs in GCS bucket fields. HMAC and anonymous endpoint handling retain their existing behavior. Exclude the Doris cluster Auth-Token header from IAM signBlob and metadata HTTP requests. Admit validated native credentials for legacy ADD_OBJ_INFO requests, preserve role-only ADD_S3_VAULT requests with an omitted provider type, and pass the locked CA-path snapshot to both GCP consumers. ### Release note Native GCP OAuth requires a Google Cloud Storage HTTPS endpoint on port 443; untrusted custom endpoints and plaintext endpoints are rejected. Legacy object storage ADD accepts native GCP credentials, and role-only vault ADD continues to default to INSTANCE_PROFILE. ### Check List (For Author) - Test: Added FE, BE, and Cloud unit coverage. Java Checkstyle, clang-format 16, build hygiene, and git diff --check passed. An isolated check compiled the actual C++ endpoint validator and passed 14 endpoint cases. Full unit tests and BE/Cloud builds were not run: standard FE/BE test entry points stopped because JDK 17 is unavailable, and this checkout has no thirdparty bundle. - Behavior changed: Yes, endpoint restrictions and corrected credential admission. - Does this need documentation: Yes, native OAuth endpoint restrictions are described in the release note above.
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: The pinned AWS SDK does not expose the four-argument constructor on S3Error, so GCP authentication failures prevent BE compilation. Construct AWSError<S3Errors> first and convert it to S3Error, preserving the error details and response codes. FE test compilation also fails because new S3 resource and vault tests use removed JMockit APIs. Migrate those mocks to scoped Mockito instances and retain the credential and no-RPC assertions. ### Release note None ### Check List (For Author) - Test: FE-core Checkstyle, clang-format 16, git diff --check, and PR-wide legacy JMockit scan passed. Constructor APIs verified against AWS SDK 1.11.221 headers. Full build and unit tests unavailable locally because JDK 17 and thirdparty dependencies are missing. - Behavior changed: No - Does this need documentation: No
Contributor
TPC-DS: Total hot run time: 150798 ms |
Contributor
ClickBench: Total hot run time: 24.49 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: S3 resources accept GCS as an alias for GCP, but the cloud object store adapter passes GCS directly to the protobuf Provider valueOf method. The protocol only defines GCP, so the alias parity test fails with IllegalArgumentException and the corresponding cloud conversion cannot succeed. Map the alias to Provider.GCP, normalize case with Locale.ROOT, and assert the protocol provider in the existing four-alias test. Keep the protobuf schema and other provider validation unchanged. ### Release note Fix cloud object store conversion for the GCS provider alias. ### Check List (For Author) - Test: FE core Maven Checkstyle passed with zero violations; git diff --check passed. Targeted run-fe-ut.sh execution was attempted for S3ResourceTest#testGsAliasesMatchConnectorAndResourceProtocols and CloudObjectStoreAdapterParityTest but stopped before tests because the local Thrift compiler is 0.16.0 while the repository requires 0.24.0. - Behavior changed: Yes; GCS/gcs maps to the existing GCP protocol value. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of exact head cd814029aad66e1f753e12b612591ed09297ef64, covering all 147 authoritative changed files. Request changes: one new P2 is inline, and three previously reported P1s remain independently confirmed. No duplicate inline comments are reposted.
Existing blockers: internal-stage credential readback still drops the native envelope in get_stage; COPY scan properties still omit the native source/account; derived-instance retry still exits on equal root keys before completing a previously failed cascade. The two stage handoffs are also covered by the existing consolidated P2 thread.
Critical checkpoint conclusions:
- Goal and proof: native ADC/Compute Engine/impersonation is wired through current FE, BE, Cloud and Iceberg clients, with broad added unit/IAM coverage. The new request-time exception and existing stage/cascade defects prevent considering the feature complete.
- Scope and reuse: the broad change remains centered on object authentication; shared C++ credential visitors/factory, API identity descriptions, host-only wire codecs and the shared Iceberg factory provide reuse and clear dependency direction.
- Concurrency: resource ALTER serializes snapshot/validation/publication; publication and policy read locks release in finally blocks. BE client creation stays outside the cache lock. The token mutex protects refresh/account access; no additional lock cycle or race was substantiated.
- Lifecycle and static initialization: cached clients own token providers and signing callbacks without a cycle; no new cross-translation-unit static dependency was found. Existing plugin/scanner ownership and classloader boundaries were traced.
- Configuration: new selectors are per-storage properties, with SDK token refresh and client reconstruction on supported updates; no new daemon dynamic configuration is introduced. Native/HMAC resource migration restrictions remain covered by an existing thread.
- Compatibility: explicit AWS/HMAC paths remain. Native activation requires the documented coordinated MetaService and relevant FE/BE upgrade; there is no hard consumer-capability gate. Native Hudi/Paimon and FE presigned uploads are explicitly deferred. The implicit anonymous-to-ADC upgrade concern remains covered by its existing P2 thread.
- Protocol IDs: every touched definition was compared with branch-3.1, 4.0, 4.1 and 4.2. The new definitions/fields match 4.2, are absent on older branches, and all additions are optional; no reused ID, changed existing meaning or required addition was found.
- Parallel paths: reviewed TVF, LOAD, OUTFILE, EXPORT, policies/resources, vault CREATE/ALTER/SHOW CREATE, stages, recycler and Iceberg catalog/FileIO paths. COPY omissions remain blockers; independent AWS table routing and explicit HadoopFileIO GCS aliases remain existing P2 reports.
- Conditions and error handling: conflict/clear checks have concrete scenarios and preserve rejected vault metadata. Ordinary token/status failures propagate, but malformed-ADC construction remains an existing P2 and the new inline identifies a separate uncaught request-time failure.
- Test coverage: inspected all changed tests, including source/target selection, partial/empty ALTER, replay, policy batching, URI normalization, negative endpoint/auth cases, real service-account OAuth exchange and IAM readback. Missing failure coverage is identified in the inline and existing reports. The role-only regression transition still lacks its static-key prerequisite, already reported.
- Expected results: no golden .out results changed. Assertions and exact signed-URL expectations were inspected; no successful test execution is claimed. The existing GCS alias parity test still reaches unsupported Cloud enum encoding despite its whitelist repair.
- Observability: token failures use rate-limited logs; signing errors preserve bounded IAM context and local error-log fallback is explicit. No additional mandatory metric or high-volume logging defect was substantiated.
- Persistence and failover: resource maps retain EditLog/Gson persistence; vault candidate copies prevent rejected rename/auth updates from being published. Native-to-static instance cleanup is repaired. The root/derived transaction split retains the existing retry blocker.
- Writes and atomicity: existing storage-write/transaction and recycler mark/delete ordering are unchanged. Authentication failures affect access; the new token exception can terminate a BE upload worker process instead of returning failure. No new visible-version or delete-bitmap algorithm is involved.
- FE/BE transport: native identity is carried by maps, Thrift and Cloud PB; policy reset now copies provider and credential. The manually constructed COPY map and stage response remain the known exceptions.
- Performance and memory: no new per-row or bulk allocation work was found; refresh caching bounds normal token work. Client setup adds small configuration copies. Link correctness, final ELF/glibc 2.17 compatibility, runtime behavior and live IAM success cannot be proved by this static review.
- Other concerns and focus: no additional user focus was supplied. Alias session-token clearing, GCS Cloud provider encoding, legacy static-vault ALTER and the other revalidated P2 concerns remain duplicate-fenced in existing threads. No unresolved new candidate or unread assigned file remains in the proposed review set.
Validation limits: no builds, tests, cluster operations or repository source modifications were performed. Packaging and ABI conclusions are limited to source/dependency inspection; added tests and author-reported runs are not substituted for execution here.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #68540 (comment), #68540 (comment), #68540 (comment)
| // threads. Serialize access even if the underlying implementation changes | ||
| // its thread-safety guarantees in a future google-cloud-cpp release. | ||
| std::lock_guard lock(_credentials_mutex); | ||
| auto token = _credentials->GetToken(std::chrono::system_clock::now()); |
Contributor
There was a problem hiding this comment.
[P2] Catch token-refresh exceptions before they escape upload workers
With DEFAULT ADC pointing to service-account JSON whose private_key is a nonempty malformed PEM, client construction succeeds, but the first object request reaches this uncaught GetToken(). The pinned SDK's OAuth refresh path signs via .value() and throws on that key; its cache wrapper does not catch it. A small BE upload reaches this through UploadFileBuffer::on_upload → S3FileWriter::_put_object → GcpS3Client::PutObject, and the upload thread pool has no exception handler, so a bad BE credential file terminates the backend instead of returning GcpAuthenticationError. Convert exceptions at the token-fetch boundary and cover a malformed nonempty PEM through real ADC construction and an object request. This is distinct from the existing construction-time ADC report: this JSON constructs successfully, and a constructor-only catch cannot cover the later request.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #68540 at 67767d4. Request changes because previously reported blocking issues still apply. Two new P2 findings identify independent AWS Glue control-plane regressions: native GCP storage overwrites Glue's region, and anonymous GCS storage replaces Glue's credentials.
Existing blockers independently confirmed, without reposting:
- 4119930342: INTERNAL get_stage reconstructs ObjectStoreInfoPB without the native credential envelope.
- 4119930347: CopyIntoInfo.validateStagePB separately omits native credentials from the BE broker-property map. These two handoffs are also covered by existing thread 4231751519.
- 4208967423: a root native-to-HMAC key update commits before derived-instance transactions; an identical retry exits on equal root keys before finishing the cascade.
Applicable code-review checkpoints:
- Goal and evidence: native DEFAULT/COMPUTE_ENGINE and impersonation reach the main FE, BE and recycler paths; added unit/regression cases cover modes, aliases, conflicts, partial updates and signing. The two new findings show Glue metadata access remains broken in the specified cases.
- Scope and reuse: the broad change fits the cross-process feature. Provider-owned authentication, shared identity descriptions and the separate host protocol module preserve useful boundaries.
- Concurrency: resource ALTER serializes snapshot/validation/publication; map and version publish under writeLock. Policy conversion releases readLock in finally. Google token/account access is mutex-protected. No additional lock-order defect found.
- Lifecycle and initialization: clients own refreshable providers; signer callbacks introduce no observed ownership cycle. TCCL restoration uses finally; no new cross-translation-unit initialization dependency identified.
- Configuration: new authentication settings are per-storage properties. Resolved mode and impersonation participate in cache identity and client reset; no new process-wide dynamic setting was added.
- Compatibility: optional Cloud field 20 and Thrift field 16, their messages and enum values were compared with branch-3.1/4.0/4.1/4.2. No field-ID collision or required-field addition found. Plugin API versions/baselines were updated.
- Parallel paths: map, policy Thrift, Cloud protobuf, Hadoop and serialized Iceberg FileIO paths were traced. Existing explicit-HadoopFileIO alias and mixed-AWS-table routing reports remain duplicate context.
- Conditions and error behavior: effective-state validation and explicit-clear semantics were inspected. ADC construction/refresh exceptions and blank session-token alias handling remain covered by existing P2 threads; no duplicate comments added.
- Test coverage: positive, negative, replay and failed-update atomicity tests were inspected. The new Glue test substitutes local object transport and never exercises Glue API authentication or region selection, so it misses both new findings.
- Expected results: no changed .out files; assertions and expected values were reviewed statically. The existing role-only test configuration prerequisite report remains known.
- Observability: credential/outcome and IAM-signing diagnostics exist; signing failure retains the local error-log path. No separate new observability defect verified.
- Persistence and failover: resources journal/replay snapshots; vault changes validate candidate metadata before persistence. The existing derived-instance retry blocker remains.
- Writes and atomicity: rejected vault credential/rename patches do not publish partial metadata; resource publication follows validation/connectivity checks. Partition visibility, rowset versions and MoW delete-bitmap semantics are unchanged.
- FE/BE propagation: typed selectors/targets reach normal backend/protocol paths and the scanner factory is now packaged. Internal-stage response and COPY scan-map omissions remain the existing blockers above.
- Performance and memory: token caching, cache equality, client ownership, request construction and dependency changes were inspected; no further concrete hot-path or significant untracked-allocation defect established.
No additional user-provided focus was supplied. The declared exclusions remain relevant: native Hudi/Paimon and FE OAuth presigned uploads are unsupported; native credentials require upgraded consumers before enablement; omitted GCS authentication now selects ADC and public access requires explicit ANONYMOUS. Historical capability/upload reports are treated as those declared deployment limits. The former service-account self-signed-JWT, MinIO policy mapping, GCS alias, Glue Hadoop-configuration and scanner wire-codec reports are repaired at this head.
Validation was static only as required: no builds, tests, live IAM operations or source modifications. Runtime integration, produced-ELF glibc compatibility and the author's uncompleted build/test matrix remain unvalidated.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #68540 (comment), #68540 (comment), #68540 (comment)
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: An Iceberg Glue catalog using native GCP storage overwrote Glue's AWS region with GCS's region. Anonymous GCS storage also replaced Glue's authenticated credential provider. Preserve Glue's metadata settings, install a FileIO-only anonymous client factory, and keep its synchronous and asynchronous clients independent of Glue's static keys or assume-role factory. ### Release note Preserve AWS Glue authentication and region when Iceberg objects use native GCP credentials or anonymous GCS access. ### Check List (For Author) - Test: Checkstyle, connector import boundary, git diff --check, and isolated JDK 17 SDK request/serialization smoke passed. Added three request-level regression tests. Standard FE and connector-reactor tests stopped before these tests at the installed Thrift 0.16.0 versus required 0.24.0 mismatch. No live cloud integration tests were run. - Behavior changed: Yes, Glue keeps its AWS metadata identity and region while GCS FileIO uses the selected storage authentication. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes. This static review found two new P2 issues: Glue metadata role settings are rejected as GCS storage credentials, and anonymous GCS storage disables independent REST SigV4 authentication. Seven previously reported P1 failures still apply and are referenced without reposting them.
Existing blocking comments independently confirmed at b14c332:
- 4119930342 and 4119930347: internal-stage responses omit the native envelope, and COPY scan properties omit its source/impersonation fields.
- 4208967423: an equal-root key-update retry returns before repairing a failed derived-instance cascade.
- 4215142986: native GCP internal-stage uploads cannot obtain a working presigned PUT URL. The PR documents this unsupported path; the existing failure remains.
- 4217722279, 4217722285, and 4217722293: Cloud publication, storage-policy pushes, and map-based jobs lack native-GCP consumer gates/routing during mixed-version operation. The documented upgrade-before-enabling requirement is operational guidance; these paths do not enforce it.
Critical checkpoint conclusions:
- Goal and evidence: native DEFAULT/COMPUTE_ENGINE/impersonation flows are implemented across filesystem/auth, codecs, BE and Recycler. Added tests cover many flows, but the new Glue-role test necessarily fails during binding and known stage/upgrade failures prevent complete end-to-end support.
- Scope and abstraction: the broad change is cohesive; provider-owned rules, data-only identity descriptions, host-only wire codecs, and shared C++ client construction separate responsibilities appropriately.
- Concurrency: token access is mutex-protected; client creation occurs outside the cache lock; resource ALTER serializes snapshot/validation/publication and uses a short write-lock publication. No additional lock-order defect was established.
- Lifecycle/static initialization: clients and signing providers use bounded shared ownership without a new cycle; FileIO factories serialize property maps; lazy local statics avoid a new cross-translation-unit initialization dependency.
- Configuration: native source and impersonation are user properties, included in authentication cache identity and propagated by resource versions/catalog resets. No new global dynamic configuration knob was added.
- Compatibility: plugin API majors and filesystem surface baseline are updated. Existing AWS/HMAC paths and explicit S3/MinIO selections have targeted coverage; mixed-version native feature support remains blocked by the existing comments above.
- Thrift/Protobuf: touched definitions were read on HEAD and current branch-3.1, branch-4.0, branch-4.1, and branch-4.2. New fields are optional, IDs are unused on older branches, and branch-4.2 assigns the same meanings. No field-ID/enum collision found. Schema compatibility does not supply feature negotiation.
- Parallel paths: TVF, LOAD, EXPORT, OUTFILE, resources, vaults, stages, Recycler, Iceberg namespace/FileIO, and scanner serialization were traced. COPY and metadata-versus-storage authentication gaps are recorded. Native Hudi/Paimon catalog support is explicitly deferred in this PR; historical reports for that deferred support are not new findings here.
- Conditions/error handling: candidate-copy vault mutation, explicit clear presence, and finally-based resource unlock improve failure handling. Known ADC construction/refresh exception reports 4248820599/4356259111 remain duplicate context; the new service-account OAuth exchange fixes the prior self-signed-JWT P1.
- Test coverage: property/conflict/clear/replay/cache/serialization/signing unit coverage and IAM regression suites are extensive. The Glue-role test exposes the first new finding; REST SigV4 plus anonymous storage lacks equivalent coverage. No builds or tests were executed under the static-only review contract.
- Test results: no golden .out result files are changed, and no runtime result is claimed. Some regression suites retain assertion-based fixed results and cleanup-after-use despite repository conventions; the existing role-only/static-key fixture concern remains known context.
- Observability: authentication and IAM failures retain error context, and failed signing falls back with a warning. No separate missing-log/metric defect was substantiated.
- Persistence/failover: resource identity uses existing Gson/EditLog storage; credential codecs preserve optional presence. Vault changes stage mutations before commit. The known derived-instance retry gap still prevents reliable repair after a partial cascade.
- Writes/atomicity: no row-transaction, visible-version, or delete-bitmap algorithm changes. Vault rename/auth updates reject before publication; object writes still depend on correct identity and consumer support, as recorded above.
- FE/BE handoffs: native fields are carried by backend maps, TS3StorageParam and ObjectStoreInfoPB and consumed by both C++ converters. The two internal-stage/COPY handoffs and old-consumer routing remain exceptions.
- Performance/memory: cached credentials avoid repeated exchanges; binding loops are small and outside row processing. No new unbounded data-sized allocation, ownership leak, or per-row work was established. Real network refresh latency was not measured.
- Other review points: the new Glue and REST findings are distinct from the repaired Glue region/anonymous-provider threads. Previously reported MinIO policy batches, explicit S3 routing, legacy DEFAULT, scanner factory/codec closure, GCS aliases, signing CA/cluster-header/default-port/key handling have source-level repairs and targeted tests.
Flow reviewed:
SQL properties -> filesystem binding/auth -> typed identity -> host Thrift/Cloud codec -> BE/Recycler token provider -> GCS XML
-> native Hadoop configuration / Iceberg FileIO -> metadata and object operations
User focus: review_focus.txt specifies no additional focus; the complete PR was reviewed. Coverage limits are static-only: compilation, final plugin/ELF ABI (including glibc 2.17), real ADC/IAM/GCS transport behavior, and live cluster regressions remain unexecuted. Convergence is reported separately from delivery and the change-request verdict.
Review converged after 3 round(s).
Existing P0/P1 findings confirmed for this head: #68540 (comment), #68540 (comment), #68540 (comment), #68540 (comment), #68540 (comment), #68540 (comment), #68540 (comment)
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Merge master 31615e0 into the native GCP IAM branch. Both branches independently advanced the connector plugin API to 20.0 for different surfaces. Use 21.0 for the combined surface and update its pinned version test, preserving master's metadata-delete API and the PR's typed filesystem credentials. ### Release note None ### Check List (For Author) - Test: Connector SPI reactor validate and Checkstyle passed with zero violations; connector import and metadata-funnel gates passed; all 138 PR-only files remain identical and PR patches in all nine automatically merged overlap files pass reverse-apply checks; git diff --check against master passed. SPI reactor unit tests were attempted but stopped before SPI at the Thrift compiler version gate (installed 0.16.0, required 0.24.0). - Behavior changed: No; reconcile API versioning for the combined surface. - Does this need documentation: No
…cation ### What problem does this PR solve? Related PR: apache#68540 Problem Summary: GCS property validation incorrectly treats Glue roles and Glue/REST credential-provider selectors as object-storage authentication. This rejects otherwise independent metadata settings and prevents the Glue-role FileIO test from reaching client construction. Add a storage credential view for validation and canonical credential selection while retaining the original raw properties and rejecting AWS storage conflicts. Anonymous GCS also overwrites the shared credential provider used for REST SigV4 authentication. Configure the existing anonymous S3 FileIO factory for every catalog flavor, preserving metadata client credentials. Add coverage for metadata-scoped options across native, anonymous and HMAC GCS modes, REST default/selected providers, and signed REST requests alongside unsigned object requests. Update the existing anonymous FileIO expectation and use URLConnection for local transport tests. ### Release note Fix GCS catalogs rejecting independent Glue authentication settings and anonymous GCS object access disabling REST catalog SigV4 authentication. ### Check List (For Author) - Test: GCS reactor tests passed, including 56 GCS and 152 S3-base tests; Checkstyle passed for all affected modules with zero violations; connector import and metadata-funnel gates and git diff --check passed. An isolated Iceberg SDK transport probe passed signed REST and unsigned object requests plus default/selected metadata provider checks. That probe uses expected SDK options and does not replace the connector suite. Full connector tests were attempted but blocked by Maven dependency TLS failures while downloading Jackson 2.21.8, before connector test execution. - Behavior changed: Yes; metadata authentication remains independent of GCS. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #68540 at 33dc40dbc201aac19402ab15b2076305fd4da47a: request changes for one new P1 compatibility regression and two independently reconfirmed existing P1 comments. All 151 changed paths, their relevant callers/consumers, tests, existing threads and raw comments were reviewed.
The new inline finding concerns a previously valid GCP HMAC resource with a dormant legacy ANONYMOUS selector: the new resolver rejects it and prevents the complete policy/resource batch from being sent. This differs from the repaired legacy DEFAULT-without-keys and MinIO reports.
Existing blockers were not reposted: 4119930342 still loses the native credential in the internal-stage get_stage response; 4119930347 still omits it from COPY's backend scan properties. Both are also covered by existing thread 4231751519. They can select ADC instead of the configured Compute Engine or impersonated identity.
Applicable code-review checkpoints:
- Goal and proof: native ADC/Compute Engine/impersonation is wired through the ordinary filesystem, SQL, vault, BE and recycler paths. The new compatibility regression and existing COPY omissions prevent a clean correctness verdict. Added tests show intended coverage; they were not executed here.
- Scope and reuse: the broad change follows the required cross-process path. Shared credential descriptions, provider-owned rules, common native client/signing code and host-owned protocol codecs provide coherent reuse.
- Concurrency: resource ALTER serializes snapshot/validation/publication; publication uses the resource write lock. Validity IO stays outside that rwlock. Request threads serialize credential refresh with its mutex. No additional concrete lock inversion was found.
- Lifecycle and initialization: immutable API descriptions avoid sharing SDK objects across plugin boundaries; client-owned signing callbacks show no ownership cycle. No new cross-TU initialization dependency was identified.
- Configuration: new selectors are per-object properties and IAM regression configuration, with no new mutable server-wide setting. Authentication participates in client/cache identity and client replacement follows existing lifecycle.
- API/storage compatibility: plugin versions/baselines are updated and wire additions are optional. Native auth requires the documented upgrade of consumers before activation; there is no automatic capability gate. Legacy compatibility still needs the inline fix.
- Protocol IDs: touched definitions were compared on HEAD and release branches 3.1, 4.0, 4.1 and 4.2; no field/enum collision, reuse or new required field was found.
- Parallel paths: TVF/load/export/outfile/repository/resource/vault and BE/recycler paths were traced, including Iceberg metadata clients, sync/async FileIO, Hadoop namespace operations and serialized scanner dependencies. COPY retains the two known omissions.
- Conditional checks: native/static conflicts, endpoint restrictions, explicit clears and effective ALTER validation have concrete failure scenarios. Legacy selector precedence is the new exception; legacy static DEFAULT vault ALTER overvalidation is already reported in 4248820622.
- Test coverage: unit and regression additions cover positive/negative auth, aliases, refresh/cache seams, serialization and ALTER atomicity. Missing legacy ANONYMOUS-plus-static replay/batch coverage corresponds to the new finding. Real IAM, full builds and packaged scanner execution remain unverified.
- Expected results: no generated regression result artifacts changed; assertions and local fake transports were inspected statically.
- Observability/errors: normal token, SDK and IAM failures have diagnostics and signing failure retains the local error-log path. Existing construction/refresh exception reports 4248820599 and 4356259111 remain applicable and were not duplicated.
- Persistence/failover: resources retain the existing journaled property-map representation; vault updates validate candidate metadata before persistence. The separate derived-instance cascade retry behavior predates this PR.
- Writes/atomicity: failed vault credential changes cannot persist an earlier rename; failed resource validity checks preserve the prior properties/version. Existing object write machinery remains in use. No rowset visibility or delete-bitmap algorithms changed.
- FE-BE values: maps, Thrift and Cloud conversions carry source/impersonation through ordinary paths; COPY's manual handoffs are the confirmed gaps above.
- Performance/memory: token caching, bounded property/request copies and client reuse were examined; no additional substantiated performance or significant memory-ownership defect was found. Native linkage and glibc 2.17 ELF compatibility require artifact validation.
- Other findings and limits: existing mixed-AWS Iceberg routing, explicit HadoopFileIO GCS aliases, resource auth-mode transition and session-token-clear issues remain duplicate reports. Current Glue/REST separation, MinIO mapping, explicit S3 routing, direct service-account OAuth exchange and Iceberg factory/codec packaging fixes were verified at source level. Native Hudi/Paimon and FE native presigned uploads are expressly unsupported; omitted GCS authentication now intentionally selects ADC.
There was no additional user-provided focus. This was static-only: no builds, tests, cluster runs, style gates or ELF/package checks were executed, and no repository source was modified. No unreviewed changed-file scope or unresolved new source candidate remains.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #68540 (comment), #68540 (comment)
| mode = type == GcpCredentialProviderType.ANONYMOUS ? GcsAuth.Mode.ANONYMOUS | ||
| : type == GcpCredentialProviderType.COMPUTE_ENGINE ? GcsAuth.Mode.COMPUTE_ENGINE : GcsAuth.Mode.ADC; | ||
| } else if (hasLegacyAnonymous) { | ||
| mode = GcsAuth.Mode.ANONYMOUS; |
Contributor
There was a problem hiding this comment.
[P1] Preserve static GCS keys over the legacy anonymous selector
A persisted provider=GCP resource with nonempty s3.access_key/s3.secret_key and s3.credentials_provider_type=ANONYMOUS was accepted before this PR: the static pair took precedence, and policy serialization ignored the dormant selector. This branch now chooses ANONYMOUS first and throws on the keys at lines 85-87. After replay/restart, PushStoragePolicyTask.toThrift() therefore fails for that resource; because ReportHandler batches all changed resources and policy updates/drops together, unrelated updates also never reach the BE. Preserve legacy static-key precedence when no native gs.* credential selector is supplied, and cover replay plus a batched push. This differs from the repaired DEFAULT-without-keys thread: it involves a static pair and the legacy ANONYMOUS branch.
### What problem does this PR solve? Related PR: apache#68540 Problem Summary: Older FEs accepted GCS resources containing static HMAC keys and a dormant legacy ANONYMOUS credential selector. Resolving the selector before the keys rejected those persisted resources after upgrade and aborted batched storage policy updates. Resolve legacy static keys first while keeping explicit native GCP authentication conflicts rejected. Cover credential aliases, partial patches, metadata replay, and batched resource/policy updates and drops. ### Release note Preserve access to legacy GCS resources with static keys and an anonymous selector when upgrading to native GCP authentication support. ### Check List (For Author) - Test: A 192-case source probe fails before the fix and passes after it; Java parsing, FE Core/GCS Checkstyle, and git diff --check passed. Added resolver and replay/batched-push unit tests. The standard FE UT runner stopped during Thrift code generation with the installed 0.16.0 compiler; FE tests were not executed. - Behavior changed: Yes, restore legacy static-key precedence without weakening explicit native GCP credential conflict validation. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of the 151 supplied changed paths at 0505a9fd63ee0926bda7107909ebc5d111f43bd8.
One new P2 is reported inline: HMAC GCS FileIO inherits the Glue metadata assume-role factory. Three previously reported P1 issues were independently confirmed and remain blockers; they are carried forward without duplicate comments:
- Internal-stage identity loss:
get_stageomits the native credential envelope. - COPY scan identity loss:
CopyIntoInfodrops the native identity from BE broker properties. - Incomplete derived-instance updates: an equal-key retry returns before reconciling children after the root committed.
Critical checkpoint conclusions:
- Goal and proof: native ADC/Compute Engine/impersonation is wired through FE, BE and Cloud, with unit and IAM regression coverage added. The identity handoff defects above prevent a complete correctness conclusion; inspected tests are not execution proof.
- Scope and reuse: the broad change is cohesive around object authentication. Shared credential variants, typed API descriptions and host-only codecs reduce duplication; HMAC FileIO separation is incomplete.
- Concurrency and locks: reviewed resource ALTER monitor serialization, read/write publication locks, policy-push unlocks, client-cache creation/reset and token-provider mutexes. No additional lock-order defect was verified. SDK refresh is serialized for credential safety and can block callers during renewal.
- Lifecycle and static initialization: client/signing providers have explicit shared ownership and no new ownership cycle or cross-translation-unit initialization dependency was found.
- Configuration and dynamic changes: no new process-level dynamic configuration was introduced. Credential changes rebuild clients; partial native source/impersonation fields preserve omission and explicit clears. Existing CA settings reach OAuth and signing.
- Compatibility: legacy AWS/HMAC fields remain. Native activation requires updated MS and relevant FE/BE consumers, as documented; unsupported activation on older consumers is a deployment limit rather than a new blocker.
- Thrift/Protobuf IDs: touched definitions were read on HEAD and branches 3.1, 4.0, 4.1 and 4.2. Additions are optional; no field/enum collision or reuse was found.
- Parallel paths: reviewed map, Thrift policy, protobuf vault/instance, recycler, stage/COPY, Iceberg FileIO/Hadoop, export/outfile and error-log signing paths. Stage/COPY omissions remain the known failures above.
- Conditions and errors: checked auth conflicts, alias precedence, empty/omitted patches, trusted endpoint selection, signer identity and failure fallback. Existing construction/refresh exception reports and partial-ALTER compatibility threads remain duplicate-fenced.
- Test coverage: added unit tests cover codecs, routing, legacy replay/push, partial ALTER, signing, credential reset and metadata/object client separation; regressions cover major IAM operations. HMAC object reads with a Glue role are missing, and malformed-ADC exception coverage remains a known gap.
- Test results: no golden result files are changed. Tests, builds and format/static-analysis tools were not run under this static-only contract.
- Observability: token errors are rate-limited, request failures use SDK/status paths, and signing failures log context and retain the local error-log path. Existing uncaught SDK exception paths still bypass normal error reporting.
- Persistence and failover: reviewed resource replay, credential protocol copies, vault candidate updates and metadata publication. The root/derived cascade retry defect remains; no additional EditLog mismatch was verified.
- Writes and atomicity: rejected vault credential updates preserve names/auth through candidate-copy validation and transactional publication. Cross-instance key updates retain the known reconciliation failure. Rowset visibility, delete-bitmap and recycler mark/delete algorithms are not modified.
- FE/BE transmission: native fields are propagated through the changed map/Thrift/Protobuf builders and decoders; the manual COPY builder remains the verified exception.
- Performance and memory: per-client credential caching and existing rate-limit wrappers are retained. No new large untracked buffer, cache ownership defect or data-size-dependent hot-loop regression was verified statically; runtime performance was not measured.
- Other integration concerns: Java scanner dependencies, wire-codec isolation, plugin surface/version changes and C++ source/link inventories were inspected. Final plugin packaging, linking and GLIBC_2.17 ELF compatibility were not executed or proven.
- User focus and limits: no additional user focus was supplied. Native Paimon/Hudi integration and FE presigned uploads are explicitly outside the PR's supported scope. Real IAM exchanges and end-to-end cluster behavior remain unexecuted validation limits.
Earlier reports for static-key precedence, explicit S3 routing, MinIO aliases, service-account OAuth exchange, Glue/REST anonymous-client isolation, Glue region/config handoff and scanner codec packaging are repaired on this head. The new inline concerns the separate HMAC branch.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #68540 (comment), #68540 (comment), #68540 (comment)
| Optional<GcsAuth> auth = GcsAuthResolver.resolve(backend.toMap()); | ||
| if (auth.filter(GcsAuth::isAnonymous).isPresent()) { | ||
| // Glue and REST metadata can require AWS authentication even when GCS objects are public. | ||
| opts.put(S3FileIOProperties.CLIENT_FACTORY, AnonymousS3FileIOAwsClientFactory.class.getName()); |
Contributor
There was a problem hiding this comment.
[P2] Keep HMAC GCS FileIO separate from the Glue role
For a Glue catalog with provider=GCP, valid gs.access_key/gs.secret_key, glue.role_arn and a Glue endpoint/region, the new credentialProperties() filter permits the configuration, but HMAC enters neither factory override here. appendGlueProperties leaves client.factory=AssumeRoleAwsClientFactory, and Iceberg 1.12's FileIO factory selection falls back to it. That factory's synchronous S3 client uses the AWS assumed role instead of the configured GCS HMAC pair, so reading a private GCS table with an s3:// metadata location fails although Glue authentication succeeds. Give HMAC GCS its own FileIO credential path and cover an object read with role-authenticated Glue. This is distinct from the repaired native/anonymous branches.
Contributor
TPC-H: Total hot run time: 28350 ms |
Contributor
TPC-DS: Total hot run time: 150750 ms |
Contributor
ClickBench: Total hot run time: 25.2 s |
Contributor
TPC-H: Total hot run time: 29214 ms |
Contributor
TPC-DS: Total hot run time: 150707 ms |
Contributor
ClickBench: Total hot run time: 24.36 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
GCS access currently depends on HMAC credentials or unsigned requests. A deployment using Application Default Credentials or a Compute Engine service account cannot consistently use the same identity for object reads and writes, Iceberg metadata operations, storage vaults, and backend error-log downloads.
Add native GCP OAuth authentication across FE, BE, and Cloud using
gs.credential_provider_type=DEFAULTorCOMPUTE_ENGINE, with optionalgs.impersonation_service_account. GCP storage without explicit authentication defaults to Application Default Credentials. Public-bucket access requires explicitANONYMOUS; existing HMAC configuration remains supported. Reject conflicting native, HMAC, and AWS-only authentication properties before creating clients.The implementation follows the current master architecture:
gs://in EXPORT and OUTFILE while preserving HTTP paths. Generate backend GCS V4 error-log URLs through IAMsignBlob, falling back to the local error-log path if signing fails.Example native authentication properties:
The impersonation property is optional. Use
COMPUTE_ENGINEto select the attached VM identity explicitly, orANONYMOUSfor public object access.Compatibility and deployment:
Release note
Add native GCP IAM authentication with Application Default Credentials, Compute Engine identities, and optional service-account impersonation for GCS-backed storage. GCP configurations without HMAC or an explicit authentication mode now use ADC; public access must select
ANONYMOUS. Improve resource/vault credential updates and support GCS error-log signing.Validation
Passed locally:
git diff --check.Not completed locally:
Check List (For Author)