[SPARK-22131][MESOS] Mesos driver secrets #19437

susanxhuynh · 2017-10-05T12:20:51Z

Background

In #18837 , @ArtRand added Mesos secrets support to the dispatcher. This PR is to add the same secrets support to the drivers. This means if the secret configs are set, the driver will launch executors that have access to either env or file-based secrets.

One use case for this is to support TLS in the driver <=> executor communication.

What changes were proposed in this pull request?

Most of the changes are a refactor of the dispatcher secrets support (#18837) - moving it to a common place that can be used by both the dispatcher and drivers. The same goes for the unit tests.

How was this patch tested?

There are four config combinations: [env or file-based] x [value or reference secret]. For each combination:

Added a unit test.
Tested in DC/OS.

…t have access to env or file-based secrets.

SparkQA · 2017-10-05T12:34:42Z

Test build #82475 has finished for PR 19437 at commit 6f062c0.

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

susanxhuynh · 2017-10-05T12:37:02Z

@ArtRand @skonto Please review. Tests passed.

skonto · 2017-10-06T15:29:48Z

resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/Utils.scala

+      .getVariablesList
+      .asScala
+    assert(envVars
+      .filter(!_.getName.startsWith("SPARK_")).length == 2)  // user-defined secret env vars


assert(envVars.count(!_.getName.startsWith("SPARK_")) == 2)

skonto · 2017-10-06T15:30:13Z

resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/Utils.scala

+      .getVariablesList
+      .asScala
+    assert(envVars
+      .filter(!_.getName.startsWith("SPARK_")).length == 2)  // user-defined secret env vars


assert(envVars.count(!_.getName.startsWith("SPARK_")) == 2)

Thanks, I'll fix it.

skonto · 2017-10-06T23:34:54Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,7 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def containerInfo(conf: SparkConf, secretConfig: MesosSecretConfig): ContainerInfo.Builder = {


IMHO a better name would be createContainerInfo or buildContainerInfo

OK, I'll use buildContainerInfo.

skonto · 2017-10-06T23:35:36Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -17,10 +17,14 @@

 package org.apache.spark.scheduler.cluster.mesos

-import org.apache.mesos.Protos.{ContainerInfo, Image, NetworkInfo, Parameter, Volume}
+import org.apache.mesos.Protos._


avoid wild card imports if possible.

Hmm, IntelliJ added this automatically - I'll fix it.

skonto · 2017-10-09T08:52:04Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    if (referenceSecrets.nonEmpty) referenceSecrets else valueSecrets
+  }
+
+  private def illegalSecretInput(dest: Seq[String], s: Seq[Secret]): Boolean = {


s -> secrets

skonto · 2017-10-09T09:00:07Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

@@ -21,6 +21,39 @@ import java.util.concurrent.TimeUnit

 import org.apache.spark.internal.config.ConfigBuilder

+private[spark] class MesosSecretConfig(taskType: String) {
+  private[spark] val SECRET_NAME =


shouldn't be SECRET_NAMES? Same for the rest of the configs below.

I see what you mean. I'll change them.

skonto · 2017-10-09T09:10:13Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+  }
+
+  private def illegalSecretInput(dest: Seq[String], s: Seq[Secret]): Boolean = {
+    if (dest.isEmpty) {  // no destination set (ie not using secrets of this type


I think this is redundant, it is covered by the last false statement anyway.
Does it make sense to have paths but no secrets?

Good point. If they specify paths but no secrets, it should throw an exception.

skonto · 2017-10-09T10:56:12Z

@susanxhuynh I think this a mesos 1.4 feature, shouldn't we document this for users?
https://issues.apache.org/jira/browse/MESOS-7418
Btw this scheduler still depends on mesos 1.3 proto,

ArtRand · 2017-10-09T17:49:44Z

@susanxhuynh @skonto The secret-containing protos will be valid in Mesos 1.3 onwards, thus why the scheduler has that requirement. DC/OS with file-based secrets has Mesos 1.4 thus why we test it there.

susanxhuynh

I'll push another commit soon.

susanxhuynh · 2017-10-09T18:12:27Z

resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/Utils.scala

+      .getVariablesList
+      .asScala
+    assert(envVars
+      .filter(!_.getName.startsWith("SPARK_")).length == 2)  // user-defined secret env vars


Thanks, I'll fix it.

susanxhuynh · 2017-10-09T18:25:07Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

@@ -21,6 +21,39 @@ import java.util.concurrent.TimeUnit

 import org.apache.spark.internal.config.ConfigBuilder

+private[spark] class MesosSecretConfig(taskType: String) {
+  private[spark] val SECRET_NAME =


I see what you mean. I'll change them.

susanxhuynh · 2017-10-09T18:40:25Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -17,10 +17,14 @@

 package org.apache.spark.scheduler.cluster.mesos

-import org.apache.mesos.Protos.{ContainerInfo, Image, NetworkInfo, Parameter, Volume}
+import org.apache.mesos.Protos._


Hmm, IntelliJ added this automatically - I'll fix it.

susanxhuynh · 2017-10-09T18:48:18Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,7 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def containerInfo(conf: SparkConf, secretConfig: MesosSecretConfig): ContainerInfo.Builder = {


OK, I'll use buildContainerInfo.

susanxhuynh · 2017-10-09T18:51:57Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    if (referenceSecrets.nonEmpty) referenceSecrets else valueSecrets
+  }
+
+  private def illegalSecretInput(dest: Seq[String], s: Seq[Secret]): Boolean = {


susanxhuynh · 2017-10-09T20:10:42Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+  }
+
+  private def illegalSecretInput(dest: Seq[String], s: Seq[Secret]): Boolean = {
+    if (dest.isEmpty) {  // no destination set (ie not using secrets of this type


Good point. If they specify paths but no secrets, it should throw an exception.

susanxhuynh · 2017-10-09T20:40:04Z

resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/Utils.scala

+      .getVariablesList
+      .asScala
+    assert(envVars
+      .filter(!_.getName.startsWith("SPARK_")).length == 2)  // user-defined secret env vars


…(2) renamed 'containerInfo' -> 'buildContainerInfo', (3) added additional case to illegalSecretInput() check.

SparkQA · 2017-10-09T21:08:05Z

Test build #82565 has finished for PR 19437 at commit 73b2cbf.

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

skonto · 2017-10-11T09:16:13Z

LGTM. @susanxhuynh have you tested this with TLS, just curious.

susanxhuynh · 2017-10-11T14:27:29Z

@skonto I haven't tested with TLS; I'll work on that in the next couple of days.

susanxhuynh · 2017-10-11T14:28:23Z

@vanzin Would you mind reviewing this PR? A followup to ArtRand's secrets PR.

ArtRand

Just a few nits mostly. Thanks.

ArtRand · 2017-10-14T00:32:43Z

...c/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosFineGrainedSchedulerBackend.scala

@@ -159,7 +160,8 @@ private[spark] class MesosFineGrainedSchedulerBackend(
      .setCommand(command)
      .setData(ByteString.copyFrom(createExecArg()))

-    executorInfo.setContainer(MesosSchedulerBackendUtil.containerInfo(sc.conf))
+    executorInfo.setContainer(


I'd almost prefer that we don't add any features to fine-grained right now. As we have virtually no test coverage on whether or not this will work.

+1 There is a PR for removing it already waiting for spark 3.0. Let's to give users the wrong impression that we maintain it.

Okay, I've reverted this change in fine-grained mode.

ArtRand · 2017-10-14T00:35:53Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,8 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def buildContainerInfo(conf: SparkConf, secretConfig: MesosSecretConfig):


Maybe change secretConfig to mesosConfig and pass the whole thing? That way if we want to add new functionality later this function is more general. Given that most of what we do is proto-generation, I bet we'll have to do this eventually anyways.

I've removed that second parameter and moved the secret stuff out of this method, since it's called by Fine-Grained mode, and we don't want to touch Fine-Grained mode.

ArtRand · 2017-10-14T00:38:51Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -170,9 +175,119 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
      containerInfo.addNetworkInfos(info)
    }

+    getSecretVolume(conf, secretConfig).foreach { volume =>
+      if (volume.getSource.getSecret.getReference.isInitialized) {
+        logInfo(s"Setting reference secret ${volume.getSource.getSecret.getReference.getName}" +


Need a space at the end of this log line (my bad!)

ArtRand · 2017-10-14T00:39:06Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+      secretConfig: MesosSecretConfig): Unit = {
+    getSecretEnvVar(conf, secretConfig).foreach { variable =>
+      if (variable.getSecret.getReference.isInitialized) {
+        logInfo(s"Setting reference secret ${variable.getSecret.getReference.getName}" +


Space at the end of this log line too.

ArtRand · 2017-10-14T00:41:24Z

docs/running-on-mesos.md

+  <td><code>spark.mesos.executor.secret.envkeys</code></td>
+  <td><code>(none)</code></td>
+  <td>
+    A comma-separated list that, if set, the contents of the secret referenced


What do you think about putting an example here like we do for spark.mesos.network.labels - something general for all secrets?

I'll add some examples. I didn't find a good general example; I added a separate example to each config option.

ArtRand · 2017-10-14T01:05:57Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

    containerInfo
  }

+  def addSecretEnvVar(


Is it possible to make this return List[Variable] like it used to as opposed to mutating the Environment.Builder, just more consistent (e.g. getSecretVolume)

I've removed this method. To be more consistent, I've moved this code back into MesosClusterScheduler. There's a little duplication, because MesosCoarseGrainedSchedulerBackend now has a similar code snippet, but it does avoid the mutation.

ArtRand · 2017-10-14T01:06:39Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+  }
+
+  private def getSecrets(conf: SparkConf, secretConfig: MesosSecretConfig):
+  Seq[Secret] = {


Indentation?

ArtRand · 2017-10-14T01:23:25Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -17,10 +17,14 @@

 package org.apache.spark.scheduler.cluster.mesos


Out of curiosity, why do we have this file and MesosSchedulerUtils?

It is for the MesosScheduler's helper functions but there are methods (eg. createResource) used all over the place. We need to cleanup the code at some point.

Ah ok. One of these days let's make a "clean up" JIRA and harmonize all of this code. The naming is also all over the place..

…e in FineGrainedSchedulerBackend, and moved the getSecretEnvVar and getSecretVolumes code blocks to be more consistent with rest of code.

susanxhuynh

@ArtRand @skonto I reverted the change in Fine-Grained mode. There's a change in the method name, but that's it.

@ArtRand I've added the doc examples, and did a little re-arranging of the code to address the consistency issue you raised (getSecretEnvVar and getSecretVolumes code blocks).

susanxhuynh · 2017-10-17T22:01:37Z

docs/running-on-mesos.md

+  <td><code>spark.mesos.executor.secret.envkeys</code></td>
+  <td><code>(none)</code></td>
+  <td>
+    A comma-separated list that, if set, the contents of the secret referenced


I'll add some examples. I didn't find a good general example; I added a separate example to each config option.

susanxhuynh · 2017-10-17T23:22:22Z

...c/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosFineGrainedSchedulerBackend.scala

@@ -159,7 +160,8 @@ private[spark] class MesosFineGrainedSchedulerBackend(
      .setCommand(command)
      .setData(ByteString.copyFrom(createExecArg()))

-    executorInfo.setContainer(MesosSchedulerBackendUtil.containerInfo(sc.conf))
+    executorInfo.setContainer(


Okay, I've reverted this change in fine-grained mode.

susanxhuynh · 2017-10-17T23:27:00Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,8 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def buildContainerInfo(conf: SparkConf, secretConfig: MesosSecretConfig):


I've removed that second parameter and moved the secret stuff out of this method, since it's called by Fine-Grained mode, and we don't want to touch Fine-Grained mode.

susanxhuynh · 2017-10-17T23:49:45Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -170,9 +175,119 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
      containerInfo.addNetworkInfos(info)
    }

+    getSecretVolume(conf, secretConfig).foreach { volume =>
+      if (volume.getSource.getSecret.getReference.isInitialized) {
+        logInfo(s"Setting reference secret ${volume.getSource.getSecret.getReference.getName}" +


susanxhuynh · 2017-10-17T23:59:35Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

    containerInfo
  }

+  def addSecretEnvVar(


I've removed this method. To be more consistent, I've moved this code back into MesosClusterScheduler. There's a little duplication, because MesosCoarseGrainedSchedulerBackend now has a similar code snippet, but it does avoid the mutation.

susanxhuynh · 2017-10-18T00:22:52Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+  }
+
+  private def getSecrets(conf: SparkConf, secretConfig: MesosSecretConfig):
+  Seq[Secret] = {


susanxhuynh · 2017-10-18T00:23:02Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+      secretConfig: MesosSecretConfig): Unit = {
+    getSecretEnvVar(conf, secretConfig).foreach { variable =>
+      if (variable.getSecret.getReference.isInitialized) {
+        logInfo(s"Setting reference secret ${variable.getSecret.getReference.getName}" +


SparkQA · 2017-10-18T00:38:26Z

Test build #82867 has finished for PR 19437 at commit 770d307.

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

SparkQA · 2017-10-18T00:43:58Z

Test build #82868 has finished for PR 19437 at commit b2a3675.

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

ArtRand

LGTM

ArtRand · 2017-10-18T00:54:35Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -17,10 +17,14 @@

 package org.apache.spark.scheduler.cluster.mesos


Ah ok. One of these days let's make a "clean up" JIRA and harmonize all of this code. The naming is also all over the place..

ArtRand · 2017-10-18T00:55:24Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+  }
+
+  def getSecretEnvVar(conf: SparkConf, secretConfig: MesosSecretConfig):
+  List[Variable] = {


indentation?

ArtRand · 2017-10-18T00:57:14Z

docs/running-on-mesos.md

@@ -501,23 +503,74 @@ See the [configuration page](configuration.html) for information on Spark config
    spark.mesos.driver.secret.names or spark.mesos.driver.secret.values will be
    written to the provided file. Paths are relative to the container's work
    directory.  Absolute paths must already exist.  Consult the Mesos Secret
-    protobuf for more information.
+    protobuf for more information. Example:


I'm not sure what the policy should be on this, but IIUC file-based secrets does require a backend module. Should we mention that here?

Mentioned custom module and added link to Mesos documentation.

ArtRand · 2017-10-18T00:59:17Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,8 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def buildContainerInfo(conf: SparkConf):
+  ContainerInfo.Builder = {


indentation?

SparkQA · 2017-10-18T15:26:38Z

Test build #82885 has finished for PR 19437 at commit a801799.

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

susanxhuynh · 2017-10-18T15:49:49Z

@srowen Would you like to help review? Adding Mesos secrets support in driver for executor tasks.

susanxhuynh · 2017-10-20T15:46:31Z

@vanzin Ping, would you mind reviewing this PR?

susanxhuynh · 2017-10-23T17:12:21Z

@srowen Ping, would you like to help review?

vanzin

I can't really comment on the functionality since I don't know Mesos, but the docs are really confusing.

vanzin · 2017-10-24T20:19:19Z

docs/running-on-mesos.md

+  <td><code>spark.mesos.executor.secret.envkeys</code></td>
+  <td><code>(none)</code></td>
+  <td>
+    A comma-separated list that, if set, the contents of the secret referenced


I cannot really understand this description.

Let me try to improve the descriptions of all these secret configs.

vanzin · 2017-10-24T20:20:28Z

docs/running-on-mesos.md

+
+    <pre>ENVKEY1,ENVKEY2</pre>
+  </td>
+  </tr>


Indentation doesn't match other rows.

vanzin · 2017-10-24T20:20:37Z

docs/running-on-mesos.md

+  </td>
+  </tr>
+  <tr>
+<td><code>spark.mesos.executor.secret.filenames</code></td>


Indentation is off.

vanzin · 2017-10-24T20:21:50Z

docs/running-on-mesos.md

+<td><code>spark.mesos.executor.secret.filenames</code></td>
+  <td><code>(none)</code></td>
+  <td>
+    A comma-separated list that, if set, the contents of the secret referenced by


This is another description that is very weirdly worded. What I can understand of this:

"A comma-separated list of file names to which secret names and values will be written."

But I can't really know whether that's the case because it's really confusing.

vanzin · 2017-10-24T20:23:17Z

docs/running-on-mesos.md

  </td>
 </tr>
 <tr>
  <td><code>spark.mesos.driver.secret.values</code></td>
  <td><code>(none)</code></td>
  <td>
    A comma-separated list of secret values. Consult the Mesos Secret
-    protobuf for more information.
+    protobuf for more information. Example:


This "protobuf" thing is mentioned in a bunch of places but there's no link nor explanation of what it is. Are Mesos users just supposed to know what that is?

I have grouped some of these properties together to make the description clearer. Please let me know what you think.

vanzin · 2017-10-24T20:26:55Z

...rs/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala

@@ -394,9 +393,10 @@ private[spark] class MesosClusterScheduler(
    }

    // add secret environment variables
-    getSecretEnvVar(desc).foreach { variable =>
+    MesosSchedulerBackendUtil.getSecretEnvVar(desc.conf, config.driverSecretConfig)
+      .foreach { variable =>
      if (variable.getSecret.getReference.isInitialized) {


This block now needs to be indented further.

vanzin · 2017-10-24T20:27:55Z

...rs/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala

+
+    MesosSchedulerBackendUtil.getSecretVolume(desc.conf, config.driverSecretConfig)
+      .foreach { volume =>
+      if (volume.getSource.getSecret.getReference.isInitialized) {


Indent further.

vanzin · 2017-10-24T20:29:48Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,8 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def buildContainerInfo(conf: SparkConf):
+    ContainerInfo.Builder = {


Move to previous line.

vanzin · 2017-10-24T20:30:45Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    secrets.zip(secretPaths).map {
+      case (s, p) =>


Move to previous line.

vanzin · 2017-10-24T20:31:02Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    secrets.zip(secretEnvKeys).map {
+      case (s, k) =>


Move to previous line.

…tation, added private constructor, fixed formatting.

susanxhuynh

@vanzin Thanks for the feedback! PTAL:

susanxhuynh · 2017-10-24T21:46:24Z

docs/running-on-mesos.md

  </td>
 </tr>
 <tr>
  <td><code>spark.mesos.driver.secret.values</code></td>
  <td><code>(none)</code></td>
  <td>
    A comma-separated list of secret values. Consult the Mesos Secret
-    protobuf for more information.
+    protobuf for more information. Example:


I have grouped some of these properties together to make the description clearer. Please let me know what you think.

susanxhuynh · 2017-10-24T21:47:50Z

docs/running-on-mesos.md

+
+    <pre>ENVKEY1,ENVKEY2</pre>
+  </td>
+  </tr>


susanxhuynh · 2017-10-24T21:48:19Z

docs/running-on-mesos.md

+  <td><code>spark.mesos.executor.secret.envkeys</code></td>
+  <td><code>(none)</code></td>
+  <td>
+    A comma-separated list that, if set, the contents of the secret referenced


Let me try to improve the descriptions of all these secret configs.

susanxhuynh · 2017-10-24T21:48:51Z

docs/running-on-mesos.md

+  </td>
+  </tr>
+  <tr>
+<td><code>spark.mesos.executor.secret.filenames</code></td>


susanxhuynh · 2017-10-24T21:57:02Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

@@ -21,6 +21,39 @@ import java.util.concurrent.TimeUnit

 import org.apache.spark.internal.config.ConfigBuilder

+private[spark] class MesosSecretConfig(taskType: String) {


Done. I've moved this MesosSecretConfig class inside the config object, and made the constructor private to that object.

susanxhuynh · 2017-10-24T22:39:26Z

...rs/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala

@@ -394,9 +393,10 @@ private[spark] class MesosClusterScheduler(
    }

    // add secret environment variables
-    getSecretEnvVar(desc).foreach { variable =>
+    MesosSchedulerBackendUtil.getSecretEnvVar(desc.conf, config.driverSecretConfig)
+      .foreach { variable =>
      if (variable.getSecret.getReference.isInitialized) {


susanxhuynh · 2017-10-24T22:40:21Z

...rs/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala

+
+    MesosSchedulerBackendUtil.getSecretVolume(desc.conf, config.driverSecretConfig)
+      .foreach { volume =>
+      if (volume.getSource.getSecret.getReference.isInitialized) {


susanxhuynh · 2017-10-24T22:41:27Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

@@ -122,7 +126,8 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
    .toList
  }

-  def containerInfo(conf: SparkConf): ContainerInfo.Builder = {
+  def buildContainerInfo(conf: SparkConf):
+    ContainerInfo.Builder = {


susanxhuynh · 2017-10-24T22:42:48Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    secrets.zip(secretPaths).map {
+      case (s, p) =>


susanxhuynh · 2017-10-24T22:43:29Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    secrets.zip(secretEnvKeys).map {
+      case (s, k) =>


SparkQA · 2017-10-25T16:11:54Z

Test build #83049 has finished for PR 19437 at commit 3cd1ae8.

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

vanzin

Mostly just want to confirm the docs look ok. They are much clearer now (aside from the question about the secret store).

vanzin · 2017-10-25T21:06:38Z

docs/running-on-mesos.md

-    A comma-separated list that, if set, the contents of the secret referenced
-    by spark.mesos.driver.secret.names or spark.mesos.driver.secret.values will be
-    set to the provided environment variable in the driver's process.
+    <code>spark.mesos.driver.secret.values</code>,


Did you generate the docs to see how this looks? I wonder how browsers will try to render this extra-wide column.

It looks okay in a browser. This column takes up about 25% of the page width. Here's a screenshot:

vanzin · 2017-10-25T21:13:15Z

docs/running-on-mesos.md

+    A secret is specified by its contents and destination. These properties
+    specify a secret's contents. To specify a secret's destination, see the cell below.
+
+    You can specify a secret's contents either (1) by value or (2) by reference.


Just want to confirm this cell renders as expected, and not in a single paragraph or something odd like that. I'm not really familiar with how jekyll translates this into HTML.

Good catch. I added <p> paragraph tags to separate paragraphs in this table cell. (Jekyll doesn't need them in normal text body, only inside a table cell.)

vanzin · 2017-10-25T21:17:45Z

docs/running-on-mesos.md

+
+    <pre>spark.mesos.driver.secret.values=guessme</pre>
+
+    (2) To specify a secret that has been placed in a secret store


What is this secret store? I don't remember Spark providing this feature, so it's not clear how this secret store is configured or used by Spark.

I added a note about this. The secret store has to be provided by the user. You can configure Mesos to integrate with a secret store via a custom Mesos module.

vanzin · 2017-10-25T21:26:15Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    if (valueSecrets.nonEmpty && referenceSecrets.nonEmpty) {
+      throw new SparkException("Cannot specify VALUE type secrets and REFERENCE types ones")


"Cannot specify both value-type and reference-type secrets."

vanzin · 2017-10-25T21:26:32Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    val referenceSecrets: Seq[Secret] =
+      conf.get(secretConfig.SECRET_NAMES).getOrElse(Nil).map(s => createReferenceSecret(s))


nit (also in other places): .map { s => ... }

SparkQA · 2017-10-26T16:00:21Z

Test build #83085 has finished for PR 19437 at commit 88dfb42.

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

susanxhuynh

@vanzin I've built the docs and made sure they look good:

susanxhuynh · 2017-10-26T14:30:14Z

docs/running-on-mesos.md

-    A comma-separated list that, if set, the contents of the secret referenced
-    by spark.mesos.driver.secret.names or spark.mesos.driver.secret.values will be
-    set to the provided environment variable in the driver's process.
+    <code>spark.mesos.driver.secret.values</code>,


It looks okay in a browser. This column takes up about 25% of the page width. Here's a screenshot:

susanxhuynh · 2017-10-26T14:35:25Z

docs/running-on-mesos.md

+    A secret is specified by its contents and destination. These properties
+    specify a secret's contents. To specify a secret's destination, see the cell below.
+
+    You can specify a secret's contents either (1) by value or (2) by reference.


Good catch. I added <p> paragraph tags to separate paragraphs in this table cell. (Jekyll doesn't need them in normal text body, only inside a table cell.)

susanxhuynh · 2017-10-26T14:40:05Z

docs/running-on-mesos.md

+
+    <pre>spark.mesos.driver.secret.values=guessme</pre>
+
+    (2) To specify a secret that has been placed in a secret store


I added a note about this. The secret store has to be provided by the user. You can configure Mesos to integrate with a secret store via a custom Mesos module.

susanxhuynh · 2017-10-26T14:46:45Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    if (valueSecrets.nonEmpty && referenceSecrets.nonEmpty) {
+      throw new SparkException("Cannot specify VALUE type secrets and REFERENCE types ones")


susanxhuynh · 2017-10-26T14:46:53Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+    }
+
+    val referenceSecrets: Seq[Secret] =
+      conf.get(secretConfig.SECRET_NAMES).getOrElse(Nil).map(s => createReferenceSecret(s))


vanzin · 2017-10-26T23:13:30Z

Merging to master.

In apache#18837 , ArtRand added Mesos secrets support to the dispatcher. **This PR is to add the same secrets support to the drivers.** This means if the secret configs are set, the driver will launch executors that have access to either env or file-based secrets. One use case for this is to support TLS in the driver <=> executor communication. Most of the changes are a refactor of the dispatcher secrets support (apache#18837) - moving it to a common place that can be used by both the dispatcher and drivers. The same goes for the unit tests. There are four config combinations: [env or file-based] x [value or reference secret]. For each combination: - Added a unit test. - Tested in DC/OS. Author: Susan X. Huynh <xhuynh@mesosphere.com> Closes apache#19437 from susanxhuynh/sh-mesos-driver-secret.

susanxhuynh added 2 commits October 4, 2017 18:02

[SPARK-22131] Mesos driver secrets. The driver launches executors tha…

b289bcc

…t have access to env or file-based secrets.

[SPARK-22131] Updated docs

6f062c0

skonto reviewed Oct 6, 2017

View reviewed changes

skonto reviewed Oct 9, 2017

View reviewed changes

susanxhuynh commented Oct 9, 2017

View reviewed changes

Addressed review comments: (1) pluralized SECRET_xyz config entries, …

73b2cbf

…(2) renamed 'containerInfo' -> 'buildContainerInfo', (3) added additional case to illegalSecretInput() check.

ArtRand suggested changes Oct 14, 2017

View reviewed changes

ArtRand reviewed Oct 14, 2017

View reviewed changes

susanxhuynh mentioned this pull request Oct 16, 2017

[SPARK-542] Add and document Driver<->Executor TLS support. mesosphere/spark-build#188

Merged

susanxhuynh added 2 commits October 17, 2017 17:16

Addressed review comments: added examples to docs, reverted the chang…

770d307

…e in FineGrainedSchedulerBackend, and moved the getSecretEnvVar and getSecretVolumes code blocks to be more consistent with rest of code.

Fixed formatting

b2a3675

susanxhuynh commented Oct 18, 2017

View reviewed changes

ArtRand reviewed Oct 18, 2017

View reviewed changes

Mentioned need for custom module in file-based secrets docs; formatting.

a801799

vanzin reviewed Oct 24, 2017

View reviewed changes

Addressed review comments: grouped properties together in the documen…

3cd1ae8

…tation, added private constructor, fixed formatting.

susanxhuynh commented Oct 25, 2017

View reviewed changes

vanzin reviewed Oct 25, 2017

View reviewed changes

Fixed formatting in docs table cell, style.

88dfb42

susanxhuynh commented Oct 26, 2017

View reviewed changes

asfgit closed this in 5415963 Oct 26, 2017

ArtRand mentioned this pull request Nov 14, 2017

[Spark-21842][Mesos] Support Kerberos ticket renewal and creation in Mesos #19272

Closed

susanxhuynh deleted the sh-mesos-driver-secret branch November 15, 2017 17:43

		@@ -17,10 +17,14 @@

		package org.apache.spark.scheduler.cluster.mesos

		@@ -21,6 +21,39 @@ import java.util.concurrent.TimeUnit

		import org.apache.spark.internal.config.ConfigBuilder

		private[spark] class MesosSecretConfig(taskType: String) {


		<pre>spark.mesos.driver.secret.values=guessme</pre>

		(2) To specify a secret that has been placed in a secret store

[SPARK-22131][MESOS] Mesos driver secrets #19437

[SPARK-22131][MESOS] Mesos driver secrets #19437

Conversation

susanxhuynh commented Oct 5, 2017

Background

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Oct 5, 2017

susanxhuynh commented Oct 5, 2017

skonto Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Oct 9, 2017 • edited

ArtRand commented Oct 9, 2017

susanxhuynh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 9, 2017

skonto commented Oct 11, 2017 • edited

susanxhuynh commented Oct 11, 2017

susanxhuynh commented Oct 11, 2017

ArtRand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susanxhuynh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 18, 2017

SparkQA commented Oct 18, 2017

ArtRand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 6, 2017 •

edited

skonto Oct 6, 2017 •

edited

skonto Oct 9, 2017 •

edited

skonto Oct 9, 2017 •

edited

skonto Oct 9, 2017 •

edited

skonto commented Oct 9, 2017 •

edited

skonto commented Oct 11, 2017 •

edited

skonto Oct 17, 2017 •

edited