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 45922: Added agent authorization flags.
Date Tue, 12 Apr 2016 09:31:45 GMT

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


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