mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 72448: Fixed operator api event inconsistencies issue.
Date Wed, 29 Apr 2020 14:58:10 GMT

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


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