[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202841144 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -100,7 +100,9 @@ private[spark] class StandaloneSchedulerBackend( val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) + args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries + , javaOpts.filterNot { opt => --- End diff -- we don't put long code inline to function argument usually. Please move this before and store it to a local val used here. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202840687 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -100,7 +100,9 @@ private[spark] class StandaloneSchedulerBackend( val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) + args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries --- End diff -- please put the comma at the end of this line instead of the beginning of the next one. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202841647 --- Diff: core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala --- @@ -282,6 +282,40 @@ class MasterSuite extends SparkFunSuite } } + test("SPARK-24621 https urls when ssl enabled") { --- End diff -- nit: we usually put a `:` after the JIRA number, eg. `SPARK-24621:` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202962897 --- Diff: core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala --- @@ -282,6 +282,40 @@ class MasterSuite extends SparkFunSuite } } + test("SPARK-24621 https urls when ssl enabled") { --- End diff -- done --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202962872 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -100,7 +100,9 @@ private[spark] class StandaloneSchedulerBackend( val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) + args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries + , javaOpts.filterNot { opt => --- End diff -- done --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202962837 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -100,7 +100,9 @@ private[spark] class StandaloneSchedulerBackend( val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) + args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries --- End diff -- done --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202977126 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -99,8 +99,10 @@ private[spark] class StandaloneSchedulerBackend( // Start executors with a few necessary configs for registering with the scheduler val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts -val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) +val javaOptsFiltered = javaOpts.filterNot { opt => +opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword")} --- End diff -- wrong indentation. This should look like: ``` val javaOptsFiltered = javaOpts.filterNot { opt => opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword") } ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202977272 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,9 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) --- End diff -- nit: rename to `sslEnabled` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202990353 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,9 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) --- End diff -- done --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202990383 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -99,8 +99,10 @@ private[spark] class StandaloneSchedulerBackend( // Start executors with a few necessary configs for registering with the scheduler val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts -val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) +val javaOptsFiltered = javaOpts.filterNot { opt => +opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword")} --- End diff -- done --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202991417 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -99,8 +99,11 @@ private[spark] class StandaloneSchedulerBackend( // Start executors with a few necessary configs for registering with the scheduler val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts -val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) +val javaOptsFiltered = javaOpts.filterNot { opt => +opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword") --- End diff -- wrong indentation: missing 2 spaces. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202991729 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -99,8 +99,11 @@ private[spark] class StandaloneSchedulerBackend( // Start executors with a few necessary configs for registering with the scheduler val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts -val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) +val javaOptsFiltered = javaOpts.filterNot { opt => +opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword") +} +val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", args, +sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOptsFiltered) --- End diff -- wrong indentation here too, missing 2 spaces. Moreover, in such cases, we usually put one argument per line, so: ``` val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", args, sc.executorEnvs, ...) ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202991834 --- Diff: core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala --- @@ -282,6 +282,40 @@ class MasterSuite extends SparkFunSuite } } + test("SPARK-24621: https urls when ssl enabled") { +implicit val formats = org.json4s.DefaultFormats +val conf = new SparkConf() +conf.set("spark.ssl.enabled", "true") +val localCluster = new LocalSparkCluster(2, 2, 512, conf) +localCluster.start() +try { + eventually(timeout(5 seconds), interval(100 milliseconds)) { +val json = Source.fromURL(s"https://localhost:${localCluster.masterWebUIPort}/json";) + .getLines().mkString("\n") +assert(json.contains('http://localhost:${localCluster.masterWebUIPort}/json";) + .getLines().mkString("\n") +assert(!json.contains('
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202991814 --- Diff: core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala --- @@ -282,6 +282,40 @@ class MasterSuite extends SparkFunSuite } } + test("SPARK-24621: https urls when ssl enabled") { +implicit val formats = org.json4s.DefaultFormats +val conf = new SparkConf() +conf.set("spark.ssl.enabled", "true") +val localCluster = new LocalSparkCluster(2, 2, 512, conf) +localCluster.start() +try { + eventually(timeout(5 seconds), interval(100 milliseconds)) { +val json = Source.fromURL(s"https://localhost:${localCluster.masterWebUIPort}/json";) + .getLines().mkString("\n") +assert(json.contains('
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/21514 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199317575 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,13 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +val uriScheme = "http://"; +if (SSL_ENABLED) { + uriScheme = "https://"; +} +masterWebUiUrl = uriScheme + masterPublicAddress + ":" + webUi.boundPort +//masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort --- End diff -- please remove this comment --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199317572 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,13 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +val uriScheme = "http://"; +if (SSL_ENABLED) { + uriScheme = "https://"; --- End diff -- have you tested this? This is a val, I doubt this can even compile... --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user pjfanning commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199344304 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,13 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +val uriScheme = "http://"; +if (SSL_ENABLED) { + uriScheme = "https://"; --- End diff -- Could you try this? It's a bit closer to the usual scala style. ``` val uriScheme = if (conf.getBoolean("spark.ssl.enabled", false)) "https://"; else "http://"; ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199345024 --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala --- @@ -100,7 +100,7 @@ private[spark] class StandaloneSchedulerBackend( val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf) val javaOpts = sparkJavaOpts ++ extraJavaOpts val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", - args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts) + args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts.filterNot(_.startsWith("-Dspark.ssl.keyStorePassword")).filterNot(_.startsWith("-Dspark.ssl.keyPassword"))) --- End diff -- does your PR still end up having the literal 'storePassword' in ps output? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199345033 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,13 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +val uriScheme = "http://"; +if (SSL_ENABLED) { + uriScheme = "https://"; +} +masterWebUiUrl = uriScheme + masterPublicAddress + ":" + webUi.boundPort +//masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort --- End diff -- update in latest commit --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user tooptoop4 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199345049 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,13 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +val uriScheme = "http://"; +if (SSL_ENABLED) { + uriScheme = "https://"; --- End diff -- woops, i just updated val to var --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r199352058 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,13 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +val uriScheme = "http://"; +if (SSL_ENABLED) { + uriScheme = "https://"; --- End diff -- yes, but still please add tests for this, in order to enforce the correctness of your solution. PS I agree with @pjfanning's suggestion.. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21514: [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21514#discussion_r202675020 --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala --- @@ -130,7 +130,12 @@ private[deploy] class Master( logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}") webUi = new MasterWebUI(this, webUiPort) webUi.bind() -masterWebUiUrl = "http://"; + masterPublicAddress + ":" + webUi.boundPort +val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false) +var uriScheme = "http://"; --- End diff -- this can be: ``` val uriScheme = if (SSL_ENABLED) { "https://"; } else { "http://"; } ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org