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

lefebsy · 2024-10-21T20:13:14Z

Description (edited) :

This is a S3 proposition of Polaris core storage implementation, copy of the aws + new parameters : endpoint, path style...

It is tested OK with :

local MinIO
Backblaze B2 (thanks to @metadaddy)
Ceph.io seems also ok thanks to @mmgaggle & @Gerrit-K tests

This should works with many S3 compatible solutions like Dell ECS, NetApp StorageGRID, etc...

By default it is trying to respect the same behavior about credentials than AWS (IAM/STS). The same dynamic policy is applied, limiting the scope to the data queried.
Otherwise if STS is not available 'skipCredentialSubscopingIndirection' = true will disabling Polaris "SubScoping" of the credentials

Let me know your opinion about this design proposal.
Thank you

curl -X POST -H "Authorization: Bearer ${SPARK_BEARER_TOKEN}" \
     -H 'Accept: application/json' -H 'Content-Type: application/json' \
     http://${POLARIS_HOST}:8181/api/management/v1/catalogs -d \
      '{
          "name": "my-s3compatible-catalog",
          "id": 100,
          "type": "INTERNAL",
          "readOnly": false,
          "properties": {
            "default-base-location": "${S3_LOCATION}"
          },
          "storageConfigInfo": {
            "storageType": "S3_COMPATIBLE",
            "allowedLocations": ["${S3_LOCATION}/"],
            "s3.endpoint": "https://localhost:9000"
          }
        }'

            # optional:
            "s3.pathStyleAccess": true / false
            "s3.region": "rack-1 or us-east-1"
            "s3.roleArn": "arn:xxx:xxx:xxx:xxxx"
            "s3.credentials.catalog.accessKeyId": "CATALOG_1_ACCESS_KEY_ENV_VARIABLE_NAME"
            "s3.credentials.catalog.secretAccessKey": "CATALOG_1_SECRET_KEY_ENV_VARIABLE_NAME"

            # optional in case STS/IAM is not available :
            "skipCredentialSubscopingIndirection": true
                # optional for skipCredentialSubscopingIndirection - served by catalog to client like spark or trino
                "s3.credentials.client.accessKeyId": "CLIENT_OF_CATALOG_1_ACCESS_KEY_ENV_VARIABLE_NAME"
                "s3.credentials.client.secretAccessKey": "CLIENT_OF_CATALOG_1_SECRET_KEY_ENV_VARIABLE_NAME"

Included Changes:

New type of storage "S3_COMPATIBLE".
Tested against MinIO with self-signed certificate
regtests/run_spark_sql_s3compatible.sh

Type of change:

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

...s-core/src/main/java/org/apache/polaris/core/storage/s3/S3CredentialsStorageIntegration.java

spec/polaris-management-service.yml

eric-maynard · 2024-11-05T08:47:39Z

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

@@ -23,6 +23,8 @@ public enum PolarisCredentialProperty {
  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_ENDPOINT(String.class, "s3.endpoint", "the aws s3 endpoint"),
+  AWS_PATH_STYLE_ACCESS(Boolean.class, "s3.path-style-access", "the aws s3 path style access"),


whether or not to use path-style access

Many S3COMPATIBLE solutions are deployed without network devices or configurations in front of them allowing support of dynamic hosts names including buckets.
TLS certificate with private AC could also be a challenge for dynamic host name. "*. domain" can also be forbidden by some enterprise security policy

Path style is useful in many cases. In ideal world I agree it should stay deprecated...

I think @eric-maynard was asking if we can change the description to something like this:

Suggested change

AWS_PATH_STYLE_ACCESS(Boolean.class, "s3.path-style-access", "the aws s3 path style access"),

AWS_PATH_STYLE_ACCESS(Boolean.class, "s3.path-style-access", "whether or not to use path-style access"),

I also agreed that we should make sure it false by default

For IBM's watsonx.data product it is set to true by default for Minio and Ceph bucket types, reason being that it's more likely to work. Path style will work regardless of whether the customer has setup wildcard DNS, a TLS certificate with a subject-alternate-name (the wildcard), and the hostname in the zonegroup (for Ceph). Virtual host style will only work if all of those things are done.

It's not a hill I would die on, but it's worthy of consideration.

To clarify, I was referring to the Polaris scope default. Users always have the option to set it to true when creating a new catalog.

.../org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java

lefebsy · 2025-03-11T18:32:10Z

Thanks a lot @lefebsy for working on it. I agreed that we don't have to put Helm change in, so that the github pipeline can pass. We can always add it back as a followup PR. It should be ready to merge as long as we fix this https://github.com/apache/polaris/pull/389/files#r1980741611.

Yes. I have restored helm from main branch. I have probably done a small test modification and forget it. I'm really confused.

omarsmak · 2025-03-11T20:39:16Z

Thanks a lot @lefebsy for working on it. I agreed that we don't have to put Helm change in, so that the github pipeline can pass. We can always add it back as a followup PR. It should be ready to merge as long as we fix this https://github.com/apache/polaris/pull/389/files#r1980741611.

Yes. I have restored helm from main branch. I have probably done a small test modification and forget it. I'm really confused.

Thanks @lefebsy . The PR looks now a lot better from helm standpoint. We just need to address last comment from @flyrain to get this PR going 🙂

omarsmak · 2025-03-12T16:33:16Z

@lefebsy you have a conflict by the way

# Conflicts: # polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialProperty.java

lefebsy · 2025-03-12T21:25:20Z

@omarsmak

=> Hello, the conflict should be resolved since rebased :-)

omarsmak · 2025-03-12T21:27:23Z

@omarsmak

=> Hello, the conflict should be resolved since rebased :-)

Thanks @lefebsy . Can you please address this last comment so we have this PR wrapped up? https://github.com/apache/polaris/pull/389/files#r1980741611

dimas-b

Thanks for your contribution @lefebsy ! Overall I think it's a valuable addition to Polaris, but I have a couple of concerns.

dimas-b · 2025-03-13T17:21:34Z

polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java

+                    s3ConfigModel.getS3PathStyleAccess(),
+                    s3ConfigModel.getS3Region(),
+                    s3ConfigModel.getS3RoleArn(),
+                    new ArrayList<>(allowedLocations));


nit: why new array?

dimas-b · 2025-03-13T17:22:36Z

polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java

+        return S3CompatibleStorageConfigInfo.builder()
+            .setStorageType(StorageConfigInfo.StorageTypeEnum.S3_COMPATIBLE)
+            .setS3Endpoint(s3Config.getS3Endpoint())
+            .setS3ProfileName(s3Config.getS3ProfileName())


these kind of attribute copy blocks of code are easy to mess up by accidental typos... Could you add a unit test for this conversion?

dimas-b · 2025-03-13T17:25:34Z

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

@@ -241,6 +243,7 @@ public void validateMaxAllowedLocations(int maxAllowedLocations) {
  /** Polaris' storage type, each has a fixed prefix for its location */
  public enum StorageType {
    S3("s3://"),
+    S3_COMPATIBLE("s3://"),


I believe s3 is sufficient for this PR because AWS also maps to only s3. Other URIs schemes (like s3a) are applicable to AWS too, but I think it is best to handle that in a follow-up PR.

dimas-b · 2025-03-13T17:30:13Z

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

+    if (roleArn.contains("aws-cn")) {
+      return "arn:aws-cn:s3:::";
+    } else if (roleArn.contains("aws-us-gov")) {
+      return "arn:aws-us-gov:s3:::";


I believe this kind of special cases should not be in Polaris code, but it's ok for this PR as I understand it mimics previous AWS-specific logic.

dimas-b · 2025-03-13T18:18:39Z

.../org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java

+      @Nonnull Set<String> allowedReadLocations,
+      @Nonnull Set<String> allowedWriteLocations) {
+
+    String caI = System.getenv(storageConfig.getS3CredentialsCatalogAccessKeyId());


I'm strongly opposed to storing credential env. variable name is the storage config (essentially in the catalog config).

The environment and catalog config are different management domains with different concerns.

Using a static env. variable name (e.g. AWS_ACCESS_KEY) is ok in the short term, IMHO.

Is it strictly necessary to support different secrets for different catalogs in this case?

I shared the same concern, see here https://github.com/apache/polaris/pull/389/files#r1980741611

dimas-b · 2025-03-13T18:21:35Z

.../org/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java

+          StaticCredentialsProvider.create(AwsBasicCredentials.create(caI, caS)));
+      LOGGER.debug("S3Compatible - stsClient using keys from catalog settings");
+    }
+    try (StsClient stsClient = stsBuilder.build()) {


Creating an StsClient for each request is an overkill, IMHO, but we can probably refactor that later.

dimas-b · 2025-03-13T18:22:58Z

regtests/README.md

@@ -40,7 +40,7 @@ follows:

 ```shell
 ./gradlew clean :polaris-quarkus-server:assemble -Dquarkus.container-image.build=true --no-build-cache
-docker compose -f ./regtests/docker-compose.yml up --build --exit-code-from regtest
+docker-compose -f ./regtests/docker-compose.yml up --build --exit-code-from regtest


This seems to be unrelated to the purpose of this PR. Please make this change in a separate PR.

dimas-b · 2025-03-13T18:24:41Z

regtests/minio/docker-compose.yml

+      - 9000:9000
+    volumes:
+      - ./miniodata:/data
+      - ./certs:/root/.minio/certs/


Is this strictly necessary for tests?

dimas-b · 2025-03-13T18:26:03Z

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

+   * @return A policy limiting scope access
+   */
+  // TODO - add KMS key access
+  public static IamPolicy policyString(


Why not reuse the policy from the code the deals with AWS S3?

dimas-b · 2025-03-17T12:49:22Z

@lefebsy : I appreciate your work on this PR, but I'd like to submit an alternative implementation for consideration by the community. Will mention it here, when it's ready.

flyrain · 2025-03-21T21:16:09Z

@lefebsy : I appreciate your work on this PR, but I'd like to submit an alternative implementation for consideration by the community. Will mention it here, when it's ready.

Hi @dimas-b , great to know that you will contribute to this as well. Given the PR is WIP for a quite long time, can we merge this first, and make changes later? We can always improve on it and make changes as needed.

dimas-b · 2025-04-05T00:18:00Z

@flyrain :

Given the PR is WIP for a quite long time, can we merge this first, and make changes later? We can always improve on it and make changes as needed.

There are unaddressed concerns with this PR (as commented on specific changes). I believe they need to be resolved before merging.

flyrain · 2025-04-06T05:44:11Z

There are unaddressed concerns with this PR (as commented on specific changes). I believe they need to be resolved before merging.

Agreed. I meant merge it after comments are addressed.

thotz · 2025-04-10T07:19:43Z

@lefebsy @flyrain I can see this feature is targeted for the 1.0 release on April 15th. Can we expect this change to be available by then?

flyrain · 2025-04-11T20:54:24Z

@thotz, I'm also waiting for the original author to make the change.

eric-maynard · 2025-04-17T21:04:21Z

Hey @lefebsy, do you think we'll be able to revive this? If you're not available we can also copy this into another PR. I think the changes are pretty much ready to go

dimas-b · 2025-04-17T21:16:58Z

I'd prefer not to copy this 1:1 as I have some concerns with some areas of this PR (as commented).

If noone is actively working on this, I'd be willing to open another PR for this feature... maybe next week.

richban · 2025-04-28T00:57:54Z

@lefebsy Would love this PR to be merged.

ashokvengala1990 · 2025-04-30T14:27:07Z

@lefebsy, @adutra , @metadaddy , @mmgaggle

We are waiting for this PR to be merged. One of our customers wants to use MinIO or Dell ECS storage for Polaris. Please let me know if I can contribute to this if it is taking longer than expected. Looking forward to your response. Thank you.

lefebsy · 2025-04-30T18:16:43Z

Hello,

I started this PR 6 months ago, and did not realize that the review process would be so time consuming, that each new review would ask to explain the changes required during the previous review or the previous commenter, or add tests on existing code etc..

Unfortunately, since some weeks I no longer have the ability to devote more personal time to these round trips. Mabe it would be preferable to close or remove this PR. I hope not everything is to throw away. If Dimas-b wants to make another better implementation : +1

eric-maynard · 2025-04-30T19:05:21Z

No worries @lefebsy! I'm sad to hear that you can't carry the PR forward but I'm thankful for your contributions so far. This is a feature I know a lot of users are excited about and it's unfortunate that it turned out to be so contentious.

@dimas-b, I'll keep an eye out for your PR, thanks for offering to pick this up.

ryanovas · 2025-04-30T19:45:50Z

@eric-maynard I am one of those users, I would be incredibly disappointed to see this PR be lost. I have been following it for weeks if not months at this point.

dimas-b · 2025-05-01T01:01:42Z

Thanks for your contribution @lefebsy !.. even if this PR does not get merged

dimas-b · 2025-05-01T01:02:21Z

I started related, but somewhat indirect refactoring in #1504

lefebsy requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau, jackye1995, flyrain, eric-maynard, collado-mike and ebyhr as code owners October 21, 2024 20:13

lefebsy changed the title ~~add s3 compatible storage - first commit~~ [FEATURE REQUEST] On-Premise S3... #32 Oct 21, 2024

lefebsy changed the title ~~[FEATURE REQUEST] On-Premise S3... #32~~ [FEATURE REQUEST #32] On-Premise S3... Oct 21, 2024

lefebsy changed the title ~~[FEATURE REQUEST #32] On-Premise S3...~~ [FEATURE REQUEST #32] On-Premises S3... Oct 21, 2024

This comment was marked as outdated.

Sign in to view

lefebsy force-pushed the s3compatible branch from ee0a8ef to 138c1f7 Compare October 22, 2024 17:48