----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 -----------------------------------------------------------
Thanks for handling this bug! At this point, the main issue with this patch is that it doesn't discern between declined authorization and authorization failure (see below). To address this, you'll likely have to extend/change the interface provided by ObjectApprover**s** (plural) so that they don't disguise authorization errors as declined authorization. Also, please link MESOS-10085 to this review ("Bugs:" on the right or `support/post-reviews.py --bugs-closed MESOS-10085`) and don't forget the "testing done" section. include/mesos/master/master.proto Lines 732 (patched) <https://reviews.apache.org/r/72448/#comment309034> Do we really need `required` here? By labelling this field as `required`, we imply that `Error` without a `message` will always be ill-formed and introduce a guarantee that all future versions of Mesos will be setting this field (see "Required is Forever" in https://developers.google.com/protocol-buffers/docs/proto) Given that in distant future a need to provide more information in `Error` might arise, and this might lead to making `message` string optional or even deprecated, I would suggest marking it as `optional` right now, because there is no compatible way back from `required`. Additionally, I'm not actually sure this should be a string - see below. src/master/master.cpp Lines 12264 (patched) <https://reviews.apache.org/r/72448/#comment309036> 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). src/master/master.cpp Lines 12266 (patched) <https://reviews.apache.org/r/72448/#comment309037> The messages in your patch seem to be rather uniform: they include a type of error ("Event Unapproved", you will need to change this after addressing the denied vs error issue above) and a type of event. Sending a type of event is definitely a good idea: if the subscriber does not really care about frameworks and only wants to know the state of tasks, and sending `FRAMEWORK_ADDED` fails, it could just proceed without worrying that it missed some update it needed. Probably, instead of a single `message` string, we could just have two enum fields? Like ``` message Error { enum ErrorType { UNKNOWN = 0; AUTHORIZATION_FAILURE = 1; } optional ErrorType error_type; optional Type event_type; } ``` This could help the subscriber, if it wants, to extract the type of the event that was dropped and act (or not act) accordingly. I doubt this will impact error message readability on the subscriber side: subscribers receiving JSON will have no problems with this, and subscribers that receive protobuf have to use protobuf reflection to log anything meaningful anyway. src/master/master.cpp Lines 12268 (patched) <https://reviews.apache.org/r/72448/#comment309038> Given that now we are sending `Error` event, the subscribers that are aware of this event can do whatever they want (proceed as if nothing happened; close connection and subscribe again, etc.) Are we aiming to fix inconsistency created by dropped events for existing subscribers, that do not handle `Error` event? If yes, then one (and probably the only) option for fixing this for existing subscribers is to close connection and drop the subscriber after sending `Error`. Probably, at this point we could just always close after Error. In a distant future, when the subsrcibers become aware of `Error`, we will probably want to add a flag to `SUBSCRIBE` so that, if the flag is set, disconnection doesn't happen, and the subscriber resubscribes on its own if it wants to. src/tests/master/mock_master_api_subscriber.cpp Lines 242 (patched) <https://reviews.apache.org/r/72448/#comment309035> 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. - Andrei Sekretenko 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 > ------- > > > Thanks, > > Dong Zhu > >