aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@apache.org>
Subject Re: Review Request 42565: Remove support for adding guice modules via command line arguments.
Date Fri, 22 Jan 2016 02:44:54 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message