Refactor storage access configuration handling #1504

dimas-b · 2025-05-01T00:39:15Z

This is a step towards supporting non-AWS S3 storage, but this refactoring is relevant to all storage backends.

There is no change to existing behaviours.

Rename PolarisCredentialProperty to IcebergStorageAccessProperty and introduce non-credential properties (as an example for now)
IcebergStorageAccessProperty values are ultimately meant to be produced by PolarisStorageIntegration implementations
Some previous entries in IcebergStorageAccessProperty are not really credential properties, but their treatment is not changed in this PR to maintain exactly the same bahaviour as before.
Add AccessConfig to represent both credential and non-credential properties related to storage access.

collado-mike

Seems like a breaking change without a clear reason. What's the purpose in separating the credentials from extraProperties?

collado-mike · 2025-05-01T06:03:08Z

polaris-core/src/main/java/org/apache/polaris/core/storage/AccessConfig.java

+  Map<String, String> credentials();
+
+  Map<String, String> extraProperties();


What's the need for breaking this out into separate fields? They all end up in the same map for every single caller

The main idea is to isolate credential and non-credential properties in REST API responses. Specifically in IcebergCatalogHandler.

This PR does not cause any functional change, but prepares for exposing properties like s3.endpoint to clients later.

These properties are not always mixed together. PR #1225 brings some distinction in load table responses.

dimas-b · 2025-05-01T17:13:44Z

Seems like a breaking change without a clear reason

@collado-mike : Could you clarify what is broken by this PR? I believe there's no behaviour change.

dimas-b · 2025-05-01T17:16:47Z

What's the purpose in separating the credentials from extraProperties?

The purpose is to allow not mixing credential and non-credential properties in loadTableResponse per #1225

Other renames in this PR are meant to clarity code-level distinction between credentials and non-credential properties related to storage access.

collado-mike · 2025-05-01T18:25:29Z

@collado-mike : Could you clarify what is broken by this PR? I believe there's no behaviour change.

Sorry, I misread the cache code without closely reading the StorageCredentialCacheEntry changes. You're right, there are no behavior changes here.

I understand the reasoning for the change now and I agree.

dimas-b · 2025-05-01T23:38:24Z

@eric-maynard @dennishuo : Please have a look too :)

adnanhemani

I quite like this refactor, it makes lots of sense to do this. One small, stylistic comment.

One minor question, why the name IcebergStorageAccessProperty? I understand that all tables today are Iceberg, but in the future if they were not, would this class still only be relevant to them only?

adnanhemani · 2025-05-02T00:07:45Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

-import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
-import org.apache.polaris.core.storage.PolarisStorageIntegration;
-import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.*;


We should revert this to ensure no wildcard imports.

Good catch - fixed

polaris-core/src/main/java/org/apache/polaris/core/storage/IcebergStorageAccessProperty.java

eric-maynard · 2025-05-02T08:16:41Z

polaris-core/src/main/java/org/apache/polaris/core/storage/IcebergStorageAccessProperty.java

+  AWS_ENDPOINT(String.class, "s3.endpoint", "the S3 endpoint to use for requests", false),
+  AWS_PATH_STYLE_ACCESS(
+      Boolean.class, "s3.path-style-access", "whether to use S3 path style access", false),


eric-maynard · 2025-05-02T08:17:47Z

The semantic changes look good to me, but there's also a rename happening here that looks a bit unrelated to those changes. I'd like to understand the intent behind the rename a little more if possible.

This is a step towards supporting non-AWS S3 storage, but this refactoring is relevant to all storage backends. There is no change to existing behaviours. * Rename PolarisCredentialProperty to IcebergStorageAccessProperty and introduce non-credential properties (as an example for now) * IcebergStorageAccessProperty values are ultimately meant to be produced by PolarisStorageIntegration implementations * Some previous entries in IcebergStorageAccessProperty are not really credential properties, but their treatment is not changed in this PR to maintain exactly the same bahaviour as before. * Add AccessConfig to represent both credential and non-credential properties related to storage access.

adutra · 2025-05-02T20:54:01Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageAccessProperty.java

+ * <p>Most of these properties are meant to configure Iceberg FileIO objects for accessing data in
+ * storage.
+ */
+public enum StorageAccessProperty {
  AWS_KEY_ID(String.class, "s3.access-key-id", "the aws access key id"),
  AWS_SECRET_KEY(String.class, "s3.secret-access-key", "the aws access key secret"),
  AWS_TOKEN(String.class, "s3.session-token", "the aws scoped access token"),
  AWS_SESSION_TOKEN_EXPIRES_AT_MS(


Is this considered "credential"?

The Key ID, Secret Key, Session Token, and expiry time is definitely together considered a "credential" for AWS. Not sure if there's a reason why you're asking this?

I'd say it is part of the credential bunch of properties.

AWS_SESSION_TOKEN_EXPIRES_AT_MS by itself is definitely not a credential. But it is a property related to credential vending and access to object storage, and so I think having it here makes sense.

adnanhemani

LGTM, thank you!

eric-maynard

LGTM! Excited to see S3-compatible storage support coming together. Thanks for working on this.

ryanovas · 2025-05-05T18:43:29Z

With the other issue arrive S3-compatible storage closed, and this step towards it merged, where can we keep up with the overall progress of S3-compatible storage now?

dimas-b · 2025-05-06T01:00:51Z

Thanks for the reminder @ryanovas . I've just filed a fresh issue for this : #1530 (since #32 has large scope)

dimas-b requested review from adutra, ashvina, dennishuo, eric-maynard, jackye1995, jbonofre, vvcephei and collado-mike as code owners May 1, 2025 00:39

github-project-automation bot added this to Basic Kanban Board May 1, 2025

dimas-b requested review from snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners May 1, 2025 00:39

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board May 1, 2025

dimas-b mentioned this pull request May 1, 2025

[FEATURE REQUEST #32] On-Premises S3 / S3 Compatible... #389

Closed

10 tasks

collado-mike reviewed May 1, 2025

View reviewed changes

collado-mike previously approved these changes May 1, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 1, 2025

adnanhemani suggested changes May 2, 2025

View reviewed changes

github-project-automation bot moved this from Ready to merge to PRs In Progress in Basic Kanban Board May 2, 2025

eric-maynard reviewed May 2, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/storage/IcebergStorageAccessProperty.java Outdated Show resolved Hide resolved

eric-maynard reviewed May 2, 2025

View reviewed changes

dimas-b added 2 commits May 2, 2025 15:34

review: fix imports

60c2a14

review: rename to StorageAccessProperty

fa3e82a

dimas-b dismissed collado-mike’s stale review via fa3e82a May 2, 2025 19:34

dimas-b force-pushed the refactor-access-config branch from b9755b3 to fa3e82a Compare May 2, 2025 19:34

adutra reviewed May 2, 2025

View reviewed changes

adnanhemani approved these changes May 2, 2025

View reviewed changes

eric-maynard approved these changes May 3, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 3, 2025

dimas-b merged commit 4db7998 into apache:main May 5, 2025
6 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 5, 2025

dimas-b deleted the refactor-access-config branch May 5, 2025 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor storage access configuration handling #1504

Refactor storage access configuration handling #1504

dimas-b commented May 1, 2025

collado-mike left a comment

collado-mike May 1, 2025

dimas-b May 1, 2025

dimas-b commented May 1, 2025

dimas-b commented May 1, 2025 •

edited

Loading

collado-mike commented May 1, 2025

dimas-b commented May 1, 2025

adnanhemani left a comment

adnanhemani May 2, 2025

dimas-b May 2, 2025

eric-maynard May 2, 2025

eric-maynard commented May 2, 2025

adutra May 2, 2025

adnanhemani May 2, 2025 •

edited

Loading

dimas-b May 2, 2025

eric-maynard May 3, 2025 •

edited

Loading

adnanhemani left a comment

eric-maynard left a comment

ryanovas commented May 5, 2025

dimas-b commented May 6, 2025

		Map<String, String> credentials();

		Map<String, String> extraProperties();

Refactor storage access configuration handling #1504

Refactor storage access configuration handling #1504

Conversation

dimas-b commented May 1, 2025

collado-mike left a comment

Choose a reason for hiding this comment

collado-mike May 1, 2025

Choose a reason for hiding this comment

dimas-b May 1, 2025

Choose a reason for hiding this comment

dimas-b commented May 1, 2025

dimas-b commented May 1, 2025 • edited Loading

collado-mike commented May 1, 2025

dimas-b commented May 1, 2025

adnanhemani left a comment

Choose a reason for hiding this comment

adnanhemani May 2, 2025

Choose a reason for hiding this comment

dimas-b May 2, 2025

Choose a reason for hiding this comment

eric-maynard May 2, 2025

Choose a reason for hiding this comment

eric-maynard commented May 2, 2025

adutra May 2, 2025

Choose a reason for hiding this comment

adnanhemani May 2, 2025 • edited Loading

Choose a reason for hiding this comment

dimas-b May 2, 2025

Choose a reason for hiding this comment

eric-maynard May 3, 2025 • edited Loading

Choose a reason for hiding this comment

adnanhemani left a comment

Choose a reason for hiding this comment

eric-maynard left a comment

Choose a reason for hiding this comment

ryanovas commented May 5, 2025

dimas-b commented May 6, 2025

dimas-b commented May 1, 2025 •

edited

Loading

adnanhemani May 2, 2025 •

edited

Loading

eric-maynard May 3, 2025 •

edited

Loading