You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/08/05 11:58:54 UTC
Review Request 37110: Enabled the Authorizer to handle
Reserve/Unreserve ACLs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37110/
-----------------------------------------------------------
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f
src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc
src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87
src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0
Diff: https://reviews.apache.org/r/37110/diff/
Testing
-------
Added tests to `src/tests/authorization_tests.cpp` + `make check`
Thanks,
Michael Park
Re: Review Request 37110: Enabled the Authorizer to handle
Reserve/Unreserve ACLs.
Posted by Till Toenshoff <to...@me.com>.
> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, lines 407-410
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line407>
> >
> > I'll be honest with you: this looks to me upside down :)
> >
> > I'd have: ANY principal can unreserve NONE's resources.
> > In fact, what would happen if one did:
> > ```
> > acl4->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
> > acl4->mutable_reserver_principals()->set_type(mesos::ACL::Entity::NONE);
> > ```
> >
> > And what happens if I do:
> > ```
> > acl4->mutable_principals()->set_type(mesos::ACL::Entity::NONE);
> > acl4->mutable_reserver_principals()->set_type(mesos::ACL::Entity::NONE);
> > ```
Seems fixed in https://reviews.apache.org/r/39986
> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, lines 427-430
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line427>
> >
> > isn't this identical to the case above?
> > if not, care to add a comment as to what differs?
https://reviews.apache.org/r/39986
> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, line 436
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line436>
> >
> > can we add a test for `ops` trying to unreserve multiple principals' resources? "foo", "bar" and "quz"'s
> > (is this even allowed? if not, let's make sure it fails)
https://reviews.apache.org/r/39986
> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, line 397
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line397>
> >
> > the comment here is wrong
https://reviews.apache.org/r/39986
> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, lines 376-377
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line376>
> >
> > what happens if the request comes from `foo` and `quz`? do I still get to reserve the resource?
> > Should `quz` get it? shouldn't it?
> >
> > Can you please add a few comments in the methods above (and correspondingly in the tests?)
> >
> > When it comes to security / ACLs, always best to have enough info for the guys who will have to solve issues; that's usually never done with the luxury of time, when it comes to security :)
https://reviews.apache.org/r/39986
> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, line 360
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line360>
> >
> > not sure what the "rules" are in tests, but shouldn't you use an `Owned` or `unique_ptr` instead of the "naked" ptr?
That's what you get from the protobuf factory.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37110/#review95011
-----------------------------------------------------------
On Sept. 12, 2015, 9:30 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 9:30 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f
> src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc
> src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87
> src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0
>
> Diff: https://reviews.apache.org/r/37110/diff/
>
>
> Testing
> -------
>
> Added tests to `src/tests/authorization_tests.cpp` + `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 37110: Enabled the Authorizer to handle
Reserve/Unreserve ACLs.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37110/#review95011
-----------------------------------------------------------
src/tests/authorization_tests.cpp (line 360)
<https://reviews.apache.org/r/37110/#comment149772>
not sure what the "rules" are in tests, but shouldn't you use an `Owned` or `unique_ptr` instead of the "naked" ptr?
src/tests/authorization_tests.cpp (lines 376 - 377)
<https://reviews.apache.org/r/37110/#comment149773>
what happens if the request comes from `foo` and `quz`? do I still get to reserve the resource?
Should `quz` get it? shouldn't it?
Can you please add a few comments in the methods above (and correspondingly in the tests?)
When it comes to security / ACLs, always best to have enough info for the guys who will have to solve issues; that's usually never done with the luxury of time, when it comes to security :)
src/tests/authorization_tests.cpp (line 397)
<https://reviews.apache.org/r/37110/#comment149774>
the comment here is wrong
src/tests/authorization_tests.cpp (lines 407 - 410)
<https://reviews.apache.org/r/37110/#comment149776>
I'll be honest with you: this looks to me upside down :)
I'd have: ANY principal can unreserve NONE's resources.
In fact, what would happen if one did:
```
acl4->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
acl4->mutable_reserver_principals()->set_type(mesos::ACL::Entity::NONE);
```
And what happens if I do:
```
acl4->mutable_principals()->set_type(mesos::ACL::Entity::NONE);
acl4->mutable_reserver_principals()->set_type(mesos::ACL::Entity::NONE);
```
src/tests/authorization_tests.cpp (lines 427 - 430)
<https://reviews.apache.org/r/37110/#comment149777>
isn't this identical to the case above?
if not, care to add a comment as to what differs?
src/tests/authorization_tests.cpp (line 436)
<https://reviews.apache.org/r/37110/#comment149778>
can we add a test for `ops` trying to unreserve multiple principals' resources? "foo", "bar" and "quz"'s
(is this even allowed? if not, let's make sure it fails)
- Marco Massenzio
On Aug. 5, 2015, 9:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 9:58 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f
> src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc
> src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87
> src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0
>
> Diff: https://reviews.apache.org/r/37110/diff/
>
>
> Testing
> -------
>
> Added tests to `src/tests/authorization_tests.cpp` + `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 37110: Enabled the Authorizer to handle
Reserve/Unreserve ACLs.
Posted by Till Toenshoff <to...@me.com>.
On Sept. 10, 2015, 3:32 a.m., Michael Park wrote:
> > Also you may want a rebase as the main logic is now moved to src/authorizer/local/authorizer.cpp
Seems fixed in https://reviews.apache.org/r/39986
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37110/#review98339
-----------------------------------------------------------
On Sept. 12, 2015, 9:30 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 9:30 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f
> src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc
> src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87
> src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0
>
> Diff: https://reviews.apache.org/r/37110/diff/
>
>
> Testing
> -------
>
> Added tests to `src/tests/authorization_tests.cpp` + `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 37110: Enabled the Authorizer to handle
Reserve/Unreserve ACLs.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37110/#review98339
-----------------------------------------------------------
src/tests/authorization_tests.cpp (line 397)
<https://reviews.apache.org/r/37110/#comment154752>
// "bar" principal cannot unreserve anyone's resources.
Also you may want a rebase as the main logic is now moved to src/authorizer/local/authorizer.cpp
- Guangya Liu
On 八月 5, 2015, 9:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> -----------------------------------------------------------
>
> (Updated 八月 5, 2015, 9:58 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f
> src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc
> src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87
> src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0
>
> Diff: https://reviews.apache.org/r/37110/diff/
>
>
> Testing
> -------
>
> Added tests to `src/tests/authorization_tests.cpp` + `make check`
>
>
> Thanks,
>
> Michael Park
>
>