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



Looks nice and clean. Three main concerns:
1. Why support multiple authorizers?
2. Don't show confusing/invalid examples
3. How will we test this? Do we need to add authz to at least one endpoint?


docs/configuration.md (line 94)
<https://reviews.apache.org/r/45922/#comment191774>

    Why would we need to support multiple authorizers? Would a particular 
request check two authorizers then and/or the results?
    Or are we somehow using one authorizer for some things and another for 
others? This could be explainable for the master's framework authz vs. http 
authz, but makes no sense for the agent.



docs/configuration.md (line 869)
<https://reviews.apache.org/r/45922/#comment191775>

    None of these examples apply to an agent. Maybe we need to implement an ACL 
(e.g. GET_ENDPOINT_WITH_PATH "/flags") before we can provide any example ACLs 
for the agent.



src/slave/flags.cpp (lines 446 - 449)
<https://reviews.apache.org/r/45922/#comment191777>

    I'd rather have no example than a confusing/invalid example. We can add an 
example after we have at least one ACL



src/slave/flags.cpp (line 777)
<https://reviews.apache.org/r/45922/#comment191778>

    I still can't imagine an agent with multiple authorizers.



src/slave/main.cpp (lines 22 - 26)
<https://reviews.apache.org/r/45922/#comment191780>

    Pretty sure `mesos/authorizer/*` goes before `mesos/master/*` alphabetically



src/slave/main.cpp (line 301)
<https://reviews.apache.org/r/45922/#comment191783>

    It seems that by naming the flag `authorizers`, you've already decided that 
we want to add support for multiple authorizers



src/tests/cluster.hpp (line 146)
<https://reviews.apache.org/r/45922/#comment191785>

    Why add this here instead of at the end? Do you expect 'authorizer' to be a 
more populer parameter than the ones after it?


- Adam B


On April 8, 2016, 7:25 a.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> -----------------------------------------------------------
> 
> (Updated April 8, 2016, 7:25 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
>     https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Reply via email to