mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 46319: Added authorization to agents' `/statistics` endpoints.
Date Fri, 22 Apr 2016 08:22:27 GMT

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


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