Skip to content

Always propagate non-credential properties from AccessConfig to clients#2615

Merged
dimas-b merged 3 commits intoapache:mainfrom
dimas-b:non-sts-s3
Sep 22, 2025
Merged

Always propagate non-credential properties from AccessConfig to clients#2615
dimas-b merged 3 commits intoapache:mainfrom
dimas-b:non-sts-s3

Conversation

@dimas-b
Copy link
Contributor

@dimas-b dimas-b commented Sep 18, 2025

This change builds on top of #2589 and further prepares Polaris code to
support non-STS S3 implementations for #2207.

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.

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Sep 18, 2025
@dimas-b dimas-b changed the title Non sts s3 Always propagate non-credential properties from AccessConfig to clients Sep 18, 2025
@dimas-b dimas-b marked this pull request as ready for review September 19, 2025 15:16
@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testAppendFiles(boolean pathStyle) throws IOException {
// TODO: @CsvSource("true,")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the issue with these test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to uncomment 😄 🤦 - fixed

Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @dimas-b ! LGTM overall! Left minor comments.

@@ -794,10 +794,6 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential
LoadTableResponse.Builder responseBuilder =
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit clutter that this method return a builder, so that caller need to invoke build(). The code would be cleaner if it returns a LoadTableResponse. Not a blocker though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe 🤔 but this PR merely changes the impl. of this method. I did not mean to refactor related code to minimize the amount of changes. It's a private method, we can adjust it any time if current return type become a nuisance 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with a followup, as it's not introduced by this PR.

Comment on lines 298 to 309
if (dm.map(VENDED_CREDENTIALS::equals).orElse(false)) {
assertThat(loadTableResponse.config())
.containsEntry(
REFRESH_CREDENTIALS_ENDPOINT,
"v1/" + catalogName + "/namespaces/test-ns/tables/t1/credentials");
assertThat(loadTableResponse.credentials()).hasSize(1);
} else {
assertThat(loadTableResponse.config()).doesNotContainKey(SECRET_ACCESS_KEY);
assertThat(loadTableResponse.config()).doesNotContainKey(ACCESS_KEY_ID);
assertThat(loadTableResponse.config()).doesNotContainKey(REFRESH_CREDENTIALS_ENDPOINT);
assertThat(loadTableResponse.credentials()).isEmpty();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the validation logic to the caller(line 238)? I think it makes more sense to validate them, as the AccessDelegationMode is one of the test parameter.
Also I think we need to validate the ENDPOINT in case AccessDelegationMode is VENDED_CREDENTIALS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Endpoint is validated on line 296

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved credential asserts to top-level test methods 👍

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.
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 22, 2025
@dimas-b dimas-b merged commit 5ca3fdc into apache:main Sep 22, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Sep 22, 2025
@dimas-b dimas-b deleted the non-sts-s3 branch September 22, 2025 17:54
dimas-b added a commit to dimas-b/polaris that referenced this pull request Sep 25, 2025
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
Closes apache#2207
dimas-b added a commit to dimas-b/polaris that referenced this pull request Sep 25, 2025
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
Closes apache#2207
dimas-b added a commit to dimas-b/polaris that referenced this pull request Sep 26, 2025
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
Closes apache#2207
dimas-b added a commit that referenced this pull request Sep 29, 2025
* 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 #2615
Relates #2207
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 <[email protected]>
Co-authored-by: Adnan Hemani <[email protected]>
Co-authored-by: Artur Rakhmatulin <[email protected]>
Co-authored-by: Mend Renovate <[email protected]>
Co-authored-by: JB Onofré <[email protected]>
Co-authored-by: Adam Christian <[email protected]>
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 <[email protected]>

* 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 <[email protected]>

* 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. <[email protected]>
Co-authored-by: Pooja Nilangekar <[email protected]>
Co-authored-by: Prashant Singh <[email protected]>
Co-authored-by: JB Onofré <[email protected]>
Co-authored-by: Mend Renovate <[email protected]>
Co-authored-by: Alexandre Dutra <[email protected]>
Co-authored-by: fabio-rizzo-01 <[email protected]>
Co-authored-by: Dennis Huo <[email protected]>
Co-authored-by: Yong Zheng <[email protected]>
Co-authored-by: Dmitri Bourlatchkov <[email protected]>
Co-authored-by: olsoloviov <[email protected]>
Co-authored-by: Eric Maynard <[email protected]>
Co-authored-by: Adnan Hemani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants