mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dong Zhu <d...@d2iq.com>
Subject Re: Review Request 72448: Fixed operator api event inconsistencies issue.
Date Mon, 20 Jul 2020 05:11:10 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.

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.


- Dong


-----------------------------------------------------------
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