mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dong Zhu <d...@d2iq.com>
Subject Re: Review Request 72448: Fixed operator api event inconsistencies issue.
Date Thu, 30 Apr 2020 03:32:32 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message