From 38edc87ad2129797877cbf2ada72ee745a7c27eb Mon Sep 17 00:00:00 2001 From: Jo Voordeckers Date: Mon, 16 Nov 2015 17:18:44 -0800 Subject: [PATCH 1/5] Messos scheduler does not respect all args from the Submit request --- .../scheduler/cluster/mesos/MesosClusterScheduler.scala | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala index 2df7b1120b1c3..c8eb0968e0bcb 100644 --- a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala +++ b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala @@ -423,6 +423,10 @@ private[spark] class MesosClusterScheduler( "--driver-cores", desc.cores.toString, "--driver-memory", s"${desc.mem}M") + val replicatedOptionsBlacklist = Set( + "spark.jars" // Avoids duplicate classes in classpath + ) + // Assume empty main class means we're running python if (!desc.command.mainClass.equals("")) { options ++= Seq("--class", desc.command.mainClass) @@ -440,6 +444,9 @@ private[spark] class MesosClusterScheduler( .mkString(",") options ++= Seq("--py-files", formattedFiles) } + desc.schedulerProperties + .filter { case (key, _) => !replicatedOptionsBlacklist.contains(key) } + .foreach { case (key, value) => options ++= Seq("--conf", Seq(key, "=\"", value, "\"").mkString("")) } options } From 17399fb70b5e50cbdbf1004cc399a18be1600215 Mon Sep 17 00:00:00 2001 From: Jo Voordeckers Date: Tue, 22 Dec 2015 19:47:42 -0800 Subject: [PATCH 2/5] Reduce line length, use interpolation with wrapped quotes --- .../spark/scheduler/cluster/mesos/MesosClusterScheduler.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala index c8eb0968e0bcb..c78195418fe6e 100644 --- a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala +++ b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala @@ -446,7 +446,7 @@ private[spark] class MesosClusterScheduler( } desc.schedulerProperties .filter { case (key, _) => !replicatedOptionsBlacklist.contains(key) } - .foreach { case (key, value) => options ++= Seq("--conf", Seq(key, "=\"", value, "\"").mkString("")) } + .foreach { case (key, value) => options ++= Seq("--conf", s"""$key="$value"""") } options } From 6c3f877fe51476fed43398444ef954ff02b9a7ce Mon Sep 17 00:00:00 2001 From: Iulian Dragos Date: Tue, 19 Jan 2016 16:34:15 +0100 Subject: [PATCH 3/5] =?UTF-8?q?Don=E2=80=99t=20pass=20deployMode=20and=20s?= =?UTF-8?q?park.master=20when=20launching=20a=20driver.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - the launched driver should be in client mode, but at submission time this setting is set to ‘cluster’. - the `spark.master` setting is set to the dispatcher address (usually on port 7077). Passing it has no effect, since we also pass `—-master` which takes precedence. But this is confusing so we should filter it out. --- .../spark/scheduler/cluster/mesos/MesosClusterScheduler.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala index c78195418fe6e..24cd45adbdc60 100644 --- a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala +++ b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala @@ -424,7 +424,9 @@ private[spark] class MesosClusterScheduler( "--driver-memory", s"${desc.mem}M") val replicatedOptionsBlacklist = Set( - "spark.jars" // Avoids duplicate classes in classpath + "spark.jars", // Avoids duplicate classes in classpath + "spark.submit.deployMode", // this would be set to `cluster`, but we need client + "spark.master" // this contains the address of the dispatcher, not master ) // Assume empty main class means we're running python From 8dc77f41913aca3e968706a838779056cfa8e38f Mon Sep 17 00:00:00 2001 From: Jo Voordeckers Date: Tue, 26 Jan 2016 11:45:29 -0800 Subject: [PATCH 4/5] Escape args for Unix-like shells (cherry picked from commit 8023da309df2e5440e244e12d449cb8107d15b00) --- .../cluster/mesos/MesosClusterScheduler.scala | 19 ++++++++++- .../mesos/MesosClusterSchedulerSuite.scala | 32 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala index 24cd45adbdc60..6202058b84458 100644 --- a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala +++ b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala @@ -448,10 +448,27 @@ private[spark] class MesosClusterScheduler( } desc.schedulerProperties .filter { case (key, _) => !replicatedOptionsBlacklist.contains(key) } - .foreach { case (key, value) => options ++= Seq("--conf", s"""$key="$value"""") } + .foreach { case (key, value) => options ++= Seq("--conf", s"$key=${shellEscape(value)}") } options } + /** + * Escape args for Unix-like shells, unless already quoted by the user. + * Based on: http://www.gnu.org/software/bash/manual/html_node/Double-Quotes.html + * and http://www.grymoire.com/Unix/Quote.html + * @param value argument + * @return escaped argument + */ + private[scheduler] def shellEscape(value: String): String = { + val WrappedInQuotes = """^(".+"|'.+')$""".r + val ShellSpecialChars = (""".*([ '<>&|\?\*;!#\\(\)"$`]).*""").r + value match { + case WrappedInQuotes(c) => value // The user quoted his args, don't touch it! + case ShellSpecialChars(c) => "\"" + value.replaceAll("""(["`\$])""", """\\$1""") + "\"" + case _: String => value // Don't touch harmless strings + } + } + private class ResourceOffer( val offerId: OfferID, val slaveId: SlaveID, diff --git a/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala b/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala index dbef6868f20e2..2acc8d06bb76a 100644 --- a/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala +++ b/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala @@ -136,4 +136,36 @@ class MesosClusterSchedulerSuite extends SparkFunSuite with LocalSparkContext wi capture.capture() ) } + + test("escapes commandline args for the shell") { + val conf = new SparkConf() + conf.setMaster("mesos://localhost:5050") + conf.setAppName("spark mesos") + val scheduler = new MesosClusterScheduler( + new BlackHoleMesosClusterPersistenceEngineFactory, conf) { + override def start(): Unit = { ready = true } + } + val escape = scheduler.shellEscape _ + def wrapped(str: String): String = "\"" + str + "\"" + + // Wrapped in quotes + assert(escape("'should be left untouched'") === "'should be left untouched'") + assert(escape("\"should be left untouched\"") === "\"should be left untouched\"") + + // Harmless + assert(escape("") === "") + assert(escape("harmless") === "harmless") + assert(escape("har-m.l3ss") === "har-m.l3ss") + + // Special Chars escape + assert(escape("should escape this \" quote") === wrapped("should escape this \\\" quote")) + assert(escape("shouldescape\"quote") === wrapped("shouldescape\\\"quote")) + assert(escape("should escape this $ dollar") === wrapped("should escape this \\$ dollar")) + assert(escape("should escape this ` backtick") === wrapped("should escape this \\` backtick")) + + // Special Chars no escape only wrap + List(" ", "'", "<", ">", "&", "|", "?", "*", ";", "!", "#", "\\", "(", ")").foreach(char => { + assert(escape(s"onlywrap${char}this") === wrapped(s"onlywrap${char}this")) + }) + } } From 82cd03dfc794c960f48d8e2e91a239a885196161 Mon Sep 17 00:00:00 2001 From: Iulian Dragos Date: Wed, 23 Mar 2016 16:49:54 +0100 Subject: [PATCH 5/5] Escape `\` in strings. --- .../scheduler/cluster/mesos/MesosClusterScheduler.scala | 2 +- .../cluster/mesos/MesosClusterSchedulerSuite.scala | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala index 6202058b84458..c41fa58607639 100644 --- a/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala +++ b/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala @@ -464,7 +464,7 @@ private[spark] class MesosClusterScheduler( val ShellSpecialChars = (""".*([ '<>&|\?\*;!#\\(\)"$`]).*""").r value match { case WrappedInQuotes(c) => value // The user quoted his args, don't touch it! - case ShellSpecialChars(c) => "\"" + value.replaceAll("""(["`\$])""", """\\$1""") + "\"" + case ShellSpecialChars(c) => "\"" + value.replaceAll("""(["`\$\\])""", """\\$1""") + "\"" case _: String => value // Don't touch harmless strings } } diff --git a/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala b/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala index 2acc8d06bb76a..a32423dc4fdeb 100644 --- a/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala +++ b/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala @@ -162,9 +162,13 @@ class MesosClusterSchedulerSuite extends SparkFunSuite with LocalSparkContext wi assert(escape("shouldescape\"quote") === wrapped("shouldescape\\\"quote")) assert(escape("should escape this $ dollar") === wrapped("should escape this \\$ dollar")) assert(escape("should escape this ` backtick") === wrapped("should escape this \\` backtick")) + assert(escape("""should escape this \ backslash""") + === wrapped("""should escape this \\ backslash""")) + assert(escape("""\"?""") === wrapped("""\\\"?""")) + // Special Chars no escape only wrap - List(" ", "'", "<", ">", "&", "|", "?", "*", ";", "!", "#", "\\", "(", ")").foreach(char => { + List(" ", "'", "<", ">", "&", "|", "?", "*", ";", "!", "#", "(", ")").foreach(char => { assert(escape(s"onlywrap${char}this") === wrapped(s"onlywrap${char}this")) }) }