[SPARK-15844] [core] HistoryServer doesn't come up if spark.authenticate = true #13579

steveloughran · 2016-06-09T16:21:16Z

What changes were proposed in this pull request?

During history server startup, the spark configuration is examined. If security.authentication is
set, log at debug and set the value to false, so that {{SecurityManager}} can be created.

How was this patch tested?

A new test in HistoryServerSuite sets the spark.authenticate property to true, tries to create a security manager via a new package-private method HistoryServer.createSecurityManager(SparkConf). This is the method used in HistoryServer.main. All other instantiations of a security manager in HistoryServerSuite have been switched to the new method, for consistency with the production code.

SparkQA · 2016-06-09T18:28:09Z

Test build #60238 has finished for PR 13579 at commit 26d1ad2.

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

vanzin · 2016-07-20T00:15:08Z

I might be missing something, but why doesn't the following work?

sparkConf.set(SecurityManager.SPARK_AUTH_CONF, "false")

That value is never read from the environment as far as I can tell.

steveloughran · 2016-10-10T15:19:08Z

The main problem here is in a cluster where auth is turned on globally, the HS gets really confused: it's enabled but doesn't have any secrets. This patch sets things up so that even in a cluster-wide security enabled, the history server doesn't fall over

vanzin · 2016-10-10T17:13:42Z

That doesn't answer my question, though. Why can't the history server explicitly set that value to false after all the settings having been read from wherever?

SparkQA · 2016-10-10T17:30:03Z

Test build #66657 has finished for PR 13579 at commit 6701922.

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

steveloughran · 2016-10-10T19:20:34Z

I see: you want the HS to set it? Yeah, that would work. I'll change this patch accordingly

vanzin · 2016-12-07T19:07:15Z

@steveloughran are you going to update this patch?

steveloughran · 2016-12-07T22:04:22Z

yeah, I've just got so many other distractions. Let me do it again while tests run in different windows

…rue; making new (visible for test) method private to the package

SparkQA · 2016-12-07T22:34:59Z

Test build #69823 has finished for PR 13579 at commit 01a0220.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-12-08T01:32:19Z

Test build #69824 has finished for PR 13579 at commit 3b3db69.

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

vanzin

Couple of small things, otherwise looks good.

vanzin · 2016-12-08T01:44:10Z

core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala

+   * always start.
+   * @param config configuration to be used in a SecurityManager constructor
+   */
+  private def patchSecuritySettings(config: SparkConf): Unit = {


Could you just inline this method? That avoids adding a method with a single call, and pretty much a copy of the whole scaladoc of the other method you're adding.

I've inlined it, which highlights something else. if the debug message is viewed as unimportant, the if() clause can be pulled completely, and the security flag always set to false.

vanzin · 2016-12-08T01:45:38Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

+    val conf = new SparkConf()
+        .set("spark.testing", "true")
+        .set(SecurityManager.SPARK_AUTH_CONF, "true")
+    HistoryServer.createSecurityManager(conf)


Can you add a comment explaining that the SecurityManager would throw an exception if spark.authenticate was true? (I think that'd be better than the comment you have before the test.)

done. You know, this test could be merged into "incomplete apps get refreshed", just by setting the security option and mentioning why it matters in a comment. I've patched that test to also use HistoryServer.createSecurityManager for consistency. it would save a fractional amount of test time

vanzin · 2016-12-08T01:45:45Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

+   */
+  test("SecurityManagerStartsWithSecureShuffle") {
+    val conf = new SparkConf()
+        .set("spark.testing", "true")


nit: indented too far

fixed, along with IDE settings

SparkQA · 2016-12-08T16:24:33Z

Test build #69864 has finished for PR 13579 at commit ad35cd9.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

steveloughran · 2016-12-08T18:10:49Z

stylecheck; unexpected, as I thought I'd run them in the mvn install of the module.

[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala:352:2:
Insert a space after the start of the comment

Comment is

  /**
   * Verify that the security manager needed for the history server can be instantiated
   * when `spark.authenticate` is `true`, rather than raise an `IllegalArgumentException`.
   **/

...the trailing **/ is the problem. Fixed

vanzin · 2016-12-08T19:03:20Z

While tests are running, can you update the description? It mentions env vars, which is misleading since they don't have anything to do with this.

steveloughran · 2016-12-08T19:13:09Z

done

vanzin · 2016-12-08T19:24:12Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

+   * Verify that the security manager needed for the history server can be instantiated
+   * when `spark.authenticate` is `true`, rather than raise an `IllegalArgumentException`.
+   */
+  test("SecurityManagerStartsWithSecureShuffle") {


nit: can you write a test description like the others in the class, instead of this camel-cased string?

Also spark.authenticate is not (just) for secure shuffle, so the string itself is not really accurate. Just mention spark.authenticate.

SparkQA · 2016-12-08T20:29:49Z

Test build #69868 has finished for PR 13579 at commit f375682.

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

steveloughran · 2016-12-09T13:58:00Z

Test failure is pretty unlikely to be related. Looks more like a timing or timeout problem.

org.apache.spark.rdd.AsyncRDDActionsSuite.async failure handling

Failing for the past 1 build (Since Failed#69868 )
Took 10 sec.
Error Message

org.scalatest.exceptions.TestFailedDueToTimeoutException: The code passed to failAfter did not complete within 10 seconds.
Stacktrace

sbt.ForkMain$ForkError: org.scalatest.exceptions.TestFailedDueToTimeoutException: The code passed to failAfter did not complete within 10 seconds.

SparkQA · 2016-12-12T16:05:06Z

Test build #70023 has finished for PR 13579 at commit 033f21e.

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

vanzin · 2016-12-12T18:00:20Z

retest this please

vanzin · 2016-12-12T19:36:37Z

LGTM, will merge once tests finish. (The interesting test is passing anyway.)

SparkQA · 2016-12-12T19:56:28Z

Test build #70029 has finished for PR 13579 at commit 033f21e.

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

vanzin · 2016-12-12T20:24:19Z

Merging to master.

steveloughran · 2016-12-12T21:08:55Z

thanks!

…te = true ## What changes were proposed in this pull request? During history server startup, the spark configuration is examined. If security.authentication is set, log at debug and set the value to false, so that {{SecurityManager}} can be created. ## How was this patch tested? A new test in `HistoryServerSuite` sets the `spark.authenticate` property to true, tries to create a security manager via a new package-private method `HistoryServer.createSecurityManager(SparkConf)`. This is the method used in `HistoryServer.main`. All other instantiations of a security manager in `HistoryServerSuite` have been switched to the new method, for consistency with the production code. Author: Steve Loughran <stevel@apache.org> Closes apache#13579 from steveloughran/history/SPARK-15844-security.

srowen mentioned this pull request Sep 20, 2016

[SPARK-17558] Bump Hadoop 2.7 version from 2.7.2 to 2.7.3 #15115

Closed

steveloughran force-pushed the history/SPARK-15844-security branch from 26d1ad2 to 6701922 Compare October 10, 2016 15:39

steveloughran added 2 commits December 7, 2016 22:08

SPARK-15844 HistoryServer doesn't come up if spark.authenticate = true

dbc1c3f

[SPARK-15844] HistoryServer doesn't come up if spark.authenticate = t…

01a0220

…rue; making new (visible for test) method private to the package

steveloughran force-pushed the history/SPARK-15844-security branch from 6701922 to 01a0220 Compare December 7, 2016 22:30

steveloughran added 2 commits December 7, 2016 22:44

[SPARK-15844] fix rebase error

80608da

[SPARK-15844] just turn security off

3b3db69

vanzin reviewed Dec 8, 2016

View reviewed changes

[SPARK-15844] address review comments

ad35cd9

[SPARK-15844] stylecheck on comment.

f375682

vanzin reviewed Dec 8, 2016

View reviewed changes

[SPARK-15844] fix name of new test

033f21e

asfgit closed this in 586d198 Dec 12, 2016

steveloughran deleted the history/SPARK-15844-security branch December 12, 2016 21:08

[SPARK-15844] [core] HistoryServer doesn't come up if spark.authenticate = true #13579

[SPARK-15844] [core] HistoryServer doesn't come up if spark.authenticate = true #13579

Uh oh!

Conversation

steveloughran commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jun 9, 2016

Uh oh!

vanzin commented Jul 20, 2016

Uh oh!

steveloughran commented Oct 10, 2016

Uh oh!

vanzin commented Oct 10, 2016

Uh oh!

SparkQA commented Oct 10, 2016

Uh oh!

steveloughran commented Oct 10, 2016

Uh oh!

vanzin commented Dec 7, 2016

Uh oh!

steveloughran commented Dec 7, 2016

Uh oh!

SparkQA commented Dec 7, 2016

Uh oh!

SparkQA commented Dec 8, 2016

Uh oh!

vanzin left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 8, 2016

Uh oh!

steveloughran commented Dec 8, 2016

Uh oh!

vanzin commented Dec 8, 2016

Uh oh!

steveloughran commented Dec 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 8, 2016

Uh oh!

steveloughran commented Dec 9, 2016

Uh oh!

SparkQA commented Dec 12, 2016

Uh oh!

vanzin commented Dec 12, 2016

Uh oh!

vanzin commented Dec 12, 2016

Uh oh!

SparkQA commented Dec 12, 2016

Uh oh!

vanzin commented Dec 12, 2016

Uh oh!

steveloughran commented Dec 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steveloughran commented Jun 9, 2016 •

edited

Loading