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