yuqi1129 commented on code in PR #8777:
URL: https://github.com/apache/gravitino/pull/8777#discussion_r2494683553


##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/authentication/kerberos/KerberosConfig.java:
##########
@@ -70,11 +71,19 @@ public class KerberosConfig extends AuthenticationConfig {
           .checkValue(value -> value > 0, 
ConfigConstants.POSITIVE_NUMBER_ERROR_MSG)
           .createWithDefault(2);
 
-  public KerberosConfig(Map<String, String> properties) {
-    super(properties);
+  public KerberosConfig(Map<String, String> properties, Configuration 
configuration) {
+    super(properties, configuration);
+    loadFromHdfsConfiguration(configuration);
     loadFromMap(properties, k -> true);
   }
 
+  private void loadFromHdfsConfiguration(Configuration configuration) {
+    String keyTab = 
configuration.get("hadoop.security.authentication.kerberos.keytab", "");

Review Comment:
   You have defined `hadoop.security.authentication.kerberos.keytab` as a 
constant, please use it. 



##########
clients/filesystem-hadoop3/src/test/resources/hd_kbs_conf/hdfs-site.xml:
##########
@@ -0,0 +1,24 @@
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+  -->
+<configuration>
+    <property>
+        <name>dfs.namenode.kerberos.principal</name>
+        <value>cli@HADOOPKRB</value>

Review Comment:
   The name `cli` is not quite suitable for `namenode`, something like hdfs or 
server may be better.



##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemConfiguration.java:
##########
@@ -184,5 +184,11 @@ public class GravitinoVirtualFileSystemConfiguration {
   /** The default value for whether to enable auto-creation of fileset 
location. */
   public static final boolean FS_GRAVITINO_AUTO_CREATE_LOCATION_DEFAULT = true;
 
+  /** The prefix for Gravitino fileset property configuration keys. */
+  public static final String FS_GRAVITINO_FILESET_PROPERT_PREFIX =
+      "fs.gravitino.fileset.properties.";
+  /** The configuration key name for Gravitino fileset property 'keyname'. */
+  public static final String FS_GRAVITINO_FILESET_PROPERT_KEYNAME_PREFIX = 
"keyname";

Review Comment:
   ditto



##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemConfiguration.java:
##########
@@ -184,5 +184,11 @@ public class GravitinoVirtualFileSystemConfiguration {
   /** The default value for whether to enable auto-creation of fileset 
location. */
   public static final boolean FS_GRAVITINO_AUTO_CREATE_LOCATION_DEFAULT = true;
 
+  /** The prefix for Gravitino fileset property configuration keys. */
+  public static final String FS_GRAVITINO_FILESET_PROPERT_PREFIX =

Review Comment:
   typo: PROPERT



##########
clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/HDFSFileSystemProxy.java:
##########
@@ -0,0 +1,174 @@
+package org.apache.gravitino.filesystem.hadoop;
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import static org.apache.gravitino.catalog.hadoop.fs.Constants.AUTH_KERBEROS;
+import static org.apache.gravitino.catalog.hadoop.fs.Constants.AUTH_SIMPlE;
+import static 
org.apache.gravitino.catalog.hadoop.fs.Constants.FS_DISABLE_CACHE;
+import static 
org.apache.gravitino.catalog.hadoop.fs.Constants.HADOOP_KRB5_CONF;
+import static 
org.apache.gravitino.catalog.hadoop.fs.Constants.HADOOP_SECURITY_AUTHENTICATION;
+import static 
org.apache.gravitino.catalog.hadoop.fs.Constants.HADOOP_SECURITY_KEYTAB;
+import static 
org.apache.gravitino.catalog.hadoop.fs.Constants.HADOOP_SECURITY_PRINCIPAL;
+import static 
org.apache.gravitino.catalog.hadoop.fs.Constants.SECURITY_KRB5_ENV;
+import static 
org.apache.gravitino.catalog.hadoop.fs.HDFSFileSystemProvider.IPC_FALLBACK_TO_SIMPLE_AUTH_ALLOWED;
+
+import java.io.IOException;
+import java.lang.reflect.Method;
+import java.security.PrivilegedExceptionAction;
+import java.time.Instant;
+import java.util.Timer;
+import java.util.TimerTask;
+import net.sf.cglib.proxy.Enhancer;
+import net.sf.cglib.proxy.MethodInterceptor;
+import net.sf.cglib.proxy.MethodProxy;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.exceptions.GravitinoRuntimeException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A FileSystem wrapper that runs all operations under a specific UGI 
(UserGroupInformation).
+ * Supports both simple and Kerberos authentication, with automatic ticket 
renewal.
+ */
+public class HDFSFileSystemProxy implements MethodInterceptor {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(HDFSFileSystemProxy.class);
+
+  private static final long DEFAULT_RENEW_INTERVAL_MS = 10 * 60 * 1000L;
+  private static final String SYSTEM_USER_NAME = 
System.getProperty("user.name");
+  private static final String SYSTEM_ENV_HADOOP_USER_NAME = "HADOOP_USER_NAME";
+
+  private final UserGroupInformation ugi;
+  private final FileSystem fs;
+  private final Configuration configuration;
+  private Timer kerberosRenewTimer;
+
+  /**
+   * Create a HDFSAuthenticationFileSystem with the given path and 
configuration. Supports both
+   * simple and Kerberos authentication, with automatic ticket renewal for 
Kerberos.
+   *
+   * @param path the HDFS path
+   * @param conf the Hadoop configuration
+   */
+  public HDFSFileSystemProxy(Path path, Configuration conf) {
+    try {
+      conf.setBoolean(FS_DISABLE_CACHE, true);
+      conf.setBoolean(IPC_FALLBACK_TO_SIMPLE_AUTH_ALLOWED, true);
+      this.configuration = conf;
+
+      String authType = conf.get(HADOOP_SECURITY_AUTHENTICATION, AUTH_SIMPlE);
+      if (AUTH_KERBEROS.equalsIgnoreCase(authType)) {
+        String krb5Config = conf.get(HADOOP_KRB5_CONF);
+
+        if (krb5Config != null) {
+          System.setProperty(SECURITY_KRB5_ENV, krb5Config);
+        }
+        UserGroupInformation.setConfiguration(conf);
+        String principal = conf.get(HADOOP_SECURITY_PRINCIPAL, null);
+        String keytab = conf.get(HADOOP_SECURITY_KEYTAB, null);
+
+        if (principal == null || keytab == null) {
+          throw new GravitinoRuntimeException(
+              "Kerberos principal and keytab must be provided for kerberos 
authentication");
+        }
+
+        this.ugi = 
UserGroupInformation.loginUserFromKeytabAndReturnUGI(principal, keytab);
+        startKerberosRenewalTask(principal);
+      } else {
+        String userName = System.getenv(SYSTEM_ENV_HADOOP_USER_NAME);
+        if (StringUtils.isEmpty(userName)) {
+          userName = SYSTEM_USER_NAME;
+        }
+        this.ugi = UserGroupInformation.createRemoteUser(userName);
+      }
+
+      this.fs =
+          ugi.doAs(
+              (PrivilegedExceptionAction<FileSystem>)
+                  () -> FileSystem.newInstance(path.toUri(), conf));
+
+    } catch (Exception e) {
+      throw new GravitinoRuntimeException(e, "Failed to create HDFS FileSystem 
with UGI: %s", path);
+    }
+  }
+
+  /**
+   * Get the proxied FileSystem instance.
+   *
+   * @return the proxied FileSystem
+   * @throws IOException if an I/O error occurs
+   */
+  public FileSystem getProxy() throws IOException {
+    Enhancer e = new Enhancer();
+    e.setClassLoader(fs.getClass().getClassLoader());
+    e.setSuperclass(fs.getClass());
+    e.setCallback(this);
+    FileSystem proxyFs = (FileSystem) e.create();
+    fs.setConf(configuration);
+    return proxyFs;
+  }
+
+  /** Schedule periodic Kerberos re-login to refresh TGT before expiry. */
+  private void startKerberosRenewalTask(String principal) {
+    kerberosRenewTimer = new Timer(true);
+    kerberosRenewTimer.scheduleAtFixedRate(
+        new TimerTask() {
+          @Override
+          public void run() {
+            try {
+              if (ugi.hasKerberosCredentials()) {
+                ugi.checkTGTAndReloginFromKeytab();
+              }
+            } catch (Exception e) {
+              LOG.error(
+                  Instant.now()

Review Comment:
   Why do we need to get time explicitly?



##########
catalogs/hadoop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/Constants.java:
##########
@@ -27,4 +27,25 @@ public class Constants {
 
   // Name of the built-in HDFS file system provider
   public static final String BUILTIN_HDFS_FS_PROVIDER = "builtin-hdfs";
+
+  // Name of the configuration property for HDFS config resources
+  public static final String HDFS_CONFIG_RESOURCES = "hdfs.config.resources";
+  // Name of the configuration property to disable HDFS FileSystem cache
+  public static final String FS_DISABLE_CACHE = "fs.hdfs.impl.disable.cache";
+  // Name of the configuration property for HDFS authentication type
+  public static final String HADOOP_SECURITY_AUTHENTICATION = 
"hadoop.security.authentication";
+  // Name of the configuration property for Kerberos principal
+  public static final String HADOOP_SECURITY_PRINCIPAL =
+      "hadoop.security.authentication.kerberos.principal";

Review Comment:
   Those constants can be found in Hadoop jars. Why do we define it here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to