jerryshao commented on code in PR #13724:
URL: https://github.com/apache/gravitino/pull/13724#discussion_r4245218569


##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/K8sClientUtils.java:
##########
@@ -0,0 +1,272 @@
+/*
+ * 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.
+ */
+package org.apache.gravitino.job.k8s;
+
+import com.google.common.base.Preconditions;
+import io.fabric8.kubernetes.client.Config;
+import io.fabric8.kubernetes.client.ConfigBuilder;
+import io.fabric8.kubernetes.client.KubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClientBuilder;
+import io.fabric8.kubernetes.client.OAuthTokenProvider;
+import io.fabric8.kubernetes.client.utils.Serialization;
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.attribute.FileTime;
+import java.util.List;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Creates the Kubernetes client of the job cluster from the job executor 
configurations. */
+public final class K8sClientUtils {
+
+  /**
+   * An {@link OAuthTokenProvider} that reads the token from a file, and reads 
it again whenever the
+   * file changes, so that a rotated token is picked up without restarting the 
server.
+   */
+  static final class FileTokenProvider implements OAuthTokenProvider {
+
+    private final Path tokenFile;
+
+    private FileTime lastModified;
+
+    private String token;
+
+    private boolean readFailureLogged;
+
+    FileTokenProvider(Path tokenFile) {
+      this.tokenFile = tokenFile;
+    }
+
+    @Override
+    public synchronized String getToken() {
+      try {
+        FileTime modified = Files.getLastModifiedTime(tokenFile);
+        if (token == null || !modified.equals(lastModified)) {
+          String content = new String(Files.readAllBytes(tokenFile), 
StandardCharsets.UTF_8).trim();
+          if (StringUtils.isNotBlank(content)) {
+            token = content;
+            lastModified = modified;
+          } else if (token == null) {
+            throw new IllegalStateException("The token file " + tokenFile + " 
is empty");
+          } else {
+            // The file is being rewritten, keep the token until the new one 
is there. Remember
+            // this version of the file, so that it is neither read nor 
reported again.
+            lastModified = modified;
+            LOG.warn("The token file {} is empty, keeping the token read 
before", tokenFile);
+          }
+        }
+      } catch (IOException e) {
+        if (token == null) {
+          throw new UncheckedIOException("Failed to read the token file " + 
tokenFile, e);
+        }
+        // For example, the file is being replaced, keep the token until the 
new one is there. It
+        // is checked again on every request, so only the first failure in a 
row is reported.
+        if (!readFailureLogged) {
+          LOG.warn("Failed to read the token file {}, keeping the token read 
before", tokenFile, e);
+          readFailureLogged = true;
+        }
+        return token;
+      }
+      readFailureLogged = false;
+      return token;
+    }
+  }
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(K8sClientUtils.class);
+
+  private K8sClientUtils() {}
+
+  /**
+   * Creates the Kubernetes client of the job cluster.
+   *
+   * @param configs the job executor configurations
+   * @return the Kubernetes client
+   */
+  public static KubernetesClient createClient(K8sJobExecutorConfigs configs) {
+    return new 
KubernetesClientBuilder().withConfig(createClientConfig(configs)).build();
+  }
+
+  /**
+   * Creates the Kubernetes client configuration of the job cluster. It is, by 
precedence:
+   *
+   * <ul>
+   *   <li>the API server URL, with its CA certificate and token file, if the 
master URL is set;
+   *   <li>the given kubeconfig file and context, if the kubeconfig is set;
+   *   <li>fabric8's default lookup otherwise, that is the {@code KUBECONFIG} 
environment variable
+   *       or {@code ~/.kube/config} with the given context, then the 
in-cluster service account.
+   * </ul>
+   *
+   * <p>In every case the default namespace of the client is the namespace of 
the job executor
+   * configurations, rather than the one of the kubeconfig context or of the 
pod Gravitino runs in.
+   *
+   * @param configs the job executor configurations
+   * @return the Kubernetes client configuration
+   * @throws IllegalArgumentException if a configured file doesn't exist, or 
the kubeconfig has no
+   *     usable context
+   */
+  static Config createClientConfig(K8sJobExecutorConfigs configs) {
+    if (configs.masterUrl() != null) {
+      ConfigBuilder builder = new 
ConfigBuilder(Config.empty()).withMasterUrl(configs.masterUrl());
+      if (configs.caCertFile() != null) {
+        checkFileExists(configs.caCertFile(), 
K8sJobExecutorConfigs.CA_CERT_FILE);
+        builder.withCaCertFile(configs.caCertFile());
+      }
+      if (configs.tokenFile() != null) {
+        checkFileExists(configs.tokenFile(), K8sJobExecutorConfigs.TOKEN_FILE);
+        builder.withOauthTokenProvider(new 
FileTokenProvider(Paths.get(configs.tokenFile())));
+      }
+      return builder.withNamespace(configs.namespace()).build();
+    }
+
+    Config config;
+    if (configs.kubeconfig() != null) {
+      checkFileExists(configs.kubeconfig(), K8sJobExecutorConfigs.KUBECONFIG);
+      String server = getKubeconfigServer(configs);
+      config = Config.fromKubeconfig(configs.context(), new 
File(configs.kubeconfig()));
+      // The client reads the file itself, so make sure it ended up with the 
server found above,
+      // whatever it does with a kubeconfig that is malformed or changed in 
between.
+      Preconditions.checkArgument(
+          
normalizeServer(server).equals(normalizeServer(config.getMasterUrl())),
+          "The kubeconfig %s set by %s selects the server %s, but the client 
would use %s",
+          configs.kubeconfig(),
+          K8sJobExecutorConfigs.KUBECONFIG,
+          server,
+          config.getMasterUrl());
+    } else {
+      config = Config.autoConfigure(configs.context());
+    }
+
+    // The client falls back to the current context if the given one doesn't 
exist, which would
+    // silently run the jobs in another cluster.
+    String context =
+        config.getCurrentContext() == null ? null : 
config.getCurrentContext().getName();
+    Preconditions.checkArgument(
+        configs.context() == null || configs.context().equals(context),
+        "The context %s set by %s doesn't exist in the kubeconfig",
+        configs.context(),
+        K8sJobExecutorConfigs.CONTEXT);
+    config.setNamespace(configs.namespace());

Review Comment:
   Decided to make `namespace` required, in 53a416931. Neither default is safe 
to assume: the namespace of the jobs needs the RBAC, the Spark operator 
watching it and the driver service account, whether it is `default` or the 
namespace of the Gravitino pod. A missing `namespace` now fails at startup with 
"namespace must be set", like `spark.image` and `spark.sparkVersion`. A job can 
still set its own through `spark.kubernetes.namespace`, and the configured 
value is what the client uses in every connection mode.
   
   _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_



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