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

Reply via email to