[#6024] feat(iceberg): refactor Iceberg credential code to reuse credential component in Gravitino server #6021

FANNG1 · 2024-12-27T01:53:21Z

What changes were proposed in this pull request?

reuse CatalogCredentialManager to manage the credentials in Iceberg catalog.
use CatalogWrapperForREST to manage some REST specific operations like credential vending.
depracate catalog-provider-type , use catalog-providers instead.

Why are the changes needed?

Fix: #6024

Does this PR introduce any user-facing change?

yes, depracate catalog-provider-type , use catalog-providers, do some compatibility work.

How was this patch tested?

run with s3 token

FANNG1 · 2024-12-27T06:50:37Z

@jerryshao @orenccl @theoryxu please help to review when you have time, thanks

FANNG1 · 2024-12-27T07:18:39Z

...rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java

    this.httpServletRequest = httpRequest;
    this.remoteHostName = httpRequest.getRemoteHost();
    this.httpHeaders = IcebergRestUtils.getHttpHeaders(httpRequest);
    this.catalogName = catalogName;
    this.userName = PrincipalUtils.getCurrentUserName();
+    this.requestCredentialVending = requestCredentialVending;


Trying to get credential information from httpHeaders, test jersey test fails for couldn't get real HTTP headers, try find some solution

jerryshao · 2024-12-27T08:28:24Z

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

+    } else if (credentialProviders.size() > 1) {
+      throw new RuntimeException("There are multiple credential providers for the catalog.");
+    }
+    return getCredential(credentialProviders.keySet().iterator().next(), context);


Can we use a more meaningful exception instead of RuntimeException?

jerryshao · 2024-12-27T08:28:49Z

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

+  // Get credential with only one credential provider.
+  public Credential getCredential(CredentialContext context) {
+    if (credentialProviders.size() == 0) {
+      throw new RuntimeException("There are not any credential provider for the catalog.");


"There is no credential..."

jerryshao · 2024-12-27T08:48:51Z

...rg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java

+  }
+
+  @SuppressWarnings("deprecation")
+  private Map<String, String> normalizeCredentialProperties(Map<String, String> properties) {


I think the name "normalize" cannot fully describe the meaning of this method, this method doesn't normalize anything, it just check and keep the compatibility of the deprecated configs. It would be better to have a new name.

updated with adjustCredentialPropertiesForCompatibility

jerryshao · 2024-12-27T08:49:57Z

...rg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java

+  public LoadTableResponse createTable(
+      Namespace namespace, CreateTableRequest request, boolean requestCredential) {
+    LoadTableResponse loadTableResponse = super.createTable(namespace, request);
+    if (requestCredential) {
+      return injectCredentialConfig(
+          TableIdentifier.of(namespace, request.name()), loadTableResponse);
+    }
+    return loadTableResponse;
+  }
+
+  public LoadTableResponse loadTable(TableIdentifier identifier, boolean requestCredential) {
+    LoadTableResponse loadTableResponse = super.loadTable(identifier);
+    if (requestCredential) {
+      return injectCredentialConfig(identifier, loadTableResponse);
+    }
+    return loadTableResponse;
+  }


I assume these two methods should add @Override annotation, right?

no, the method is different for requestCredential parameter

jerryshao · 2024-12-27T08:51:15Z

...rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java

+   *
+   * @return true if the request is for credential vending, false otherwise.
+   */
+  public boolean isRequestCredentialVending() {


Can we rename to requestCredentialVending?

orenccl

I have read the code, found no issues, and learned from it.

jerryshao · 2024-12-28T09:02:04Z

Seems like there's some conflicts need to be resolved.

FANNG1 · 2024-12-28T15:00:24Z

Seems like there's some conflicts need to be resolved.

updated

jerryshao · 2024-12-30T02:00:35Z

core/src/main/java/org/apache/gravitino/credential/config/CredentialConfig.java

+          .doc("Credential providers, separated by comma.")
+          .version(ConfigConstants.VERSION_0_8_0)
+          .stringConf()
+          .create();


Can you use toSequence() for this configuration.

jerryshao · 2024-12-30T02:06:58Z

...rg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java

+  }
+
+  @SuppressWarnings("deprecation")
+  private Map<String, String> adjustCredentialPropertiesForCompatibility(


The name is too long, I think we can use checkForCompatibility.

jerryshao · 2024-12-30T02:07:19Z

...rg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java

+        MapUtils.getFilteredMap(
+            config.getIcebergCatalogProperties(),
+            key -> catalogPropertiesToClientKeys.contains(key));
+    // To compatibility with old version


"To be compatible with..."

jerryshao · 2024-12-30T03:05:38Z

core/src/main/java/org/apache/gravitino/credential/config/CredentialConfig.java

      new ConfigBuilder(CredentialConstants.CREDENTIAL_PROVIDERS)
          .doc("Credential providers, separated by comma.")
          .version(ConfigConstants.VERSION_0_8_0)
          .stringConf()
-          .create();
+          .toSequence()
+          .createWithDefault(Collections.emptyList());


It's weird that changes to List here alone is enough? Do you need to change to other places that uses this variable, how can they pass the type check?

The config entry defined here is not used to get providers, updated the code, please review again

FANNG1 force-pushed the iceberg_credential branch 2 times, most recently from 45db206 to 1582bd3 Compare December 27, 2024 06:29

FANNG1 changed the title ~~[SIP] refactor Iceberg credential~~ [#6024] feat(iceberg): refactor Iceberg credential code to reuse credential component in Gravitino server Dec 27, 2024

FANNG1 commented Dec 27, 2024

View reviewed changes

jerryshao reviewed Dec 27, 2024

View reviewed changes

orenccl reviewed Dec 28, 2024

View reviewed changes

FANNG1 added 5 commits December 28, 2024 21:44

iceberg rest cache

c6e4300

xx

01785bd

xx

4b23041

fix comment

300c142

fix style

ee2e233

FANNG1 force-pushed the iceberg_credential branch from ae4a7c4 to ee2e233 Compare December 28, 2024 13:52

jerryshao reviewed Dec 30, 2024

View reviewed changes

fix comment

93f2f5e

jerryshao reviewed Dec 30, 2024

View reviewed changes

use credential config to get providers

b9b873d

jerryshao assigned FANNG1 Dec 30, 2024

jerryshao approved these changes Dec 30, 2024

View reviewed changes

jerryshao merged commit 528d0ea into apache:main Dec 30, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#6024] feat(iceberg): refactor Iceberg credential code to reuse credential component in Gravitino server #6021

[#6024] feat(iceberg): refactor Iceberg credential code to reuse credential component in Gravitino server #6021

FANNG1 commented Dec 27, 2024 •

edited

Loading

FANNG1 commented Dec 27, 2024

FANNG1 Dec 27, 2024

jerryshao Dec 27, 2024

FANNG1 Dec 27, 2024

FANNG1 Dec 28, 2024

jerryshao Dec 27, 2024

FANNG1 Dec 28, 2024

jerryshao Dec 27, 2024

FANNG1 Dec 27, 2024

FANNG1 Dec 28, 2024

jerryshao Dec 27, 2024

FANNG1 Dec 27, 2024

jerryshao Dec 27, 2024

FANNG1 Dec 28, 2024

orenccl left a comment

jerryshao commented Dec 28, 2024

FANNG1 commented Dec 28, 2024

jerryshao Dec 30, 2024

FANNG1 Dec 30, 2024

jerryshao Dec 30, 2024

FANNG1 Dec 30, 2024

jerryshao Dec 30, 2024

FANNG1 Dec 30, 2024

jerryshao Dec 30, 2024

FANNG1 Dec 30, 2024

[#6024] feat(iceberg): refactor Iceberg credential code to reuse credential component in Gravitino server #6021

[#6024] feat(iceberg): refactor Iceberg credential code to reuse credential component in Gravitino server #6021

Conversation

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

orenccl left a comment

Choose a reason for hiding this comment

jerryshao commented Dec 28, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Dec 27, 2024 •

edited

Loading