> On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote: > > src/common/http.hpp > > Lines 57 (patched) > > <https://reviews.apache.org/r/65311/diff/1/?file=1946005#file1946005line57> > > > > Is this case needed for type deduction? clang seems to build this code > > fine without it.
>From the research I did, not getting the underlying type of an enum just >defaults to int. I think this was done by benh in our effort to move towards >class enums, so I would prefer to keep this one. > On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote: > > src/common/http.cpp > > Lines 870 (patched) > > <https://reviews.apache.org/r/65311/diff/1/?file=1946006#file1946006line870> > > > > This seems like very simple validation to do. I would suggest if we > > want to ensure this, let's just do it now and remove the TODO? > > > > Should it be a CHECK? I just add everything to a set which will remove duplicates. Not sure if having them even demands a log line. > On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote: > > src/common/http.cpp > > Lines 875-887 (patched) > > <https://reviews.apache.org/r/65311/diff/1/?file=1946006#file1946006line875> > > > > Since this is a pretty hot code path, should we consider optimizations > > now? > > > > Since the authorizer is not SOME or NONE on a per-action basis, we > > could have an `AcceptingObjectApprovers` which we return immediately if > > there is no authorizer: > > > > ``` > > if (authorizer.isNone()) { > > return Owned<ObjectApprovers>( > > new AcceptingObjectApprovers()); > > } else { > > return process::collect( etc... > > ``` > > > > WDYT? I agree that it makes no sense to waste time creating `ObjectApprover` instances which will be accepting ones, I just figured out it can be done before and faster. > On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote: > > src/common/http.cpp > > Lines 1057-1059 (patched) > > <https://reviews.apache.org/r/65311/diff/1/?file=1946006#file1946006line1057> > > > > Instead of a free function helper, what do you think about adding a > > specialization/overload to handle resources with the VIEW_ROLE action, like: > > ``` > > template <> > > bool ObjectApprovers::approved<authorization::VIEW_ROLE>( > > const Resource& resource) > > { > > // Necessary because recovered agents are presented in old format. > > if (resource.has_role() && resource.role() != "*" && > > !approved<authorization::VIEW_ROLE>(resource.role())) { > > return false; > > } > > > > // Reservations follow a path model where each entry is a child of the > > // previous one. Therefore, to accept the resource the acceptor has to > > // accept all entries. > > foreach (Resource::ReservationInfo reservation, > > resource.reservations()) { > > if (!approved<authorization::VIEW_ROLE>(reservation.role())) { > > return false; > > } > > } > > > > if (resource.has_allocation_info() && > > !approved<authorization::VIEW_ROLE>( > > resource.allocation_info().role())) { > > return false; > > } > > > > return true; > > } > > ``` > > > > And then the callsites could look like: > > ``` > > approvers->approved<VIEW_ROLE>(resource); > > ``` > > > > WDYT? I liked the idea but somehow the program didn't resolved the templates correctly. I would leave it like this initially, and I will try to figure out the right way of doing it. If you're ok with it. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65311/#review196303 ----------------------------------------------------------- On Jan. 24, 2018, 6:30 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65311/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2018, 6:30 p.m.) > > > Review request for mesos, Benjamin Hindman and Greg Mann. > > > Bugs: MESOS-8434 > https://issues.apache.org/jira/browse/MESOS-8434 > > > Repository: mesos > > > Description > ------- > > Introduces the class `ObjectApprovers` which unifies the different > mechanisms used in Mesos in order to perform authorization. > > This new class will supersede the `Acceptor` interfaces and their > logic. > > Follow up patches will make use of this interface and remove > deprecated code. > > > Diffs > ----- > > src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 > src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 > > > Diff: https://reviews.apache.org/r/65311/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
