> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote: > > src/master/master.cpp, lines 3041-3043 > > <https://reviews.apache.org/r/39989/diff/12/?file=1150229#file1150229line3041> > > > > We validate later on that in `principal` is `None`, reserve is aborted. > > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve > > resources. So the question is: do we need to check this first and only then > > proceed with authz? > > Jie Yu wrote: > Alex, from the impl. perspective, we thought about that in the earlier > iteration, and found that it's quite difficult to do it cleanly given the > current structure of the code. We want validation to be done at the same > place (`Master::_accept`). > > ALso, we had some discussion on whether 'principal' in ReservationInfo > needs to be required or not (MESOS-3064). In the future, we might want to > make it 'optional' so that a framework without principal can also reserve > resources (it's reserved resources can be unreserved by anyone). > > So I would suggest we keep the current structure and add a comment here > saying that: currently, if framework's principal is not specified, the > operation validation will fail in `_accept` even thought authorization might > succeed.
I added a comment along the lines of Jie's suggestion. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39989/#review108654 ----------------------------------------------------------- On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39989/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2015, 9:41 a.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff. > > > Bugs: MESOS-3062 > https://issues.apache.org/jira/browse/MESOS-3062 > > > Repository: mesos > > > Description > ------- > > Added framework authorization for dynamic reservation. > Note: this review is continued from https://reviews.apache.org/r/37127/ > > > Diffs > ----- > > src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 > src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 > > Diff: https://reviews.apache.org/r/39989/diff/ > > > Testing > ------- > > This is the fifth in a chain of 5 patches. New reservation tests were added > to `reservation_tests.cpp` to validate the authentication of framework > reserve and unreserve operations using ACLs. `make check` was run to test > after all patches were applied. > > > Thanks, > > Greg Mann > >
