> On Feb. 9, 2017, 8:53 a.m., Adam B wrote: > > src/master/master.cpp, lines 2178-2179 > > <https://reviews.apache.org/r/56178/diff/6/?file=1626414#file1626414line2178> > > > > Why not check `if(protobuf::framework::getRoles(frameworkInfo).size() > > <= 1)` instead of checking the capability? A legacy authorizer could still > > authorize a multi-role-capable framework if it's only trying to register > > with a single role.
This cannot be used instead of the current condition; what you propose would amount to an additional branch to set `value` to `roles[0]` for single role, `MULTI_ROLE` frameworks. I am not sure of the value this would bring; we might effectively allow users to run multirole frameworks, but these frameworks could only be in a single role, and all this for authorizers already relying on a deprecated field. Currently a framework's `role` cannot be changed (not enforced, only per documentation); with the upcoming possibility to change framework `roles`, `MULTI_ROLE` frameworks we would enable here to work with these legacy authorizers would only fail to authz if they added another role. I do see value in disallowing use of `MULTI_ROLE` features in these legacy setups as early as possible, instead of letting them fail later. It would also help us avoid introducing new, but already deprecated behavior. Anything else to keep in mind here? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56178/#review164868 ----------------------------------------------------------- On Feb. 9, 2017, 9:59 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:59 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 > >
