----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58096/#review175213 -----------------------------------------------------------
Looks pretty good to me. Just a few minor comments src/master/http.cpp Lines 3504 (patched) <https://reviews.apache.org/r/58096/#comment248641> What makes a role "active"? Having `active` frameworks registered for that role? This function seems to return a list of all roles that have one or more weights, quota, reservations, or registered frameworks associated with them. More accurate would be to call it `knownRoles`. src/master/http.cpp Lines 3507 (patched) <https://reviews.apache.org/r/58096/#comment248639> Nit: `ObjectApprover` singular, since it's a single approver that can approve/deny multiple frameworks, not an approver per framework. src/master/http.cpp Lines 3524 (patched) <https://reviews.apache.org/r/58096/#comment248644> `futures` is an over-vague variable name, especially since neither are Futures by this point. Can we do better? - Adam B On May 10, 2017, 9:33 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58096/ > ----------------------------------------------------------- > > (Updated May 10, 2017, 9:33 a.m.) > > > Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler. > > > Bugs: MESOS-7260 > https://issues.apache.org/jira/browse/MESOS-7260 > > > Repository: mesos > > > Description > ------- > > While /roles displays a list of frameworksIds that register with > a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which > impose a security risk. This patch fixed this issue by taking a > frameworksApprover in `Master::Http::roles()` which is used to > filter framework IDs. > > > Diffs > ----- > > src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 > > > Diff: https://reviews.apache.org/r/58096/diff/7/ > > > Testing > ------- > > see next patch in the chain. > > > Thanks, > > Jay Guo > >
