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