aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 42565: Remove support for adding guice modules via command line arguments.
Date Fri, 22 Jan 2016 17:49:30 GMT


> On Jan. 22, 2016, 2:44 a.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
line 80
> > <https://reviews.apache.org/r/42565/diff/3/?file=1203749#file1203749line80>
> >
> >     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").
> 
> Bill Farner wrote:
>     In case the concern about a custom main is that it sounds dautning, here's an illustration:
>     
>     ```
>     public class CustomMain {
>       public static void main(String[] args) {
>         SchedulerMain.applyStaticArgumentValues(args);
>         SchedulerMain.publicMain(new CustomRealmModule());
>       }
>     }
>     ```
>     
>     (It may be worth building in the `applyStaticArgumentValues` step, so we can reduce
even further.)
>     
>     I see 2 upsides with this:
>     - The API for customization (whether security or other arbitrary feature addition)
is uniform.
>     - The `CustomMain` can use the same commane line arguments system as the rest of
the scheduler (this will work with the forthcoming replacement as well).
>     
>     I believe your suggestion to use an INI section falls apart in 2 ways:
>     - the `[main]` section only applies when using pure INI configuration for shiro.
I believe we fall under the programmatic configuration section, with shiro-guice doing the
programmatic bits.  The ini file aurora accepts is read by `org.apache.shiro.realm.text.IniRealm`
and it only uses the `users` and `roles` sections (see https://shiro.apache.org/static/1.2.3/apidocs/org/apache/shiro/realm/text/IniRealm.html)
>     - i don't believe classes configured with pure shiro INI configuration would participate
in guice injection
>     
>     I'm open to ideas here.  Ultimately i'm trying to preserve integration of custom
secruity controls with features that exist today:
>     1. support for participation in guice injection
>     2. support for participation in the same configuration system as the rest of the
scheduler
>     
>     If (1) is not necessary, that simplifies things.  An alternative to (2) is to force
custom code to handle its own configuration (e.g. with environment variables).
> 
> Maxim Khutornenko wrote:
>     Perhaps a script example will help evaluating this approach? Bill, would you mind
giving a simple example of how aurora-scheduler.conf looked had we added a custom module in
our e2e tests?
> 
> Bill Farner wrote:
>     `aurora-scheduler.conf` would not change, as you would only alter the main class.
 The change would be in `build.gradle`:
>     
>     From
>     ```
>     mainClassName = 'org.apache.aurora.scheduler.app.SchedulerMain'
>     ```
>     
>     to
>     ```
>     mainClassName = 'com.my.package.CustomScheduler'
>     ```

This means anyone who wants to add custom modules would be forced to fork Aurora codebase
instead of a purely pluggable model that we have now? I am not sure I like it. This is error
and merge conflict prone.

Perhaps having a well documented gradle option taking a path to CustomScheduler would be a
better middle ground? At least that would not require forking Aurora.


- Maxim


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


On Jan. 20, 2016, 8: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, 8: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