-
Notifications
You must be signed in to change notification settings - Fork 68
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
MYRIAD-243 Support Authentication in Myriad UI and REST APIs #96
base: master
Are you sure you want to change the base?
Conversation
|
||
/** | ||
* Status codes for MyriadWebServer | ||
*/ | ||
public enum Status {STARTED, RUNNING, STOPPED, FAILED, UNKNOWN} | ||
|
||
@Inject | ||
public MyriadWebServer(Server jetty, Connector connector, GuiceFilter filter) { | ||
public MyriadWebServer(Server jetty, Connector connector, GuiceFilter filter, MyriadConfiguration myriadConf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all we're using is isSecurityEnabled maybe just pass a boolean, if you think we'll need more of the config later leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now it is only for boolean, but we may need more from config later. WOuld be a pain to redo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then leave it.
@@ -20,6 +20,7 @@ servedBinaryPath: /tmp/myriadBinary | |||
mesosMaster: 10.0.2.15:5050 | |||
haEnabled: false | |||
checkpoint: false | |||
isSecurityEnabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add to src/main/resources/myriad-default-config for a minimal documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - good point, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to docs, so users have a clue how to enable this and what it means.
Looks good but haven't tested, if you're comfortable then merge away! |
Thank you Darrin, I would like somebody outside to test it. I did test it on MapR distro, will try to test with SPNEGO, but it is hard for me to test with other distros |
I can do some tests early to mid next week, unfortunately I'm swamped tomorrow. |
Updated default value in myriad-config-default.yml. For some reason Travis is not triggering the build |
@DarinJ - wonder if you have more comments to this PR? |
@yufeldman Nope - I'm behind on some other items so haven't been able to test yet. Hopefully will get to it tonight/tomorrow. |
Tested a bit, seemed OK, had to add user:password@ to the URI's for binary dist and config if using myriad to distribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising, but I didn't review the tests in detail.
@@ -92,6 +92,7 @@ | |||
*/ | |||
public static final Boolean DEFAULT_HA_ENABLED = false; | |||
|
|||
public static final Boolean DEFAULT_SECURITY_ENABLED = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFAULT_AUTHENTICATION_ENABLED or DEFAULT_AUTH_ENABLED? "Security" is too vague, since encryption, authorization, and isolation are all parts of "security".
@@ -20,6 +20,7 @@ servedBinaryPath: /tmp/myriadBinary | |||
mesosMaster: 10.0.2.15:5050 | |||
haEnabled: false | |||
checkpoint: false | |||
isSecurityEnabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to docs, so users have a clue how to enable this and what it means.
Essentially reusing Authentication (AuthenticationFilter with some variation) provided by Hadoop