You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2017/04/04 09:21:33 UTC
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/
-----------------------------------------------------------
(Updated April 4, 2017, 11:21 a.m.)
Review request for mesos, Adam B and Benjamin Bannier.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Adds mechanisms to support authorization of hierarchical roles,
that is, it allows operators to write ACLs of the form `role/%`
which will enforce the rule for any nested role, e.g. `role/a`,
`role/b` and such.
Diffs (updated)
-----
src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
Diff: https://reviews.apache.org/r/57473/diff/5/
Changes: https://reviews.apache.org/r/57473/diff/4-5/
Testing
-------
`make check`
Thanks,
Alexander Rojas
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/#review171379
-----------------------------------------------------------
src/authorizer/local/authorizer.cpp
Lines 401 (patched)
<https://reviews.apache.org/r/57473/#comment244272>
"a specialized"
src/authorizer/local/authorizer.cpp
Lines 573 (patched)
<https://reviews.apache.org/r/57473/#comment244273>
Seems a bit inconsistent: we use `*(object->value)` elsewhere, but we omit the extra set of parentheses here.
src/authorizer/local/authorizer.cpp
Line 895 (original)
<https://reviews.apache.org/r/57473/#comment244274>
What do you think about splitting the `break` changes into a separate review?
- Neil Conway
On April 4, 2017, 9:21 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> -----------------------------------------------------------
>
> (Updated April 4, 2017, 9:21 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
>
>
> Diffs
> -----
>
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/57473/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/#review173166
-----------------------------------------------------------
Ship it!
Ship It!
- Adam B
On April 11, 2017, 3:58 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> -----------------------------------------------------------
>
> (Updated April 11, 2017, 3:58 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Bannier.
>
>
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
>
>
> Diffs
> -----
>
> src/authorizer/local/authorizer.cpp 1c1f912794cfe61112a0e513b217ba3a755f35f1
>
>
> Diff: https://reviews.apache.org/r/57473/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/
-----------------------------------------------------------
(Updated April 11, 2017, 12:58 p.m.)
Review request for mesos, Adam B and Benjamin Bannier.
Bugs: MESOS-7026
https://issues.apache.org/jira/browse/MESOS-7026
Repository: mesos
Description
-------
Adds mechanisms to support authorization of hierarchical roles,
that is, it allows operators to write ACLs of the form `role/%`
which will enforce the rule for any nested role, e.g. `role/a`,
`role/b` and such.
Diffs (updated)
-----
src/authorizer/local/authorizer.cpp 1c1f912794cfe61112a0e513b217ba3a755f35f1
Diff: https://reviews.apache.org/r/57473/diff/7/
Changes: https://reviews.apache.org/r/57473/diff/6-7/
Testing
-------
`make check`
Thanks,
Alexander Rojas
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/
-----------------------------------------------------------
(Updated April 11, 2017, 1:48 a.m.)
Review request for mesos, Adam B and Benjamin Bannier.
Bugs: MESOS-7026
https://issues.apache.org/jira/browse/MESOS-7026
Repository: mesos
Description
-------
Adds mechanisms to support authorization of hierarchical roles,
that is, it allows operators to write ACLs of the form `role/%`
which will enforce the rule for any nested role, e.g. `role/a`,
`role/b` and such.
Diffs
-----
src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
Diff: https://reviews.apache.org/r/57473/diff/6/
Testing
-------
`make check`
Thanks,
Alexander Rojas
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On April 11, 2017, 8:21 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp
> > Line 458 (original), 405 (patched)
> > <https://reviews.apache.org/r/57473/diff/5/?file=1684280#file1684280line467>
> >
> > Why isn't this a `return Error();` too?
Originally we used the `UNKNOWN` in case someone was using an old authorizer in a newer mesos (without newer actions). In that case we will return false, but I can change that I guess.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/#review171392
-----------------------------------------------------------
On April 11, 2017, 10:48 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> -----------------------------------------------------------
>
> (Updated April 11, 2017, 10:48 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Bannier.
>
>
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
>
>
> Diffs
> -----
>
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/57473/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/#review171392
-----------------------------------------------------------
Minor comments, but it looks pretty close to shippable to me.
src/authorizer/local/authorizer.cpp
Line 202 (original), 202 (patched)
<https://reviews.apache.org/r/57473/#comment244300>
If GET_ENDPOINT_WITH_PATH is the only one now, then you can update the comment here to be less generic.
src/authorizer/local/authorizer.cpp
Line 458 (original), 405 (patched)
<https://reviews.apache.org/r/57473/#comment244299>
Why isn't this a `return Error();` too?
src/authorizer/local/authorizer.cpp
Lines 517-519 (patched)
<https://reviews.apache.org/r/57473/#comment244301>
Why are these the only checks with the `!= nullptr`? These checks weren't written that way before, and now we're inconsistent. I'd leave it out unless there's some reason to include it everywhere.
src/authorizer/local/authorizer.cpp
Lines 601 (patched)
<https://reviews.apache.org/r/57473/#comment244473>
Unnecessary `break` after a `return`
src/authorizer/local/authorizer.cpp
Lines 677 (patched)
<https://reviews.apache.org/r/57473/#comment244474>
Sounds like reason for an assert, not a mere comment.
- Adam B
On April 10, 2017, 3:11 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> -----------------------------------------------------------
>
> (Updated April 10, 2017, 3:11 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
>
>
> Diffs
> -----
>
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/57473/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 57473: Added support for authorization of
Hierachical roles.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57473/
-----------------------------------------------------------
(Updated April 10, 2017, 12:11 p.m.)
Review request for mesos, Adam B and Benjamin Bannier.
Repository: mesos
Description
-------
Adds mechanisms to support authorization of hierarchical roles,
that is, it allows operators to write ACLs of the form `role/%`
which will enforce the rule for any nested role, e.g. `role/a`,
`role/b` and such.
Diffs (updated)
-----
src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
Diff: https://reviews.apache.org/r/57473/diff/6/
Changes: https://reviews.apache.org/r/57473/diff/5-6/
Testing
-------
`make check`
Thanks,
Alexander Rojas