----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65311/#review196303 -----------------------------------------------------------
src/common/http.hpp Lines 38-39 (original), 39-44 (patched) <https://reviews.apache.org/r/65311/#comment275862> Nit: I think just one newline here? And below, at the end of the namespace. src/common/http.hpp Lines 57 (patched) <https://reviews.apache.org/r/65311/#comment275863> Is this case needed for type deduction? clang seems to build this code fine without it. src/common/http.hpp Lines 166 (patched) <https://reviews.apache.org/r/65311/#comment275855> Nit: I think there should be just one newline here? src/common/http.hpp Lines 209 (patched) <https://reviews.apache.org/r/65311/#comment275858> Suggestion: s/UNEXPECTED/unexpected/ src/common/http.hpp Lines 231 (patched) <https://reviews.apache.org/r/65311/#comment275860> Indented too far. src/common/http.hpp Lines 232-233 (patched) <https://reviews.apache.org/r/65311/#comment275864> I would probably do: ``` hashmap< authorization::Action, process::Owned<ObjectApprover>>&& _approvers, ``` up to you src/common/http.hpp Lines 282-283 (original), 357-359 (patched) <https://reviews.apache.org/r/65311/#comment275861> One more newline here? src/common/http.cpp Lines 870 (patched) <https://reviews.apache.org/r/65311/#comment275865> 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? src/common/http.cpp Lines 875-887 (patched) <https://reviews.apache.org/r/65311/#comment275868> 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? src/common/http.cpp Lines 1057-1059 (patched) <https://reviews.apache.org/r/65311/#comment275872> 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? src/common/http.cpp Lines 1071-1075 (patched) <https://reviews.apache.org/r/65311/#comment275871> Suggestion: could you move this conditional to the end? Then the two reservation-related blocks will be adjacent. - Greg Mann On Jan. 24, 2018, 5: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, 5: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 > >
