Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
steveloughran wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dbc1c3f
SPARK-15844 HistoryServer doesn't come up if spark.authenticate = true
steveloughran Jun 9, 2016
01a0220
[SPARK-15844] HistoryServer doesn't come up if spark.authenticate = t…
steveloughran Jun 9, 2016
80608da
[SPARK-15844] fix rebase error
steveloughran Dec 7, 2016
3b3db69
[SPARK-15844] just turn security off
steveloughran Dec 7, 2016
ad35cd9
[SPARK-15844] address review comments
steveloughran Dec 8, 2016
f375682
[SPARK-15844] stylecheck on comment.
steveloughran Dec 8, 2016
033f21e
[SPARK-15844] fix name of new test
steveloughran Dec 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ object HistoryServer extends Logging {
Utils.initDaemon(log)
new HistoryServerArguments(conf, argStrings)
initSecurity()
val securityManager = new SecurityManager(conf)
val securityManager = createSecurityManager(conf)

val providerName = conf.getOption("spark.history.provider")
.getOrElse(classOf[FsHistoryProvider].getName())
Expand All @@ -289,6 +289,30 @@ object HistoryServer extends Logging {
while(true) { Thread.sleep(Int.MaxValue) }
}

/**
* Create a security manager.
* This includes any fixup of the configurations needed to produce a security manager
* capable of starting the History Server.
* @param config configuration for the SecurityManager constructor
* @return
*/
private[history] def createSecurityManager(config: SparkConf): SecurityManager = {
patchSecuritySettings(config)
new SecurityManager(config)
}

/**
* Fix up the configuration of a spark configuration so that the security manager will
* always start.
* @param config configuration to be used in a SecurityManager constructor
*/
private def patchSecuritySettings(config: SparkConf): Unit = {

@vanzin vanzin Dec 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@steveloughran steveloughran Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@steveloughran steveloughran Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (config.getBoolean(SecurityManager.SPARK_AUTH_CONF, false)) {
logDebug(s"Clearing ${SecurityManager.SPARK_AUTH_CONF}")
config.set(SecurityManager.SPARK_AUTH_CONF, "false")
}
}

def initSecurity() {
// If we are accessing HDFS and it has security enabled (Kerberos), we have to login
// from a keytab file so that we can access HDFS beyond the kerberos ticket expiration.
Expand Down
13 changes: 12 additions & 1 deletion core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with Matchers
.set("spark.testing", "true")
provider = new FsHistoryProvider(conf)
provider.checkForLogs()
val securityManager = new SecurityManager(conf)
val securityManager = HistoryServer.createSecurityManager(conf)

server = new HistoryServer(conf, provider, securityManager, 18080)
server.initialize()
Expand Down Expand Up @@ -349,6 +349,17 @@ class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with Matchers

}

/**
* Verify that the security manager needed for the history server can be instantiated
* even if spark.authenticate is set
*/
test("SecurityManagerStartsWithSecureShuffle") {

@vanzin vanzin Dec 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val conf = new SparkConf()
.set("spark.testing", "true")

@vanzin vanzin Dec 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indented too far

@steveloughran steveloughran Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, along with IDE settings

.set(SecurityManager.SPARK_AUTH_CONF, "true")
HistoryServer.createSecurityManager(conf)

@vanzin vanzin Dec 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@steveloughran steveloughran Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

test("incomplete apps get refreshed") {

implicit val webDriver: WebDriver = new HtmlUnitDriver
Expand Down