Unify create/loadTable call paths #2589
Merged
Conversation
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
adutra
previously approved these changes
Sep 17, 2025
| LoadTableResponse.Builder responseBuilder = | ||
| LoadTableResponse.builder().withTableMetadata(tableMetadata); | ||
|
|
||
| if (!delegationModes.contains(VENDED_CREDENTIALS)) { |
Contributor
There was a problem hiding this comment.
Probably safer when more delegation modes get supported:
Suggested change
| if (!delegationModes.contains(VENDED_CREDENTIALS)) { | |
| if (delegationModes.isEmpty()) { |
Contributor
Author
There was a problem hiding this comment.
I used .isEmpty() in other cases, but in this method the code below specifically performs credential vending, so I think this check is correct :)
We can certainly refactor when we support remote S3 request signing. WDYT?
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
In preparation for implementing sending non-credential config to REST Catalog clients for apache#2207 this PR unifies calls paths for create/load table operations. This change does not have any differences in authorization. This change is not expecte to have any material behaviour differences to the affected code paths. The main idea is to consolidate decision-making for that to include into REST responses and use method parameters like `EnumSet<AccessDelegationMode> delegationModes` for driving those decisions.
dimas-b
force-pushed
the
refactor-load-table
branch
from
dab8b08 to
1a0d7ea
Compare
Contributor
Author
|
Rebased and resolved conflicts. @adnanhemani : please take a look to double check that I did not break any of your changes from #2480. |
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 18, 2025
This change builds on top of apache#2589 and further prepares Polaris code to support non-STS S3 implementations for apache#2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties.
adutra
approved these changes
Sep 19, 2025
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 19, 2025
This change builds on top of apache#2589 and further prepares Polaris code to support non-STS S3 implementations for apache#2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties.
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 19, 2025
This change builds on top of apache#2589 and further prepares Polaris code to support non-STS S3 implementations for apache#2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties.
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 19, 2025
This change builds on top of apache#2589 and further prepares Polaris code to support non-STS S3 implementations for apache#2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties.
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 19, 2025
This change builds on top of apache#2589 and further prepares Polaris code to support non-STS S3 implementations for apache#2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties.
dimas-b
added a commit
that referenced
this pull request
Sep 22, 2025
…ts (#2615) * Always propagate non-credential properties from AccessConfig to clients This change builds on top of #2589 and further prepares Polaris code to support non-STS S3 implementations for #2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties.
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 29, 2025
This is a follow-up change to apache#2672 and apache#2589 * Fix a big in `delegationModes` parameter propagation in `createTableStaged()` * Add checks (leading to 400) that credentials are vended when requested. * The check is disabled if `SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION` is set * Disable credential vending tests in file-based catalog since the "FILE" storage integration code never vends any credentials. These tests are still executed under `PolarisRestCatalogMinIOIT`
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Sep 30, 2025
This is follow-up bugfix for apache#2589 The bugfix part apache#2711 is extracted here since apache#2711 proved to be non-trivial and may require extra time. * Use the `delegationModes` method parameter as intended (as opposed to a local constant).
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Nov 20, 2025
* Avoid calling deprecated `TableMetadataParser.read(FileIO, InputFile)` method. (apache#2609) Call `read(InputFile)` instead, as instructed by Iceberg javadoc. * Add doc notes about EclipseLink removal (apache#2605) * chore(docs): add polaris-api-specs section (apache#2598) * docs(README): Updating the READMEs to Reflect the Project Structure (apache#2599) * docs(README): Updating the READMEs to Reflect the Project Structure * fix(deps): update dependency io.opentelemetry:opentelemetry-bom to v1.54.1 (apache#2613) * Add Code of Conduct entry to the ASF menu (apache#2537) * Use the ASF Code Of Conduct * Update site/hugo.yaml Co-authored-by: Robert Stupp <snazy@snazy.de> --------- Co-authored-by: Robert Stupp <snazy@snazy.de> * fix(deps): update dependency org.postgresql:postgresql to v42.7.8 (apache#2619) * chore(deps): update dependency mypy to >=1.18, <=1.18.2 (apache#2617) * Update registry.access.redhat.com/ubi9/openjdk-21-runtime Docker tag to v1.23-6.1758133907 (apache#2612) * Introduce alternate in-memory buffering event listener (apache#2574) * fix(deps): update dependency org.assertj:assertj-core to v3.27.5 (apache#2618) * chore(deps): update dependency virtualenv to >=20.34.0,<20.35.0 (apache#2614) * Add Community Meeting 20250918 (apache#2622) * Add 1.1.0-incubating release on the website (apache#2621) * Add 1.1.0-incubating release content (apache#2625) * chore(errorprone): Enabling EqualsGetClass, PatternMatchingInstanceof, and UnusedMethod in ErrorProne (apache#2600) * fix(deps): update dependency com.adobe.testing:s3mock-testcontainers to v4.9.1 (apache#2626) * Unify create/loadTable call paths (apache#2589) In preparation for implementing sending non-credential config to REST Catalog clients for apache#2207 this PR unifies calls paths for create/load table operations. This change does not have any differences in authorization. This change is not expecte to have any material behaviour differences to the affected code paths. The main idea is to consolidate decision-making for that to include into REST responses and use method parameters like `EnumSet<AccessDelegationMode> delegationModes` for driving those decisions. * Remove numeric identifier from PolarisPrincipal (apache#2388) This change removes the requirement for Polaris principals to have a numeric identifier, by removing the only sites where such an identifier was required: - In the `Resolver`. Instead, the `Resolver` now performs a lookup by principal name. - In `PolarisAdminService`. Instead, the code now compares the principal name against the entity name. Note: the lookup in the `Resolver` is still necessary, because the `Resolver` also needs to fetch the grant records. * Include principal name in Polaris tokens (apache#2389) * Include principal name in Polaris tokens Summary of changes: - Instead of including the principal id twice in the token, the principal name is now used as the subject claim. While the default authenticator doesn't need the principal name and works with just the principal id, not having the "real" principal name available could be a problem for other authenticator implementations. - `DecodedToken` has been refactored and renamed to `InternalPolarisCredential`. It is also now a package-private component. - `TokenBroker.verify()` now returns PolarisCredential. * rename to InternalPolarisToken * main: bump to 1.2.0-incubating-SNAPSHOT (apache#2624) * bump version.txt to 1.2.0-incubating-SNAPSHOT * virtualenv: wider version range (apache#2623) see apache#2614 (comment) * Remove ActiveRolesProvider (apache#2390) Summary of changes: - As proposed on the ML, `ActiveRolesProvider` is removed, and `DefaultActiveRolesProvider` is merged into `DefaultAuthenticator`. `ActiveRolesAugmentor` is also merged into `AuthenticatingAugmentor`. - The implicit convention that no roles in credentials == all roles requested is removed as it is ambiguous. Credentials must explicitly include the `PRINCIPAL_ROLE:ALL` pseudo-role to request all roles available. - PersistedPolarisPrincipal is removed. It existed merely as a means of passing the `PrincipalEntity` from the authenticator to the roles provider. This is not necessary anymore. * NoSQL: adaptions * Last merged commit d1d359a --------- Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Artur Rakhmatulin <artur.rakhmatulin@gmail.com> Co-authored-by: Adam Christian <105929021+adam-christian-software@users.noreply.github.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Alexandre Dutra <adutra@apache.org>
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Nov 20, 2025
* Always propagate non-credential properties from AccessConfig to clients (apache#2615) * Always propagate non-credential properties from AccessConfig to clients This change builds on top of apache#2589 and further prepares Polaris code to support non-STS S3 implementations for apache#2589. For S3 implementations that do have STS, this change enables clients to run with local credentials (no credential vending) and still receive endpoint configuration from the catalog. * Call `SupportsCredentialDelegation.getAccessConfig()` on all relevant create/load requests (previously it was called only when `vended-credentials` was requested * Always sent `AccessConfig.extraProperties()` to clients * Expose credentials to clients only when the `vended-credentials` access delegation mode is requested. * There is not client-visible behaviour change for implementations of `PolarisStorageIntegration` that do not produce "extra" `AccessConfig` properties. * [OpenAPI Modification] Return created objects (apache#2603) As per the ML thread [here](https://lists.apache.org/thread/q7q0rrsmw5gcqv30g4hr9ffq3gtr72yk), this PR introduces the change to return all objects that are created within their respective API calls. * chore(docs): reorganize getting-started section (apache#2611) * fix(deps): update dependency org.assertj:assertj-core to v3.27.6 (apache#2651) * fix: fix broken markdown-link-check CI job after apache#2611 got merged (apache#2655) * Release artifacts should use dlcdn.apache.org (signature and checksum must refer downloads.apache.org) (apache#2647) * docs: Update S3 getting started guides (apache#2652) * fix(deps): update dependency com.google.cloud:google-cloud-storage-bom to v2.58.0 (apache#2660) * fix(deps): update dependency org.apache.spark:spark-sql_2.12 to v3.5.7 (apache#2659) * fix(deps): update dependency org.apache.spark:spark-sql_2.12 to v3.5.7 (apache#2658) * Fix Issue 2024 for Rendering Blockquotes (apache#2656) * Fix Issue 2024 for Rendering Blockquotes * Small fix for URLs * Update license * Last merged commit fcb6b33 --------- Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Adnan Hemani <adnan.h@berkeley.edu> Co-authored-by: Artur Rakhmatulin <artur.rakhmatulin@gmail.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Adam Christian <105929021+adam-christian-software@users.noreply.github.com>
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Nov 20, 2025
* (Based on PR#2223)Support Namespace/Table level RBAC for external passthrough catalogs (apache#2673) Creates missing synthetic entities for securables in external passthrough catalogs. Based on Option 1 discussed in the RBAC section of catalog federation design doc. In the future, we could remove calls to PolarisEntity.Builder() and replace them with entities fetched from the remote catalog. (enabling Option 2). --------- Co-authored-by: Pooja Nilangekar <poojan@umd.edu> * Docs: Add more details about v1 schema user to upgrade from 1.0 to 1.1 (apache#2674) * Site: The link https://iceberg.apache.org/concepts/catalog/ doesn't exist anymore. (apache#2683) * Docs: Add analytics for polaris.apache.org (apache#2676) * Make ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS configurable per catalog (apache#2688) * Update ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS to be configurable per catalog * chore(deps): update postgres docker tag to v18 (apache#2692) * fix(deps): update dependency org.eclipse.persistence:eclipselink to v4.0.8 (apache#2682) * fix(deps): update dependency org.apache.logging.log4j:log4j-core to v2.25.2 (apache#2646) * chore(deps): update dependency openapi-generator-cli to v7.15.0 (apache#2410) * chore(deps): update dependency io.quarkus to v3.27.0 (apache#2663) Co-authored-by: Mend Renovate <bot@renovateapp.com> * Publish Develocity builds scans for PRs and local use (apache#2596) This PR enables Develocity build scans for all PRs and contributors w/o an Apache account. CI build scans in the `apache/polaris` repo against branches and tags and having access to the ASF's Develocity secret continue to publish to the ASF's Develocity instance (no behavioral change). All other build scans are published to Gradle's public Develocity instance: - Build scans from local developer (non-CI) runs are only published, if Gradle is invoked with the `--scan` option. - Build scans from or targeting another repository than `apache/polaris` do need be enabled explicity by accepting Gradle's terms of service, via a repository variable, because this is a decision of the owner of a repository. Advanced options to configure another Develocity server or project-ID are available (for non-`apache/polaris` repositories). Detailed instructions in the `README.md`. * Fix & enhancements to the Events API hierarchy (apache#2629) Summary of changes: - Turned `PolarisEventListener` into an interface to facilitate implementation / mocking - Added missing `implements PolarisEvent` to many event records - Removed unused method overrides - Added missing method overrides to `TestPolarisEventListener` * fix(deps): update dependency org.kordamp.gradle:jandex-gradle-plugin to v2.3.0 (apache#2694) * Auth: reorganize internal authentication components (apache#2634) This PR contains no functional and no user-facing change. It is merely a refactor to better organize auth code. Summary of changes: - Moved all internal authentication components to the `org.apache.polaris.service.auth.internal` package and subpackages - Reduced visibility of utility classes - Renamed `TokenBroker` class hierarchy to stick to the naming standard: `<Algorithm>JWTBroker` - Introduced `@PolarisImmutable` whenever appropriate - Removed unused `NoneTokenBrokerFactory` (we already have `DisabledOAuth2ApiService`) - Removed unused `TokenBrokerFactoryConfig` * Enhancement : adding support for Aurora postgres AWS IAM authentication (apache#2650) Add support for postgres AWS IAM authentication using the `apache-client` lib. * Remove unused `name` arg from findCatalogByName in PolarisAdminService (apache#2691) * remove unused name param * Rename for better readability * Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures (apache#2693) * Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures The semantics of the createNonExistingNamespaces method used during sendNotification were supposed to be "create if needed". However, the behavior ended up surfacing an AlreadyExistsException if multiple concurrent sendNotification attempts were made for a brand-new namespace (where the notifications may be different tables). This would cause a table sync to fail if a sibling table was being synced at the same time, even though the new table should successfully get created under the shared namespace. * Also better future-proof the createNamespaceInternal logic by explicitly checking for ENTITY_ALREADY_EXISTS, per review suggestion. Log a less scary message since it's not an error scenario type of race condition, per review suggestion * Client: add credential reset option (apache#2698) * Client: add credential reset option * Client: add credential reset option * Client: add credential reset option * Add integration testing * Fix lint * fix(deps): update dependency software.amazon.awssdk:bom to v2.34.5 (apache#2702) * fix(deps): update dependency com.gradleup.shadow:shadow-gradle-plugin to v9.2.2 (apache#2661) * Support S3 storage that does not have STS (apache#2672) * Support S3 storage that does not have STS This change is backward compatible with old catalogs that have storage configuration for S3 systems with STS. * Add new property to S3 storage config: `stsUnavailable` (defaults to "available"). * Do not call STS when unavailable in `AwsCredentialsStorageIntegration`, but still put other properties (e.g. s3.endpoint) into `AccessConfig` Relates to apache#2615 Relates apache#2207 * Docs/improve idp documentation (apache#2695) * Fix Github links in IDP documentation * Separate IDP docs for usage and development * - Add telemetry config example - Fix link to getting started from landing page - Fix mentioning role-arn as required * Fix some relative links (local Hugo resolves them properly, but PR auto checks still fails) * Docs: narrow down --role-arn usage for AWS S3 only; fix a link in keycloak guide. * Docs: fix a link in keycloak guide. * chore(deps): update gradle/actions digest to 748248d (apache#2708) * Client: fix integration testing (apache#2700) * Add fallback in case the VERSION table is not present (apache#2653) * initial commit * wire up * pastefix * change to postgres specific code * [Catalog Federation] Add feature flag to disallow setting sub-RBAC for federated catalog at catalog level (apache#2696) In apache#2688 (comment), we've identified that configuring polaris.config.enable-sub-catalog-rbac-for-federated-catalogs at catalog level should not be allowed in all cases, especially when the owner is not the same subject as the catalog user or admin. This PR add a feature flag, ALLOW_SETTING_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS to allow owner to disable catalog level setting polaris.config.enable-sub-catalog-rbac-for-federated-catalogs * Fix `delegationModes` parameter propagation in `createTableStaged()` (apache#2713) This is follow-up bugfix for apache#2589 The bugfix part apache#2711 is extracted here since apache#2711 proved to be non-trivial and may require extra time. * Use the `delegationModes` method parameter as intended (as opposed to a local constant). * Generate Request IDs (if not specified); Return Request ID as a Header (apache#2602) * fix(deps): update dependency org.junit:junit-bom to v5.14.0 (apache#2715) * NoSQL persistence: add Java/Vert.X executor abstraction layer (apache#2527) Provides an abstraction to submit asynchronous tasks, optionally with a delay or delay + repetition and implementations based on Java's `ThreadPoolExecutor` and Vert.X. * Fix RDS devservices config + adopt for `:polaris-admin:test` (apache#2723) Changes: * Disables devservices for `:polaris-admin` tests as well, which is necessary to _not_ spin up test containers. * Use the explicit devservices-config as everywhere else. The first bullet point can cause excessive memory usage, especially with more test classes, eventually killing the whole GH runner. * fix(deps): update dependency io.smallrye:jandex to v3.5.0 (apache#2722) * fix(deps): update dependency org.jboss.weld:weld-junit5 to v5.0.2.final (apache#2721) * chore(deps): update quay.io/keycloak/keycloak docker tag to v26.4.0 (apache#2719) * Last merged commit 4024557 * NoSQL: Minor-ish changes to "nodes" projects Adopt nodes projects to OSS PR content * NoSQL: adapt to async package rename * Build: remove unnecessary explicit vertx-core dependency The async-vertx implementation should not propagate a different Vert.X dependency than Quarkus provides. This wouldn't be an issue if we could just use `enforcedPlatform()` for all Quarkus-builds, but sadly we cannot for the spark-plugin-inttests. --------- Co-authored-by: Honah (Jonas) J. <honahx@apache.org> Co-authored-by: Pooja Nilangekar <poojan@umd.edu> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Alexandre Dutra <adutra@apache.org> Co-authored-by: fabio-rizzo-01 <fabio.rizzocascio@jpmorgan.com> Co-authored-by: Dennis Huo <7410123+dennishuo@users.noreply.github.com> Co-authored-by: Yong Zheng <yongzheng0809@gmail.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: olsoloviov <40199597+olsoloviov@users.noreply.github.com> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com> Co-authored-by: Adnan Hemani <adnan.h@berkeley.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
In preparation for implementing sending non-credential config to REST Catalog clients for #2207 this PR unifies calls paths for create/load table operations.
This change does not have any differences in authorization.
This change is not expected to have any material behaviour differences to the affected code paths.
The main idea is to consolidate decision-making for what to include into REST responses and use method parameters like
EnumSet<AccessDelegationMode> delegationModesfor driving those decisions.