[SPARK-2938] Support SASL authentication in NettyBlockTransferService #3087

aarondav · 2014-11-04T02:32:55Z

Also lays the groundwork for supporting it inside the external shuffle service.

aarondav · 2014-11-04T02:34:55Z

@tgravescs @andrewor14 @rxin PTAL

SparkQA · 2014-11-04T02:37:43Z

Test build #22853 has started for PR 3087 at commit 8191bcb.

This patch merges cleanly.

andrewor14 · 2014-11-04T02:45:53Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

@@ -150,7 +149,7 @@ private[spark] class BlockManager(
  private val peerFetchLock = new Object
  private var lastPeerFetchTime = 0L

-  initialize()
+//  initialize()


Are you planning to remove this?

There's another one at L110

SparkQA · 2014-11-04T03:39:05Z

Test build #22853 has finished for PR 3087 at commit 8191bcb.

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

AmplabJenkins · 2014-11-04T03:39:08Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22853/
Test FAILed.

SparkQA · 2014-11-04T05:44:55Z

Test build #22862 has started for PR 3087 at commit 7b42adb.

This patch merges cleanly.

SparkQA · 2014-11-04T06:09:53Z

Test build #22864 has started for PR 3087 at commit f6177d7.

This patch merges cleanly.

aarondav · 2014-11-04T06:29:49Z

Alright, added unit tests and cleaned up docs and such, this is ready to go from my end.

AmplabJenkins · 2014-11-04T06:47:23Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22865/
Test FAILed.

SparkQA · 2014-11-04T07:11:04Z

Test build #22862 has finished for PR 3087 at commit 7b42adb.

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

AmplabJenkins · 2014-11-04T07:11:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22862/
Test PASSed.

SparkQA · 2014-11-04T07:34:07Z

Test build #22864 has finished for PR 3087 at commit f6177d7.

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

AmplabJenkins · 2014-11-04T07:34:11Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22864/
Test PASSed.

aarondav · 2014-11-04T07:49:09Z

network/shuffle/src/main/java/org/apache/spark/network/sasl/SparkSaslServer.java

+
+  /** Encode a password as a base64-encoded char[] array. */
+  public static char[] encodePassword(String password) {
+    if (password != null) {


This is a change from the prior version -- before, only the client did this transformation, now both the server and client do.

I don't follow your comment? It used to encode it for both client and server before as well.

It seems bad if you are trying to do a sasl negotiation with a null password.

I'm referring to this: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/SparkSaslClient.scala#L118

I'm not sure what should be done about the case where both sides have a null password. Could throw exceptions on both sides.

SparkQA · 2014-11-04T07:50:27Z

Test build #22869 has started for PR 3087 at commit 785bbde.

This patch merges cleanly.

rxin · 2014-11-04T08:00:32Z

core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala

@@ -17,13 +17,17 @@

 package org.apache.spark.network.netty

+import org.apache.spark.network.util.TransportConf


import out of order here

rxin · 2014-11-04T08:50:36Z

This looks good pretty to me. @tgravescs it would be great if you can take a look since you wrote the original sasl code. Thanks.

SparkQA · 2014-11-04T09:15:41Z

Test build #22869 has finished for PR 3087 at commit 785bbde.

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

AmplabJenkins · 2014-11-04T09:15:45Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22869/
Test PASSed.

tgravescs · 2014-11-04T14:12:28Z

network/shuffle/src/main/java/org/apache/spark/network/sasl/SparkSaslClient.java

+ * initial state to the "authenticated" state. This client initializes the protocol via a
+ * firstToken, which is then followed by a set of challenges and responses.
+ */
+public class SparkSaslClient {


Why was this (and other classes) changed to java?

For inclusion in the YARN node manager with minimal dependencies (no dependencies on Spark or Scala).

tgravescs · 2014-11-04T15:52:04Z

Nothing major was jumping out at me. I haven't had time to look at the netty implementation in detail though either.

Did you do any manual testing? I would suggest creating some external process and just trying to hack the connection (pass bad password, auth off, etc) just as a full integration test.

tgravescs · 2014-11-04T16:14:50Z

network/common/src/main/java/org/apache/spark/network/util/TransportConf.java

@@ -55,4 +55,7 @@ public int connectionTimeoutMs() {

  /** Send buffer size (SO_SNDBUF). */
  public int sendBuf() { return conf.getInt("spark.shuffle.io.sendBuffer", -1); }
+
+  /** Timeout for a single round trip of SASL token exchange, in milliseconds. */
+  public int saslRTTimeout() { return conf.getInt("spark.shuffle.sasl.timeout", 30000); }


we should document this config

Documentation for all network-related things is forthcoming soon, we don't have a place for it within this patch.

aarondav · 2014-11-04T17:06:48Z

Addressed all non-documentation and testing related comments, I will work on those soon. For testing, I just need to add a NettyBlockTransfer-level integration test which matches the ConnectionManager's equivalent one.

SparkQA · 2014-11-04T17:07:34Z

Test build #22881 has started for PR 3087 at commit a6b95f1.

This patch merges cleanly.

aarondav · 2014-11-04T18:23:20Z

@tgravescs @rxin Thanks for the review, I believe I've addressed all comments and I've added a full end-to-end test at the NettyBlockTransfer level.

SparkQA · 2014-11-04T18:25:00Z

Test build #22885 has started for PR 3087 at commit 3481718.

This patch merges cleanly.

SparkQA · 2014-11-04T18:33:06Z

Test build #22881 has finished for PR 3087 at commit a6b95f1.

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

AmplabJenkins · 2014-11-04T18:33:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22881/
Test PASSed.

rxin · 2014-11-04T18:52:01Z

Merging this in master.

@tgravescs would be great if you continue looking at the new code. If you find new problems or have suggestions on better implementation/design, we will create new PRs to address them.

rxin · 2014-11-04T19:03:56Z

ASF git is down at the moment. Will merge when it comes back up.

andrewor14 · 2014-11-04T19:23:27Z

network/shuffle/src/main/java/org/apache/spark/network/sasl/SaslMessage.java

+ * with the given appId. This appId allows a single SaslRpcHandler to multiplex different
+ * applications which may be using different sets of credentials.
+ */
+class SaslMessage implements Encodable {


Hey question, should this be fully public? Is it because there's no private[spark] equivalent in Java and we need to access it outside of its package?

I wonder if we should mark this @DeveloperAPI or something to prevent people from using it themselves.

This is actually package-private and is not accessible outside the package. In general, your point still stands, though, and there are other places where we are public because it must be accessed from a diff. package. We should have such an annotation.

Oh yeah, sorry, this is package-private. I'm referring to SparkSaslServer and SparkSaslClient

Gotcha, yeah, we need to figure out a scheme for @DeveloperAPI in this module....

andrewor14 · 2014-11-04T19:46:55Z

LGTM. If there are any outstanding issues we can fix them in subsequent patches.

SparkQA · 2014-11-04T19:55:41Z

Test build #22885 has finished for PR 3087 at commit 3481718.

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

AmplabJenkins · 2014-11-04T19:55:44Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22885/
Test PASSed.

rxin · 2014-11-04T22:34:39Z

We shouldn't add developer api tag because this whole package should be private and not exposed to users. We should exclude this from all public docs.

Also lays the groundwork for supporting it inside the external shuffle service. Author: Aaron Davidson <[email protected]> Closes #3087 from aarondav/sasl and squashes the following commits: 3481718 [Aaron Davidson] Delete rogue println 44f8410 [Aaron Davidson] Delete documentation - muahaha! eb9f065 [Aaron Davidson] Improve documentation and add end-to-end test at Spark-level a6b95f1 [Aaron Davidson] Address comments 785bbde [Aaron Davidson] Cleanup 79973cb [Aaron Davidson] Remove unused file 151b3c5 [Aaron Davidson] Add docs, timeout config, better failure handling f6177d7 [Aaron Davidson] Cleanup SASL state upon connection termination 7b42adb [Aaron Davidson] Add unit tests 8191bcb [Aaron Davidson] [SPARK-2938] Support SASL authentication in NettyBlockTransferService

[SPARK-2938] Support SASL authentication in NettyBlockTransferService

8191bcb

Also lays the groundwork for supporting it inside the external shuffle service.

andrewor14 reviewed Nov 4, 2014
View reviewed changes

Add unit tests

7b42adb

Cleanup SASL state upon connection termination

f6177d7

Add docs, timeout config, better failure handling

151b3c5

Remove unused file

79973cb

Cleanup

785bbde

aarondav reviewed Nov 4, 2014
View reviewed changes

rxin reviewed Nov 4, 2014
View reviewed changes

tgravescs reviewed Nov 4, 2014
View reviewed changes

Address comments

a6b95f1

aarondav force-pushed the sasl branch from 83cfbd9 to a6b95f1 Compare November 4, 2014 17:05

aarondav added 3 commits November 4, 2014 10:18

Improve documentation and add end-to-end test at Spark-level

eb9f065

Delete documentation - muahaha!

44f8410

Delete rogue println

3481718

andrewor14 reviewed Nov 4, 2014
View reviewed changes

asfgit closed this in 5e73138 Nov 5, 2014

zsxwing mentioned this pull request Jan 28, 2015

SPARK-1934 [CORE] "this" reference escape to "selectorThread" during construction in ConnectionManager #4225

Closed

		@@ -17,13 +17,17 @@

		package org.apache.spark.network.netty

		import org.apache.spark.network.util.TransportConf

[SPARK-2938] Support SASL authentication in NettyBlockTransferService #3087

[SPARK-2938] Support SASL authentication in NettyBlockTransferService #3087

Uh oh!

Conversation

aarondav commented Nov 4, 2014

Uh oh!

aarondav commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

AmplabJenkins commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

aarondav commented Nov 4, 2014

Uh oh!

AmplabJenkins commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

AmplabJenkins commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

AmplabJenkins commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

AmplabJenkins commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarondav commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

aarondav commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

SparkQA commented Nov 4, 2014

Uh oh!

AmplabJenkins commented Nov 4, 2014

Uh oh!

rxin commented Nov 4, 2014

Uh oh!

rxin commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!