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