[#5619] feat(api): supports credential api for Gravitino #5690

FANNG1 · 2024-11-27T08:48:57Z

What changes were proposed in this pull request?

This is the first PR of credential vending for Gravitino server to add credential API , please refer to #5682 for the overall workflow. Examples to use the API:

    Catalog catalog = metalake.loadCatalog(catalogName);
    Credential[] credentials =
        catalog.supportsCredentials().getCredentials();

    Credential credential =
        catalog.supportsCredentials().getCredential(“s3-token”);

    Fileset fileset = catalog.asFilesetCatalog().loadFileset(filesetIdent);
    Credential[] credentials =
        fileset.supportsCredentials().getCredentials();
    Credential credential =
       fileset.supportsCredentials().getCredential(“s3-token”);

Why are the changes needed?

Fix: #5619

Does this PR introduce any user-facing change?

no

How was this patch tested?

using draft PR to do E2E test

FANNG1 · 2024-11-27T08:53:47Z

@jerryshao @yuqi1129 please help to review this, thanks

yuqi1129 · 2024-11-27T09:03:04Z

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

+  /**
+   * Retrieves an array of {@link Credential} objects based on the specified credential type.
+   *
+   * @param credentialType The server will return credentials according to the configuration if


You'd better give some example value of credentialType, from your description, I could not infer the possible value of it.

api/src/main/java/org/apache/gravitino/exceptions/CredentialDeserializeException.java

yuqi1129 · 2024-11-27T09:05:56Z

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

+   * @throws CredentialDeserializeException If there are problems when deserializing the credential
+   *     in client side.
+   */
+  Credential[] getCredentials(Optional<String> credentialType)


So we only need parameter credentialType to abtain credentails?

yes, is it necessary to add a more complicated parameter like CredentialRequest which contains credentialType

yuqi1129 · 2024-11-27T09:07:38Z

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

+   * @return An array of {@link Credential} objects. There will be only one credential for one
+   *     credential type. In most cases the array only contains one credential. If the object like
+   *     {@link org.apache.gravitino.file.Fileset} contains multi locations for different storages
+   *     like HDFS, S3, etc. The array will contain multi credentials. The array could be empty if


There will be only one credential for one credential type.

In most cases the array only contains one credential

If the object like {@link org.apache.gravitino.file.Fileset} contains multi locations for different storages

These three sentences are in conflict with each other.

FANNG1 · 2024-12-02T00:56:30Z

@jerryshao could you help to review this, thanks

yuqi1129 · 2024-12-03T06:25:47Z

I'm fine with the current implementation.

FANNG1 · 2024-12-03T07:07:57Z

@jerryshao do you have time to review this PR?

jerryshao · 2024-12-03T09:57:48Z

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

+   * @throws NoSuchCredentialException If the specified credential type cannot be found in the
+   *     server side.
+   */
+  Credential[] getCredentials(Optional<String> credentialType) throws NoSuchCredentialException;


I would suggest using two interfaces:

Credentials[] getCredentials(); Credentials getCredentials(String credentialType)

The second interface can have a default implementation and be implemented based on the first one.

FANNG1 · 2024-12-03T11:30:08Z

@jerryshao @yuqi1129 , all comments are addressed, please help to review again.

jerryshao · 2024-12-03T11:42:42Z

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

+   *
+   * @return An array of {@link Credential} objects. In most cases the array only contains one
+   *     credential. If the object like {@link org.apache.gravitino.file.Fileset} contains multi
+   *     locations for different storages like HDFS, S3, the array will contain multi credentials.


Change from "multi" to "multiple" in all the places.

jerryshao · 2024-12-03T11:43:43Z

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

+  Credential[] getCredentials() throws NoSuchCredentialException;
+
+  /**
+   * Retrieves an {@link Credential} object based on the specific credential type.


"... based on the specified credential type."

jerryshao · 2024-12-03T11:44:29Z

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

+   *
+   * @param credentialType The type of the credential like s3-token, s3-secret-key which defined in
+   *     the specific credentials.
+   * @return An {@link Credential} object with the specific credential type.


"... with the specified..."

jerryshao · 2024-12-03T13:09:31Z

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

+          "No credential found for the credential type: %s", credentialType);
+    } else if (credentials.length > 1) {
+      throw new IllegalStateException(
+          "Multi credentials found for the credential type:" + credentialType);


Also change this multi to multiple.

jerryshao · 2024-12-03T14:10:51Z

@FANNG1 can you please update the PR description to reflect your updated API?

FANNG1 · 2024-12-03T14:15:11Z

@FANNG1 can you please update the PR description to reflect your updated API?

done

### What changes were proposed in this pull request? This is the first PR of credential vending for Gravitino server to add credential API , please refer to #5682 for the overall workflow. Examples to use the API: ```java Catalog catalog = metalake.loadCatalog(catalogName); Credential[] credentials = catalog.supportsCredentials().getCredentials(); Credential credential = catalog.supportsCredentials().getCredential(“s3-token”); ``` ```java Fileset fileset = catalog.asFilesetCatalog().loadFileset(filesetIdent); Credential[] credentials = fileset.supportsCredentials().getCredentials(); Credential credential = fileset.supportsCredentials().getCredential(“s3-token”); ``` ### Why are the changes needed? Fix: #5619 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? using draft PR to do E2E test

yuqi1129 reviewed Nov 27, 2024

View reviewed changes

jerryshao reviewed Dec 3, 2024

View reviewed changes

FANNG1 added 9 commits December 3, 2024 19:35

fix comment

2e8462b

fix comment

eb2084e

fix comment

27eb1b7

fix comment

159ad63

fix comment

8b74dd6

fix comment

5ceed18

split interface

72eb9ad

split interface

ca46d00

split interface

85a5762

FANNG1 force-pushed the credential-api branch from f6fae1f to 85a5762 Compare December 3, 2024 11:38

jerryshao reviewed Dec 3, 2024

View reviewed changes

split interface

9605d63

jerryshao reviewed Dec 3, 2024

View reviewed changes

split interface

fb19eb1

jerryshao approved these changes Dec 3, 2024

View reviewed changes

yuqi1129 approved these changes Dec 4, 2024

View reviewed changes

jerryshao assigned FANNG1 Dec 4, 2024

jerryshao merged commit 8750ae8 into apache:main Dec 4, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5619] feat(api): supports credential api for Gravitino #5690

[#5619] feat(api): supports credential api for Gravitino #5690

FANNG1 commented Nov 27, 2024 •

edited

Loading

FANNG1 commented Nov 27, 2024

yuqi1129 Nov 27, 2024 •

edited

Loading

FANNG1 Nov 27, 2024

yuqi1129 Nov 27, 2024

FANNG1 Nov 27, 2024

yuqi1129 Nov 27, 2024

yuqi1129 Nov 27, 2024

FANNG1 Nov 27, 2024

FANNG1 commented Dec 2, 2024

yuqi1129 commented Dec 3, 2024

FANNG1 commented Dec 3, 2024

jerryshao Dec 3, 2024

FANNG1 Dec 3, 2024

FANNG1 commented Dec 3, 2024

jerryshao Dec 3, 2024

FANNG1 Dec 3, 2024

jerryshao Dec 3, 2024

FANNG1 Dec 3, 2024

jerryshao Dec 3, 2024

FANNG1 Dec 3, 2024

jerryshao Dec 3, 2024

FANNG1 Dec 3, 2024

jerryshao commented Dec 3, 2024

FANNG1 commented Dec 3, 2024

[#5619] feat(api): supports credential api for Gravitino #5690

[#5619] feat(api): supports credential api for Gravitino #5690

Conversation

FANNG1 commented Nov 27, 2024 • edited 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 Nov 27, 2024

yuqi1129 Nov 27, 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

FANNG1 commented Dec 2, 2024

yuqi1129 commented Dec 3, 2024

FANNG1 commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Dec 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Dec 3, 2024

FANNG1 commented Dec 3, 2024

FANNG1 commented Nov 27, 2024 •

edited

Loading

yuqi1129 Nov 27, 2024 •

edited

Loading