Skip to content

Commit 5280d93

Browse files
jerryshaoMarcelo Vanzin
authored andcommitted
[SPARK-20239][CORE] Improve HistoryServer's ACL mechanism
## What changes were proposed in this pull request? Current SHS (Spark History Server) two different ACLs: * ACL of base URL, it is controlled by "spark.acls.enabled" or "spark.ui.acls.enabled", and with this enabled, only user configured with "spark.admin.acls" (or group) or "spark.ui.view.acls" (or group), or the user who started SHS could list all the applications, otherwise none of them can be listed. This will also affect REST APIs which listing the summary of all apps and one app. * Per application ACL. This is controlled by "spark.history.ui.acls.enabled". With this enabled only history admin user and user/group who ran this app can access the details of this app. With this two ACLs, we may encounter several unexpected behaviors: 1. if base URL's ACL (`spark.acls.enable`) is enabled but user A has no view permission. User "A" cannot see the app list but could still access details of it's own app. 2. if ACLs of base URL (`spark.acls.enable`) is disabled, then user "A" could download any application's event log, even it is not run by user "A". 3. The changes of Live UI's ACL will affect History UI's ACL which share the same conf file. The unexpected behaviors is mainly because we have two different ACLs, ideally we should have only one to manage all. So to improve SHS's ACL mechanism, here in this PR proposed to: 1. Disable "spark.acls.enable" and only use "spark.history.ui.acls.enable" for history server. 2. Check permission for event-log download REST API. With this PR: 1. Admin user could see/download the list of all applications, as well as application details. 2. Normal user could see the list of all applications, but can only download and check the details of applications accessible to him. ## How was this patch tested? New UTs are added, also verified in real cluster. CC tgravescs vanzin please help to review, this PR changes the semantics you did previously. Thanks a lot. Author: jerryshao <[email protected]> Closes #17582 from jerryshao/SPARK-20239.
1 parent 8a272dd commit 5280d93

File tree

4 files changed

+33
-11
lines changed

4 files changed

+33
-11
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ private[history] abstract class ApplicationHistoryProvider {
8686
* @return Count of application event logs that are currently under process
8787
*/
8888
def getEventLogsUnderProcess(): Int = {
89-
return 0;
89+
0
9090
}
9191

9292
/**
@@ -95,7 +95,7 @@ private[history] abstract class ApplicationHistoryProvider {
9595
* @return 0 if this is undefined or unsupported, otherwise the last updated time in millis
9696
*/
9797
def getLastUpdatedTime(): Long = {
98-
return 0;
98+
0
9999
}
100100

101101
/**

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,14 @@ object HistoryServer extends Logging {
301301
logDebug(s"Clearing ${SecurityManager.SPARK_AUTH_CONF}")
302302
config.set(SecurityManager.SPARK_AUTH_CONF, "false")
303303
}
304+
305+
if (config.getBoolean("spark.acls.enable", config.getBoolean("spark.ui.acls.enable", false))) {
306+
logInfo("Either spark.acls.enable or spark.ui.acls.enable is configured, clearing it and " +
307+
"only using spark.history.ui.acl.enable")
308+
config.set("spark.acls.enable", "false")
309+
config.set("spark.ui.acls.enable", "false")
310+
}
311+
304312
new SecurityManager(config)
305313
}
306314

core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,14 +184,27 @@ private[v1] class ApiRootResource extends ApiRequestContext {
184184
@Path("applications/{appId}/logs")
185185
def getEventLogs(
186186
@PathParam("appId") appId: String): EventLogDownloadResource = {
187-
new EventLogDownloadResource(uiRoot, appId, None)
187+
try {
188+
// withSparkUI will throw NotFoundException if attemptId exists for this application.
189+
// So we need to try again with attempt id "1".
190+
withSparkUI(appId, None) { _ =>
191+
new EventLogDownloadResource(uiRoot, appId, None)
192+
}
193+
} catch {
194+
case _: NotFoundException =>
195+
withSparkUI(appId, Some("1")) { _ =>
196+
new EventLogDownloadResource(uiRoot, appId, None)
197+
}
198+
}
188199
}
189200

190201
@Path("applications/{appId}/{attemptId}/logs")
191202
def getEventLogs(
192203
@PathParam("appId") appId: String,
193204
@PathParam("attemptId") attemptId: String): EventLogDownloadResource = {
194-
new EventLogDownloadResource(uiRoot, appId, Some(attemptId))
205+
withSparkUI(appId, Some(attemptId)) { _ =>
206+
new EventLogDownloadResource(uiRoot, appId, Some(attemptId))
207+
}
195208
}
196209

197210
@Path("version")
@@ -291,7 +304,6 @@ private[v1] trait ApiRequestContext {
291304
case None => throw new NotFoundException("no such app: " + appId)
292305
}
293306
}
294-
295307
}
296308

297309
private[v1] class ForbiddenException(msg: String) extends WebApplicationException(

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -565,13 +565,12 @@ class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with Matchers
565565
assert(jobcount === getNumJobs("/jobs"))
566566

567567
// no need to retain the test dir now the tests complete
568-
logDir.deleteOnExit();
569-
568+
logDir.deleteOnExit()
570569
}
571570

572571
test("ui and api authorization checks") {
573-
val appId = "app-20161115172038-0000"
574-
val owner = "jose"
572+
val appId = "local-1430917381535"
573+
val owner = "irashid"
575574
val admin = "root"
576575
val other = "alice"
577576

@@ -590,8 +589,11 @@ class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with Matchers
590589

591590
val port = server.boundPort
592591
val testUrls = Seq(
593-
s"http://localhost:$port/api/v1/applications/$appId/jobs",
594-
s"http://localhost:$port/history/$appId/jobs/")
592+
s"http://localhost:$port/api/v1/applications/$appId/1/jobs",
593+
s"http://localhost:$port/history/$appId/1/jobs/",
594+
s"http://localhost:$port/api/v1/applications/$appId/logs",
595+
s"http://localhost:$port/api/v1/applications/$appId/1/logs",
596+
s"http://localhost:$port/api/v1/applications/$appId/2/logs")
595597

596598
tests.foreach { case (user, expectedCode) =>
597599
testUrls.foreach { url =>

0 commit comments

Comments
 (0)