> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/master/master.cpp > > Lines 12264 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12264> > > > > As implemented now, `false` returned by `ObjectApprovers::approved()` > > is not necessarily an error case. This might just be a declined > > authorization, i.e. the subscriber is not even allowed to see the object > > in question due to the missing permissions. Events for such objects should > > just be not sent to the subscriber. > > > > To distinguish between **declined authorization** and **authorization > > error** here, you will have to change/extend the interface provided by > > `ObjectApprovers` (plural) so that it doesn't hide `Error`s returned by > > `approved()` method of `ObjectApprover`(singular). > > Dong Zhu wrote: > Why will we need to distinguish those two circumstances ? > `ObjectApprovers::approved()` returned with `false` when **declined > authorization** or **authorization error** happens. The purpose of this patch > just handling the `false` part by sending a notification to the subscriber. > > Andrei Sekretenko wrote: > The `false` part (i.e. declined authorization) is not an issue. > Subscriber does not need to be informed about the fact that somewhere on > the cluster there are some objects (tasks/frameworks/whatever) that the > subscriber is not allowed to access. > It is frequently the case that there are, and there is nothing wrong with > it. > > Declined authorization is pretty much expected to occur, and existing > subscribers (such as DC/OS UI) are designed not to expect any information > about the objects their principal is not authorized to access. > > Probably https://issues.apache.org/jira/browse/MESOS-7785 points to more > detailed explanations why this decision was made. > > > On the other hand, subscribers expect to see all events for all objects > they are authorized to see. > Here lies the issue described in MESOS-10085: in case of an authorization > failure, the event will be silently dropped, but the subscriber will have no > knowledge of that and will continue working as if there was no event. > > Dong Zhu wrote: > In summary: > - for declined authorization circumstance: > There is no need to inform the subscriber just drop the message. eg, > for `FRAMEWORK_ADDED` event, if there is no proper permission like > `VIEW_FRAMEWORK` configured just drop the message. > - for authorization error circumstance: > Send an event to subscriber to inform the error. It fixed the issue > intorducted in MESOS-10085 **"in case of an authorization failure, the event > will be silently dropped, but the subscriber will have no knowledge of that > and will continue working as if there was no event."** > > Am I correct ? > > Andrei Sekretenko wrote: > Yes, exactly. > > Note that to accomplish this, you will have to change the return type of > `ObjectApprovers::approve()`, as it currently does not allow the caller to > distinguish between a declined authorization and an error. > > Dong Zhu wrote: > I came up with two ways for solving this issue: > > 1. Change/Extend the interface provided by > `ObjectApprovers::approved()`. eg, change the return type from `bool` to > `Try<bool>` > https://github.com/apache/mesos/blob/master/src/common/http.hpp#L395-L418 > then distinguish the **declined authorization** and **authorization error** > in the caller side `void Master::Subscribers::Subscriber::send()`. Since > there are many callers in mesos it will result in tremendous code changes and > introduce code duplication. > 2. Add a new element to `class ObjectApprovers` eg, > `StreamingHttpConnection<v1::master::Event> http;`. Consturct the return > message in `ObjectApprovers::approved()` and send to the subscriber if > **authorization error** occurs. I am not sure whether this way broke any > mesos developing principals though. > > What's your option here ? or do you have any other good way ? Thanks !
Hi Andrei, I have updated the patch, could you please help reviewing it again ? - Dong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 ----------------------------------------------------------- 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 > >
