You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2018/02/02 15:00:59 UTC
Re: Review Request 65311: Added the ObjectApprovers to which unifies
authorization logic.
> 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
>
>