----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56178/#review165014 -----------------------------------------------------------
Looks good to me, just a few questions, I will let Adam ship this. src/authorizer/local/authorizer.cpp (line 246) <https://reviews.apache.org/r/56178/#comment236851> newline between comment blocks? e.g. ``` // We also update the deprecated `value` field to support custom // authorizers not yet modified to examine `framework_info`. // // TODO(bbannier): Clean up use of `value` here, see MESOS-7091. ``` src/authorizer/local/authorizer.cpp (line 247) <https://reviews.apache.org/r/56178/#comment236854> Not yours, but I find it rather confusing as to what the object value is, looking at the other code, is it the role? It would be nice to clarify how one figures out what `value` represents. src/master/master.cpp (line 2173) <https://reviews.apache.org/r/56178/#comment236845> comma after "frameworks"? src/master/master.cpp (line 2178) <https://reviews.apache.org/r/56178/#comment236841> newline between the comment blocks? e.g. // For non-`MULTI_ROLE` frameworks also propagate its single role // via the request's `value` field. This is purely for backwards // compatibility as the `value` field is deprecated. Note that this // means that authorizers relying on the deprecated field will see // an empty string in `value` for for `MULTI_ROLE` frameworks. // // TODO(bbannier): Remove this at the end of `value`'s deprecation // cycle, see MESOS-7073. src/master/master.cpp (lines 2533 - 2535) <https://reviews.apache.org/r/56178/#comment236840> Longer term, are there any thoughts on how we might be able to know which role is not authorized? E.g. getting the authorization message via `Future<Option<Error>>`? - Benjamin Mahler On Feb. 9, 2017, 9:26 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56178/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2017, 9:26 a.m.) > > > Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler. > > > Bugs: MESOS-7022 > https://issues.apache.org/jira/browse/MESOS-7022 > > > Repository: mesos > > > Description > ------- > > This updates the local authorizer so that MULTI_ROLE frameworks can be > authorized. > > For non-MULTI_ROLE frameworks we continue to support use of the > deprecated 'value' field in the authorization request's 'Object'; > however for MULTI_ROLE frameworks the 'value' field will not be set, > and authorizers still relying on it should be updated to instead use > the object's 'framework_info' field to extract roles to authorize > against from. > > > Diffs > ----- > > src/authorizer/local/authorizer.cpp > b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 > src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c > > Diff: https://reviews.apache.org/r/56178/diff/ > > > Testing > ------- > > Tested on various configurations in internal CI. > > > Thanks, > > Benjamin Bannier > >
