You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2015/11/06 00:11:55 UTC
Re: Review Request 37110: Enabled the Authorizer to handle
Reserve/Unreserve ACLs.
> 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
>
>