[#4994][#4369] feat(core): support S3 credential vending #4966

FANNG1 · 2024-09-19T11:18:21Z

What changes were proposed in this pull request?

support S3 credential vending, include s3 token and s3 static key

Why are the changes needed?

Fix: #4994
Fix: #4369

Does this PR introduce any user-facing change?

no

How was this patch tested?

add IT to do Iceberg operation by using S3 token

### What changes were proposed in this pull request? support credential vending framework ### Why are the changes needed? Fix: #4992 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? 1. add UT 2. propose a draft PR in #4966 , and could run pass S3 token with Gravitino IcebergRESTServer

FANNG1 · 2024-10-24T06:37:55Z

@yuqi1129 @jerqi @jerryshao please help to review when you have time, thanks

FANNG1 · 2024-10-25T06:45:42Z

api/src/main/java/org/apache/gravitino/credential/S3TokenCredential.java

+public class S3TokenCredential implements Credential {
+
+  /** S3 token credential type. */
+  public static final String S3_TOKEN_CREDENTIAL_TYPE = "s3-token";


what do you mean about token and session-token?

jerqi · 2024-10-25T08:29:57Z

bundles/aws-bundle/src/main/java/org/apache/gravitino/s3/credential/S3TokenProvider.java

+    return uri.getHost();
+  }
+
+  private Credentials getS3Token(


createS3Token?

jerqi · 2024-10-25T08:33:54Z

bundles/aws-bundle/src/main/java/org/apache/gravitino/s3/credential/S3TokenProvider.java

+        StaticCredentialsProvider.create(
+            AwsBasicCredentials.create(
+                s3CredentialConfig.accessKeyID(), s3CredentialConfig.secretAccessKey()));
+    String region = s3CredentialConfig.region();


Builder builder = StsClient.builder() .credentialsProvider(credentialsProvider); if (StringUtils.isNotBlank(region)) { builder.region(Region.of(region)); } return builder.build();

jerqi · 2024-10-25T08:34:23Z

bundles/aws-bundle/src/main/java/org/apache/gravitino/s3/credential/S3TokenProvider.java

+    }
+  }
+
+  private IamPolicy getPolicy(


createPolicy?

jerqi · 2024-10-25T08:38:44Z

...ndle/src/main/resources/META-INF/services/org.apache.gravitino.credential.CredentialProvider

+# specific language governing permissions and limitations
+# under the License.
+#
+org.apache.gravitino.s3.credential.S3TokenProvider


This is AWS Hadoop Credential provider names

org.apache.hadoop.fs.s3a.TemporaryAWSCredentialsProvider, org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider, software.amazon.awssdk.auth.credentials.EnvironmentVariableCredentialsProvider, org.apache.hadoop.fs.s3a.auth.IAMInstanceCredentialsProvider

Do we need to refer to them? Many people may be used to using them.

This is for developers only, the user only care about credential provider type, like s3-token for this, I'm ok to use this name, do you have some suggestion?

It's ok if it's only used by developers. But we need to make sure that s3-token is easy to understand by users.

Do you think s3-token and s3-secret-key is clear for users?

What name do other systems use?

polaris use storage-type = s3 please refer to https://polaris.io/#tag/Polaris-Entities/Catalog, uc get s3 properties if the location is s3 schema, see https://docs.unitycatalog.io/server/configuration/

Two consideration points.

GCP is gcp-token? Should be this aws-token?

I am not sure token is correct enough? Does S3 have multiple tokens? Do we support all?

GCP is gcs-token, I prefer to keep s3-token, because it's used for s3 storage.

We just support token from AsummRole, I think the name is simple and clear. You could provide your option if have better name.

OK. I just discuss some possible names to make sure. I'm ok with current name.

FANNG1 · 2024-10-28T00:32:32Z

@jerqi any other comments?

jerqi

LGTM.

FANNG1 · 2024-10-28T06:56:11Z

@jerryshao @yuqi1129 , do you have time to take another look?

jerryshao · 2024-10-28T07:07:38Z

api/src/main/java/org/apache/gravitino/credential/S3TokenCredential.java

+  /** The static access key ID used to access S3 data. */
+  public static final String GRAVITINO_S3_ACCESS_KEY_ID = "s3-access-key-id";
+  /** The static secret access key used to access S3 data. */
+  public static final String GRAVITINO_S3_SECRET_ACCESS_KEY = "s3-secret-access-key";


This is duplicated here and above, can you please consolidate them?

yes, they are different properties in S3TokenCredential and S3SecretKeyCredential with same name.

So please tell me what's the difference? If they're different, why do you use the same name and the same definition here?

You didn't address this comment.

sorry for miss this comment, Sts will response a sessionToken with AccessKeyId and secretAccessKey which is different from the AKSK, do you think we should use a different name like sessionAccessKeyId to make it more clear , but this maybe inconsistent with traditional s3 naming.

I update the comment for the properties

jerryshao · 2024-10-28T07:08:31Z

api/src/main/java/org/apache/gravitino/credential/S3SecretKeyCredential.java

+  private String accessKeyId;
+  private String secretAccessKey;


Make them final here and below.

jerryshao · 2024-10-28T09:16:26Z

@FANNG1 please fix the ci problem.

FANNG1 · 2024-10-28T11:02:34Z

@FANNG1 please fix the ci problem.

caused by temporal network error, fixed by rerun

FANNG1 · 2024-10-28T13:04:38Z

@jerryshao any other comments?

jerryshao · 2024-10-29T08:10:57Z

api/src/main/java/org/apache/gravitino/credential/S3TokenCredential.java

+  /** The session access key ID used to access S3 data. */
+  public static final String GRAVITINO_S3_ACCESS_KEY_ID = "s3-access-key-id";
+  /** The session secret access key used to access S3 data. */
+  public static final String GRAVITINO_S3_SECRET_ACCESS_KEY = "s3-secret-access-key";


Although the name of two key "static" and "session" are the same in S3, in the gravitino side, we can choose a different name to define, like "GRAVITINO_STATIC_S3_SECRET_ACCESS_KEY" and "GRAVITINO_SESSION_S3_SECRET_ACCESS_KEY"

jerryshao · 2024-10-29T09:07:49Z

api/src/main/java/org/apache/gravitino/credential/S3SecretKeyCredential.java

  /** The static secret access key used to access S3 data. */
-  public static final String GRAVITINO_S3_SECRET_ACCESS_KEY = "s3-secret-access-key";
+  public static final String GRAVITINO_S3_STATIC_SECRET_ACCESS_KEY = "s3-static-secret-access-key";


I mean that we can still follow AWS convention to use "s3-secret-access-key", but the definition here in Gravitino can be a more differentiable name. like:

public static final String GRAVITINO_S3_STATIC_SECRET_ACCESS_KEY = "s3-secret-access-key";

Also for other keys, what do you think? @FANNG1

jerryshao · 2024-10-29T09:39:18Z

Please make the CI pass.

### What changes were proposed in this pull request? support S3 credential vending, include s3 token and s3 static key ### Why are the changes needed? Fix: #4994 Fix: #4369 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? add IT to do Iceberg operation by using S3 token

FANNG1 marked this pull request as draft September 19, 2024 11:18

FANNG1 force-pushed the s3-provider branch from 1835140 to 44f0f1d Compare September 19, 2024 11:39

FANNG1 mentioned this pull request Sep 23, 2024

[#4992] support credential vending framework #4995

Merged

FANNG1 force-pushed the s3-provider branch from 44f0f1d to 3f03715 Compare October 23, 2024 09:09

FANNG1 changed the title ~~[SIP] support S3 credential vending for IcebergRESTServer~~ [#4994] feat(core): support S3 credential vending Oct 24, 2024

FANNG1 self-assigned this Oct 24, 2024

FANNG1 marked this pull request as ready for review October 24, 2024 02:23

FANNG1 force-pushed the s3-provider branch from 281bbfa to 0f61209 Compare October 25, 2024 06:32

FANNG1 commented Oct 25, 2024

View reviewed changes

jerqi approved these changes Oct 25, 2024

View reviewed changes

jerqi reviewed Oct 25, 2024

View reviewed changes

jerqi approved these changes Oct 28, 2024

View reviewed changes

jerryshao added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 28, 2024

jerryshao changed the title ~~[#4994] feat(core): support S3 credential vending~~ [#4994][#4369] feat(core): support S3 credential vending Oct 28, 2024

jerryshao reviewed Oct 28, 2024

View reviewed changes

FANNG1 force-pushed the s3-provider branch from 6512dea to 5a14b46 Compare October 28, 2024 07:28

FANNG1 added 3 commits October 29, 2024 14:56

s3 credential

55e8df3

fix

a69edb1

fix

ebe23c7

FANNG1 added 4 commits October 29, 2024 14:56

fix

cc4ec18

make it final

3042b4d

fix comment

36f5c9f

fix comment

0eb0fa6

FANNG1 force-pushed the s3-provider branch from 5a14b46 to 0eb0fa6 Compare October 29, 2024 07:05

fix comment

d5536c3

jerryshao reviewed Oct 29, 2024

View reviewed changes

use static and session name

4088c32

jerryshao reviewed Oct 29, 2024

View reviewed changes

use static and session name

809b7f6

jerryshao approved these changes Oct 29, 2024

View reviewed changes

jerryshao merged commit f265e50 into apache:main Oct 29, 2024
26 checks passed

[#4994][#4369] feat(core): support S3 credential vending #4966

[#4994][#4369] feat(core): support S3 credential vending #4966

Conversation

FANNG1 commented Sep 19, 2024 • edited by jerryshao Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Oct 28, 2024

jerqi left a comment

Choose a reason for hiding this comment

FANNG1 commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Oct 28, 2024

FANNG1 commented Oct 28, 2024

FANNG1 commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Oct 29, 2024

FANNG1 commented Sep 19, 2024 •

edited by jerryshao

Loading

jerqi Oct 25, 2024 •

edited

Loading

jerqi Oct 28, 2024 •

edited

Loading

FANNG1 Oct 28, 2024 •

edited

Loading

FANNG1 commented Oct 28, 2024 •

edited

Loading