LuciferYang commented on code in PR #58010:
URL: https://github.com/apache/spark/pull/58010#discussion_r4173516236
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala:
##########
@@ -142,6 +142,21 @@ private[spark] class BasicExecutorFeatureStep(
case _ => Nil
}.getOrElse(Nil)
+ // SparkConf.isExecutorStartupConf withholds the spark.ssl.* passwords
from the
+ // executor conf. The pod allocator resolves them from the driver
SecurityManager
+ // and passes them in through the environment, as the standalone worker
does in
+ // CommandUtils. A name the user already binds, via
+ // spark.kubernetes.executor.secretKeyRef, spark.executorEnv or the pod
template,
+ // is skipped and the user's value wins: buildEnvVars does not
deduplicate and
+ // Kubernetes resolves a repeated name last-wins.
+ val userBoundEnvNames = kubernetesConf.secretEnvNamesToKeyRefs.keySet ++
+ kubernetesConf.environment.keySet ++
+ Option(pod.container).flatMap(c => Option(c.getEnv))
Review Comment:
The skip-list reads only the template container's `env`, not its `envFrom`.
A user who binds `_SPARK_SSL_RPC_*` through `envFrom: secretRef` on the
executor template gets a same-name literal added to `env` on every executor pod
after this change. Kubernetes gives `env` precedence over `envFrom` for a
duplicate key, so the literal also replaces the Secret's value. That falls
under the pod-template workaround the description says is unaffected, and
neither "bindings still win" nor "no literal password is added beside them"
holds for it.
The keys behind an `envFrom` are unknown without reading the Secret, so the
safe option is to skip the injection whenever the container has any `envFrom`,
with a one-time WARN that lists the variable names it would have injected. The
cost is that a user with an unrelated `envFrom` does not get this fix and still
binds the names via `secretKeyRef` as today, but their binding is never
replaced by a literal. Please also add an `envFrom` test and a sentence in the
docs.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala:
##########
@@ -216,7 +216,8 @@ class KubernetesExecutorConf(
val executorId: String,
val driverPod: Option[Pod],
val resourceProfileId: Int = DEFAULT_RESOURCE_PROFILE_ID,
- customAuthSecret: Option[String] = None)
+ customAuthSecret: Option[String] = None,
+ val sslRpcPasswordEnvs: Map[String, String] = Map.empty)
Review Comment:
The primary constructor gained `sslRpcPasswordEnvs`, but the Java-friendly
constructor from SPARK-59500 still forwards `Map.empty`. A Java caller either
gets no SSL passwords, so its executors fail exactly as before this fix, or has
to build `scala.Option` and `scala.collection.immutable.Map` values to call the
primary constructor, which is what that constructor was added to avoid.
If this lands in 4.4.0, neither constructor has shipped yet, so adding a
`java.util.Map[String, String]` parameter to the Java one (null meaning empty)
is cheap now. One more assertion in the existing Java-constructor test in
`KubernetesConfSuite` would cover it.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -536,7 +536,8 @@ class ExecutorPodsAllocator(
applicationId,
driverPod,
resourceProfileId,
- Option(secMgr.getSecretKey()))
+ Option(secMgr.getSecretKey()),
+ secMgr.getEnvironmentForSslRpcPasswords)
Review Comment:
The injected values are whatever the driver's `rpcSSLOptions` resolved,
which may come from the driver's Hadoop credential provider. If executors read
the same provider through a core-site.xml under `HADOOP_CONF_DIR`, they already
get the passwords at startup today, with nothing in the pod spec. After this
change the driver still writes those values as literals into every executor
pod; the executor never uses them, but they are exposed. The skip-list cannot
see this case, and unlike `spark.authenticate.secret.executor.file` for the
auth secret there is no switch to turn it off.
What the executor is missing at startup is only the SparkConf passwords that
`isExecutorStartupConf` withholds; values from a credential provider or the
driver's environment already have to be provided on the executor side today.
Could the injection apply only when the value that finally takes effect comes
from SparkConf (the rpc or global `spark.ssl` namespace), and leave the rest
for the user to bind as today?
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala:
##########
@@ -421,6 +421,140 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite
with BeforeAndAfter {
checkEnv(executor, conf, Map(SecurityManager.ENV_AUTH_SECRET ->
"my-auth-secret"))
}
+ test("SSL RPC password propagation") {
+ val conf = baseConf.clone()
+ .set("spark.ssl.rpc.enabled", "true")
+ .set("spark.ssl.rpc.keyStorePassword", "keyStorePass")
+ .set("spark.ssl.rpc.keyPassword", "keyPass")
+ .set("spark.ssl.rpc.privateKeyPassword", "privateKeyPass")
+ .set("spark.ssl.rpc.trustStorePassword", "trustStorePass")
+
+ val secMgr = new SecurityManager(conf)
+ val step = new BasicExecutorFeatureStep(
+ KubernetesTestConf.createExecutorConf(
+ sparkConf = conf,
+ sslRpcPasswordEnvs = secMgr.getEnvironmentForSslRpcPasswords),
+ defaultProfile)
+
+ val executor = step.configurePod(SparkPod.initialPod())
+ checkEnv(executor, conf, Map(
+ SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD -> "keyStorePass",
+ SSLOptions.ENV_RPC_SSL_KEY_PASSWORD -> "keyPass",
+ SSLOptions.ENV_RPC_SSL_PRIVATE_KEY_PASSWORD -> "privateKeyPass",
+ SSLOptions.ENV_RPC_SSL_TRUST_STORE_PASSWORD -> "trustStorePass"))
+ }
+
+ test("SSL RPC passwords shouldn't propagate if RPC SSL is disabled") {
Review Comment:
This test asserts that no `_SPARK_SSL_RPC_*` reaches the pod when RPC SSL is
off, but in that case `getEnvironmentForSslRpcPasswords` already returns an
empty map, so it still passes with the Kubernetes change fully reverted. What
it actually covers is the SSL option parsing in core.
It could move to `SecurityManagerSuite` and assert the empty map directly;
if it stays here, a name that says what it covers would help.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala:
##########
@@ -142,6 +142,21 @@ private[spark] class BasicExecutorFeatureStep(
case _ => Nil
}.getOrElse(Nil)
+ // SparkConf.isExecutorStartupConf withholds the spark.ssl.* passwords
from the
+ // executor conf. The pod allocator resolves them from the driver
SecurityManager
+ // and passes them in through the environment, as the standalone worker
does in
+ // CommandUtils. A name the user already binds, via
+ // spark.kubernetes.executor.secretKeyRef, spark.executorEnv or the pod
template,
+ // is skipped and the user's value wins: buildEnvVars does not
deduplicate and
+ // Kubernetes resolves a repeated name last-wins.
+ val userBoundEnvNames = kubernetesConf.secretEnvNamesToKeyRefs.keySet ++
Review Comment:
The comment says the passwords travel "as the standalone worker does in
CommandUtils", then that a user-bound name is skipped so the user's value wins.
`CommandUtils` does the opposite: it appends the passwords with `++=` after
`spark.executorEnv`, so the worker-side value overrides the user's, and the
passwords come from the worker rather than the driver. The stated reason
(`buildEnvVars` does not dedup, Kubernetes takes the last duplicate) holds for
`spark.executorEnv` and template env, but not for `secretKeyRef`: that binding
is appended later by `EnvSecretsFeatureStep` and would win even without the
skip.
SPARK-59408 will likely copy this when wiring YARN, so it would help to
state that the precedence differs from standalone, and that for a
`secretKeyRef` binding the skip is what keeps a plaintext copy out of the pod
spec.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala:
##########
@@ -356,9 +357,11 @@ private[spark] object KubernetesConf {
appId: String,
driverPod: Option[Pod],
resourceProfileId: Int = DEFAULT_RESOURCE_PROFILE_ID,
- authSecret: Option[String] = None): KubernetesExecutorConf = {
+ authSecret: Option[String] = None,
+ sslRpcPasswordEnvs: Map[String, String] = Map.empty):
KubernetesExecutorConf = {
Review Comment:
The fix only takes effect through the three allocators passing
`secMgr.getEnvironmentForSslRpcPasswords`, but the new `createExecutorConf`
parameter defaults to `Map.empty`, so dropping the argument at any call site
still compiles and every test still passes: the allocator suites stub
`buildFromFeatures` with `any(classOf[KubernetesExecutorConf])`, and the new
tests hand-build the map at the feature-step level.
The cheapest guard is to remove the default on `createExecutorConf`, so the
compiler pins all three call sites. For a test-level check, capture the conf
the way `DeploymentAllocatorSuite` already does with
`invocation.getArgument[KubernetesExecutorConf](0)` and assert on it;
`authSecret` can be covered in the same place.
##########
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)
+above. As with the authentication secret, any user who can list pods in the
namespace can read
+them, so access control rules should be properly set up or alternatively these
passwords should
+be supplied using `spark.kubernetes.executor.secretKeyRef`, for example
+`spark.kubernetes.executor.secretKeyRef._SPARK_SSL_RPC_KEY_STORE_PASSWORD=<secret-name>:<key>`.
Review Comment:
This paragraph tells users to supply the passwords through `secretKeyRef`,
but only names `_SPARK_SSL_RPC_KEY_STORE_PASSWORD`. The other three
(`_SPARK_SSL_RPC_KEY_PASSWORD`, `_SPARK_SSL_RPC_PRIVATE_KEY_PASSWORD`,
`_SPARK_SSL_RPC_TRUST_STORE_PASSWORD`) appear nowhere in the docs, so a user
following the example still has the key and trust store passwords in plaintext.
Please list all four names, and add a line for the driver: in cluster mode,
passwords set in SparkConf are written in plaintext to the driver's ConfigMap,
so the driver should bind the same variables through
`spark.kubernetes.driver.secretKeyRef` rather than SparkConf.
--
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]
