Always propagate non-credential properties from AccessConfig to clients #2615
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
dimas-b
changed the title
adutra
reviewed
Sep 19, 2025
| @ParameterizedTest | ||
| @ValueSource(booleans = {true, false}) | ||
| public void testAppendFiles(boolean pathStyle) throws IOException { | ||
| // TODO: @CsvSource("true,") |
There was a problem hiding this comment.
What is the issue with these test cases?
There was a problem hiding this comment.
Forgot to uncomment 😄 🤦 - fixed
flyrain
reviewed
Sep 19, 2025
| EnumSet<AccessDelegationMode> delegationModes, | ||
| Set<PolarisStorageActions> actions, | ||
| Optional<String> refreshCredentialsEndpoint) { | ||
| LoadTableResponse.Builder responseBuilder = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Endpoint is validated on line 296
There was a problem hiding this comment.
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.
snazy
approved these changes
Sep 22, 2025
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
eric-maynard
approved these changes
Sep 22, 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 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
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.
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 relevantcreate/load requests (previously it was called only when
vended-credentialswas requestedAlways sent
AccessConfig.extraProperties()to clientsExpose credentials to clients only when the
vended-credentialsaccessdelegation mode is requested.
There is not client-visible behaviour change for implementations of
PolarisStorageIntegrationthat do not produce "extra"AccessConfigproperties.