You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Quinn Leng <qu...@gmail.com> on 2017/07/27 18:25:37 UTC
Review Request 61189: Added authorization for V1 events.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
Repository: mesos
Description
-------
Added authorization filtering for V1 streaming events, the
subscriber should only receive events that are authorized
based on their principal and ACLs.
Diffs
-----
src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
Diff: https://reviews.apache.org/r/61189/diff/1/
Testing
-------
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
Thanks,
Quinn Leng
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183028
-----------------------------------------------------------
Bad patch!
Reviews applied: [61189]
Failed command: python support/apply-reviews.py -n -r 61189
Error:
error: patch failed: src/master/http.cpp:826
error: src/master/http.cpp: patch does not apply
error: patch failed: src/tests/api_tests.cpp:1921
error: src/tests/api_tests.cpp: patch does not apply
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/235/console
- Mesos Reviewbot Windows
On July 27, 2017, 6:25 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 6:25 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
>
>
> Diffs
> -----
>
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
> src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/2/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review181716
-----------------------------------------------------------
src/master/master.hpp
Lines 1835 (patched)
<https://reviews.apache.org/r/61189/#comment257364>
Does `Subscriber` need to store the authorizer? Can it just do `master->authorizer`?
src/master/master.cpp
Lines 9586 (patched)
<https://reviews.apache.org/r/61189/#comment258210>
Perhaps we could make this a member function of `Subscriber` and eliminate the `subscriber` parameter? Then you could do `subscriber->send(...)` here.
- Greg Mann
On July 27, 2017, 6:25 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 6:25 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
>
>
> Diffs
> -----
>
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
> src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/2/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183191
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61189]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On Aug. 17, 2017, 3:59 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2017, 3:59 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/3/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183274
-----------------------------------------------------------
src/master/master.cpp
Lines 9626 (patched)
<https://reviews.apache.org/r/61189/#comment259285>
Need a space before the `{`
src/master/master.cpp
Lines 9705-9713 (patched)
<https://reviews.apache.org/r/61189/#comment259296>
If you use `else if` here instead, we can avoid unnecessary comparisons.
src/tests/api_tests.cpp
Lines 2190-2192 (patched)
<https://reviews.apache.org/r/61189/#comment259314>
Maybe:
"Verifies that operators subscribed to the master's operator API event stream only receive events that they are authorized to see."
src/tests/api_tests.cpp
Lines 2198-2233 (patched)
<https://reviews.apache.org/r/61189/#comment259313>
Similar to the recent authZ filtering test we did, could you not set the permissive bit to false and explicitly specify the ACLs we need? This means you could also get rid of the `register_agents` and `register_frameworks` ACLs here.
src/tests/api_tests.cpp
Lines 2208 (patched)
<https://reviews.apache.org/r/61189/#comment259288>
s/use/user/
src/tests/api_tests.cpp
Lines 2261-2262 (patched)
<https://reviews.apache.org/r/61189/#comment259307>
Let's not abbreviate these and just call them `executor1` and `executor2`
src/tests/api_tests.cpp
Lines 2264 (patched)
<https://reviews.apache.org/r/61189/#comment259308>
s/execs/executors/
src/tests/api_tests.cpp
Lines 2317 (patched)
<https://reviews.apache.org/r/61189/#comment259294>
Instead:
```
ASSERT_FALSE(offers->offers().empty());
```
src/tests/api_tests.cpp
Lines 2346-2356 (patched)
<https://reviews.apache.org/r/61189/#comment259300>
Could you put each one of these event-handling sections into a scope?
src/tests/api_tests.cpp
Lines 2358-2362 (patched)
<https://reviews.apache.org/r/61189/#comment259301>
This could go into a scope.
src/tests/api_tests.cpp
Lines 2381-2391 (patched)
<https://reviews.apache.org/r/61189/#comment259302>
Could you move this block above the declaration of `task1`?
src/tests/api_tests.cpp
Lines 2388 (patched)
<https://reviews.apache.org/r/61189/#comment259305>
How about `execTask1` instead?
src/tests/api_tests.cpp
Lines 2434-2436 (patched)
<https://reviews.apache.org/r/61189/#comment259306>
Let's use a separate `Future<v1::scheduler::Event::Offers> offers2` object here instead of re-using the one from before. In this case, should probably rename the first one to `offers1` as well.
src/tests/api_tests.cpp
Lines 2442-2444 (patched)
<https://reviews.apache.org/r/61189/#comment259303>
You should do the same check here: `ASSERT_FALSE(offers->offers().empty());`
src/tests/api_tests.cpp
Lines 2458-2465 (patched)
<https://reviews.apache.org/r/61189/#comment259309>
Let's move this block above the declaration of `task2`.
src/tests/api_tests.cpp
Lines 2458-2460 (patched)
<https://reviews.apache.org/r/61189/#comment259310>
Let's create a new Future for this update rather than re-using the first one.
src/tests/api_tests.cpp
Lines 2462 (patched)
<https://reviews.apache.org/r/61189/#comment259304>
How about `execTask2` instead?
src/tests/api_tests.cpp
Lines 2491 (patched)
<https://reviews.apache.org/r/61189/#comment259311>
Do we need this?
src/tests/api_tests.cpp
Lines 2493-2498 (patched)
<https://reviews.apache.org/r/61189/#comment259312>
Status updates will be retried until the scheduler acknowledges them. To prevent this, you can have the scheduler ack the update immediately after it's received, like this: https://github.com/apache/mesos/blob/4d0e692e72e56e827d2136b78311db0b72ada6a2/src/tests/scheduler_tests.cpp#L690-L702
- Greg Mann
On Aug. 18, 2017, 9:53 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 9:53 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/4/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183291
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262, 61189]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On Aug. 18, 2017, 8:33 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 8:33 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/5/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review184024
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262, 61189]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Aug. 18, 2017, 8:33 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 8:33 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/6/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review184100
-----------------------------------------------------------
Ship it!
LGTM, thanks Quinn! A couple small comments below, but I can fix them while committing.
src/tests/api_tests.cpp
Lines 2015 (patched)
<https://reviews.apache.org/r/61189/#comment260142>
I think you moved the wrong `scheduler` declaration - I was referring to the one in the test you added :)
I can fix this while committing.
src/tests/api_tests.cpp
Lines 2420-2422 (patched)
<https://reviews.apache.org/r/61189/#comment260143>
Ah sorry, let's place this block directly after the `AWAIT_READY(event)` call. I can do this while committing.
src/tests/api_tests.cpp
Lines 2421-2422 (patched)
<https://reviews.apache.org/r/61189/#comment260144>
Fits on one line. I can fix this while committing.
- Greg Mann
On Aug. 19, 2017, 3:33 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/6/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183318
-----------------------------------------------------------
Failed to apply patch!
Reviews applied: [61262, 61189]
Logs available here: http://104.210.40.105/logs/master/61189
- Mesos Reviewbot Windows
On Aug. 19, 2017, 3:33 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/5/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review184021
-----------------------------------------------------------
Failed to apply patch!
Reviews applied: [61262, 61189]
Logs available here: http://104.210.40.105/logs/master/61189
- Mesos Reviewbot Windows
On Aug. 19, 2017, 3:33 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/6/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183429
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61262, 61189]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Aug. 18, 2017, 8:33 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 8:33 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/5/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Quinn Leng <qu...@gmail.com>.
> On 八月 23, 2017, 12:29 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 2214 (patched)
> > <https://reviews.apache.org/r/61189/diff/5/?file=1800370#file1800370line2214>
> >
> > Since you don't modify these flags at all, you can get rid of this. If you start the master without a flags parameter, it will use the flags produced by `CreateMasterFlags()` by default.
This masterFlags because we are using the "masterFlags.allocation_interval" in later section.
> On 八月 23, 2017, 12:29 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 2222 (patched)
> > <https://reviews.apache.org/r/61189/diff/5/?file=1800370#file1800370line2222>
> >
> > Can you move this down where you register the `connected` expectation on the scheduler?
I don't quite understand it, this is pretty close to the connected expectation, why we need to move it?
- Quinn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183548
-----------------------------------------------------------
On 八月 19, 2017, 3:33 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated 八月 19, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/5/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183548
-----------------------------------------------------------
src/master/master.cpp
Lines 9694 (patched)
<https://reviews.apache.org/r/61189/#comment259564>
Indent 4 more spaces. Here and below.
src/master/master.cpp
Lines 9766-9767 (patched)
<https://reviews.apache.org/r/61189/#comment259559>
Could you make the indentation here consistent with what's above?
src/tests/api_tests.cpp
Lines 2192 (patched)
<https://reviews.apache.org/r/61189/#comment259565>
s/Events/Event/
src/tests/api_tests.cpp
Lines 2214 (patched)
<https://reviews.apache.org/r/61189/#comment259570>
Since you don't modify these flags at all, you can get rid of this. If you start the master without a flags parameter, it will use the flags produced by `CreateMasterFlags()` by default.
src/tests/api_tests.cpp
Lines 2222 (patched)
<https://reviews.apache.org/r/61189/#comment259574>
Can you move this down where you register the `connected` expectation on the scheduler?
src/tests/api_tests.cpp
Lines 2285 (patched)
<https://reviews.apache.org/r/61189/#comment259575>
You can get rid of this and then just do
```
call.mutable_subscribe()->mutable_framework_info()->CopyFrom(frameworkInfo);
```
src/tests/api_tests.cpp
Lines 2334 (patched)
<https://reviews.apache.org/r/61189/#comment259579>
You can make this `ASSERT_EQ`, since the following line would likely crash hard if this event is of the wrong type.
src/tests/api_tests.cpp
Lines 2369 (patched)
<https://reviews.apache.org/r/61189/#comment259583>
Let's make these v1 TaskInfos instead of unversioned.
src/tests/api_tests.cpp
Lines 2422-2434 (patched)
<https://reviews.apache.org/r/61189/#comment259587>
Let's move this block directly below the `AWAIT_READY(update);` above on L2404.
src/tests/api_tests.cpp
Lines 2457 (patched)
<https://reviews.apache.org/r/61189/#comment259589>
s/execTask1/execTask2/
src/tests/api_tests.cpp
Lines 2490 (patched)
<https://reviews.apache.org/r/61189/#comment259591>
I know we removed it previously, but let's add a line doing `EXPECT_TRUE(event.isPending());` after this. This will help ensure that there wasn't a heartbeat event waiting on the reader even before we launched task2.
src/tests/api_tests.cpp
Lines 2496-2497 (patched)
<https://reviews.apache.org/r/61189/#comment259590>
Could you add a comment here explaining what we're doing. Something like:
"If the next event is a heartbeat, then we know that the TASK_ADDED event for `task2` was filtered correctly."
- Greg Mann
On Aug. 19, 2017, 3:33 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/5/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/
-----------------------------------------------------------
(Updated Aug. 19, 2017, 3:33 a.m.)
Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
Bugs: MESOS-7785
https://issues.apache.org/jira/browse/MESOS-7785
Repository: mesos
Description
-------
Added authorization filtering for the V1 operator events on the
master, the subscriber should only receive events that are
authorized based on their principal and ACLs. Since the authorizer
is called for every event emitted on the stream, the change of
authorization rule will affect events that the subscribers can
receive.
Diffs (updated)
-----
src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
Diff: https://reviews.apache.org/r/61189/diff/5/
Changes: https://reviews.apache.org/r/61189/diff/4-5/
Testing
-------
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
Thanks,
Quinn Leng
Re: Review Request 61189: Added authorization for V1 events.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183268
-----------------------------------------------------------
Bad patch!
Reviews applied: [61189, 61262]
Failed command: python support/apply-reviews.py -n -r 61262
Error:
error: patch failed: include/mesos/master/master.proto:484
error: include/mesos/master/master.proto: patch does not apply
error: patch failed: include/mesos/v1/master/master.proto:484
error: include/mesos/v1/master/master.proto: patch does not apply
error: patch failed: src/master/http.cpp:855
error: src/master/http.cpp: patch does not apply
error: patch failed: src/master/master.hpp:1829
error: src/master/master.hpp: patch does not apply
error: patch failed: src/tests/api_tests.cpp:73
error: src/tests/api_tests.cpp: patch does not apply
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/254/console
- Mesos Reviewbot Windows
On Aug. 18, 2017, 9:53 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 9:53 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/4/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/
-----------------------------------------------------------
(Updated Aug. 18, 2017, 9:53 p.m.)
Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
Bugs: MESOS-7785
https://issues.apache.org/jira/browse/MESOS-7785
Repository: mesos
Description
-------
Added authorization filtering for the V1 operator events on the
master, the subscriber should only receive events that are
authorized based on their principal and ACLs. Since the authorizer
is called for every event emitted on the stream, the change of
authorization rule will affect events that the subscribers can
receive.
Diffs (updated)
-----
src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
Diff: https://reviews.apache.org/r/61189/diff/4/
Changes: https://reviews.apache.org/r/61189/diff/3-4/
Testing
-------
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
Thanks,
Quinn Leng
Re: Review Request 61189: Added authorization for V1 events.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183242
-----------------------------------------------------------
src/master/master.hpp
Lines 1832-1833 (original), 1835-1841 (patched)
<https://reviews.apache.org/r/61189/#comment259263>
It looks like in this file, the indentation style we follow more often is:
```
Subscriber(Master* _master,
const HttpConnection& _http,
const Option<process::http::authentication::Principal> _principal)
: master(_master),
http(_http),
principal(_principal) {}
```
src/master/master.hpp
Lines 1848-1851 (patched)
<https://reviews.apache.org/r/61189/#comment259271>
Can we pass these by const ref?
- Greg Mann
On Aug. 17, 2017, 10:59 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2017, 10:59 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/3/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Quinn Leng <qu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/
-----------------------------------------------------------
(Updated Aug. 17, 2017, 10:59 p.m.)
Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
Bugs: MESOS-7785
https://issues.apache.org/jira/browse/MESOS-7785
Repository: mesos
Description (updated)
-------
Added authorization filtering for the V1 operator events on the
master, the subscriber should only receive events that are
authorized based on their principal and ACLs. Since the authorizer
is called for every event emitted on the stream, the change of
authorization rule will affect events that the subscribers can
receive.
Diffs (updated)
-----
src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
Diff: https://reviews.apache.org/r/61189/diff/3/
Changes: https://reviews.apache.org/r/61189/diff/2-3/
Testing
-------
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
Thanks,
Quinn Leng
Re: Review Request 61189: Added authorization for V1 events.
Posted by Quinn Leng <qu...@gmail.com>.
> On 八月 16, 2017, 9:21 a.m., Alexander Rojas wrote:
> > Can you also update the summary to mention that it only affects the `SUBSCRIBE` event.
The authorization filtering does affect othre events. And I have updated the summary to make it more specific, thanks.
- Quinn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183027
-----------------------------------------------------------
On 八月 19, 2017, 3:33 a.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated 八月 19, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8
> src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6
>
>
> Diff: https://reviews.apache.org/r/61189/diff/5/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review183027
-----------------------------------------------------------
Can you also update the summary to mention that it only affects the `SUBSCRIBE` event.
- Alexander Rojas
On July 27, 2017, 8:25 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 8:25 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
>
>
> Diffs
> -----
>
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
> src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/2/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Quinn Leng <qu...@gmail.com>.
> On Aug. 8, 2017, 9:36 a.m., Alexander Rojas wrote:
> > src/master/master.cpp
> > Lines 9547-9569 (patched)
> > <https://reviews.apache.org/r/61189/diff/2/?file=1785310#file1785310line9547>
> >
> > I don't think this part should be done as it is. Consider the case when you have an `Acceptor` which uses a security module which connects to LDAP for ACLs.
> >
> > There is a delay for each request you make. It would be wasteful to create an Acceptor which you will later not use.
After discussion in the #ship-mesos-api-filter channel, we decide to keep the logic. Thus the authorizer is called for every events emitted by the stream.
> On Aug. 8, 2017, 9:36 a.m., Alexander Rojas wrote:
> > src/master/master.cpp
> > Lines 9547-9569 (patched)
> > <https://reviews.apache.org/r/61189/diff/2/?file=1785310#file1785310line9547>
> >
> > In fact, why not having one acceptor for each event, something like:
> >
> > ```c++
> > // This code doesn't necesarily compile.
> >
> > class EventTaskAddedAcceptor
> > {
> > public:
> > static Owned<EventTaskAddedAcceptor> create(Principal, Authorizer);
> >
> > bool accept(Task, FrameworkInfo);
> >
> > private:
> > EventTaskAddedAcceptor(
> > Owned<AuthorizationAcceptor> authorizeTask,
> > Owned<AuthorizationAcceptor> authorizeFramework);
> >
> > Owned<AuthorizationAcceptor> authorizeTask_;
> > Owned<AuthorizationAcceptor> authorizeFramework_;
> > };
> >
> > Owned<EventTaskAddedAcceptor> EventTaskAddedAcceptor::create(
> > Principal principal,
> > Authorizer authorizer)
> > {
> > Future<Owned<AuthorizationAcceptor>> authorizeRole =
> > AuthorizationAcceptor::create(
> > subscriber->principal,
> > subscriber->master->authorizer,
> > authorization::VIEW_ROLE);
> >
> > Future<Owned<AuthorizationAcceptor>> authorizeFramework =
> > AuthorizationAcceptor::create(
> > subscriber->principal,
> > subscriber->authorizer,
> > authorization::VIEW_FRAMEWORK);
> >
> > return collect(authorizeRole, authorizeFramework)
> > .then([](tuple<Owned<AuthorizationAcceptor>,
> > Owned<AuthorizationAcceptor>> acceptors) {
> > return new EventTaskAddedAcceptor(acceptors[0], acceptors[1]);
> > });
> > }
> >
> > EventTaskAddedAcceptor::EventTaskAddedAcceptor(
> > Owned<AuthorizationAcceptor> authorizeTask,
> > Owned<AuthorizationAcceptor> authorizeFramework)
> > : authorizeTask_(authorizeTask),
> > authorizeFramework_(authorizeFramework)
> > {
> > }
> >
> > bool EventTaskAddedAcceptor::accept(Task task, FrameworkInfo info)
> > {
> > return authorizeTask->accept(task, info) &&
> > authorizeFramework->accept(info);
> > }
> > ```
> >
> > This way you only need one acceptor per event and you hid the details on how the authorization is made inside the acceptor API and remove that code from the caller.
Given our decision to call authorizer for every event, this combination doesn't bring in much benefit.
- Quinn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review182374
-----------------------------------------------------------
On Aug. 17, 2017, 10:59 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2017, 10:59 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
>
>
> Diffs
> -----
>
> src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54
> src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995
> src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99
> src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/3/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review182374
-----------------------------------------------------------
src/master/master.hpp
Lines 899 (patched)
<https://reviews.apache.org/r/61189/#comment258263>
Why not making the principal part of the `HttpConnection`? any reason in particular
src/master/master.hpp
Line 1831 (original), 1834-1835 (patched)
<https://reviews.apache.org/r/61189/#comment258265>
Why passing the `master` and the `authorizer` if one can extract the latter from the former.
In fact `master.cpp` line 9550 you do `subscriber->master->authorizer` but all the other ones use `subcriber->authorizer`.
Let's get consistent there or add a comment.
src/master/master.cpp
Lines 9547-9569 (patched)
<https://reviews.apache.org/r/61189/#comment258266>
I don't think this part should be done as it is. Consider the case when you have an `Acceptor` which uses a security module which connects to LDAP for ACLs.
There is a delay for each request you make. It would be wasteful to create an Acceptor which you will later not use.
src/master/master.cpp
Lines 9547-9569 (patched)
<https://reviews.apache.org/r/61189/#comment258268>
In fact, why not having one acceptor for each event, something like:
```c++
// This code doesn't necesarily compile.
class EventTaskAddedAcceptor
{
public:
static Owned<EventTaskAddedAcceptor> create(Principal, Authorizer);
bool accept(Task, FrameworkInfo);
private:
EventTaskAddedAcceptor(
Owned<AuthorizationAcceptor> authorizeTask,
Owned<AuthorizationAcceptor> authorizeFramework);
Owned<AuthorizationAcceptor> authorizeTask_;
Owned<AuthorizationAcceptor> authorizeFramework_;
};
Owned<EventTaskAddedAcceptor> EventTaskAddedAcceptor::create(
Principal principal,
Authorizer authorizer)
{
Future<Owned<AuthorizationAcceptor>> authorizeRole =
AuthorizationAcceptor::create(
subscriber->principal,
subscriber->master->authorizer,
authorization::VIEW_ROLE);
Future<Owned<AuthorizationAcceptor>> authorizeFramework =
AuthorizationAcceptor::create(
subscriber->principal,
subscriber->authorizer,
authorization::VIEW_FRAMEWORK);
return collect(authorizeRole, authorizeFramework)
.then([](tuple<Owned<AuthorizationAcceptor>,
Owned<AuthorizationAcceptor>> acceptors) {
return new EventTaskAddedAcceptor(acceptors[0], acceptors[1]);
});
}
EventTaskAddedAcceptor::EventTaskAddedAcceptor(
Owned<AuthorizationAcceptor> authorizeTask,
Owned<AuthorizationAcceptor> authorizeFramework)
: authorizeTask_(authorizeTask),
authorizeFramework_(authorizeFramework)
{
}
bool EventTaskAddedAcceptor::accept(Task task, FrameworkInfo info)
{
return authorizeTask->accept(task, info) &&
authorizeFramework->accept(info);
}
```
This way you only need one acceptor per event and you hid the details on how the authorization is made inside the acceptor API and remove that code from the caller.
- Alexander Rojas
On July 27, 2017, 8:25 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 8:25 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
>
>
> Diffs
> -----
>
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
> src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/2/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>
Re: Review Request 61189: Added authorization for V1 events.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61189/#review182380
-----------------------------------------------------------
There is a weakness in the implementation here. If the ACLs of a user change after subscribing, a subscriber could end with an inconsistent view of the state.
For example, when a user subscribes to the event stream at T0, he has no right to see framework _foo_, so it is filtered out of the response. Then at T1 he is granted access to _foo_ but he still cannot see it since we don’t react to changes to ACLs, later at T2 _foo_ launches a task which the subscriber can see, then he receives the event of a new task from a framework he doesn’t know.
What we discussed would be the best solution is to cache the set of approves a subscriber used when it did so and use the same ACLs for the life of the connection.
- Alexander Rojas
On July 27, 2017, 8:25 p.m., Quinn Leng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 8:25 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
>
>
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization filtering for V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
>
>
> Diffs
> -----
>
> src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70
> src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043
> src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32
> src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f
>
>
> Diff: https://reviews.apache.org/r/61189/diff/2/
>
>
> Testing
> -------
>
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" --verbose --gtest_repeat=100 --gtest_break_on_failure
>
>
> Thanks,
>
> Quinn Leng
>
>