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



Looks great! Just some minor points, and then we need to settle on how the 
ACCESS_ENDPOINT API/ACLs will work. Once that's settled, it should be easy to 
land this patch quickly.


src/slave/http.cpp (line 613)
<https://reviews.apache.org/r/46319/#comment193677>

    s/context/pid/? What is the 'context' referring to?



src/slave/http.cpp (line 616)
<https://reviews.apache.org/r/46319/#comment193676>

    authorizeEndpoint() needs to be told that this is a GET request, since some 
endpoints may have different ACLs for different verbs. We'll have to resolve 
that in the /flags authz patch.



src/slave/http.cpp (lines 620 - 622)
<https://reviews.apache.org/r/46319/#comment193679>

    Not sure why you reversed the boolean/order here, but ok.



src/slave/http.cpp (line 626)
<https://reviews.apache.org/r/46319/#comment193684>

    s/discard/discarded/



src/slave/http.cpp (lines 642 - 643)
<https://reviews.apache.org/r/46319/#comment193687>

    Why change the wrapping/tabbing? If you were going to drop the first 
parameter down to the next line, then each parameter must be on its own line.
    I would just leave the wrapping as is, personally.



src/slave/slave.hpp (lines 471 - 472)
<https://reviews.apache.org/r/46319/#comment193690>

    How did this come up? The original `_statistics()` is not static, and it 
had no issues.



src/tests/slave_tests.cpp (line 1895)
<https://reviews.apache.org/r/46319/#comment193698>

    Should we create a TYPED_TEST that tests this ACL in the local authorizer 
(direct and as a module), or do we only need these tests for one (of the many) 
ACCESS_ENDPOINT_WITH_PATH endpoints?



src/tests/slave_tests.cpp (lines 1926 - 1927)
<https://reviews.apache.org/r/46319/#comment193696>

    I would expect you to await/validate the response after validating the 
request.



src/tests/slave_tests.cpp (line 1935)
<https://reviews.apache.org/r/46319/#comment193697>

    These two parameters need to be on different lines if the whole command 
doesn't fit on one line. I'd be ok though if you moved the first param up to 
the `EXPECT_EQ(` line



src/tests/slave_tests.cpp (line 1956)
<https://reviews.apache.org/r/46319/#comment193693>

    Shadows the `agent` variable on line 1905



src/tests/slave_tests.cpp (lines 1959 - 1965)
<https://reviews.apache.org/r/46319/#comment193694>

    Can't you leave these out as default?


- Adam B


On April 18, 2016, 6:42 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> -----------------------------------------------------------
> 
> (Updated April 18, 2016, 6:42 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
>     https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added authorization to agents' `/statistics` endpoints.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> -------
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Reply via email to