LuciferYang commented on code in PR #58010:
URL: https://github.com/apache/spark/pull/58010#discussion_r3939091569
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala:
##########
@@ -143,6 +143,15 @@ private[spark] class BasicExecutorFeatureStep(
case _ => Nil
}.getOrElse(Nil)
+ // SparkConf.isExecutorStartupConf withholds the spark.ssl.* passwords
from the
+ // executor conf. Pass them through the environment, as the standalone
worker
+ // does in CommandUtils. Names the user already supplies via
+ // spark.kubernetes.executor.secretKeyRef are skipped, so that an
explicit
+ // secret reference is not shadowed by a literal password in the pod
spec.
+ val sslRpcPasswords = secMgr.getEnvironmentForSslRpcPasswords.filterNot {
+ case (name, _) => kubernetesConf.secretEnvNamesToKeyRefs.contains(name)
Review Comment:
The filter only skips names bound via
`spark.kubernetes.executor.secretKeyRef`. A
`spark.executorEnv._SPARK_SSL_RPC_KEY_STORE_PASSWORD` enters the same env Seq
earlier via `kubernetesConf.environment`, `buildEnvVars` does not dedup, and a
same-name env predefined on a pod template (including valueFrom) is unchecked.
Kubernetes validation does not reject duplicate env names; runtimes resolve
them last-wins, and the literal is appended after the user entry, so it
silently overrides the value the user set. A template secretKeyRef password
effectively becomes a plaintext literal, opposite of the precedence this PR
intends for secretKeyRef.
Please extend the skip-list to `kubernetesConf.environment` keys plus env
names already on the container (with a template, that is the predefined set);
skipping means the user value wins. For the test, group the container env by
name and assert no duplicates — `checkEnv` collapses to a Map and cannot detect
them. The description's "the only way is a Secret" claim needs a correction too.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala:
##########
@@ -187,6 +196,7 @@ private[spark] class BasicExecutorFeatureStep(
++ attributes
++ kubernetesConf.environment
++ sparkAuthSecret
+ ++ sslRpcPasswords
Review Comment:
This fix only wires Kubernetes. On YARN, `ExecutorRunnable.prepareCommand`
filters executor java opts with the same `SparkConf.isExecutorStartupConf`
(passwords withheld there too), and `prepareEnvironment` only forwards env vars
starting with `SPARK` plus `spark.executorEnv`; it never calls
`getEnvironmentForSslRpcPasswords`. Under `--master yarn` the same
configuration fails executors with exactly the symptom this PR describes.
`ExecutorRunnable` already receives `securityMgr` as a constructor param, so
appending the same map at the end of `prepareEnvironment`, mirroring
`CommandUtils` on standalone, is a one-liner. The HashMap overwrite semantics
deduplicate naturally; note the driver-side value then overrides a same-name
`spark.executorEnv` entry — the standalone behavior, opposite of the k8s-side
user-value precedence. If YARN is out of scope, please say so in the
description and open a follow-up JIRA.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala:
##########
@@ -143,6 +143,15 @@ private[spark] class BasicExecutorFeatureStep(
case _ => Nil
}.getOrElse(Nil)
+ // SparkConf.isExecutorStartupConf withholds the spark.ssl.* passwords
from the
+ // executor conf. Pass them through the environment, as the standalone
worker
+ // does in CommandUtils. Names the user already supplies via
+ // spark.kubernetes.executor.secretKeyRef are skipped, so that an
explicit
+ // secret reference is not shadowed by a literal password in the pod
spec.
+ val sslRpcPasswords = secMgr.getEnvironmentForSslRpcPasswords.filterNot {
Review Comment:
One behavioral edge, no code change requested: the `_SPARK_SSL_RPC_*` env
fallback lives in the shared `SSLOptions.parse`, so every `spark.ssl.*`
namespace reads it, ahead of the defaults fallback. Global `spark.ssl.*`
passwords are withheld from executors too, so once rpc is enabled another
enabled namespace resolves the rpc password on executors while the driver uses
its own, and the two sides disagree. Standalone injects the same env vars and
behaves identically, and before this PR executors had no password on that path
at all, so this breaks no working setup. Worth one sentence in the PR
description or the docs.
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala:
##########
@@ -417,6 +417,58 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite
with BeforeAndAfter {
}
}
+ test("SSL RPC password propagation") {
Review Comment:
The three new tests all set `spark.ssl.rpc.*` passwords explicitly. The
shape where a user sets `spark.ssl.enabled=true` with the global
`spark.ssl.keyStorePassword`, sets no rpc-side password, and separately enables
`spark.ssl.rpc.enabled=true` also works: `rpcSSLOptions` inherits the global
password through the defaults fallback on the driver, and the env carries the
inherited value (`spark.ssl.enabled` must be on, or nothing is inherited). That
is one of the common usages this PR claims to fix, and right now no test guards
the inheritance path — could you add one for this shape?
##########
docs/security.md:
##########
@@ -183,6 +183,14 @@ Unlike the other SSL settings for the UI, the RPC SSL is
*not* automatically ena
`spark.ssl.enabled` is set. It must be explicitly enabled, to ensure a safe
migration path for users
upgrading Spark versions.
+On Kubernetes, the passwords used by RPC SSL are propagated to executor pods
using environment
+variables, the same way as the authentication mechanics described under
[Kubernetes](#Kubernetes)
Review Comment:
The `[Kubernetes](#Kubernetes)` link in this new paragraph does not resolve:
the target heading is `### Kubernetes`, whose generated slug is lowercase
`kubernetes`, and fragment matching is case-sensitive. Changing the link to
`(#kubernetes)` fixes it.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
