-
Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-58776][K8S] Propagate spark.ssl.rpc.* passwords to executors #58010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,20 @@ 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. 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)) | ||
| .map(_.asScala.map(_.getName).toSet).getOrElse(Set.empty) | ||
| val sslRpcPasswords = secMgr.getEnvironmentForSslRpcPasswords.filterNot { | ||
| case (name, _) => userBoundEnvNames.contains(name) | ||
| }.toSeq | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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*.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you for your help! I'll take a look at the docs.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| val userOpts = kubernetesConf.get(EXECUTOR_JAVA_OPTIONS).toSeq.flatMap { opts => | ||
| val subsOpts = Utils.substituteAppNExecIds(opts, kubernetesConf.appId, | ||
| kubernetesConf.executorId) | ||
|
|
@@ -187,6 +201,7 @@ private[spark] class BasicExecutorFeatureStep( | |
| ++ attributes | ||
| ++ kubernetesConf.environment | ||
| ++ sparkAuthSecret | ||
| ++ sslRpcPasswords | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fix only wires Kubernetes. On YARN,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| ++ Seq(ENV_CLASSPATH -> kubernetesConf.get(EXECUTOR_CLASS_PATH).orNull) | ||
| ++ allOpts) ++ | ||
| KubernetesUtils.buildEnvVarsWithFieldRef( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import com.google.common.net.InternetDomainName | |
| import io.fabric8.kubernetes.api.model._ | ||
| import org.scalatest.BeforeAndAfter | ||
|
|
||
| import org.apache.spark.{SecurityManager, SparkConf, SparkException, SparkFunSuite, SparkIllegalArgumentException} | ||
| import org.apache.spark.{SecurityManager, SparkConf, SparkException, SparkFunSuite, SparkIllegalArgumentException, SSLOptions} | ||
| import org.apache.spark.deploy.k8s.{KubernetesExecutorConf, KubernetesTestConf, SecretVolumeUtils, SparkPod} | ||
| import org.apache.spark.deploy.k8s.Config._ | ||
| import org.apache.spark.deploy.k8s.Constants._ | ||
|
|
@@ -417,6 +417,118 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { | |
| } | ||
| } | ||
|
|
||
| test("SSL RPC password propagation") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The three new tests all set
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 2285333 in test |
||
| 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 step = new BasicExecutorFeatureStep(KubernetesTestConf.createExecutorConf(sparkConf = conf), | ||
| new SecurityManager(conf), 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") { | ||
| val conf = baseConf.clone() | ||
| .set("spark.ssl.rpc.enabled", "false") | ||
| .set("spark.ssl.rpc.keyStorePassword", "keyStorePass") | ||
| .set("spark.ssl.rpc.trustStorePassword", "trustStorePass") | ||
|
|
||
| val step = new BasicExecutorFeatureStep(KubernetesTestConf.createExecutorConf(sparkConf = conf), | ||
| new SecurityManager(conf), defaultProfile) | ||
|
|
||
| val executor = step.configurePod(SparkPod.initialPod()) | ||
| SSLOptions.SPARK_RPC_SSL_PASSWORD_ENVS.foreach { env => | ||
| assert(!KubernetesFeaturesTestUtils.containerHasEnvVar(executor.container, env)) | ||
| } | ||
| } | ||
|
|
||
| test("SSL RPC passwords shouldn't override an explicit secretKeyRef") { | ||
| val conf = baseConf.clone() | ||
| .set("spark.ssl.rpc.enabled", "true") | ||
| .set("spark.ssl.rpc.keyStorePassword", "keyStorePass") | ||
| .set("spark.ssl.rpc.trustStorePassword", "trustStorePass") | ||
|
|
||
| val step = new BasicExecutorFeatureStep( | ||
| KubernetesTestConf.createExecutorConf( | ||
| sparkConf = conf, | ||
| secretEnvNamesToKeyRefs = Map( | ||
| SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD -> "rpc-secret:keystore-password")), | ||
| new SecurityManager(conf), defaultProfile) | ||
|
|
||
| val executor = step.configurePod(SparkPod.initialPod()) | ||
| checkEnv(executor, conf, Map( | ||
| SSLOptions.ENV_RPC_SSL_TRUST_STORE_PASSWORD -> "trustStorePass")) | ||
| } | ||
|
|
||
| test("SSL RPC passwords shouldn't override an explicit spark.executorEnv entry") { | ||
| val conf = baseConf.clone() | ||
| .set("spark.ssl.rpc.enabled", "true") | ||
| .set("spark.ssl.rpc.keyStorePassword", "keyStorePass") | ||
| .set("spark.ssl.rpc.trustStorePassword", "trustStorePass") | ||
| .set(s"spark.executorEnv.${SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD}", "userKeyStorePass") | ||
|
|
||
| val step = new BasicExecutorFeatureStep(KubernetesTestConf.createExecutorConf(sparkConf = conf), | ||
| new SecurityManager(conf), defaultProfile) | ||
|
|
||
| val executor = step.configurePod(SparkPod.initialPod()) | ||
| checkEnv(executor, conf, Map( | ||
| SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD -> "userKeyStorePass", | ||
| SSLOptions.ENV_RPC_SSL_TRUST_STORE_PASSWORD -> "trustStorePass")) | ||
| } | ||
|
|
||
| test("SSL RPC passwords shouldn't override an env var predefined on the pod template") { | ||
| val conf = baseConf.clone() | ||
| .set("spark.ssl.rpc.enabled", "true") | ||
| .set("spark.ssl.rpc.keyStorePassword", "keyStorePass") | ||
| .set("spark.ssl.rpc.trustStorePassword", "trustStorePass") | ||
|
|
||
| val step = new BasicExecutorFeatureStep(KubernetesTestConf.createExecutorConf(sparkConf = conf), | ||
| new SecurityManager(conf), defaultProfile) | ||
|
|
||
| val templatePod = SparkPod.initialPod() | ||
| val templateContainer = new ContainerBuilder(templatePod.container) | ||
| .addNewEnv() | ||
| .withName(SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD) | ||
| .withNewValueFrom() | ||
| .withNewSecretKeyRef() | ||
| .withKey("keystore-password") | ||
| .withName("rpc-secret") | ||
| .endSecretKeyRef() | ||
| .endValueFrom() | ||
| .endEnv() | ||
| .build() | ||
|
|
||
| val executor = step.configurePod(SparkPod(templatePod.pod, templateContainer)) | ||
| checkEnv(executor, conf, Map( | ||
| SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD -> null, | ||
| SSLOptions.ENV_RPC_SSL_TRUST_STORE_PASSWORD -> "trustStorePass")) | ||
| } | ||
|
|
||
| test("SSL RPC passwords inherited from the global spark.ssl.* namespace propagate") { | ||
| val conf = baseConf.clone() | ||
| .set("spark.ssl.enabled", "true") | ||
| .set("spark.ssl.keyStorePassword", "globalKeyStorePass") | ||
| .set("spark.ssl.trustStorePassword", "globalTrustStorePass") | ||
| .set("spark.ssl.rpc.enabled", "true") | ||
|
|
||
| val step = new BasicExecutorFeatureStep(KubernetesTestConf.createExecutorConf(sparkConf = conf), | ||
| new SecurityManager(conf), defaultProfile) | ||
|
|
||
| val executor = step.configurePod(SparkPod.initialPod()) | ||
| checkEnv(executor, conf, Map( | ||
| SSLOptions.ENV_RPC_SSL_KEY_STORE_PASSWORD -> "globalKeyStorePass", | ||
| SSLOptions.ENV_RPC_SSL_TRUST_STORE_PASSWORD -> "globalTrustStorePass")) | ||
| } | ||
|
|
||
| test("SPARK-32661 test executor offheap memory") { | ||
| baseConf.set(MEMORY_OFFHEAP_ENABLED, true) | ||
| baseConf.set("spark.memory.offHeap.size", "42m") | ||
|
|
@@ -776,6 +888,10 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter { | |
| s"$ENV_JAVA_OPT_PREFIX${ind + extraJavaOptsStart}" -> opt | ||
| }.toMap | ||
|
|
||
| val duplicateEnvNames = executorPod.container.getEnv.asScala | ||
| .groupBy(_.getName).filter(_._2.size > 1).keys | ||
| assert(duplicateEnvNames.isEmpty, s"duplicate env names: ${duplicateEnvNames.mkString(", ")}") | ||
|
|
||
| val containerEnvs = executorPod.container.getEnv.asScala.map { | ||
| x => (x.getName, x.getValue) | ||
| }.toMap | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One behavioral edge, no code change requested: the
_SPARK_SSL_RPC_*env fallback lives in the sharedSSLOptions.parse, so everyspark.ssl.*namespace reads it, ahead of the defaults fallback. Globalspark.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a paragraph to the PR description