-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38399/#review99042
-----------------------------------------------------------
Thanks for picking this up!
A few overall things that you missed (consider adding them to separate reviews that depend
on this on):
* The HTTP endpoints need to be authenticated likewise, see this example: https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1245-L1275
* You need to update docs/authorization.md to include the new ACL.
Also, can you add the following people as reviewers? (hartem, jvanremoortere, kaysoky)
include/mesos/authorizer/authorizer.proto (line 74)
<https://reviews.apache.org/r/38399/#comment155868>
I think you should reconsider which ACL goes where. There are 4 maintenance endpoints
(currently):
* `/maintenance/schedule` schedules machines. This probably belongs in an ACL on it's
own. (Like this new one.)
* `/machine/down` and `/machine/up` bring machines up and down. I think this falls either
in it's own ACL or together with `ShutdownFramework`. Also, it's important to note that these
two actions are *not* maintenance specific. (Implementation-wise, they are restricted to
maintenance for now though.)
* `/maintenance/status` is read-only. So it might not need to be authenticated.
include/mesos/authorizer/authorizer.proto (lines 78 - 79)
<https://reviews.apache.org/r/38399/#comment155869>
Consider renaming to `machine_ids`.
You should also consider a string representation of the MachineID protobuf. (https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L164-L167)
Both fields are important for identifying a machine. The hostname is not enough.
- Joseph Wu
On Sept. 15, 2015, 3:05 a.m., Zhiwei Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38399/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 3:05 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: mesos-2222
> https://issues.apache.org/jira/browse/mesos-2222
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ACLs for the maintenance HTTP endpoints
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp d667a52f90f970a313580446a5a006cec4b5e25b
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
> src/authorizer/local/authorizer.hpp 32de102fd588f029882ef2222121ca83a7410c65
> src/authorizer/local/authorizer.cpp 6d7da87731a438c2180cf91003e09d4aa5a1c773
> src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364
>
> Diff: https://reviews.apache.org/r/38399/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhiwei Chen
>
>
|