Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

> On Sept. 25, 2020, 10:26 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 422-426 (patched)
> > 
> >
> > Just `return approved(...)` ?
> 
> Dong Zhu wrote:
> I got totally confused here, I have no idea what you mean. Can you 
> develop a patch and post here that I can make a reference ?

I mean, why define a variable when you can just return the result of the 
function call?

Instead of
```
T f(){
  const T result = g();
  return result;
}
```
you can just write
```
T f() {
  return g();
}
```
?


- Andrei


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


On July 23, 2020, 11:03 a.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated July 23, 2020, 11:03 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/3/
> 
> 
> Testing
> ---
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On Sept. 26, 2020, 4:26 a.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 422-426 (patched)
> > 
> >
> > Just `return approved(...)` ?

I got totally confused here, I have no idea what you mean. Can you develop a 
patch and post here that I can make a reference ?


- Dong


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


On July 23, 2020, 5:03 p.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated July 23, 2020, 5:03 p.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/3/
> 
> 
> Testing
> ---
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

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




src/common/http.hpp
Lines 402-404 (original), 401-403 (patched)


The comment about MESOS-10085 is not relevant anymore after this patch?



src/common/http.hpp
Lines 419 (patched)


Does this correctly handle `tryApproved(...)`?

If no, then can you make sure (at the very least, with a `static_assert`) 
that it is not accidentally used for VIEW_ROLE?



src/common/http.hpp
Lines 422-426 (patched)


Just `return approved(...)` ?



src/master/master.cpp
Line 12277 (original), 12286 (patched)


Can you follow the same approach for obtaining `VIEW_ROLE` authorization as 
well, here and below?


- Andrei Sekretenko


On July 23, 2020, 11:03 a.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated July 23, 2020, 11:03 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/3/
> 
> 
> Testing
> ---
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On July 18, 2020, 1:10 a.m., Andrei Sekretenko wrote:
> > Thanks! Now this patch addresses the main issue.
> 
> Dong Zhu wrote:
> I updated the patch, this time I do not handle 
> `approvers->approved(resource)` in `void 
> Master::Subscribers::Subscriber::send()` specifically since I do not find any 
> `Error()` return from 
> https://github.com/apache/mesos/blob/master/src/authorizer/local/authorizer.cpp#L601-L797
> Could you please help reviewing it again ?

Hi, Could you help taking a look at it ? It's been a long time since the last 
update.


- Dong


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


On July 23, 2020, 5:03 p.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated July 23, 2020, 5:03 p.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/3/
> 
> 
> Testing
> ---
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On July 18, 2020, 1:10 a.m., Andrei Sekretenko wrote:
> > Thanks! Now this patch addresses the main issue.

I updated the patch, this time I do not handle 
`approvers->approved(resource)` in `void 
Master::Subscribers::Subscriber::send()` specifically since I do not find any 
`Error()` return from 
https://github.com/apache/mesos/blob/master/src/authorizer/local/authorizer.cpp#L601-L797
Could you please help reviewing it again ?


- Dong


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


On July 23, 2020, 5:03 p.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated July 23, 2020, 5:03 p.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/3/
> 
> 
> Testing
> ---
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72448]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On July 23, 2020, 9:03 a.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated July 23, 2020, 9:03 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/3/
> 
> 
> Testing
> ---
> 
> - Manually tested
>   - make check
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

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

(Updated July 23, 2020, 9:03 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 (updated)
-

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

Changes: https://reviews.apache.org/r/72448/diff/2-3/


Testing
---

- Manually tested
  - make check


Thanks,

Dong Zhu

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

> On July 17, 2020, 5:10 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 428 (patched)
> > 
> >
> > It would be highly beneficial to keep the error sending logic separate 
> > from `ObjectApprovers` and move it closer to `Subscribers`.
> > 
> > `ObjectApprovers` by design are oblivious of the exact purpose for 
> > which they are called, and it is preferable to keep them decoupled from 
> > implementation details of the external APIs. 
> > Also, we have a related  API issue 
> > https://issues.apache.org/jira/browse/MESOS-10099 which will requre 
> > handling authorization errors in a totally different way.
> > 
> > If you want to avoid changing return type of the of the existing 
> > `approved(..)` method, I would suggest adding a general-purpose 
> > `ObjectApprovers` method that would return `Try`(something like 
> > `template<..> Try ObjectApprovers::tryApprove<>(..)`) 
> > **and to make the old method `bool ObjectApprovers::approved<..>(..)` 
> > into a thin wrapper around this method**, so that we don't need to care 
> > about the duplicated logic (especially the one in 
> > `approved(..)`) in the future.
> > 
> > 
> > To use the returned value of this method in `Subscriber::send()` you 
> > will probably want to wrap the calls to it into some callable that will 
> > return `bool` and write down the error.
> > Something like 
> > ```
> > Option error;
> > auto splitError = [](Try&& approved) {
> >   if (approved.isError()) {
> > error = std::move(approved.error());
> > return false;
> >   }
> >   return *approved;
> > }
> > // Code that checks authorizations and composes the message to be sent
> > ...
> > if (splitError(approvers->approved(
> >   event.framework_added().framework().framework_info({
> > ...
> > }
> > ...
> > // At the end of send(), after all the authorizations have been checked
> > if (error.isSome()) {
> >   // Send error event
> >   ...
> >   // Close connection
> >   ...
> >   return;
> > }
> > 
> > // All is OK, send the event
> > ...
> >   
> > ```
> > 
> > Note that my choice of names `tryApproved` and `splitError` is 
> > questionable, would be great if you come up with something better.
> 
> Dong Zhu wrote:
> I plan to add another method as below and extract the error in 
> `Subscriber::send()` as you introduced, eg 
> `splitError(approvers->tryApproved(event.task_added().task(), 
> *frameworkInfo))`:
> 
> ```
>   template 
>   Try tryApproved(const Args&... args) const
>   {
> const Try approval =
>   approved(action, ObjectApprover::Object(args...));
> 
> if (approval.isError()) {
>   LOG(WARNING) << "Failed to authorize principal "
><< " '" << (principal.isSome() ? stringify(*principal) 
> : "")
><< "' for action " << 
> authorization::Action_Name(action)
><< ": " << approval.error();
> }
> 
> return approval;
>   }
> ```
> 
> For `approvers->approved(resource)` I think it is a specific 
> case, I have to add another method as below:
> ```
> template <>
> inline Try ObjectApprovers::tryApproved(
>   const Resource& resource) const
> {
>   Try result = true;
>   // Necessary because recovered agents are presented in old format.
>   if (resource.has_role() && resource.role() != "*") {
> result = tryApproved(resource.role());
> if (result.isError() || !result.get())
>   return result;
>   }
> 
>   // Reservations follow a path model where each entry is a child of the
>   // previous one. Therefore, to accept the resource the acceptor has to
>   // accept all entries.
>   foreach (Resource::ReservationInfo reservation, 
> resource.reservations()) {
> result = tryApproved(reservation.role());
> if (result.isError() || !result.get())
>   return result;
>   }
> 
>   if (resource.has_allocation_info()) {
> result =
>   
> tryApproved(resource.allocation_info().role());
> if (result.isError() || !result.get())
>   return result;
>   }
> 
>   return result;
> }
> ```
> 
> 
> I do not place original method `bool approved(const Args&... args) const` 
> into `Try tryApproved(const Args&... args) const` since I need to get 
> the error message. 
> 
> Please let me know whether it make sense.

Yes, your specialization for VIEW_ROLE makes sense: VIEW_ROLE was a specialized 
case of `approved<>`, and you will have to keep this logic in 
`tryApproved` (modified exactly like in your snippet, so that the 
method returns the error).
I don't think 

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 436-441 (patched)
> > 
> >
> > Thanks for taking care about logging!
> > 
> > We definitely don't want to write a log line per every authorization 
> > error, as the errors might (and likely will) come in large bunches.
> > 
> > This should happen at most once per disconnecting the subscriber (one 
> > more reason to move this into `Subscribers::send()`).
> > 
> > Also, it probably would be beneficial to log some other details 
> > allowing the persons reading the log to identify the subscriber (at the 
> > very least, IP + principal, and probably something else  - honestly, I 
> > don't remember what else Mesos master knows about a subscriber).
> 
> Dong Zhu wrote:
> So you mean there is not need to print those errors to logs anymore ?

There is a need to log the error, but, in case of master API events this should 
happen once per event (and, as a result, once per disconnecting the subscriber) 
but not once per object.


> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 437 (patched)
> > 
> >
> > Is it necessary to use an intermediate `ostringstream` instead of 
> > directly writing into the glog's stream returned by `LOG(WARNING)`?
> 
> Dong Zhu wrote:
> I need those logs to inform subscriber about the exact fail reason.
> 
> Dong Zhu wrote:
> Is it necessary to notify user regarding the fail reason? If so what sort 
> of message would you prefer ?

Hmm... you are not sending the value of `stringStream.str()` to the subscriber, 
right? 
If the log os the only place where is is being written to, then why not just 
write directly into glog's stream:
```
LOG(WARNING) << "Failed to authorize principal "
 << " '" << (principal.isSome() ? stringify(*principal) : "")
...
```?
This will notify the cluster operator about the fail reason.

As for notifying the subscriber, sending the error message returned by 
authorizer's `ObjectApprover::approve()` (what your code does now) shoud be 
sufficient.
I also believe this should be safe (i.e. that authorizers should not expose 
sensitive information via this error message), but I would advise you to 
double-check the authorization docs and the authorizer implemenations.


- Andrei


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


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 437 (patched)
> > 
> >
> > Is it necessary to use an intermediate `ostringstream` instead of 
> > directly writing into the glog's stream returned by `LOG(WARNING)`?
> 
> Dong Zhu wrote:
> I need those logs to inform subscriber about the exact fail reason.

Is it necessary to notify user regarding the fail reason? If so what sort of 
message would you prefer ?


- Dong


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


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 436-441 (patched)
> > 
> >
> > Thanks for taking care about logging!
> > 
> > We definitely don't want to write a log line per every authorization 
> > error, as the errors might (and likely will) come in large bunches.
> > 
> > This should happen at most once per disconnecting the subscriber (one 
> > more reason to move this into `Subscribers::send()`).
> > 
> > Also, it probably would be beneficial to log some other details 
> > allowing the persons reading the log to identify the subscriber (at the 
> > very least, IP + principal, and probably something else  - honestly, I 
> > don't remember what else Mesos master knows about a subscriber).

So you mean there is not need to print those errors to logs anymore ?


> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 437 (patched)
> > 
> >
> > Is it necessary to use an intermediate `ostringstream` instead of 
> > directly writing into the glog's stream returned by `LOG(WARNING)`?

I need those logs to inform subscriber about the exact fail reason.


- Dong


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


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On July 17, 2020, 5:10 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 428 (patched)
> > 
> >
> > It would be highly beneficial to keep the error sending logic separate 
> > from `ObjectApprovers` and move it closer to `Subscribers`.
> > 
> > `ObjectApprovers` by design are oblivious of the exact purpose for 
> > which they are called, and it is preferable to keep them decoupled from 
> > implementation details of the external APIs. 
> > Also, we have a related  API issue 
> > https://issues.apache.org/jira/browse/MESOS-10099 which will requre 
> > handling authorization errors in a totally different way.
> > 
> > If you want to avoid changing return type of the of the existing 
> > `approved(..)` method, I would suggest adding a general-purpose 
> > `ObjectApprovers` method that would return `Try`(something like 
> > `template<..> Try ObjectApprovers::tryApprove<>(..)`) 
> > **and to make the old method `bool ObjectApprovers::approved<..>(..)` 
> > into a thin wrapper around this method**, so that we don't need to care 
> > about the duplicated logic (especially the one in 
> > `approved(..)`) in the future.
> > 
> > 
> > To use the returned value of this method in `Subscriber::send()` you 
> > will probably want to wrap the calls to it into some callable that will 
> > return `bool` and write down the error.
> > Something like 
> > ```
> > Option error;
> > auto splitError = [](Try&& approved) {
> >   if (approved.isError()) {
> > error = std::move(approved.error());
> > return false;
> >   }
> >   return *approved;
> > }
> > // Code that checks authorizations and composes the message to be sent
> > ...
> > if (splitError(approvers->approved(
> >   event.framework_added().framework().framework_info({
> > ...
> > }
> > ...
> > // At the end of send(), after all the authorizations have been checked
> > if (error.isSome()) {
> >   // Send error event
> >   ...
> >   // Close connection
> >   ...
> >   return;
> > }
> > 
> > // All is OK, send the event
> > ...
> >   
> > ```
> > 
> > Note that my choice of names `tryApproved` and `splitError` is 
> > questionable, would be great if you come up with something better.

I plan to add another method as below and extract the error in 
`Subscriber::send()` as you introduced, eg 
`splitError(approvers->tryApproved(event.task_added().task(), 
*frameworkInfo))`:

```
  template 
  Try tryApproved(const Args&... args) const
  {
const Try approval =
  approved(action, ObjectApprover::Object(args...));

if (approval.isError()) {
  LOG(WARNING) << "Failed to authorize principal "
   << " '" << (principal.isSome() ? stringify(*principal) : "")
   << "' for action " << authorization::Action_Name(action)
   << ": " << approval.error();
}

return approval;
  }
```

For `approvers->approved(resource)` I think it is a specific case, I 
have to add another method as below:
```
template <>
inline Try ObjectApprovers::tryApproved(
  const Resource& resource) const
{
  Try result = true;
  // Necessary because recovered agents are presented in old format.
  if (resource.has_role() && resource.role() != "*") {
result = tryApproved(resource.role());
if (result.isError() || !result.get())
  return result;
  }

  // Reservations follow a path model where each entry is a child of the
  // previous one. Therefore, to accept the resource the acceptor has to
  // accept all entries.
  foreach (Resource::ReservationInfo reservation, resource.reservations()) {
result = tryApproved(reservation.role());
if (result.isError() || !result.get())
  return result;
  }

  if (resource.has_allocation_info()) {
result =
  tryApproved(resource.allocation_info().role());
if (result.isError() || !result.get())
  return result;
  }

  return result;
}
```


I do not place original method `bool approved(const Args&... args) const` into 
`Try tryApproved(const Args&... args) const` since I need to get the 
error message. 

Please let me know whether it make sense.


- Dong


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


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: 

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

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




src/common/http.hpp
Lines 436-441 (patched)


Thanks for taking care about logging!

We definitely don't want to write a log line per every authorization error, 
as the errors might (and likely will) come in large bunches.

This should happen at most once per disconnecting the subscriber (one more 
reason to move this into `Subscribers::send()`).

Also, it probably would be beneficial to log some other details allowing 
the persons reading the log to identify the subscriber (at the very least, IP + 
principal, and probably something else  - honestly, I don't remember what else 
Mesos master knows about a subscriber).



src/common/http.hpp
Lines 437 (patched)


Is it necessary to use an intermediate `ostringstream` instead of directly 
writing into the glog's stream returned by `LOG(WARNING)`?


- Andrei Sekretenko


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

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



Thanks! Now this patch addresses the main issue.


src/common/http.hpp
Lines 428 (patched)


It would be highly beneficial to keep the error sending logic separate from 
`ObjectApprovers` and move it closer to `Subscribers`.

`ObjectApprovers` by design are oblivious of the exact purpose for which 
they are called, and it is preferable to keep them decoupled from 
implementation details of the external APIs. 
Also, we have a related  API issue 
https://issues.apache.org/jira/browse/MESOS-10099 which will requre handling 
authorization errors in a totally different way.

If you want to avoid changing return type of the of the existing 
`approved(..)` method, I would suggest adding a general-purpose 
`ObjectApprovers` method that would return `Try`(something like 
`template<..> Try ObjectApprovers::tryApprove<>(..)`) 
**and to make the old method `bool ObjectApprovers::approved<..>(..)` into 
a thin wrapper around this method**, so that we don't need to care about the 
duplicated logic (especially the one in `approved(..)`) in the 
future.

To use the returned value of this method in `Subscriber::send()` you will 
probably want to wrap the calls to it into some callable that will return 
`bool` and write down the error.
Something like 
```
Option error;
auto splitError = [](Try&& approved) {
  if (approved.isError()) {
error = std::move(approved.error());
return false;
  }
  return *approved;
}
// Code that checks authorizations and composes the message to be sent
...
if (splitError(approvers->approved(
  event.framework_added().framework().framework_info({
...
}
...
// At the end of send(), after all the authorizations have been checked
if (error.isSome()) {
  // Send error event
  ...
  // Close connection
  ...
  return;
}

// All is OK, send the event
...
  
```

Note that my choice of names `tryApproved` and `splitError` is 
questionable, would be great if you come up with something better.



src/common/http.hpp
Lines 435 (patched)


If you implement the method returning `Try`, you will have this 
`TODO` addrerssed - dont forget to remove it and to adjust the `NOTE` above for 
your fix ;)


- Andrei Sekretenko


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72448]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12264 (patched)
> > 
> >
> > 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` 
> 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 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 

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

---
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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/72448/diff/1-2/


Testing
---

- Manually tested
  - make check


Thanks,

Dong Zhu

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12264 (patched)
> > 
> >
> > 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.

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


- 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 

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12264 (patched)
> > 
> >
> > 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 ?

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.


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12264 (patched)
> > 
> >
> > 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.

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 ?


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > src/master/master.cpp
> > Lines 12264 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

> 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.
> 
> Dong Zhu wrote:
> Thanks for your info, I replied them one by one as below.

@Andrei Sekretenko Could you take a look at the following comments ?


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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

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

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Andrei Sekretenko]

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


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)


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)


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)


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)


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, 

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 29, 2020, 3:30 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, 3:30 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> 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
> 
>