mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 61189: Added authorization for V1 events.
Date Sat, 19 Aug 2017 00:17:34 GMT

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


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