mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 72448: Fixed operator api event inconsistencies issue.
Date Tue, 21 Jul 2020 13:11:15 GMT


> On July 17, 2020, 5:10 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 428 (patched)
> > <https://reviews.apache.org/r/72448/diff/2/?file=2235829#file2235829line428>
> >
> >     It would be highly beneficial to keep the error sending logic separate from
`ObjectApprovers` and move it closer to `Subscribers`.
> >     
> >     `ObjectApprovers` by design are oblivious of the exact purpose for which they
are called, and it is preferable to keep them decoupled from implementation details of the
external APIs. 
> >     Also, we have a related  API issue https://issues.apache.org/jira/browse/MESOS-10099
which will requre handling authorization errors in a totally different way.
> >     
> >     If you want to avoid changing return type of the of the existing `approved(..)`
method, I would suggest adding a general-purpose `ObjectApprovers` method that would return
`Try<bool>`(something like `template<..> Try<bool> ObjectApprovers::tryApprove<>(..)`)

> >     **and to make the old method `bool ObjectApprovers::approved<..>(..)`
into a thin wrapper around this method**, so that we don't need to care about the duplicated
logic (especially the one in `approved<VIEW_ROLE>(..)`) in the future.
> >     
> >     
> >     To use the returned value of this method in `Subscriber::send()` you will probably
want to wrap the calls to it into some callable that will return `bool` and write down the
error.
> >     Something like 
> >     ```
> >     Option<Error> error;
> >     auto splitError = [&error](Try<bool>&& approved) {
> >       if (approved.isError()) {
> >         error = std::move(approved.error());
> >         return false;
> >       }
> >       return *approved;
> >     }
> >     // Code that checks authorizations and composes the message to be sent
> >     ...
> >         if (splitError(approvers->approved<VIEW_FRAMEWORK>(
> >                   event.framework_added().framework().framework_info()))){
> >         ...
> >         }
> >     ...
> >     // At the end of send(), after all the authorizations have been checked
> >     if (error.isSome()) {
> >       // Send error event
> >       ...
> >       // Close connection
> >       ...
> >       return;
> >     }
> >     
> >     // All is OK, send the event
> >     ...
> >       
> >     ```
> >     
> >     Note that my choice of names `tryApproved` and `splitError` is questionable,
would be great if you come up with something better.
> 
> Dong Zhu wrote:
>     I plan to add another method as below and extract the error in `Subscriber::send()`
as you introduced, eg `splitError(approvers->tryApproved<VIEW_TASK>(event.task_added().task(),
*frameworkInfo))`:
>     
>     ```
>       template <authorization::Action action, typename... Args>
>       Try<bool> tryApproved(const Args&... args) const
>       {
>         const Try<bool> approval =
>           approved(action, ObjectApprover::Object(args...));
>     
>         if (approval.isError()) {
>           LOG(WARNING) << "Failed to authorize principal "
>                        << " '" << (principal.isSome() ? stringify(*principal)
: "")
>                        << "' for action " << authorization::Action_Name(action)
>                        << ": " << approval.error();
>         }
>     
>         return approval;
>       }
>     ```
>     
>     For `approvers->approved<VIEW_ROLE>(resource)` I think it is a specific
case, I have to add another method as below:
>     ```
>     template <>
>     inline Try<bool> ObjectApprovers::tryApproved<authorization::VIEW_ROLE>(
>       const Resource& resource) const
>     {
>       Try<bool> result = true;
>       // Necessary because recovered agents are presented in old format.
>       if (resource.has_role() && resource.role() != "*") {
>         result = tryApproved<authorization::VIEW_ROLE>(resource.role());
>         if (result.isError() || !result.get())
>           return result;
>       }
>     
>       // 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()) {
>         result = tryApproved<authorization::VIEW_ROLE>(reservation.role());
>         if (result.isError() || !result.get())
>           return result;
>       }
>     
>       if (resource.has_allocation_info()) {
>         result =
>           tryApproved<authorization::VIEW_ROLE>(resource.allocation_info().role());
>         if (result.isError() || !result.get())
>           return result;
>       }
>     
>       return result;
>     }
>     ```
>     
>     
>     I do not place original method `bool approved(const Args&... args) const` into
`Try<bool> tryApproved(const Args&... args) const` since I need to get the error
message. 
>     
>     Please let me know whether it make sense.

Yes, your specialization for VIEW_ROLE makes sense: VIEW_ROLE was a specialized case of `approved<>`,
and you will have to keep this logic in `tryApproved<VIEW_ROLE>` (modified exactly like
in your snippet, so that the method returns the error).
I don't think there is a need to log the error in a non-specialized `tryApproved<>`:
this becomes a responsibility of the caller (which now can see the error).

After that, you can implement the old `approved<>(..)` in terms of `tryApproved<>(..)`:
if there is an error, return `false` and **log** the error, otherwise return the authorization
result. 
If I'm not missing something, this will allow you to get rid of the `approved<VIEW_ROLE>`
specialization.


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72448/#review221252
-----------------------------------------------------------


On July 14, 2020, 9:43 a.m., Dong Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> -----------------------------------------------------------
> 
> (Updated July 14, 2020, 9:43 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch intends to fix issue MESOS-10085.
> 
> When the authorization failed happens master return nothing to the
> subscriber, subscriber isn't aware of what is happening, this issue
> can lead to inconsistencies in Event stream.
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 
>   include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d 
>   src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
>   src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c 
>   src/tests/master/mock_master_api_subscriber.cpp 893d3e366164ccebd2847ed4c2874ab00e0e5b7b

> 
> 
> Diff: https://reviews.apache.org/r/72448/diff/2/
> 
> 
> Testing
> -------
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message