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 Tue, 14 Jul 2020 09:45:40 GMT


> 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).
> 
> Dong Zhu wrote:
>     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.
> 
> Andrei Sekretenko wrote:
>     The `false` part (i.e. declined authorization) is not an issue.
>     Subscriber does not need to be informed about the fact that somewhere on the cluster
there are some objects (tasks/frameworks/whatever) that the subscriber is not allowed to access.
>     It is frequently the case that there are, and there is nothing wrong with it.
>     
>     Declined authorization is pretty much expected to occur, and existing subscribers
(such as DC/OS UI) are designed not to expect any information about the objects their principal
is not authorized to access.
>     
>     Probably https://issues.apache.org/jira/browse/MESOS-7785 points to more detailed
explanations why this decision was made.
>     
>     
>     On the other hand, subscribers expect to see all events for all objects they are
authorized to see. 
>     Here lies the issue described in MESOS-10085: in case of an authorization failure,
the event will be silently dropped, but the subscriber will have no knowledge of that and
will continue working as if there was no event.
> 
> Dong Zhu wrote:
>     In summary:
>       - for declined authorization circumstance: 
>         There is no need to inform the subscriber just drop the message. eg, for `FRAMEWORK_ADDED`
event, if there is no proper permission like `VIEW_FRAMEWORK` configured just drop the message.

>       - for authorization error circumstance:
>         Send an event to subscriber to inform the error. It fixed the issue intorducted
in MESOS-10085 **"in case of an authorization failure, the event will be silently dropped,
but the subscriber will have no knowledge of that and will continue working as if there was
no event."**
>         
>     Am I correct ?
> 
> Andrei Sekretenko wrote:
>     Yes, exactly.
>     
>     Note that to accomplish this, you will have to change the return type of `ObjectApprovers::approve()`,
as it currently does not allow the caller to distinguish between a declined authorization
and an error.
> 
> Dong Zhu wrote:
>     I came up with two ways for solving this issue:
>     
>       1. Change/Extend the interface provided by `ObjectApprovers::approved()`. eg, change
the return type from `bool` to `Try<bool>` https://github.com/apache/mesos/blob/master/src/common/http.hpp#L395-L418
then distinguish the **declined authorization** and **authorization error** in the caller
side `void Master::Subscribers::Subscriber::send()`. Since there are many callers in mesos
it will result in tremendous code changes and introduce code duplication.
>       2. Add a new element to `class ObjectApprovers` eg, `StreamingHttpConnection<v1::master::Event>
http;`. Consturct the return message in `ObjectApprovers::approved()` and send to the subscriber
if **authorization error** occurs. I am not sure whether this way broke any mesos developing
principals though.
>     
>     What's your option here ? or do you have any other good way ? Thanks !

Hi Andrei,

I have updated the patch, could you please help reviewing it again ?


- Dong


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72448/#review220548
-----------------------------------------------------------


On July 14, 2020, 9:43 a.m., Dong Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> -----------------------------------------------------------
> 
> (Updated July 14, 2020, 9:43 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/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
>   src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c 
>   src/tests/master/mock_master_api_subscriber.cpp 893d3e366164ccebd2847ed4c2874ab00e0e5b7b

> 
> 
> Diff: https://reviews.apache.org/r/72448/diff/2/
> 
> 
> Testing
> -------
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>


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