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