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 Wed, 23 Aug 2017 00:29:14 GMT

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


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