[SPARK-18547][core] Propagate I/O encryption key when executors register. #15981

vanzin · 2016-11-22T21:24:15Z

This change modifies the method used to propagate encryption keys used during
shuffle. Instead of relying on YARN's UserGroupInformation credential propagation,
this change explicitly distributes the key using the messages exchanged between
driver and executor during registration. When RPC encryption is enabled, this means
key propagation is also secure.

This allows shuffle encryption to work in non-YARN mode, which means that it's
easier to write unit tests for areas of the code that are affected by the feature.

The key is stored in the SecurityManager; because there are many instances of
that class used in the code, the key is only guaranteed to exist in the instance
managed by the SparkEnv. This path was chosen to avoid storing the key in the
SparkConf, which would risk having the key being written to disk as part of the
configuration (as, for example, is done when starting YARN applications).

Tested by new and existing unit tests (which were moved from the YARN module to
core), and by running apps with shuffle encryption enabled.

vanzin · 2016-11-22T21:25:05Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

I had to modify this to avoid going over the 10 argument limit the style checker imposes. Which is also a reason why the key is not stored in the SparkEnv itself.

vanzin · 2016-11-22T21:26:22Z

/cc @zsxwing @JoshRosen

SparkQA · 2016-11-22T21:36:41Z

Test build #69029 has finished for PR 15981 at commit 7ed0d7c.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SparkAppConfig(

…ter. This change modifies the method used to propagate encryption keys used during shuffle. Instead of relying on YARN's UserGroupInformation credential propagation, this change explicitly distributes the key using the messages exchanged between driver and executor during registration. When RPC encryption is enabled, this means key propagation is also secure. This allows shuffle encryption to work in non-YARN mode, which means that it's easier to write unit tests for areas of the code that are affected by the feature. The key is stored in the SecurityManager; because there are many instances of that class used in the code, the key is only guaranteed to exist in the instance managed by the SparkEnv. This path was chosen to avoid storing the key in the SparkConf, which would risk having the key being written to disk as part of the configuration (as, for example, is done when starting YARN applications). Test by new and existing unit tests (which were moved from the YARN module to core), and by running apps with shuffle encryption enabled.

SparkQA · 2016-11-23T00:44:32Z

Test build #69033 has finished for PR 15981 at commit b7486cc.

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

vanzin · 2016-11-28T17:01:37Z

Ping.

zsxwing

Since it sends the key over the wire, I think it's better to throw an exception if securityManager.isSaslEncryptionEnabled() is false.

zsxwing · 2016-11-28T22:37:52Z

core/src/main/scala/org/apache/spark/serializer/SerializerManager.scala

+    conf: SparkConf,
+    encryptionKey: Option[Array[Byte]]) {
+
+  def this(defaultSerializer: Serializer, conf: SparkConf) = this(defaultSerializer, conf, None)


nit: why not use the parameter default value?

Hmm, I thought I was calling this from Java code but must have removed it. I guess I can use a default argument now.

Actually, I'm calling this from UnsafeShuffleWriterSuite.java in the accompanying change, and default values make it hard to call the code from Java.

I see. Thanks for your clarifying.

zsxwing · 2016-11-28T22:46:08Z

mesos/src/main/scala/org/apache/spark/executor/MesosExecutorBackend.scala

    val port = conf.getInt("spark.executor.port", 0)
    val env = SparkEnv.createExecutorEnv(
-      conf, executorId, slaveInfo.getHostname, port, cpusPerTask, isLocal = false)
+      conf, executorId, slaveInfo.getHostname, port, cpusPerTask, None, isLocal = false)


This means mesos is not suppported. Right? Could you throw an exception if IO encryption is enabled for mesos?

vanzin · 2016-11-28T23:31:23Z

Since it sends the key over the wire, I think it's better to throw an exception if securityManager.isSaslEncryptionEnabled() is false.

We don't do that now, and there's a whole lot of things that need to configured correctly for things to be really secure (like HDFS and YARN encryption). It would also make unit tests slightly more complicated (or at least their set up). I think it's fine to just have a warning here.

zsxwing · 2016-11-28T23:40:59Z

I think it's fine to just have a warning here.

Sounds good. Just make sure you mention it when updating the doc.

zsxwing · 2016-11-28T23:58:36Z

I think it's fine to just have a warning here.

I cannot find the warning log in your latest commit. Otherwise, LGTM.

vanzin · 2016-11-29T00:01:09Z

Looks like I removed the warning from SparkEnv at some point, let me add it back.

SparkQA · 2016-11-29T02:59:57Z

Test build #69277 has finished for PR 15981 at commit d97f27b.

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

SparkQA · 2016-11-29T03:57:35Z

Test build #69273 has finished for PR 15981 at commit e76dd15.

This patch fails from timeout after a configured wait of `250m`.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-11-29T05:10:06Z

LGTM. The build for the latest commit passed. Merging to master and 2.1.

…ter. This change modifies the method used to propagate encryption keys used during shuffle. Instead of relying on YARN's UserGroupInformation credential propagation, this change explicitly distributes the key using the messages exchanged between driver and executor during registration. When RPC encryption is enabled, this means key propagation is also secure. This allows shuffle encryption to work in non-YARN mode, which means that it's easier to write unit tests for areas of the code that are affected by the feature. The key is stored in the SecurityManager; because there are many instances of that class used in the code, the key is only guaranteed to exist in the instance managed by the SparkEnv. This path was chosen to avoid storing the key in the SparkConf, which would risk having the key being written to disk as part of the configuration (as, for example, is done when starting YARN applications). Tested by new and existing unit tests (which were moved from the YARN module to core), and by running apps with shuffle encryption enabled. Author: Marcelo Vanzin <[email protected]> Closes #15981 from vanzin/SPARK-18547. (cherry picked from commit 8b325b1) Signed-off-by: Shixiong Zhu <[email protected]>

…ter. This change modifies the method used to propagate encryption keys used during shuffle. Instead of relying on YARN's UserGroupInformation credential propagation, this change explicitly distributes the key using the messages exchanged between driver and executor during registration. When RPC encryption is enabled, this means key propagation is also secure. This allows shuffle encryption to work in non-YARN mode, which means that it's easier to write unit tests for areas of the code that are affected by the feature. The key is stored in the SecurityManager; because there are many instances of that class used in the code, the key is only guaranteed to exist in the instance managed by the SparkEnv. This path was chosen to avoid storing the key in the SparkConf, which would risk having the key being written to disk as part of the configuration (as, for example, is done when starting YARN applications). Tested by new and existing unit tests (which were moved from the YARN module to core), and by running apps with shuffle encryption enabled. Author: Marcelo Vanzin <[email protected]> Closes apache#15981 from vanzin/SPARK-18547.

vanzin commented Nov 22, 2016

View reviewed changes

vanzin mentioned this pull request Nov 22, 2016

[SPARK-18546][core] Fix merging shuffle spills when using encryption. #15982

Closed

Marcelo Vanzin added 2 commits November 22, 2016 13:38

Fix mesos compilation.

b7486cc

vanzin force-pushed the SPARK-18547 branch from 7ed0d7c to b7486cc Compare November 22, 2016 21:42

zsxwing requested changes Nov 28, 2016

View reviewed changes

Throw error from mesos code, update docs.

e76dd15

Add warning about RPC encryption being disabled.

d97f27b

asfgit closed this in 8b325b1 Nov 29, 2016

vanzin deleted the SPARK-18547 branch November 29, 2016 17:28

Uh oh!

[SPARK-18547][core] Propagate I/O encryption key when executors register. #15981

[SPARK-18547][core] Propagate I/O encryption key when executors register. #15981

Uh oh!

Conversation

vanzin commented Nov 22, 2016

Uh oh!

vanzin Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 23, 2016

Uh oh!

vanzin commented Nov 28, 2016

Uh oh!

zsxwing left a comment

Choose a reason for hiding this comment

Uh oh!

zsxwing Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

zsxwing Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

zsxwing Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin commented Nov 28, 2016

Uh oh!

zsxwing commented Nov 28, 2016

Uh oh!

zsxwing commented Nov 28, 2016

Uh oh!

vanzin commented Nov 29, 2016

Uh oh!

SparkQA commented Nov 29, 2016

Uh oh!

SparkQA commented Nov 29, 2016

Uh oh!

zsxwing commented Nov 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants