[#1451] feat(hive): Hive catalog supports to execute operations in the Kerberos mode. #1685

qqqttt123 · 2024-01-24T09:23:05Z

What changes were proposed in this pull request?

Hive catalog supports to execute operations in the Kerberos mode.

Why are the changes needed?

Fix: #1451

Does this PR introduce any user-facing change?

Yes, added the document.

How was this patch tested?

Manual test.

qqqttt123 · 2024-01-24T09:23:40Z

Wait for #596

jerryshao · 2024-01-31T02:19:59Z

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

+        try {
+          URI uri = new URI(keyTabUri);
+          String scheme = Optional.ofNullable(uri.getScheme()).orElse("file");
+          switch (scheme) {
+            case "http":
+            case "https":
+            case "ftp":
+              int timeout =
+                  (int)
+                      catalogPropertiesMetadata.getOrDefault(
+                          conf, HiveCatalogPropertiesMeta.FETCH_TIMEOUT_SEC);
+              URLConnection uc = uri.toURL().openConnection();
+              uc.setConnectTimeout(timeout * 1000);
+              uc.setReadTimeout(timeout * 1000);
+              uc.connect();
+
+              Files.copy(uc.getInputStream(), keyTabFile.toPath());
+              break;
+
+            case "file":
+              Files.createSymbolicLink(keyTabFile.toPath(), new File(uri.getPath()).toPath());
+              break;
+
+              // TODO: Supports to download keytab from HCFS, it's unsafe to store
+              // keytab in an unsecured HDFS cluster. If we use a secured HDFS cluster,
+              // we should have a keytab first.
+            default:
+              throw new IllegalArgumentException(
+                  String.format("Don't support the scheme %s", scheme));
+          }
+        } catch (URISyntaxException ue) {
+          throw new IllegalArgumentException("The uri of keytab has the wrong format", ue);
+        }


Can you please move this to a common module with a generic method?

jerryshao · 2024-01-31T02:21:41Z

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

+              // we should have a keytab first.
+            default:
+              throw new IllegalArgumentException(
+                  String.format("Don't support the scheme %s", scheme));


"Doesn't support...."

jerryshao · 2024-01-31T02:22:04Z

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

+              Files.createSymbolicLink(keyTabFile.toPath(), new File(uri.getPath()).toPath());
+              break;
+
+              // TODO: Supports to download keytab from HCFS, it's unsafe to store


I would suggest to add HDFS support, it is quite common to use.

What is HCFS?

Hadoop Compatible File System.

I would suggest to add HDFS support, it is quite common to use.

Like the comment. If we use a unsecured cluster to download a key tab. It's weird.

jerryshao · 2024-01-31T02:25:45Z

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

+      refreshScheduledExecutor = null;
+    }
+
+    File keyTabFile = new File(String.format("/tmp/gravitino-%s-keytab", entity.id()));


It is not safe to put keytab in /tmp folder, /tmp folder will be cleaned by the administrator. You'd better choose a new folder.

jerryshao · 2024-01-31T02:26:18Z

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

@@ -204,6 +322,7 @@ public HiveSchema createSchema(
      NameIdentifier ident, String comment, Map<String, String> properties)
      throws NoSuchCatalogException, SchemaAlreadyExistsException {
    try {
+      LOG.warn("Current user : " + UserGroupInformation.getCurrentUser().getUserName());


What is the usage here?

warning level?

jerryshao · 2024-01-31T02:27:25Z

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

+          .put(
+              REFRESH_INTERVAL_SEC,
+              PropertyEntry.integerOptionalPropertyEntry(
+                  REFRESH_INTERVAL_SEC, "The interval to refresh the principal", true, 60, false))


60 seconds is too frequent for key refreshment.

This interval just check whether the UserGroupInformation is valid. It won't login every time. I will optimize the name and description.

yuqi1129 · 2024-01-31T02:39:25Z

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

+        File keyTabFile = new File(String.format("/tmp/gravitino-%s-keytab", entity.id()));
+        keyTabFile.deleteOnExit();
+        if (keyTabFile.exists() && !keyTabFile.delete()) {
+          LOG.warn("Fail to delete key tab file {}", keyTabFile.getAbsolutePath());


I believe the warn level is not enough for the failure of dropping keyTableFile.

yuqi1129 · 2024-01-31T02:48:06Z

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

+              Files.createSymbolicLink(keyTabFile.toPath(), new File(uri.getPath()).toPath());
+              break;
+
+              // TODO: Supports to download keytab from HCFS, it's unsafe to store


What is HCFS?

yuqi1129 · 2024-01-31T02:49:40Z

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

+            case "https":
+            case "ftp":
+              int timeout =
+                  (int)


FileUtils.copyURLToFile();

yuqi1129 · 2024-01-31T02:56:34Z

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

+              throw new IllegalArgumentException(
+                  String.format("Don't support the scheme %s", scheme));
+          }
+        } catch (URISyntaxException ue) {


I believe we can simplify the nested try-catch block.

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

jerryshao · 2024-01-31T06:18:04Z

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

+  private DownloadUtils() {}
+
+  public static void downloadFile(
+      String keyTabUri, File keyTabFile, int timeout, Configuration conf) throws IOException {


Why the parameter should be keyTabXXX, it is a common method. Please carefully review your code again.

Also, can you please move this method to common module. It actually can be used by other modules.

We can't put this to common module. If we put this to common module, the Hadoop class will be loaded by the root JVM. We can't isolate the class of Hadoop.

jerryshao · 2024-01-31T06:20:15Z

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

+              String.format("Doesn't support the scheme %s", scheme));
+      }
+    } catch (URISyntaxException ue) {
+      throw new IllegalArgumentException("The uri of keytab has the wrong format", ue);


Can you have a UT for this?

jerryshao · 2024-01-31T06:22:11Z

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

+        File keyTabFile = new File(String.format("tmp/gravitino-%s-keytab", entity.id()));
+        keyTabFile.deleteOnExit();
+        if (keyTabFile.exists() && !keyTabFile.delete()) {
+          LOG.error("Fail to delete key tab file {}", keyTabFile.getAbsolutePath());


Should you throw an exception here, or continue to execute your code?

jerryshao · 2024-01-31T06:25:53Z

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

+          file.mkdir();
+        }
+
+        File keyTabFile = new File(String.format("tmp/gravitino-%s-keytab", entity.id()));


“keytab” is a word here, so don't use camel style like "keyTab".

Why don't you use a random name to avoid duplication?

entity id is a unique id. We won't duplicate.

jerryshao · 2024-01-31T06:29:20Z

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

+                        return client.getDelegationToken(
+                            currentUser.getUserName(),
+                            PrincipalUtils.getCurrentPrincipal().getName());
+                      });


Did you get delegation token every time you do the action?

Yes, we will have many users. It's useless if we cache the token.

yuqi1129 · 2024-01-31T08:04:46Z

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

+      checkScheduledExecutor = null;
+    }
+
+    File keyTabFile = new File(String.format("tmp/gravitino-%s-keytab", entity.id()));


I noticed that you have used deleteOnExit to remove the file when JVM terminates, do we need to drop it explicitly here?

I dropped them in the close method, too.

yuqi1129 · 2024-01-31T08:06:29Z

...logs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/TestDownloadUtils.java

+    DownloadUtils.downloadFile(srcFile.toURI().toString(), destFile, 10, new Configuration());
+    Assertions.assertTrue(destFile.exists());
+
+    srcFile.delete();


It's best to test one remote file system to ensure it functions properly.

Do you mean that I should add a MiniHdfsCluster for this pr?

jerryshao · 2024-01-31T13:28:10Z

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

+          break;
+
+        case "hdfs":
+          FileSystem.get(conf).copyToLocalFile(new Path(uri), new Path(destFile.toURI()));


Do you need a break here?

jerryshao · 2024-01-31T13:28:36Z

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

+              String.format("Doesn't support the scheme %s", scheme));
+      }
+    } catch (URISyntaxException ue) {
+      throw new IllegalArgumentException("The uri of keytab has the wrong format", ue);


The exception message is not correct.

jerryshao · 2024-01-31T13:29:40Z

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

+    if (UserGroupInformation.AuthenticationMethod.KERBEROS
+        == SecurityUtil.getAuthenticationMethod(hadoopConf)) {
+      try {
+        File keytabTemporaryDirectory = new File("tmp");


I would suggest to use name of "keytabs", is it better?

jerryshao · 2024-01-31T13:31:39Z

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

+        }
+
+        // The id of entity is a random unique id.
+        File keytabFile = new File(String.format("tmp/gravitino-%s-keytab", entity.id()));


It is a unique id, not a random id. So if you have your gravitino restart, the id will not changed, so the keytab will be existed anyway.

The file is deleteOnExit.

jerryshao · 2024-01-31T13:31:49Z

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

+        keytabFile.deleteOnExit();
+        if (keytabFile.exists() && !keytabFile.delete()) {
+          throw new IllegalStateException(
+              String.format("Fail to delete key tab file %s", keytabFile.getAbsolutePath()));


keytab is a word here and below.

jerryshao · 2024-01-31T13:33:50Z

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

+        String catalogPrincipal = (String) catalogPropertiesMetadata.getOrDefault(conf, PRINCIPAL);
+        Preconditions.checkArgument(
+            StringUtils.isNotBlank(catalogPrincipal),
+            "If you use Kerberos, principal can't be blank");


Please use the third-person statement here and above.

qqqttt123 marked this pull request as draft January 24, 2024 09:23

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

qqqttt123 force-pushed the ISSUE-1451 branch from 7f1a6c3 to 53e355e Compare January 24, 2024 11:48

init

5700a9e

qqqttt123 force-pushed the ISSUE-1451 branch from 53e355e to 5700a9e Compare January 29, 2024 12:39

Heng Qin added 9 commits January 29, 2024 20:40

fix

ef79b24

revert unnecessary changes

b26080c

typo

65a368e

Remove kdc

ff8fc0a

fix

25e840c

fix

7f26959

fix

37d64dd

Support DelegationToken

a1f22ff

Fix todos

34cec8b

qqqttt123 marked this pull request as ready for review January 30, 2024 15:47

fix ut

3d340a2

qqqttt123 requested review from yuqi1129, jerryshao, mchades, xunliu, FANNG1, diqiu50 and Clearvive and removed request for yuqi1129 and jerryshao January 31, 2024 02:33

jerryshao reviewed Jan 31, 2024

View reviewed changes

yuqi1129 reviewed Jan 31, 2024

View reviewed changes

Heng Qin added 3 commits January 31, 2024 12:41

address comments

7b7da6a

address comments

5d37ab2

commit

65f3944

jerryshao reviewed Jan 31, 2024

View reviewed changes

Heng Qin added 2 commits January 31, 2024 14:50

fix

6ecf4ac

fix

2f7face

yuqi1129 reviewed Jan 31, 2024

View reviewed changes

Heng Qin added 11 commits January 31, 2024 18:50

Commit fix

530fa3c

rename keyTab to keytab

baa7054

refactor

2a87e70

refactor

fd0580b

fix

fab6a73

fix

a465aaa

fix

5a62326

polish

ea181c9

fix

0a4e914

fix

80e0fdb

fix

8ac673f

jerryshao reviewed Jan 31, 2024

View reviewed changes

Heng Qin added 5 commits January 31, 2024 21:45

fix

3b7e4b2

address the comments

df21165

fix

09af091

fix

e2f96f6

fix

8202fc9

jerryshao approved these changes Jan 31, 2024

View reviewed changes

jerryshao merged commit 1a8768c into apache:main Jan 31, 2024
5 checks passed

[#1451] feat(hive): Hive catalog supports to execute operations in the Kerberos mode. #1685

[#1451] feat(hive): Hive catalog supports to execute operations in the Kerberos mode. #1685

Conversation

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

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

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 commented Jan 24, 2024 •

edited

Loading