aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 41525: Add flag to set FrameworkInfo.principal
Date Thu, 17 Dec 2015 20:53:47 GMT

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


Thanks, overall this looks great!

Can you also add a line in the NEWS file to call out the new flag?  Doesn't need formal documentation,
just a brief blurb is fine.


src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java (line
137)
<https://reviews.apache.org/r/41525/#comment171037>

    How about `Optional<String>` and a branch to avoid  the call to `setPrinciPal` when
it's absent?  I'm not sure how an empty string is handled on the other end, but it seems safest
to omit the field and match current behavior.


- Bill Farner


On Dec. 17, 2015, 10:39 a.m., R.B. Boyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41525/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2015, 10:39 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-687
>     https://issues.apache.org/jira/browse/AURORA-687
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Bugs closed: AURORA-687
> 
> Copying the intent from one of the trailing comments:
> 
> """
> There are several situations to consider:
> 
> 1. fresh installs that will not have authN/authZ (may want to add authN/authZ later)
> 2. fresh installs that will begin life with authN/authZ (no problems in the future unless
the principal is changed)
> 3. existing installs that have authN (may want to add authZ later)
> 
> The current defaults are friendly for (1) above, where the typical test-driver won't
be turning any flavor of Auth on in their mesos cluster. These defaults are good.
> 
> If someone elects to go down (2) above, the defaults you get from -framework_authentication_file
are not awesome, as this is where the misstep lies.
> 
> If someone went down (3) above already, and used the -framework_authentication_file non-awesome
setting, you also don't want _them_ to break their cluster.
> 
> The bare minimum change to be the least breaking would be to introduce a new setting
like -framework_announce_principal or something so that (2) can be happy without breaking
(3).
> 
> Similarly to the mesos checkpoint breaking change, you could roll this out as opt-in
and then in a future release make it opt-out when setting -framework_authentication_file and
announce it as a breaking change in the changelogs for the poor folks in group (3).
> """
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
68aeda1692271841d10e5f29d439806576bd691c 
>   src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
513391ff82c3e985bf9f673d35414e9245807b4a 
> 
> Diff: https://reviews.apache.org/r/41525/diff/
> 
> 
> Testing
> -------
> 
> Ran normal unit tests.
> 
> Built the patched scheduler and deployed it in an authN+authZ mesos cluster (3 control
plane boxes, 2 execution plane boxes) and the framework could actually register with mesos.
> 
> 
> Thanks,
> 
> R.B. Boyer
> 
>


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