jerryshao commented on code in PR #13982:
URL: https://github.com/apache/gravitino/pull/13982#discussion_r4375444365
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -216,6 +260,112 @@ public static GenericKubernetesResource
buildSparkApplication(
return app;
}
+ /**
+ * Returns the Spark configurations that a job sets itself, which override
the default Spark
+ * configurations of the job executor.
+ *
+ * <p>They are the configurations of the job template, and for a built-in
job also those of its
+ * {@code spark_conf} parameter, which override them. The driver of a
built-in job applies its
+ * {@code spark_conf} only once it starts, too late for the configurations
that spark-submit
+ * reads, such as {@code spark.jars}, so they are set in the
SparkApplication as well. {@code
+ * spark.master} and {@code spark.submit.deployMode} there are ignored, as
the driver also ignores
+ * them, and so are the {@code spark.kubernetes.*} configurations, which
only the job executor
+ * configurations and job templates may set: whoever can run a built-in job
sets its {@code
+ * spark_conf}. A blank value leaves a configuration unset, as it is what an
optional template
+ * parameter becomes, so it doesn't override the default of the job executor.
+ */
+ private static Map<String, String> getJobSparkConf(
+ JobContext context, SparkJobTemplate template) {
+ Map<String, String> jobSparkConf = new TreeMap<>();
+ template
+ .configs()
+ .forEach(
+ (key, value) -> {
+ if (StringUtils.isNotBlank(value)) {
+ checkExecutorEnvironmentName(template, key);
+ jobSparkConf.put(key, value);
+ }
+ });
+ if (!template.name().startsWith(JobTemplateProvider.BUILTIN_NAME_PREFIX)) {
+ return jobSparkConf;
+ }
+
+ getBuiltinSparkConf(template)
Review Comment:
Thanks for spelling this out. We'll keep the current behavior: running a
built-in job could already run arbitrary code before this change, so applying
spark_conf at submit time doesn't lower the security bar.
The driver applies spark_conf before it creates the executor pods, so a
runner could already get code execution in the job's pods. For example,
spark.executor.extraJavaOptions with -XX:OnOutOfMemoryError=<command> runs a
shell command in every executor, and spark.jars ships the runner's jar to the
executors' class path. Those pods carry the job's catalog and storage
credentials as well. So RUN_JOB + USE_JOB_TEMPLATE on a built-in template has
always meant trusting the runner with code execution in the job's Spark pods.
This PR adds the driver to that set; it doesn't create the trust relationship.
What the k8s executor does keep out of the runner's hands is the Kubernetes
side: spark.kubernetes.* (namespace, image, service accounts, Secrets, volumes)
is ignored in spark_conf, so the pods stay under the identity and isolation the
admin configured. The docs PR (#13950) states that spark_conf can add code to a
job, e.g. through spark.jars, and that the driver's service account should be
limited to what Spark needs.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -216,6 +260,112 @@ public static GenericKubernetesResource
buildSparkApplication(
return app;
}
+ /**
+ * Returns the Spark configurations that a job sets itself, which override
the default Spark
+ * configurations of the job executor.
+ *
+ * <p>They are the configurations of the job template, and for a built-in
job also those of its
+ * {@code spark_conf} parameter, which override them. The driver of a
built-in job applies its
+ * {@code spark_conf} only once it starts, too late for the configurations
that spark-submit
+ * reads, such as {@code spark.jars}, so they are set in the
SparkApplication as well. {@code
+ * spark.master} and {@code spark.submit.deployMode} there are ignored, as
the driver also ignores
+ * them, and so are the {@code spark.kubernetes.*} configurations, which
only the job executor
+ * configurations and job templates may set: whoever can run a built-in job
sets its {@code
+ * spark_conf}. A blank value leaves a configuration unset, as it is what an
optional template
+ * parameter becomes, so it doesn't override the default of the job executor.
+ */
+ private static Map<String, String> getJobSparkConf(
+ JobContext context, SparkJobTemplate template) {
+ Map<String, String> jobSparkConf = new TreeMap<>();
+ template
+ .configs()
+ .forEach(
+ (key, value) -> {
+ if (StringUtils.isNotBlank(value)) {
+ checkExecutorEnvironmentName(template, key);
+ jobSparkConf.put(key, value);
+ }
+ });
+ if (!template.name().startsWith(JobTemplateProvider.BUILTIN_NAME_PREFIX)) {
+ return jobSparkConf;
+ }
+
+ getBuiltinSparkConf(template)
+ .forEach(
+ (key, value) -> {
+ if (StringUtils.isBlank(value)) {
+ return;
+ }
+ if (SUBMIT_DECIDED_CONFIGS.contains(key)) {
+ LOG.warn(
+ "Ignored {}={} in spark_conf of job {} of built-in job
template {}, as the"
+ + " Spark operator decides it",
+ key,
+ value,
+ context.jobId(),
+ template.name());
+ } else if (key.startsWith(SPARK_KUBERNETES_PREFIX)) {
+ // The value isn't logged, as it may name a Secret.
+ LOG.warn(
+ "Ignored {} in spark_conf of job {} of built-in job
template {}, as only the"
+ + " k8s job executor configurations and job templates
may set {}*",
+ key,
+ context.jobId(),
+ template.name(),
+ SPARK_KUBERNETES_PREFIX);
+ } else {
+ checkExecutorEnvironmentName(template, key);
Review Comment:
Documented it rather than rejecting it. A spark.executorEnv.<name> in
spark_conf overrides only the executors' value, which is Spark's own meaning of
that key. spark_conf can't set spark.kubernetes.driverEnv.*, so the driver
keeps the template's value. Added this to the Environment Variables section of
the k8s executor docs in #13950 (fd7e39f44).
_🤖 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]
