[#5620] feat(fileset): Support credential vending for fileset catalog #5682

FANNG1 · 2024-11-26T12:28:33Z

What changes were proposed in this pull request?

Support credential vending for fileset catalog

add credential-providers properties for the fileset catalog, schema, and fileset.
try to get credential-providers from the order of fileset, schema, and catalog.
The user could set multi-credential providers

Why are the changes needed?

Fix: #5620

Does this PR introduce any user-facing change?

will add document after this PR is merged

How was this patch tested?

Add IT and test with local setup Gravitino server

### 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

FANNG1 · 2024-12-18T09:35:38Z

@jerryshao @yuqi1129 this PR is ready now, please help to review, thanks

jerryshao · 2024-12-19T11:26:43Z

...-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/SecureHadoopCatalogOperations.java


  public static final Logger LOG = LoggerFactory.getLogger(SecureHadoopCatalogOperations.class);

  private final HadoopCatalogOperations hadoopCatalogOperations;
+  private CatalogCredentialManager catalogCredentialManager;


Can we achieve this credential operations in the core module, like in operation dispatcher? It's a bit tricky to mix this with the catalog, WDYT?

You mean like tag operation?

Yes, my feeling is that the credential may be orthogonal to the catalog implementation, we don't have to embed the code to each catalog implementation.

Let me think more, what's different for Credential operations is the classloader, it should load the credential provider in catalog classloader.

jerryshao · 2024-12-19T11:27:46Z

...-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/SecureHadoopCatalogOperations.java

+      return getCatalogCredentialContexts();
+    }
+    return getFilesetCredentialContexts(nameIdentifier, privilege);
+  }


Do we support credential in the schema level?

The java client only supports Catalog and Fileset, but there is no extra check for the metadata object type in server side, I'll add it.

I mean why don't we support credential in the schema level?

The catalog credentials is mainly used for static credentials like AKSK, and the fileset credential is used for dymatic credentials like S3 tokens, is there any scenarios about schema credentials?

jerryshao · 2024-12-19T11:31:30Z

...-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/SecureHadoopCatalogOperations.java

+  private Map<String, CredentialContext> getFilesetCredentialContexts(
+      NameIdentifier filesetIdentifier, CredentialPrivilege privilege) {
+    Fileset fileset = loadFileset(filesetIdentifier);
+    String location = fileset.storageLocation();


Do you need to check if the location and the provider is mapped? For example, what if the storage location is s3 and we don't have s3 credential provider?

It's hard to do the mapping especially for custom credential provider, how about let it throw a runtime exception?

jerryshao · 2024-12-19T11:33:46Z

core/src/main/java/org/apache/gravitino/catalog/CredentialManager.java

+  @SuppressWarnings("UnusedVariable")
+  private CredentialPrivilege getCredentialPrivilege(String user, NameIdentifier identifier)
+      throws NotAuthorizedException {
+    return CredentialPrivilege.WRITE;


Why do we return write privilege here?

Besides, do we need a CredentialManager, or is the class name proper? Basically, my feeling is that it manages nothing, just a dispatcher.

As Gravitino doesn't provide the feature to check which privileges are granted to a user to a specific fileset, we provide a default WRITE privilege to make the access to fileset works well.

jerryshao · 2024-12-19T11:37:00Z

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

+/**
+ * Manages credentials in one catalog, including different credential providers, credential caching.
+ */
+public class CatalogCredentialManager {


It is confusing we have a CredentialManager and CatalogCredentialManager, I would suggest we have a better name.

You'd better figure out a good name.

jerryshao · 2024-12-23T09:30:39Z

...-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/SecureHadoopCatalogOperations.java

+      return getCatalogCredentialContexts();
+    }
+    return getFilesetCredentialContexts(nameIdentifier, privilege);
+  }


I mean why don't we support credential in the schema level?

jerryshao · 2024-12-23T09:32:52Z

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

+        .filter(Objects::nonNull)
+        .collect(Collectors.toList());
+  }
+}


If this interface should be leveraged by catalog implementor, we should put this into the connector package.

jerryshao · 2024-12-23T09:38:30Z

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

+/**
+ * Manages credentials in one catalog, including different credential providers, credential caching.
+ */
+public class CatalogCredentialManager {


You'd better figure out a good name.

jerryshao · 2024-12-23T09:46:50Z

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

+   *
+   * @return An instance of {@link CatalogCredentialManager}.
+   */
+  CatalogCredentialManager getCatalogCredentialManager();


This interface design is not so good, why do we need to expose this class CatalogCredentialManager to the catalog level? I assume an API to get credential or credential context is enough.

removed CatalogCredentialManager and add getCredentialProviders interface

FANNG1 · 2024-12-23T13:14:12Z

@jerryshao , comments are addressed, please review again

jerryshao · 2024-12-23T14:27:55Z

core/src/main/java/org/apache/gravitino/connector/SupportsCredentialOperations.java

+   *
+   * @return A map with of the credential provider type {@link CredentialProvider}.
+   */
+  Map<String, CredentialProvider> getCatalogCredentialProviders();


I think the class CredentialContext and CredentialProvider should also move to connector package.

moved CredentialContext to connector package, not movingCredentialProvider for it is not exported to catalog developer in the latest implementation.

jerryshao · 2024-12-23T14:41:19Z

core/src/main/java/org/apache/gravitino/connector/SupportsCredentialOperations.java

+        .filter(Objects::nonNull)
+        .collect(Collectors.toList());
+  }
+}


This version looks better. I need to take a deep look.

With the limitation of the interface, some common logic(like loading credential providers) is implemented in CredentialUtils, another way is to introduce a new class BaseCatalogOperations to enclosure the common logic, but I'm not sure whether it's a good idea.

Can we achieve this in the BaseCatalog, to mix this interface into BaseCatalog?

yes, seems a better solution, we could use catalog.supportsCredentials().getCredential(MetadataObject) to get the credential and the specific catalog only provide the getMetadataObjectProperties() getLocations() interface, etc.

using PathBasedCredentialContextInfo to include both location and properites info to avoid load fileset multi times.

jerryshao · 2024-12-24T09:48:07Z

core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java

@@ -78,6 +97,8 @@ public abstract class BaseCatalog<T extends BaseCatalog>

  private volatile Map<String, String> properties;

+  private volatile CatalogCredentialManager catalogCredentialManager;


I still think we should use a different name, the name of CatalogCredentialManager is too close to CredentialManager, and easily gets misled.

How about CatalogCredentialProvider or CatalogCredentialDispatcher?

jerryshao · 2024-12-24T09:52:02Z

core/src/main/java/org/apache/gravitino/connector/credential/CredentialPrivilege.java

+public enum CredentialPrivilege {
+  READ,
+  WRITE,
+}


Please add javadoc for class, and also add @DeveloperApi tag to all the classes in this package.

jerryshao · 2024-12-24T09:54:36Z

core/src/main/java/org/apache/gravitino/connector/credential/SupportsPathBasedCredentials.java

+   * @param schemaIdentifier The schema identifier.
+   * @return The schema properties.
+   */
+  Map<String, String> schemaProperties(NameIdentifier schemaIdentifier);


Why do we need schema properties?

jerryshao · 2024-12-24T09:57:08Z

core/src/main/java/org/apache/gravitino/connector/credential/CatalogCredentialManager.java

+import org.slf4j.LoggerFactory;
+
+/** Manages credentials in one catalog. */
+public class CatalogCredentialManager implements Closeable {


It's not proper to expose this class to the catalog implementors, catalog implementors don't need know the details of this class. If you want to expose something to let users to leverage, it is better to expose interface, not implementation.

jerryshao · 2024-12-24T10:01:04Z

core/src/main/java/org/apache/gravitino/connector/credential/SupportsCredentialOperations.java

+ * This interface defines how to get credentials, the catalog operation must implement this
+ * interface to support credential vending.
+ */
+public interface SupportsCredentialOperations {


Why do we need two SupportsXXX interfaces, one for BaseCatalog, one for CatalogOperations? It's hard to understand, also confuses the users.

jerryshao · 2024-12-25T04:12:02Z

core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java

+  public List<Credential> getCredentials(
+      NameIdentifier nameIdentifier, CredentialPrivilege privilege) {
+    Map<String, CredentialContext> contexts = getCredentialContexts(nameIdentifier, privilege);
+    return contexts.entrySet().stream()
+        .map(entry -> getCatalogCredentialManager().getCredential(entry.getKey(), entry.getValue()))
+        .filter(Objects::nonNull)
+        .collect(Collectors.toList());
+  }
+
+  private CatalogCredentialProvider getCatalogCredentialManager() {
+    if (catalogCredentialProvider == null) {
+      synchronized (this) {
+        if (catalogCredentialProvider == null) {
+          this.catalogCredentialProvider = new CatalogCredentialProvider(name(), properties());
+        }
+      }
+    }
+    return catalogCredentialProvider;
+  }
+
+  private Map<String, CredentialContext> getCredentialContexts(
+      NameIdentifier nameIdentifier, CredentialPrivilege privilege) {
+    if (nameIdentifier.equals(NameIdentifierUtil.getCatalogIdentifier(nameIdentifier))) {
+      return getCatalogCredentialContexts();
+    }
+
+    if (ops() instanceof SupportsPathBasedCredentials) {
+      List<PathWithCredentialType> pathWithCredentialTypes =
+          ((SupportsPathBasedCredentials) ops()).getPathWithCredentialTypes(nameIdentifier);
+      return CredentialUtils.getPathBasedCredentialContexts(privilege, pathWithCredentialTypes);
+    }
+    throw new NotSupportedException(
+        String.format("Catalog %s doesn't support generate credentials", name()));
+  }
+
+  private Map<String, CredentialContext> getCatalogCredentialContexts() {
+    CatalogCredentialContext context =
+        new CatalogCredentialContext(PrincipalUtils.getCurrentUserName());
+    Set<String> providers = CredentialUtils.getCredentialProviders(() -> properties());
+    return providers.stream().collect(Collectors.toMap(provider -> provider, provider -> context));
+  }


Can we move this logic to CredentialManager? So from my understanding, each catalog can provide the list of PathWithCredentialType to the CredentialManager, credential manager can get credentials base on the provided list. We don't have to 1) introduce so manage concepts in the catalog level; 2) The credential management mechanism can all be achieved in one place. In the meantime, we don't have to introduce a concept of CatalogCredentialProvider to manage the credentials per catalog.

CredentialOperationDispatcher is used to use catalog specific resource to generate credentials.
CatalogCredentialOperationDispatcher manages the life of catalog credential provider and dispatch request to corresponding credential provider.

WDYT of the naming?

jerryshao · 2024-12-25T04:12:47Z

core/src/main/java/org/apache/gravitino/connector/credential/PathWithCredentialType.java

+ * The {@code PathWithCredentialType} class represents a combination of a path and its associated
+ * credential type.
+ */
+@Evolving


Change to @DeveloperApi.

updated, What's the difference of DeveloperApi and Evolving, the other classes in connector package use Evolving

jerryshao · 2024-12-25T04:16:29Z

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

+   * <p>If there are multiple properties suppliers, will try to get the credential providers in the
+   * input order.


How does user guarantee the input order if he doesn't know the background, can we have a better solution?

How about rename to getCredentialProvidersByOrder or add order information to the arguments? like getCredentialProviders(firstPropertiesSupplier, secondPropertiesSupplier, thirdPropertiesSupplier)

using getCredentialProvidersByOrder

FANNG1 · 2024-12-25T07:11:28Z

@jerryshao comments addressed, please help to review again

FANNG1 · 2024-12-25T08:26:54Z

rename catalogCredentialOperationDispatcher to catalogCredentialManager. and rename PathWithCredentialType to PathBasedContext

jerryshao · 2024-12-25T14:27:13Z

core/src/main/java/org/apache/gravitino/connector/credential/PathBasedContext.java

+   * @param path The path string.
+   * @param credentialType The type of the credential.
+   */
+  public PathBasedContext(String path, String credentialType) {


PathBasedContext is quite similar to PathBasedCredentialContext, can we have a better name, like PathContext?

jerryshao · 2024-12-25T14:29:25Z

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

+
+  private static Set<String> getCredentialProvidersFromProperties(Map<String, String> properties) {
+    if (properties == null) {
+      return ImmutableSet.of();


We can use Collections.emptySet() for here and other places.

jerryshao · 2024-12-25T14:31:15Z

Generally LGTM, just two small issues.

jerryshao · 2024-12-26T02:39:34Z

core/src/main/java/org/apache/gravitino/connector/credential/SupportsPathBasedCredentials.java

+   * @param nameIdentifier, The identifier for fileset, table, etc.
+   * @return A list of {@link PathContext}
+   */
+  List<PathContext> getPathBasedContext(NameIdentifier nameIdentifier);


Can you please change to getPathContext?

…atalog (apache#5682) ### What changes were proposed in this pull request? Support credential vending for fileset catalog 1. add `credential-providers` properties for the fileset catalog, schema, and fileset. 2. try to get `credential-providers` from the order of fileset, schema, and catalog. 3. The user could set multi-credential providers ### Why are the changes needed? Fix: apache#5620 ### Does this PR introduce _any_ user-facing change? will add document after this PR is merged ### How was this patch tested? Add IT and test with local setup Gravitino server

FANNG1 marked this pull request as draft November 26, 2024 13:42

FANNG1 force-pushed the gravitino-credential branch 3 times, most recently from bacb98c to cf38676 Compare November 27, 2024 08:25

FANNG1 mentioned this pull request Nov 27, 2024

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

Merged

FANNG1 force-pushed the gravitino-credential branch from 92a3167 to ba917f4 Compare November 28, 2024 08:32

FANNG1 force-pushed the gravitino-credential branch 3 times, most recently from 6d361c5 to 995c130 Compare December 11, 2024 09:06

FANNG1 force-pushed the gravitino-credential branch 3 times, most recently from 4594934 to c046392 Compare December 18, 2024 09:15

FANNG1 changed the title ~~[SIP] POC of gravitino credential vending~~ [#5620] feat(fileset): Support credential vending for fileset catalog Dec 18, 2024

FANNG1 marked this pull request as ready for review December 18, 2024 09:26

jerryshao reviewed Dec 19, 2024

View reviewed changes

jerryshao reviewed Dec 23, 2024

View reviewed changes

FANNG1 force-pushed the gravitino-credential branch from 6c2d66f to 94fd8c0 Compare December 23, 2024 13:00

jerryshao reviewed Dec 23, 2024

View reviewed changes

FANNG1 force-pushed the gravitino-credential branch from bfb55b4 to 48ca294 Compare December 24, 2024 08:08

FANNG1 marked this pull request as draft December 24, 2024 08:16

FANNG1 force-pushed the gravitino-credential branch from 39504b2 to 4cc944e Compare December 24, 2024 09:19

FANNG1 marked this pull request as ready for review December 24, 2024 09:21

jerryshao reviewed Dec 24, 2024

View reviewed changes

FANNG1 added 2 commits December 24, 2024 19:18

support credential vending for fileset

b8a47e4

refactor

364c412

FANNG1 force-pushed the gravitino-credential branch 2 times, most recently from 98af70c to 84a23a8 Compare December 24, 2024 14:08

jerryshao reviewed Dec 25, 2024

View reviewed changes

FANNG1 added 2 commits December 25, 2024 14:17

refactor2

277dff5

refactor3

785664c

FANNG1 force-pushed the gravitino-credential branch from 674f0d4 to 785664c Compare December 25, 2024 06:51

FANNG1 added 2 commits December 25, 2024 14:55

rename getCredentialProvidersByOrder

7df3df6

xx

e6fb39c

FANNG1 added 2 commits December 25, 2024 15:52

xx

c4f75c8

rename

c37d0f9

jerryshao reviewed Dec 25, 2024

View reviewed changes

FANNG1 added 2 commits December 26, 2024 09:48

fix comment

14185a6

fix comment

63df12d

jerryshao reviewed Dec 26, 2024

View reviewed changes

path context

b6b7355

jerryshao approved these changes Dec 26, 2024

View reviewed changes

jerryshao merged commit 082bbdc into apache:main Dec 26, 2024
26 checks passed

jerryshao assigned FANNG1 Dec 26, 2024

		@@ -78,6 +97,8 @@ public abstract class BaseCatalog<T extends BaseCatalog>

		private volatile Map<String, String> properties;

		private volatile CatalogCredentialManager catalogCredentialManager;

		* <p>If there are multiple properties suppliers, will try to get the credential providers in the
		* input order.

[#5620] feat(fileset): Support credential vending for fileset catalog #5682

[#5620] feat(fileset): Support credential vending for fileset catalog #5682

Conversation

FANNG1 commented Nov 26, 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 Dec 18, 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

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

Choose a reason for hiding this comment

FANNG1 commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Dec 23, 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 Dec 24, 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 Dec 25, 2024

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

jerryshao commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Nov 26, 2024 •

edited

Loading

jerryshao Dec 23, 2024 •

edited

Loading

FANNG1 Dec 24, 2024 •

edited

Loading