[#596] feat(hive): Hive catalog supports to impersonate users to execute operations in simple mode. #1450

qqqttt123 · 2024-01-11T07:01:36Z

What changes were proposed in this pull request?

Hive catalog supports to impersonate users to execute operations in simple mode.
For Kerberos mode, I have created an new issue. I will finish it in the later pull request.
We use a Hive client cache pool referring to the Iceberg cache pool. We use user name as the key of cache pool.

Why are the changes needed?

Fix: #596

Does this PR introduce any user-facing change?

Yes, we will add a new document.

How was this patch tested?

Add a new IT

…ute operations

qqqttt123 · 2024-01-11T12:07:57Z

...src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/ProxyCatalogHiveIT.java

+  @BeforeAll
+  public static void startIntegrationTest() throws Exception {
+    originHadoopUser = System.getenv(HADOOP_USER_NAME);
+    setEnv(HADOOP_USER_NAME, null);


We should remove HADOOP_USER_NAME, otherwise we will still use HADOOP_USER_NAME when we use unsecured mode.

What is the unsecured mode and does the user know about it?

What is the unsecured mode and does the user know about it?

It's the concept of Hadoop. Unsecured mode is the simple mode.

This is a hard-coded implement in the HiveClient actually.

core/src/main/java/com/datastrato/gravitino/catalog/PropertyEntry.java

qqqttt123 · 2024-01-16T02:46:40Z

@mchades @jerryshao @yuqi1129 Could you help me review this pr?

qqqttt123 · 2024-01-17T07:42:58Z

@FANNG1 @jerryshao @yuqi1129 @diqiu50 @Clearvive Could you help me review this pr?

yuqi1129 · 2024-01-17T09:30:35Z

catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/CachedClientPool.java

+ *
+ * <p>A ClientPool that caches the underlying HiveClientPool instances.
+ */
+public class CachedClientPool implements ClientPool<IMetaStoreClient, TException> {


What's the difference between CachedClientPool and HiveClientPool?

You can see the comment A ClientPool that caches the underlying HiveClientPool instances.

yuqi1129 · 2024-01-17T09:32:22Z

...alog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogPropertiesMeta.java

+      "client.pool.cache.eviction-interval-ms";
+
+  public static final long DEFAULT_CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS =
+      TimeUnit.MINUTES.toMillis(5);;


extra comma here.

yuqi1129 · 2024-01-17T09:37:51Z

catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/CachedClientPool.java

+  static Key extractKey() {
+    List<Object> elements = Lists.newArrayList();
+    try {
+      elements.add(UserGroupInformation.getCurrentUser().getUserName());


I'm not sure why the Key contains multiple user names. Shouldn't each user have their own client pool?

Elements contain one user name. Why do we use elements. It's because Iceberg uses elements. There are other config options in the elements. For Gravitino, our cached keys are only username. I reserve the elements. Because it's easy to extend other cache keys.

yuqi1129 · 2024-01-17T10:02:38Z

core/src/main/java/com/datastrato/gravitino/catalog/CatalogOperationsProxy.java

+import java.lang.reflect.Proxy;
+import java.util.Collections;
+
+public class CatalogOperationsProxy implements InvocationHandler {


You should include a Java doc on how you use the proxy mechanism here.

yuqi1129 · 2024-01-17T10:03:26Z

core/src/main/java/com/datastrato/gravitino/catalog/CatalogProxyPlugin.java

+import java.security.Principal;
+import java.util.Map;
+
+public interface CatalogProxyPlugin {


Class java doc and method java doc.

yuqi1129 · 2024-01-17T10:04:00Z

core/src/main/java/com/datastrato/gravitino/catalog/CatalogProxyPlugin.java

+public interface CatalogProxyPlugin {
+  Object doAs(
+      Principal principal, Executable<Object, Exception> action, Map<String, String> properties)
+      throws Throwable;


Can we use a more detailed exception class?

No. We can't.

yuqi1129 · 2024-01-17T10:06:26Z

docs/apache-hive-catalog.md

+| `metastore.uris`                         | The Hive metastore service URIs, separate multiple addresses with commas. Such as `thrift://127.0.0.1:9083`                                                                                                                                                                      | (none)        | Yes      | 0.2.0         |
+| `client.pool-size`                       | The maximum number of Hive metastore clients in the pool for Gravitino.                                                                                                                                                                                                          | 1             | No       | 0.2.0         |
+| `gravitino.bypass.`                      | Property name with this prefix passed down to the underlying HMS client for use. Such as `gravitino.bypass.hive.metastore.failure.retries = 3` indicate 3 times of retries upon failure of Thrift metastore calls                                                                | (none)        | No       | 0.2.0         |
+| `client.pool.cache.eviction-interval-ms` | The cache pool eviction interval.                                                                                                                                                                                                                                                |               | No       | 0.4.0         |


The default value of client.pool.cache.eviction-interval-ms is empty.

yuqi1129 · 2024-01-17T10:08:31Z

...catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogProxyPlugin.java

+      } else if (innerException instanceof InvocationTargetException) {
+        throw innerException.getCause();
+      } else {
+        throw e.getCause();


throw innerException

yuqi1129 · 2024-01-17T10:10:51Z

core/src/main/java/com/datastrato/gravitino/catalog/BaseCatalog.java

+    return CatalogOperationsProxy.getProxy(ops, plugin);
+  }
+
+  protected CatalogProxyPlugin newProxyPlugin(Map<String, String> config) {


Can we make CatalogProxyPlugin invisible to detailed catalogs? Making it as a method of catalog seems too invasive?

Every catalog should be proxied actually. So I choose to move CatalogProxyPlugin to here.

jerryshao · 2024-01-23T12:56:59Z

catalogs/catalog-hive/build.gradle.kts

@@ -17,6 +17,9 @@ dependencies {
  compileOnly(libs.lombok)
  annotationProcessor(libs.lombok)

+  compileOnly(libs.immutables.value)
+  annotationProcessor(libs.immutables.value)


Maybe you should also update license.bin?

jerryshao · 2024-01-23T13:04:29Z

core/src/main/java/com/datastrato/gravitino/catalog/BaseCatalog.java

@@ -87,7 +87,13 @@ public CatalogOperations ops() {
        if (ops == null) {
          Preconditions.checkArgument(
              entity != null && conf != null, "entity and conf must be set before calling ops()");
-          ops = newOps(conf);
+          CatalogOperations newOps = newOps(conf);


If the partition API is merged, how do you handle that case?

Maybe we will have TableOperations. We will create proxy object for it.

I think you have to figure out a solution with @mchades ASAP before the interface is settle down.

Think twice. If the type of value is Table, I return a proxy object, too.

public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { Object object = plugin.doAs( PrincipalUtils.getCurrentPrincipal(), () -> method.invoke(ops, args), Collections.emptyMap()); if (object instanceof Table) { return createProxy(object, plugin); } return object; }

I recover the code after discussion with @mchades if we have TableOperation.

I have other pull requests relying on this pr. Maybe I can change this place to adapt @mchades 's solution in later pull requests.

jerryshao · 2024-01-23T13:05:01Z

core/src/main/java/com/datastrato/gravitino/catalog/BaseCatalog.java

+  }
+
+  protected ProxyPlugin newProxyPlugin(Map<String, String> config) {
+    return null;


It is better using optional to avoid null check.

jerryshao · 2024-01-23T13:05:25Z

core/src/main/java/com/datastrato/gravitino/catalog/OperationsProxy.java

+    this.ops = ops;
+  }
+
+  public static <T> T getProxy(T ops, ProxyPlugin plugin) {


Is it better to rename to "createProxy"?

jerryshao · 2024-01-23T13:08:35Z

...src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/ProxyCatalogHiveIT.java

+            .withDataType(Types.StringType.get())
+            .withComment("col_3_comment")
+            .build();
+    return new ColumnDTO[] {col1, col2, col3};


Change all the ColumnDTO to use ColumnImpl API.

jerryshao · 2024-01-24T08:26:14Z

core/src/main/java/com/datastrato/gravitino/catalog/OperationsProxy.java

+  private static <T> T createProxyInternal(T ops, ProxyPlugin plugin, Class<?>[] interfaces) {
+    return (T)
+        Proxy.newProxyInstance(
+            ops.getClass().getClassLoader(), interfaces, new OperationsProxy(plugin, ops));


Have you tested this feature in the deployed Gravitino, I'm not sure if there's a classloader issue?

I have written an IT ProxyCatalogHiveIT to test this.

It's not enough to test using mini gravitino, you should verify it manually in your local environment.

Verified with the playground.

jerryshao · 2024-01-24T08:36:14Z

...src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/ProxyCatalogHiveIT.java

+            anotherSchemaName.toLowerCase()));
+    Assertions.assertThrows(
+        RuntimeException.class,
+        () -> anotherCatalog.asSchemas().createSchema(anotherIdent, comment, properties));


Can you please verify the exception message?

jerryshao · 2024-01-24T08:36:47Z

...src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/ProxyCatalogHiveIT.java

+    Database db = hiveClientPool.run(client -> client.getDatabase(schemaName));
+    Assertions.assertEquals(EXPECT_USER, db.getOwnerName());
+    Assertions.assertEquals(
+        EXPECT_USER, hdfs.getFileStatus(new Path(db.getLocationUri())).getOwner());


What is the default user for HDFS?

root is the default user for HDFS.

jerryshao · 2024-01-24T08:37:02Z

...src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/ProxyCatalogHiveIT.java

+                    columns,
+                    comment,
+                    ImmutableMap.of(),
+                    Partitioning.EMPTY_PARTITIONING));


Please also add necessary white space and comments to the code blocks. You put everything together without any ws and comments, which is hard to review.

jerryshao · 2024-01-24T11:17:20Z

.../catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java

-      clientPool.close();
-      clientPool = null;
-    }
+    // We used a cached client pool, the pool will close clients after expiration.


We still need to close the all the dangling clients immediately when catalog is closed, otherwise it will be leaked.

Just left one comment, please think about it @qqqttt123

[apache#596] feat(hive): Hive catalog supports to impersonate to exec…

b5e44c2

…ute operations

qqqttt123 marked this pull request as draft January 11, 2024 07:01

Heng Qin added 3 commits January 11, 2024 15:02

Merge remote-tracking branch 'upstream/main' into ISSUE-596

ef17af3

fix

1020290

fix

312c808

qqqttt123 changed the title ~~[#596] feat(hive): Hive catalog supports to impersonate to execute operations in simple mode.~~ [#596] feat(hive): Hive catalog supports to impersonate users to execute operations in simple mode. Jan 11, 2024

Heng Qin added 4 commits January 11, 2024 17:25

fix

b2ec506

fix

e07f7be

fix

8c867ca

Merge remote-tracking branch 'upstream/main' into ISSUE-596

0d206e8

qqqttt123 commented Jan 11, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/catalog/PropertyEntry.java Show resolved Hide resolved

Heng Qin added 4 commits January 11, 2024 20:16

fix

eb4f93d

fix

ca5851b

fix

0b4d45c

fix

0b9294b

qqqttt123 marked this pull request as ready for review January 11, 2024 13:36

qqqttt123 requested review from jerryshao, yuqi1129, xunliu, FANNG1, diqiu50, mchades and Clearvive and removed request for jerryshao January 11, 2024 13:37

fix

479cdee

yuqi1129 reviewed Jan 17, 2024

View reviewed changes

qqqttt123 reopened this Jan 23, 2024

jerryshao reviewed Jan 23, 2024

View reviewed changes

Heng Qin added 2 commits January 23, 2024 21:38

address comments

63d1368

Modify License.bin

8f3c6fe

qqqttt123 closed this Jan 23, 2024

qqqttt123 reopened this Jan 23, 2024

Heng Qin added 3 commits January 24, 2024 10:14

fix

0994a5a

Create proxy for table object

0b0ba2e

fix

23c55bb

qqqttt123 requested a review from jerryshao January 24, 2024 04:13

jerryshao reviewed Jan 24, 2024

View reviewed changes

address comments

5a0c0de

qqqttt123 requested a review from jerryshao January 24, 2024 09:16

address comments

970898f

qqqttt123 closed this Jan 24, 2024

qqqttt123 reopened this Jan 24, 2024

Heng Qin added 3 commits January 24, 2024 18:10

Merge remote-tracking branch 'upstream/main' into ISSUE-596

2255deb

fix

0180884

fix

393f69e

qqqttt123 added this to the Gravitino 0.4.0 milestone Jan 24, 2024

jerryshao reviewed Jan 24, 2024

View reviewed changes

Add close method

a35bf29

jerryshao previously approved these changes Jan 24, 2024

View reviewed changes

Set null

74d6ed1

qqqttt123 dismissed jerryshao’s stale review via 74d6ed1 January 24, 2024 11:26

jerryshao approved these changes Jan 24, 2024

View reviewed changes

jerryshao merged commit c045c31 into apache:main Jan 24, 2024
5 checks passed

[#596] feat(hive): Hive catalog supports to impersonate users to execute operations in simple mode. #1450

[#596] feat(hive): Hive catalog supports to impersonate users to execute operations in simple mode. #1450

Conversation

qqqttt123 commented Jan 11, 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?

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

qqqttt123 commented Jan 16, 2024

qqqttt123 commented Jan 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

qqqttt123 Jan 17, 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

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

qqqttt123 Jan 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

qqqttt123 commented Jan 11, 2024 •

edited

Loading

qqqttt123 Jan 17, 2024 •

edited

Loading

qqqttt123 Jan 24, 2024 •

edited

Loading