Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/23174#discussion_r239694862
--- Diff:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala
---
@@ -87,44 +88,61 @@ private[spark] class
BasicExecutorFeatureStep(kubernetesConf: KubernetesExecutor
val executorCpuQuantity = new QuantityBuilder(false)
.withAmount(executorCoresRequest)
.build()
- val executorExtraClasspathEnv = executorExtraClasspath.map { cp =>
- new EnvVarBuilder()
- .withName(ENV_CLASSPATH)
- .withValue(cp)
- .build()
- }
- val executorExtraJavaOptionsEnv = kubernetesConf
- .get(EXECUTOR_JAVA_OPTIONS)
- .map { opts =>
- val subsOpts = Utils.substituteAppNExecIds(opts,
kubernetesConf.appId,
- kubernetesConf.executorId)
- val delimitedOpts = Utils.splitCommandString(subsOpts)
- delimitedOpts.zipWithIndex.map {
- case (opt, index) =>
- new
EnvVarBuilder().withName(s"$ENV_JAVA_OPT_PREFIX$index").withValue(opt).build()
+
+ val executorEnv: Seq[EnvVar] = {
+ (Seq(
+ (ENV_DRIVER_URL, driverUrl),
+ (ENV_EXECUTOR_CORES, executorCores.toString),
+ (ENV_EXECUTOR_MEMORY, executorMemoryString),
+ (ENV_APPLICATION_ID, kubernetesConf.appId),
+ // This is to set the SPARK_CONF_DIR to be /opt/spark/conf
+ (ENV_SPARK_CONF_DIR, SPARK_CONF_DIR_INTERNAL),
+ (ENV_EXECUTOR_ID, kubernetesConf.executorId)
+ ) ++ kubernetesConf.environment).map { case (k, v) =>
+ new EnvVarBuilder()
+ .withName(k)
+ .withValue(v)
+ .build()
}
- }.getOrElse(Seq.empty[EnvVar])
- val executorEnv = (Seq(
- (ENV_DRIVER_URL, driverUrl),
- (ENV_EXECUTOR_CORES, executorCores.toString),
- (ENV_EXECUTOR_MEMORY, executorMemoryString),
- (ENV_APPLICATION_ID, kubernetesConf.appId),
- // This is to set the SPARK_CONF_DIR to be /opt/spark/conf
- (ENV_SPARK_CONF_DIR, SPARK_CONF_DIR_INTERNAL),
- (ENV_EXECUTOR_ID, kubernetesConf.executorId)) ++
- kubernetesConf.environment)
- .map(env => new EnvVarBuilder()
- .withName(env._1)
- .withValue(env._2)
- .build()
- ) ++ Seq(
- new EnvVarBuilder()
- .withName(ENV_EXECUTOR_POD_IP)
- .withValueFrom(new EnvVarSourceBuilder()
- .withNewFieldRef("v1", "status.podIP")
+ } ++ {
+ Seq(new EnvVarBuilder()
+ .withName(ENV_EXECUTOR_POD_IP)
+ .withValueFrom(new EnvVarSourceBuilder()
+ .withNewFieldRef("v1", "status.podIP")
+ .build())
.build())
- .build()
- ) ++ executorExtraJavaOptionsEnv ++ executorExtraClasspathEnv.toSeq
+ } ++ {
+ Option(secMgr.getSecretKey()).map { authSecret =>
+ new EnvVarBuilder()
+ .withName(SecurityManager.ENV_AUTH_SECRET)
+ .withValue(authSecret)
--- End diff --
Ah I thought about this a bit more and realized that this is more insecure
than I originally read it to be.
If the secret is put directly in the environment variable field itself,
then anyone who has permission to get the pod metadata from the Kubernetes API
server can now read the secret generated by this application. In practice
permissioning on pod specs is often far looser than permissioning on Kubernetes
secret objects. In this solution the administrator has to restrict access to
pod specs to only the user.
I think at the very least we want this to be configured via creating a
Kubernetes secret object, then [loading the environment
variable](https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-environment-variables)
to point to the secret object.
In the meantime I'm going to push the PR that allows secrets to be
specified as file paths directly. I will also file a Spark ticket to avoid
putting the environment variable directly in the pod spec object itself.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
