Skip to content
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.

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

[SPARK-4223] [Core] Support * in acls. #8398

Closed
wants to merge 6 commits into from
Closed

Conversation

zhuoliu
Copy link

@zhuoliu zhuoliu commented Aug 24, 2015

SPARK-4223.

Currently we support setting view and modify acls but you have to specify a list of users. It would be nice to support * meaning all users have access.

Manual tests to verify that: "*" works for any user in:
a. Spark ui: view and kill stage. Done.
b. Spark history server. Done.
c. Yarn application killing. Done.

@zhuoliu zhuoliu changed the title Spark-4223 Support * in acls. [SPARK-4223] Support * in acls. Aug 24, 2015
@zhuoliu zhuoliu changed the title [SPARK-4223] Support * in acls. [SPARK-4223] [Core] Support * in acls. Aug 24, 2015
@@ -310,7 +310,13 @@ private[spark] class SecurityManager(sparkConf: SparkConf)
setViewAcls(Set[String](defaultUser), allowedUsers)
}

def getViewAcls: String = viewAcls.mkString(",")
def getViewAcls: String = {
if (viewAcls.contains("*")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good here to put a comment that YARN requires only a * to be returned. you can't have *,user1,user2 to explain why we do this

@tgravescs
Copy link
Contributor

Jenkins, test this please

@SparkQA
Copy link

SparkQA commented Aug 26, 2015

Test build #41646 has finished for PR 8398 at commit 0568bb1.

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

conf.set("spark.ui.view.acls", "*")
conf.set("spark.modify.acls", "user4")

val securityManager = new SecurityManager(conf);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: nuke ';'

@vanzin
Copy link
Contributor

vanzin commented Aug 26, 2015

Just minor nits, otherwise LGTM. Also, minor, but we generally write "YARN" instead of "Yarn" in comments.

@vanzin
Copy link
Contributor

vanzin commented Aug 26, 2015

retest this please

@zhuoliu
Copy link
Author

zhuoliu commented Aug 26, 2015

Thanks @vanzin, comments addressed.

@SparkQA
Copy link

SparkQA commented Aug 27, 2015

Test build #41651 has finished for PR 8398 at commit 0568bb1.

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

@tgravescs
Copy link
Contributor

I think it would be nice if we update the docs to tell users * is supported. Can you update docs/configuration.md. Perhaps under each description of modify.acsl, view.acls, admin.acls add something that says Special value of * means anyone

@zhuoliu
Copy link
Author

zhuoliu commented Aug 27, 2015

Sure. Docs updated.

@tgravescs
Copy link
Contributor

Jenkins, ok to test

@tgravescs
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Aug 27, 2015

Test build #41699 has finished for PR 8398 at commit b1d49b3.

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

@tgravescs
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Aug 27, 2015

Test build #41703 has finished for PR 8398 at commit b1d49b3.

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

def getModifyAcls: String = modifyAcls.mkString(",")
/**
* Checking the existence of "*" is necessary as YARN can't recognize the "*" in "defaultuser,*"
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - spacing off. remove space before *

@tgravescs
Copy link
Contributor

@rxin @JoshRosen Since this is in core would one of you like to take a look.

Minor nit in spacing otherwise LGTM.

@SparkQA
Copy link

SparkQA commented Aug 28, 2015

Test build #41746 has finished for PR 8398 at commit 77869c9.

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

@rxin
Copy link
Contributor

rxin commented Sep 1, 2015

LGTM - I've merged this.

@asfgit asfgit closed this in ec01280 Sep 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants