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



Great start! Some concerns:
- We need a JIRA to make sure we document which endpoints require authorization 
for what.
- Handling multiple HTTP verbs that authorize differently
- Handling custom base urls (e.g. reverse proxies)
- Should use TYPED_TEST_CASE to test the module API as well


docs/configuration.md (line 893)
<https://reviews.apache.org/r/46203/#comment193230>

    acls.proto



docs/configuration.md (line 897)
<https://reviews.apache.org/r/46203/#comment193231>

    We're going to have to start documenting which endpoints can/must be 
authorized this way, similar to how Joerg added authentication to endpoint 
help. Can you create a new JIRA for this?



include/mesos/authorizer/acls.proto (line 150)
<https://reviews.apache.org/r/46203/#comment193233>

    s/for given paths/at the given paths/?



include/mesos/authorizer/acls.proto (line 151)
<https://reviews.apache.org/r/46203/#comment193232>

    Let's consider calling this `GetEndpoint`, to match the HTTP verb? There 
may be some users that are allowed to GET the endpoint at a particular path, 
but cannot POST to it.



include/mesos/authorizer/acls.proto (line 195)
<https://reviews.apache.org/r/46203/#comment193234>

    Why the blank line?



include/mesos/authorizer/authorizer.proto (lines 68 - 70)
<https://reviews.apache.org/r/46203/#comment193235>

    This won't be agent-only for long. Or do you imagine a separate enum for 
ACCESS_MASTER_ENDPOINT_WITH_PATH? Maybe "agent-only" is better as an //end of 
line comment, so it doesn't impact enum ordering
    If we're going to keep adding master and agent ACLs in the same enum, I'd 
rather keep them in enum id order, so we don't accidentally duplicate enum ids.
    If we do want to have master only, agent only, and both sections, we should 
start them at 0, 100, and 200; or 0, 1000, and 2000 to leave room for expansion.



src/slave/flags.cpp (line 457)
<https://reviews.apache.org/r/46203/#comment193236>

    acls.proto



src/slave/flags.cpp (line 464)
<https://reviews.apache.org/r/46203/#comment193237>

    Shouldn't some of these be forward-slashes, e.g. /b and /c?
    Did you try running the --help after building your changes, to see what 
this looks like in a console?



src/slave/http.cpp (line 354)
<https://reviews.apache.org/r/46203/#comment193241>

    Forgive my lambda-ignorance here, but are you creating this locally scoped 
pointer just so that you can expose it to the lambda? Can you not reference 
`slave` directly?



src/slave/http.cpp (lines 658 - 660)
<https://reviews.apache.org/r/46203/#comment193247>

    Where did you come up with the magic number 3? What if we reorganize the 
operator endpoints in the (1.0) future? How will we know what the new value 
should be here?
    What if the user setup a reverse proxy (like in dcos) and these requests 
are actually coming from a different base url than expected?



src/slave/http.cpp (line 667)
<https://reviews.apache.org/r/46203/#comment193249>

    How did you know it was a GET? That's not part of the function signature.



src/tests/slave_authorization_tests.cpp (line 49)
<https://reviews.apache.org/r/46203/#comment193251>

    Any reason why these shouldn't just go in authorization_tests.cpp?



src/tests/slave_authorization_tests.cpp (line 54)
<https://reviews.apache.org/r/46203/#comment193252>

    Could you please use TYPED_TEST_CASE and TYPED_TEST so that we can test 
this with both the LocalAuthorizer directly, and the TestLocalAuthorizer module.



src/tests/slave_authorization_tests.cpp (lines 64 - 66)
<https://reviews.apache.org/r/46203/#comment193253>

    You should be setting acls.permissive = false instead



src/tests/slave_authorization_tests.cpp (line 71)
<https://reviews.apache.org/r/46203/#comment193254>

    Why do you need to start a master to access a slave's /flags?



src/tests/slave_authorization_tests.cpp (line 98)
<https://reviews.apache.org/r/46203/#comment193261>

    I'm not really convinced that this test gives us much, since it's using 
default ACLs. Can you write a comment explaining what this tests that we don't 
get from other existing tests?



src/tests/slave_authorization_tests.cpp (lines 103 - 106)
<https://reviews.apache.org/r/46203/#comment193255>

    Unnecessary, since acls.permissive=true by default.



src/tests/slave_authorization_tests.cpp (lines 110 - 114)
<https://reviews.apache.org/r/46203/#comment193258>

    This seems wrong. You don't even bother to reset the authenticator after 
you're done?
    Why do you even need to unset the authenticator when you already set 
`slaveFlags.authenticate_http = false`?



src/tests/slave_authorization_tests.cpp (line 119)
<https://reviews.apache.org/r/46203/#comment193259>

    You shouldn't need to change these



src/tests/slave_authorization_tests.cpp (lines 125 - 127)
<https://reviews.apache.org/r/46203/#comment193260>

    Won't this all fit on one line?


- Adam B


On April 19, 2016, 5:08 a.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 5:08 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
>     https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac 
>   src/authorizer/local/authorizer.cpp 
> c744a16041c2466d3839a37fbee2bf86887bf4e1 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Reply via email to