----------------------------------------------------------- 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 > >
