Conversation
|
@pan3793 may I ask you for a review? Thanks a lot! |
|
Yes, I'm not expert in this part - so PTAL @pan3793 ^^ |
|
Hey @uros-b anyone else we could ask for a review? :) |
|
@LuciferYang and @dongjoon-hyun are also experts in K8S |
|
The fix looks correct from the functional perspective, but sorry, I don't use this feature, and given that it involves security parts, I think I'm not the right person to review it. |
uros-b
left a comment
There was a problem hiding this comment.
perhaps we should update the docs a bit too
| val sslRpcPasswords = secMgr.getEnvironmentForSslRpcPasswords.filterNot { | ||
| case (name, _) => kubernetesConf.secretEnvNamesToKeyRefs.contains(name) | ||
| }.toSeq | ||
|
|
There was a problem hiding this comment.
docs/security.md already says that on Kubernetes the auth secret is injected as an env var, so anyone who can list pods can read it. This PR puts keystore / truststore / key passwords on that same channel. That’s the intended standalone design, but it is a user-facing security change on K8s and should be called out next to the existing auth-secret paragraph. Users who don’t want literals can keep using spark.kubernetes.executor.secretKeyRef.SPARK_SSL_RPC*.
There was a problem hiding this comment.
thank you for your help! I'll take a look at the docs.
| // 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) |
There was a problem hiding this 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.
There was a problem hiding this comment.
Addressed in 2285333 - I also adjusted the PR description.
| ++ attributes | ||
| ++ kubernetesConf.environment | ||
| ++ sparkAuthSecret | ||
| ++ sslRpcPasswords |
There was a problem hiding this 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.
There was a problem hiding this comment.
If it's okay with you, I'd keep YARN out of this PR? I am happy to create a follow up JIRA / PR it that works for you. I added a scope note to the PR description and created this ticket: https://issues.apache.org/jira/browse/SPARK-59408
| // 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 { |
There was a problem hiding this 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.
There was a problem hiding this comment.
I added a paragraph to the PR description
| } | ||
| } | ||
|
|
||
| test("SSL RPC password propagation") { |
There was a problem hiding this 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?
There was a problem hiding this comment.
Addressed in 2285333 in test SSL RPC passwords inherited from the global spark.ssl.* namespace propagate
| 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) |
There was a problem hiding this 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.
|
@LuciferYang thank you for your review! New commit was added, addressing your feedback. |
|
@LuciferYang may I ask for another review? Thanks! |
What changes were proposed in this pull request?
BasicExecutorFeatureStepnow populates the executor environment fromSecurityManager.getEnvironmentForSslRpcPasswords, immediately alongside the existing_SPARK_AUTH_SECRETinjection. This is the same call the standalone worker already makes inCommandUtils.Variable names the user already binds through
spark.kubernetes.executor.secretKeyRefare skipped, so an explicit secret reference does not end up with a literal password beside it in the pod spec.Scope note: this fixes Kubernetes only. YARN has the same issue and requires a follow up. This is tracked in https://issues.apache.org/jira/browse/SPARK-59408
Why are the changes needed?
spark.ssl.rpc.enabled=truecannot work on Kubernetes today.SparkConf.isExecutorStartupConfdeliberately withholdsspark.ssl.*keys containingPasswordfrom the executor startup conf, with the comment "Passwords are propagated separately though". That separate channel is the_SPARK_SSL_RPC_*environment variables, read back bySSLOptions.parse, and the Kubernetes backend never wrote them. Executors thus start with no keystore password and die building theirRpcEnv:The application then aborts on
Max number of executor failures (4) reachedwithout submitting a job. Using RPC SSL on Kubernetes currently requires setting the four variables by hand, throughspark.kubernetes.executor.secretKeyRef,spark.executorEnv, or an executor pod template.Does this PR introduce any user-facing change?
Yes. Previously, setting
spark.ssl.rpc.enabled=trueon Kubernetes without also settingspark.kubernetes.executor.secretKeyRef._SPARK_SSL_RPC_*caused every executor to fail duringRpcEnvconstruction and the application to abort. Now the passwords reach the executors from the driver's configuration and the application runs.Users already applying the
secretKeyRefworkaround are unaffected: those bindings still win, and no literal password is added beside them.The
_SPARK_SSL_RPC_*fallbacks inSSLOptions.parseare not scoped to therpcnamespace. Once RPC SSL is enabled, another enabled namespace on the executor can therefore resolve the RPC password.How was this patch tested?
Three new tests in
BasicExecutorFeatureStepSuite:SSL RPC password propagationSSL RPC passwords shouldn't propagate if RPC SSL is disabledSSL RPC passwords shouldn't override an explicit secretKeyRefAlso verified end-to-end on minikube, k8s v 1.35.1
Was this patch authored or co-authored using generative AI tooling?
Claude Code was used when debugging the original issue that lead to this PR - the first version of this PR was drafted in that session as well.