[SPARK-19033][Core] Add admin acls for history server #16470
Conversation
Change-Id: Idc5509c7ee24c6cf717e35c0bdaebf3384baa59f
|
Test build #70865 has finished for PR 16470 at commit
|
tgravescs
left a comment
tgravescs
left a comment
There was a problem hiding this comment.
Mostly looks good. Please make sure to test all the different scenarios. history admin acls blank with app acls set, user level admin acls blank with history acls set, both blank.
| SparkListenerEnvironmentUpdate(Map( | ||
| "Spark Properties" -> Seq( | ||
| ("spark.admin.acls", "user"), | ||
| ("spark.admin.acls.groups", "group")), |
| } | ||
|
|
||
|
|
||
| test("support history server ui admin acls") { |
There was a problem hiding this comment.
it would be nice to have tests for empty history admin acls to make sure parsing/concatenation is working properly.
| ui.getSecurityManager.setViewAcls(attempt.sparkUser, | ||
| appListener.viewAcls.getOrElse("")) | ||
| ui.getSecurityManager.setAdminAclsGroups(appListener.adminAclsGroups.getOrElse("")) | ||
| val adminAcls = conf.get("spark.history.ui.admin.acls", "") + "," + |
There was a problem hiding this comment.
it feels a bit odd that this could be have a blank value and then comma ie " ,value", but setAdminAcls handles it so I guess its ok. Lets make sure to add unit test for this scenario and that way if someone accidentally changes that we will catch it.
There was a problem hiding this comment.
Also rather then reading from the conf on every application UI creation lets move the history.ui config reads up to a class val, similar to UPDATE_INTERVAL_S. It would also be nice to print off those values into the log files at initial start up. If you don't mind could you do the same for spark.history.ui.acls.enable. I know it was this way before your change but would be nice to make consistent.
Change-Id: Id11665cfac20c5112b52868ebbc67d40f676f528
|
Thanks @tgravescs for your comments, I've updated the code to add more tests accordingly. Please review. |
|
Test build #70900 has finished for PR 16470 at commit
|
|
Jenkins, retest this please. |
|
Test build #70906 has started for PR 16470 at commit |
|
Jenkins, retest this please. |
|
Test build #70930 has finished for PR 16470 at commit
|
|
+1 |
## What changes were proposed in this pull request? Current HistoryServer's ACLs is derived from application event-log, which means the newly changed ACLs cannot be applied to the old data, this will become a problem where newly added admin cannot access the old application history UI, only the new application can be affected. So here propose to add admin ACLs for history server, any configured user/group could have the view access to all the applications, while the view ACLs derived from application run-time still take effect. ## How was this patch tested? Unit test added. Author: jerryshao <[email protected]> Closes #16470 from jerryshao/SPARK-19033. (cherry picked from commit 4a4c3dc) Signed-off-by: Tom Graves <[email protected]>
## What changes were proposed in this pull request? Current HistoryServer's ACLs is derived from application event-log, which means the newly changed ACLs cannot be applied to the old data, this will become a problem where newly added admin cannot access the old application history UI, only the new application can be affected. So here propose to add admin ACLs for history server, any configured user/group could have the view access to all the applications, while the view ACLs derived from application run-time still take effect. ## How was this patch tested? Unit test added. Author: jerryshao <[email protected]> Closes apache#16470 from jerryshao/SPARK-19033.
## What changes were proposed in this pull request? Current HistoryServer's ACLs is derived from application event-log, which means the newly changed ACLs cannot be applied to the old data, this will become a problem where newly added admin cannot access the old application history UI, only the new application can be affected. So here propose to add admin ACLs for history server, any configured user/group could have the view access to all the applications, while the view ACLs derived from application run-time still take effect. ## How was this patch tested? Unit test added. Author: jerryshao <[email protected]> Closes apache#16470 from jerryshao/SPARK-19033.
3 participants
What changes were proposed in this pull request?
Current HistoryServer's ACLs is derived from application event-log, which means the newly changed ACLs cannot be applied to the old data, this will become a problem where newly added admin cannot access the old application history UI, only the new application can be affected.
So here propose to add admin ACLs for history server, any configured user/group could have the view access to all the applications, while the view ACLs derived from application run-time still take effect.
How was this patch tested?
Unit test added.