[#4993] feat(iceberg): integrate credential framework to iceberg REST server #5134

FANNG1 · 2024-10-15T03:39:05Z

What changes were proposed in this pull request?

integrate credential framework to iceberg REST server

Why are the changes needed?

Does this PR introduce any user-facing change?

No

How was this patch tested?

set up a local environment to request credential with the configuration spark.sql.catalog.rest.header.X-Iceberg-Access-Delegation=vended-credentials

FANNG1 · 2024-10-15T10:58:10Z

@jerryshao , it's ready for review now, please help to review when you are free, thanks

jerryshao · 2024-10-16T10:29:00Z

catalogs/catalog-common/src/main/java/org/apache/gravitino/credential/CredentialConstants.java

+  public static final String CREDENTIAL_TYPE = "credential-type";
+
+  private CredentialConstants() {}
+}


I remembered that we already defined this in Credential, we don't have to define this again here.

It's not the same, one is the credential type in Credential , this is the credential type in catalog properties. They are happen to the same name.

Why can't you use a different name?

use credential-provider-type

If it is a catalog property, it is better to define in catalog-common, so other engines can also use it.

the property already defined in catalog-common

jerryshao · 2024-10-16T10:36:09Z

...g/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java

@@ -110,7 +110,7 @@ public void initialize(
    resultConf.put("catalog_uuid", info.id().toString());
    IcebergConfig icebergConfig = new IcebergConfig(resultConf);

-    this.icebergCatalogWrapper = new IcebergCatalogWrapper(icebergConfig);
+    this.icebergCatalogWrapper = new IcebergCatalogWrapper(icebergConfig, false);


Can you add a comment here to explain the reason why we hard code the false here?

jerryshao · 2024-10-16T10:38:26Z

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

+    // todo: transform specific credential to iceberg properties
+    return credential.toProperties();
+  }
+}


Why do we add this Iceberg specific method in common module, can it be in iceberg-common module?

This will also be used in client or spark to transform Credential properties to the properties used for Iceberg. so I place it in common.

jerryshao · 2024-10-16T10:40:18Z

...berg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java

-  public IcebergCatalogWrapper() {
-    this(new IcebergConfig(Collections.emptyMap()));


We can still maintain this default constructor like:

public IcebergCatalogWrapper() { this(xxxx, false) }

What do you think?

it's used to test only, seems useless to maintain the default constructor

jerryshao · 2024-10-16T10:55:26Z

...berg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java

+      // todo(fanng): check user privilege.
+      PathBasedCredentialContext pathBasedCredentialContext =
+          new PathBasedCredentialContext(
+              PrincipalUtils.getCurrentUserName(), ImmutableSet.of(location), ImmutableSet.of());


Do you need to set write path?

yes, because we don't know user will write or read after loading table. We have to give the write privilege to the user .After we supporting ACL, we could grant write privilege to the user only with write privilege.

jerryshao · 2024-10-16T10:58:33Z

...berg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java

    return catalogConfigToClients;
  }

+  private Map<String, String> vendCredentials(String location) {
+    // ifPresentOrElse is not supported in Java8
+    if (credentialProvider.isPresent()) {


You can change to simplify your code here:

if (!xx.isPresent) { throw new XXXException(); } ....

jerryshao · 2024-10-16T11:03:45Z

...ain/java/org/apache/gravitino/iceberg/provider/ConfigBasedIcebergCatalogWrapperProvider.java

@@ -75,7 +75,7 @@ public IcebergCatalogWrapper getIcebergTableOps(String catalogName) {
      LOG.warn(errorMsg);
      throw new RuntimeException(errorMsg);
    }
-    return new IcebergCatalogWrapper(icebergConfig);
+    return new IcebergCatalogWrapper(icebergConfig, true);


Why do we need to have a boolean to control whether to use credential vending or not, is there any better way?

Also, I saw there're lot of hard-code value in this PR, if it is enabled by default, maybe we can figure out a better way other than hard-coded boolean.

removed credential vending logic from IcebergCatalogWrapper, do credential vending after related operation is finished outside.

jerryshao · 2024-10-16T11:04:22Z

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java

    LOG.info(
-        "Create Iceberg table, namespace: {}, create table request: {}",
+        "Create Iceberg table, namespace: {}, create table request: {}, accessDelegation: {}, isCredentialVending:{}",


Add space before {}

FANNG1 · 2024-10-17T09:38:31Z

...rver/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java

+  }
+
+  @Test
+  void testCreateTableWithCredentialVending() {


add credential vending test here

FANNG1 · 2024-10-17T09:41:05Z

@jerryshao please help to review again

jerryshao · 2024-10-17T12:25:52Z

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

+  public static Map<String, String> toIcebergProperties(Credential credential) {
+    // todo: transform specific credential to iceberg properties
+    return credential.toProperties();


This two lines code is not worthy to create a new class here, why can't you put this in iceberg package?

it's like a placeholder function to support more credential properties transform logic, this function will be used in client side and Iceberg REST server side, so I place it in common module.

jerryshao · 2024-10-17T12:28:58Z

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

+
+  public void registerCredentialProvider(
+      String catalogName, CredentialProvider credentialProvider) {
+    CredentialProvider current = credentialProviders.putIfAbsent(catalogName, credentialProvider);


Do we support one catalog with multiple credential vendors? If so, seems this map cannot support it.

Not in current PR, I prefer to support it later in integrate Credential vending to Gravitino server to have a better overall design.

jerryshao · 2024-10-17T12:35:57Z

...main/java/org/apache/gravitino/iceberg/provider/ConfigBasedIcebergCatalogConfigProvider.java

@@ -40,11 +39,12 @@
 * gravitino.iceberg-rest.catalog.hive_proxy.catalog-backend = hive
 * gravitino.iceberg-rest.catalog.hive_proxy.uri = thrift://{host}:{port} ...
 */
-public class ConfigBasedIcebergCatalogWrapperProvider implements IcebergCatalogWrapperProvider {
+public class ConfigBasedIcebergCatalogConfigProvider implements IcebergCatalogConfigProvider {


The class name is a little tricky, is there a better name for it?

FileBasedIcebergCatalogConfigProvider or StaticIcebergCatalogConfigProvider ?

FileBasedXXX and CatalogBasedXXX, how about this?

CatalogBasedCatalogConfigProvider seems a little odd

use StaticCatalogConfigProvider and DynamicCatalogConfigProvider , cc @theoryxu

jerryshao · 2024-10-18T07:36:06Z

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java

+      @HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation) {
+    boolean isCredentialVending = isCredentialVending(accessDelegation);
+    LOG.info(
+        "Load iceberg table, namespace: {}, table: {}, accessDelegation: {}, is credential vending: {}",


access delegation: {}..., credential vending: {}

jerryshao · 2024-10-18T07:39:28Z

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

+    Preconditions.checkState(
+        !credentialProvider.equals(current),
+        String.format(
+            "Should not register multi times to CredentialProviderManager, catalog:%s, credential provider:%s",


"multiple times to..."

jerryshao · 2024-10-18T07:44:47Z

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java

+          X_ICEBERG_ACCESS_DELEGATION
+              + ": "
+              + accessDelegation
+              + " is illegal, Iceberg REST spec supports:[vended-credentials,remote-signing], Gravitino Iceberg REST server supports: vended-credentials");


This line is too long, it should be no longer than 100 chars.

jerryshao · 2024-10-18T07:46:53Z

...t-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java

+          X_ICEBERG_ACCESS_DELEGATION
+              + ": "
+              + accessDelegation
+              + " is illegal, Iceberg REST spec supports:[vended-credentials,remote-signing], Gravitino Iceberg REST server supports: vended-credentials");


if (xxx.isBlank) { return false; } if (xxx.equals...) { return true; } if (xxx.equals...) { throw xxx; } throw xxx; }

jerryshao · 2024-10-18T08:11:55Z

Please fix the build issue here.

FANNG1 · 2024-10-18T09:27:09Z

Please fix the build issue here.

updated, please help to review again

…g REST server (apache#5134) ### What changes were proposed in this pull request? integrate credential framework to iceberg REST server ### Why are the changes needed? Fix: apache#4993 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? set up a local environment to request credential with the configuration `spark.sql.catalog.rest.header.X-Iceberg-Access-Delegation=vended-credentials`

FANNG1 marked this pull request as draft October 15, 2024 03:39

integrate iceberg

81f620e

FANNG1 force-pushed the iceberg_credential branch from 0a5f592 to 81f620e Compare October 15, 2024 09:50

fix comment

a239f0c

FANNG1 changed the title ~~[SIP] integrate credential framework to iceberg REST server~~ [#4993] feat(iceberg): integrate credential framework to iceberg REST server Oct 15, 2024

fix comment

63fc419

FANNG1 marked this pull request as ready for review October 15, 2024 10:57

jerryshao reviewed Oct 16, 2024

View reviewed changes

fix comment

c872e43

jerryshao reviewed Oct 16, 2024

View reviewed changes

FANNG1 added 3 commits October 17, 2024 09:08

fix comment

3e35c8e

fix comment

b72294a

fix comment

6b37a88

FANNG1 marked this pull request as draft October 17, 2024 06:15

FANNG1 added 5 commits October 17, 2024 14:17

fix comment

92e2690

fix comment

81b29e0

fix comment

d142d02

fix comment

e05e0e3

fix comment

bbe673d

FANNG1 commented Oct 17, 2024

View reviewed changes

FANNG1 marked this pull request as ready for review October 17, 2024 09:40

fix comment

491e08d

jerryshao reviewed Oct 17, 2024

View reviewed changes

FANNG1 added 3 commits October 18, 2024 10:01

fix comment

38f9d6d

fix comment

135b3cd

fix comment

8ef9409

jerryshao reviewed Oct 18, 2024

View reviewed changes

fix comment

bc63bad

jerryshao previously approved these changes Oct 18, 2024

View reviewed changes

jerryshao assigned FANNG1 Oct 18, 2024

fix comment

d4d4662

FANNG1 dismissed jerryshao’s stale review via d4d4662 October 18, 2024 09:24

jerryshao approved these changes Oct 18, 2024

View reviewed changes

jerryshao merged commit 10e2943 into apache:main Oct 18, 2024
26 checks passed

		public IcebergCatalogWrapper() {
		this(new IcebergConfig(Collections.emptyMap()));

[#4993] feat(iceberg): integrate credential framework to iceberg REST server #5134

[#4993] feat(iceberg): integrate credential framework to iceberg REST server #5134

Conversation

FANNG1 commented Oct 15, 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 Oct 15, 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

jerryshao Oct 16, 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 Oct 17, 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

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

jerryshao commented Oct 18, 2024

FANNG1 commented Oct 18, 2024

FANNG1 commented Oct 15, 2024 •

edited

Loading

jerryshao Oct 16, 2024 •

edited

Loading

jerryshao Oct 18, 2024 •

edited

Loading