> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > 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.
Thanks for your info, I replied them one by one as below.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > include/mesos/master/master.proto
> > Lines 732 (patched)
> > <https://reviews.apache.org/r/72448/diff/1/?file=2229244#file2229244line732>
> >
> > 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.
Sure. Let me change it.
> 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).
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.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12266 (patched)
> > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12266>
> >
> > 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.
```
message Error {
enum ErrorType {
UNKNOWN = 0;
AUTHORIZATION_FAILURE = 1;
}
optional ErrorType error_type;
optional Type event_type;
}
```
This form of message is better and this is one thing that I want to confirm
with. I will make change to the patch.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12268 (patched)
> > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12268>
> >
> > 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.
OK, close this http connection after sending the `Error` 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.
Can I send subsequent patch for adding test ?
- Dong
-----------------------------------------------------------
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
> -------
>
>
> Thanks,
>
> Dong Zhu
>
>
