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 Thu, 09 Jul 2020 10:38:11 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.

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.


> 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.
> 
> Dong Zhu wrote:
>     Can I send subsequent patch for adding test ?

Sure, it is in fact more convenient to have a test into a separate depending patch.

We will definitely want to commit these two patches together, though.


- Andrei


-----------------------------------------------------------
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
> -------
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>


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