> On Feb. 3, 2017, 11:26 a.m., Adam B wrote: > > src/authorizer/local/authorizer.cpp, line 244 > > <https://reviews.apache.org/r/56178/diff/1/?file=1621348#file1621348line244> > > > > Seems like framework_info is always set, so how/why would we ever fall > > through to the other cases? > > Benjamin Bannier wrote: > Yes, this is currently dead code. I was also wondering whether it should > be removed, but decided against it since it provides some level of redundancy > as long as `value` still exists and code in the master and in authorizers > might not evolve consistently. > > Do you believe it should be removed? > > Alexander Rojas wrote: > The main reason the `object->value` is still there, is that the local > authorizer is a reference implementation for module writers who want to build > their own modules, as such, it does provide a reference. I myself will vote > to remove the `value` field if possible. However, we makred as deprecated in > November 2016 which means we need it there until June (at the same time it > had said it is supposed to be removed on 1.2). > > Adam B wrote: > Alright. Please add a comment explaining why we're keeping dead code > around, so I am not tempted to delete it next time I see it. > > Benjamin Bannier wrote: > Yes, a comment is in order here, updated the patch. I also slipped in a > `TODO` to remove this branch when `value` is purged. > > Adam B wrote: > This code is in the LocalAuthorizer, which doesn't support other custom > authorizers. While Mesos master/agent should continue to set both > FrameworkInfo and `value`, to support custom authorizers, an authorizer > implementation like LocalAuthorizer doesn't need to look at `value` if it's > already looking at FrameworkInfo. Even as a reference implementation, we > should not show an example of using the deprecated field. I think we can > safely remove this entire block. > I am, however, happy that we now have MESOS-7073 to track the eventual > removal of the value field altogether.
This requires updating a number of tests which still set value. I would like to avoid pulling this work into this change and created MESOS-7091 to track this work. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56178/#review164109 ----------------------------------------------------------- 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 > >
