[SPARK-26998][CORE] Remove SSL configuration from executors

[gaborgsomogyi]

What changes were proposed in this pull request?

Different SSL passwords shown up as command line argument on executor side in standalone mode:

• keyStorePassword
• keyPassword
• trustStorePassword

In this PR I've removed SSL configurations from executors.

How was this patch tested?

Existing + additional unit tests.
Additionally tested with standalone mode and checked the command line arguments:

[gaborsomogyi:~/spark] SPARK-26998(+4/-0,3)+ ± jps
94803 CoarseGrainedExecutorBackend
94818 Jps
90149 RemoteMavenServer
91925 Nailgun
94793 SparkSubmit
94680 Worker
94556 Master
398
[gaborsomogyi:~/spark] SPARK-26998(+4/-1,3)+ ± ps -ef | egrep "94556|94680|94793|94803"
502 94556 1 0 2:02PM ttys007 0:07.39 /Library/Java/JavaVirtualMachines/jdk1.8.0_152.jdk/Contents/Home/bin/java -cp /Users/gaborsomogyi/spark/conf/:/Users/gaborsomogyi/spark/assembly/target/scala-2.12/jars/* -Xmx1g org.apache.spark.deploy.master.Master --host gsomogyi-MBP.local --port 7077 --webui-port 8080 --properties-file conf/spark-defaults.conf
502 94680 1 0 2:02PM ttys007 0:07.27 /Library/Java/JavaVirtualMachines/jdk1.8.0_152.jdk/Contents/Home/bin/java -cp /Users/gaborsomogyi/spark/conf/:/Users/gaborsomogyi/spark/assembly/target/scala-2.12/jars/* -Xmx1g org.apache.spark.deploy.worker.Worker --webui-port 8081 --properties-file conf/spark-defaults.conf spark://gsomogyi-MBP.local:7077
502 94793 94782 0 2:02PM ttys007 0:35.52 /Library/Java/JavaVirtualMachines/jdk1.8.0_152.jdk/Contents/Home/bin/java -cp /Users/gaborsomogyi/spark/conf/:/Users/gaborsomogyi/spark/assembly/target/scala-2.12/jars/* -Dscala.usejavacp=true -Xmx1g org.apache.spark.deploy.SparkSubmit --master spark://gsomogyi-MBP.local:7077 --class org.apache.spark.repl.Main --name Spark shell spark-shell
502 94803 94680 0 2:03PM ttys007 0:05.20 /Library/Java/JavaVirtualMachines/jdk1.8.0_152.jdk/Contents/Home/bin/java -cp /Users/gaborsomogyi/spark/conf/:/Users/gaborsomogyi/spark/assembly/target/scala-2.12/jars/* -Xmx1024M -Dspark.ssl.ui.port=0 -Dspark.driver.port=60902 org.apache.spark.executor.CoarseGrainedExecutorBackend --driver-url spark://CoarseGrainedScheduler@172.30.65.186:60902 --executor-id 0 --hostname 172.30.65.186 --cores 8 --app-id app-20190326140311-0000 --worker-url spark://Worker@172.30.65.186:60899
502 94910 57352 0 2:05PM ttys008 0:00.00 egrep 94556|94680|94793|94803

[SparkQA]

Test build #103780 has finished for PR 24170 at commit 0ef3f8b.

• This patch fails RAT tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #103789 has finished for PR 24170 at commit f6174a5.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[HeartSaVioR]

Looks good overall. Minor and nit comments.

[gaborgsomogyi]

cc @vanzin

[SparkQA]

Test build #103817 has finished for PR 24170 at commit f26288d.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[gaborgsomogyi]

Seems like there is a jenkins issue...

[gaborgsomogyi]

retest this please

[SparkQA]

Test build #103829 has finished for PR 24170 at commit f26288d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vanzin]

So there are two things here.

First, the bug. It's about the value showing up in the command line of executors. There was a time when executors might need this SSL configuration, but they don't anymore; so the SSL-related things can just be removed from the list (see SparkConf.isExecutorStartupConf).

Second, even if the above were not true, I think this wouldn't be the right fix. I'd instead have written the stuff returned by SparkConf.isExecutorStartupConf to a file, and have the executor process pick up the config from there.

But given the first, I think just can be a one line fix in SparkConf instead.

[gaborgsomogyi]

There was a time when executors might need this SSL configuration, but they don't anymore

Thanks for helping me out.

The bug is about the executor side but the same problem exists when one provides config like spark-shell --conf spark.ssl.keyPassword=foo.

 502 80304 80292 0 9:22AM ttys008 0:34.34 /Library/Java/JavaVirtualMachines/jdk1.8.0_152.jdk/Contents/Home/bin/java -cp /usr/local/Cellar/apache-spark/2.4.0/libexec/conf/:/usr/local/Cellar/apache-spark/2.4.0/libexec/jars/* -Dscala.usejavacp=true -Xmx1g org.apache.spark.deploy.SparkSubmit --conf spark.ssl.keyPassword=foo --class org.apache.spark.repl.Main --name Spark shell spark-shell

All in all I've created this PR to solve this as a whole. Do you think the driver side shouldn't have to be solved or just not in this PR?

[vanzin]

In that case you can already provide a config file with --properties-file, so I don't see the point in adding yet another config file. And properties also can refer to env variables, for example, so you don't even need to hardcode the password in the file itself.

--conf spark.ssl.keyPassword='${env:KEYSTORE_PASSWD}'

[gaborgsomogyi]

The trick is I've tried originally --conf spark.ssl.keyPassword="$KEY_PASSWD" which is evaluated at shell script level. '${env:KEY_PASSWD}' is not highlighting the password.

[attilapiros]

one minor thing otherwise LGTM

[vanzin]

'${env:KEY_PASSWD}' is not highlighting the password.

I don't know what that means. Here's a line from how we deploy our SHSs:

# cat 666-spark_on_yarn-SPARK_YARN_HISTORY_SERVER/spark-conf/spark-history-server.conf | grep keyStorePass
spark.ssl.historyServer.keyStorePassword=${env:KEYSTORE_PASSWORD}

[vanzin]

(That being said the latest patch looks good.)

[gaborgsomogyi]

The reason why I've changed the approach is because I see your point is good.

'${env:KEY_PASSWD}' is not highlighting the password.

This means not showing the password in plain text but formed the sentence in a bad way.

[SparkQA]

Test build #103978 has finished for PR 24170 at commit a1aa23c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #104161 has finished for PR 24170 at commit fa8cdcd.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[attilapiros]

LGTM

[vanzin]

Merging to master / 2.4 / 2.3.