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 58964: Added authorization support for operator endpoints.
Date Wed, 17 May 2017 09:57:49 GMT

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



Didn't get to the tests, but here's something to start with.


include/mesos/authorizer/acls.proto
Line 354 (original), 354 (patched)
<https://reviews.apache.org/r/58964/#comment248672>

    Should this be `agents` plural?



include/mesos/authorizer/acls.proto
Lines 364 (patched)
<https://reviews.apache.org/r/58964/#comment248660>

    Why do we think `machines` is the entity we want to authorize on? What if we decide we
want to authorize on `schedules` in the future? This required field isn't very flexible.
    Also, why not `agents` like in `RegisterAgent` above. Is there a distinction between agents
and machines?



include/mesos/authorizer/acls.proto
Lines 377 (patched)
<https://reviews.apache.org/r/58964/#comment248661>

    s/in/on/



include/mesos/authorizer/acls.proto
Lines 387 (patched)
<https://reviews.apache.org/r/58964/#comment248662>

    s/in/on/



include/mesos/authorizer/acls.proto
Lines 391 (patched)
<https://reviews.apache.org/r/58964/#comment248663>

    Missed the comment on this object entity



include/mesos/authorizer/acls.proto
Lines 394-395 (patched)
<https://reviews.apache.org/r/58964/#comment248664>

    s/status of  maintenance in/maintenance status of/



include/mesos/authorizer/authorizer.proto
Lines 58 (patched)
<https://reviews.apache.org/r/58964/#comment248673>

    Unused?!?



include/mesos/authorizer/authorizer.proto
Lines 204 (patched)
<https://reviews.apache.org/r/58964/#comment248665>

    s/he's/is/g in all of these comments



include/mesos/authorizer/authorizer.proto
Lines 213 (patched)
<https://reviews.apache.org/r/58964/#comment248666>

    "on all nodes or none" in each of these comments



include/mesos/authorizer/authorizer.proto
Lines 221 (patched)
<https://reviews.apache.org/r/58964/#comment248667>

    "of all nodes or none"



src/authorizer/local/authorizer.cpp
Lines 391-398 (original), 391-406 (patched)
<https://reviews.apache.org/r/58964/#comment248668>

    Why not merge these all into a single case body?



src/authorizer/local/authorizer.cpp
Lines 866-867 (original), 879-885 (patched)
<https://reviews.apache.org/r/58964/#comment248669>

    These should probably be alpha-sorted as well



src/authorizer/local/authorizer.cpp
Lines 1024-1025 (original), 1042-1048 (patched)
<https://reviews.apache.org/r/58964/#comment248670>

    These should probably be alpha-sorted into the list above.


- Adam B


On May 12, 2017, 5:51 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58964/
> -----------------------------------------------------------
> 
> (Updated May 12, 2017, 5:51 a.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7415
>     https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds the actions `UPDATE_MAINTENANCE_SCHEDULE`,
> `GET_MAINTENANCE_SCHEDULE`, `START_MAINTENANCE`, `STOP_MAINTENANCE`
> and `GET_MAINTENANCE_STATUS` to the authorizer API as well as the
> necesary code to handle these new actions.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db 
>   include/mesos/authorizer/authorizer.proto c9184d151befa4cea9bdebb36a315c760e6424b2

>   src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101 
>   src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0 
> 
> 
> Diff: https://reviews.apache.org/r/58964/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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