[#4992] support credential vending framework #4995

FANNG1 · 2024-09-23T10:15:04Z

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?

add UT
propose a draft PR in [#4994][#4369] feat(core): support S3 credential vending #4966 , and could run pass S3 token with Gravitino IcebergRESTServer

jerryshao · 2024-10-09T03:50:02Z

common/src/main/java/org/apache/gravitino/credential/Credential.java

+import java.util.Map;
+
+/** Interface representing a credential with type, expiration time, and additional information. */
+public interface Credential {


What is the purpose of put it to common package?

It maybe used in the client side.

Can you please give me an example about how to use it in client side?

Take Spark read Iceberg table for example,

// Fetch credential from Gravitino Credential credentail = gravitinoClient.fetchCredential(NameIdentifier identifer, String credentialType) // Transform credential properties to engine specific properties Map credentailProperties = CredentialUtils.toIcebergProperties(credential) // using credential properties to construct FileIO FileIO file = new FileIO(credentailProperties)

If so, I would suggest you move to API module, not in common module.

yes, this seems reasonable

jerryshao · 2024-10-09T03:51:53Z

core/src/main/java/org/apache/gravitino/credential/CredentialProviderFactory.java

+    ServiceLoader<CredentialProvider> serviceLoader =
+        ServiceLoader.load(CredentialProvider.class, classLoader);
+    List<Class<? extends CredentialProvider>> providers =
+        Streams.stream(serviceLoader.iterator())


What is the consideration of using service loader?

Different catalogs may use different credential providers so that users could place the s3 provider in catalogA, place gcs provider in catalogB, so we using service loader to load corresponding jar in catalog classpaths.

jerryshao · 2024-10-09T03:57:55Z

core/src/main/java/org/apache/gravitino/credential/CredentialProvider.java

+ *
+ * <p>A credential provider is responsible for managing and retrieving credentials.
+ */
+public interface CredentialProvider {


I think you can change to like public interface Credential extends Closeable to avoid defining void stop() here.

jerryshao · 2024-10-09T03:59:47Z

common/src/main/java/org/apache/gravitino/credential/Credential.java

+   *
+   * @return a map of credential information.
+   */
+  Map<String, String> getCredentialInfo();


I would suggest change the style of getXXX to xxx, like credentialType().

Do we need to define an interface for Token like Hadoop?

I would suggest that you can refer to Hadoop's Credentials, Token and TokenIdentifer to see how to define our credential system.

Credential is like Token in hadoop, the difference is Token in hadoop focus on the security of the identifer, while we focus on how to represent diverse tokens like S3-token, AKSK, delegation token with different properties.

The concrete Credential has the clear definition , take S3TokenCredential for example

public class S3TokenCredential implements Credential { private String accessKeyId; private String secretAccessKey; private String sessionToken; private long expireMs; public S3TokenCredential( String accessKeyId, String secretAccessKey, String sessionToken, long expireMs) { this.accessKeyId = accessKeyId; this.secretAccessKey = secretAccessKey; this.sessionToken = sessionToken; this.expireMs = expireMs; } @Override public String getCredentialType() { return CredentialConstants.S3_TOKEN_CREDENTIAL_TYPE; } @Override public long getExpireTime() { return expireMs; } @Override public Map<String, String> getCredentialInfo() { return (new ImmutableMap.Builder<String, String>()) .put(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, accessKeyId) .put(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, secretAccessKey) .put(S3Properties.GRAVITINO_S3_TOKEN, sessionToken) .build(); } }

jerryshao · 2024-10-09T04:02:03Z

common/build.gradle.kts

@@ -28,6 +28,7 @@ plugins {

 dependencies {
  implementation(project(":api"))
+  implementation(project(":catalogs:catalog-common"))


The dependency here looks strange if you only want to define two properties in catalog-common, it is easy to introduce the cyclic dependency.

catalog-common is mainly used to define some properties used by catalog, client, connectors, I think it's ok to add the dependence here.

No, it's weird to make common module rely on catalog-common module, which have different purposes and easy to introduce cyclic dependency, please change it.

jerryshao · 2024-10-14T07:49:22Z

common/src/main/java/org/apache/gravitino/credential/Credential.java

+public interface Credential {
+
+  /**
+   * Returns the type of the credential. It should same with the credential type of the credential


"It should be the same with..."

"It should be the same as..."

jerryshao · 2024-10-14T07:49:45Z

common/src/main/java/org/apache/gravitino/credential/Credential.java

+   *
+   * @return the expiration time as a long.
+   */
+  long expireTimeMs();


"expireTimeInMs"

jerryshao · 2024-10-14T07:55:55Z

common/src/main/java/org/apache/gravitino/credential/Credential.java

+import java.util.Map;
+
+/** Interface representing a credential with type, expiration time, and additional information. */
+public interface Credential {


Can you please give me an example about how to use it in client side?

jerryshao · 2024-10-14T07:59:24Z

core/src/main/java/org/apache/gravitino/credential/Context.java

+package org.apache.gravitino.credential;
+
+/** Contains context information to get credential from credential provider. */
+public interface Context {


Please rename to CredentialContext, CatalogCredentialContext, something like this.

jerryshao · 2024-10-14T09:19:29Z

core/src/main/java/org/apache/gravitino/credential/LocationContext.java

+ * LocationContext is generated when user requesting resources associated with storage location like
+ * table, fileset, etc.
+ */
+public class LocationContext implements Context {


I think we can rename this class name to PathBasedCredentialContext, and change from "locations" to "paths".

jerryshao · 2024-10-14T09:20:04Z

core/src/main/java/org/apache/gravitino/credential/LocationContext.java

+    return userName;
+  }
+
+  public Set<String> getWriteLocations() {


"writePaths"

FANNG1 · 2024-10-14T10:26:10Z

@jerryshao comments are addressed, please help to review again

jerryshao · 2024-10-14T14:13:02Z

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

+  /** Credential type in the credential. */
+  public static final String CREDENTIAL_TYPE = "credential-type";
+  /** Credential expire time at ms since the epoch. */
+  public static final String EXPIRE_TIME_AT_MS = "expire-time-at-ms";


I think you can also define these two property keys in Credential, you don't have to create a new class to maintain them.

Also please change to "expire-time-in-ms".

credential framework

bdf04fa

FANNG1 force-pushed the credential-framework branch from 55c1ad6 to bdf04fa Compare September 23, 2024 10:26

polish

22a4709

jerryshao reviewed Oct 9, 2024

View reviewed changes

FANNG1 added 2 commits October 9, 2024 15:51

polish

8397303

use millisecons

b963158

jerryshao reviewed Oct 14, 2024

View reviewed changes

FANNG1 added 2 commits October 14, 2024 18:20

xx

0ca5e98

xx

f66d189

FANNG1 added 2 commits October 14, 2024 19:57

xx

0666c87

fix api doc

a2f76d3

jerryshao reviewed Oct 14, 2024

View reviewed changes

FANNG1 added 2 commits October 15, 2024 09:32

fix comment

7a7a629

fix comment

dd0d578

jerryshao approved these changes Oct 15, 2024

View reviewed changes

jerryshao merged commit 53bd227 into apache:main Oct 15, 2024
26 checks passed

jerryshao assigned FANNG1 Oct 15, 2024

[#4992] support credential vending framework #4995

[#4992] support credential vending framework #4995

Conversation

FANNG1 commented Sep 23, 2024

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?

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

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

FANNG1 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Oct 14, 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

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

FANNG1 commented Oct 14, 2024

jerryshao Oct 14, 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 9, 2024 •

edited

Loading

jerryshao Oct 14, 2024 •

edited

Loading

jerryshao Oct 14, 2024 •

edited

Loading