aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Sirois" <jsir...@apache.org>
Subject Re: Review Request 42565: Remove support for adding guice modules via command line arguments.
Date Fri, 22 Jan 2016 18:21:50 GMT


> On Jan. 21, 2016, 7:44 p.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'
>     ```
> 
> Maxim Khutornenko wrote:
>     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.
> 
> John Sirois wrote:
>     Wait - how do you extend aurora today?  My understanding is you need to muck with
start scripts to ammend the classpath.  Under that assumption, I saw mucking with classpath
+ changing main class name as not an undue burden over and above the status quo.
> 
> John Sirois wrote:
>     Basically - if you actually want to claim the label pluggable, you need an out-of-packaging
way to ammend the classpath.  IE: an optional file in - say ubuntu - /etc/default/aurora.d/my_plugins.conf
- that adds a, say 'my_extensions/*' entry to the claaspath.  The need to write a main could
be converted to using a requirement to implement a ServiceLoader interface that would allow
the standard scheduler main to scan for service implementations - that service might just
be a SAM that returns an Iterable of Modules or else it could be a more rigid service interface
with 1 method (defaulted) per new extension point that comes up.
>     
>     I think this sort of puggability would be great to have, but it seems to me that
should be decoupled from the new args system if this change is good-enough, ie: maintains
the status quo of having to 1.) compile your own jars out-of-band, 2.) deliver them to aurora
clusters where the aurora start scripts are modified to add them to the classpath and - now
- change the name of the main in the 'java ...' command line.

And even then - Aurora won't be pluggable by any reasonable definition unless it publishes
the extension APIs as a library.  It looks like that was attempted once but is not kept up2date:
http://search.maven.org/#artifactdetails%7Corg.apache.aurora%7Caurora-api%7C0.8.0%7Cjar


- John


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


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