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