-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42565/#review115765
-----------------------------------------------------------


My primary objection to this review is to how it changes the security 
configuration. I'm +1 to the idea of not accepting modules on the CLI for the 
reasons specified in the review summary and because I think accepting a bunch 
of modules blindly is a lot of fire power just for extending behaviour. Users 
who need to do that might be better served with more precise extention points.

Bill, I don't think anyone uses the `extra_modules` argument, git log doesn't 
indicate a really good reason for introducing the feautre, and I think blindly 
accepting a bunch of modules to extend behaviour is not the way to go. If you 
would like, feel free to peel that out into a seperate review so it for sure 
can make it into 0.12.0.


src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (line 77)
<https://reviews.apache.org/r/42565/#comment176828>

    Just to be clear, this command line argument will only accept two modules, 
the ini realm module and the kerberos realm module?
    
    This means users who need to setup a custom realm (probably any medium to 
large cluster operator), must create a custom main.
    
    I'm unsure if this is the ideal approach to take w.r.t security. I think 
this requires some discussion on the mailing list. For example, an alternative 
to what you have done here would be to remove this argument entirely, and 
request users specify realms in the ini file (see 
http://shiro.apache.org/configuration.html section "INI Sections").


- Zameer Manji


On Jan. 20, 2016, 12:21 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42565/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2016, 12:21 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch proposes that users installing custom modules do so via a custom 
> main.
> 
> Specifying custom modules on the command line has proven troublesome for 
> replacing the command line args system with one where all arguments are 
> injected and explicitly-defined.  It also adds complexity to the scheduler 
> application by unnecessarily punching holes at specific places.
> 
> If this proposal is agreeable, i will add a NEWS entry and document how one 
> might implement a custom main to add modules.  The tl;dr, however, is to 
> invoke `SchedulerMain.publicMain(customModule)`
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/app/MoreModules.java 
> d5f96543d1068bf30b9d173a247c2806faf35578 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 0659c358479283756179c2cabebc8416730cc3e3 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ModuleParser.java 
> ccd9a20e8b18831458cba2d53e6b8b84fef06162 
>   src/test/java/org/apache/aurora/scheduler/app/MoreModulesTest.java 
> b2fb3c9dcba64f69a05894f506ba43766f74ddaa 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  3e811a4f4d2c82892217ca1f950ac792303fbcf3 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ModuleParserTest.java
>  baaeb2390a909de1a92e4328d35a49f7b74c36cb 
> 
> Diff: https://reviews.apache.org/r/42565/diff/
> 
> 
> Testing
> -------
> 
> end-to-end tests, `./gradlew run`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to