> 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.
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. > On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/tests/master/mock_master_api_subscriber.cpp > > Lines 242 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229247#file2229247line242> > > > > Given that we will need to add a test for this fix, doesn't it make > > sense to add one more callback, for `ERROR`? > > This way, tests will be able to use `EXPECT_CALL` when they need to > > check that the error was sent. > > Dong Zhu wrote: > Can I send subsequent patch for adding test ? Sure, it is in fact more convenient to have a test into a separate depending patch. We will definitely want to commit these two patches together, though. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 ----------------------------------------------------------- On April 29, 2020, 6:18 a.m., Dong Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > ----------------------------------------------------------- > > (Updated April 29, 2020, 6:18 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/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/1/ > > > Testing > ------- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
