----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39989/#review106534 -----------------------------------------------------------
src/master/master.cpp (lines 3028 - 3040) <https://reviews.apache.org/r/39989/#comment165247> Hum, this looks problematic to me. The authorization results are stored in 'futures'. The ordering in 'futures' is important because we'll read the authorization results in `_accept` based on the ordering. However, you call `drop` here and skip the authorization. That means the ordering invariant is no longer hold. I would suggest that you perform operation validation in authorizeReserveResources and returns a Failure if validation fails. In that way, you can drop the operation in `_accept` if authorization returns a failure. Please also add a comment about the fact that ordering in 'futures' is very important. Also, you may want to add a test to test the cases where RESERVE and LAUNCH are in one single `accept` call. - Jie Yu On Nov. 13, 2015, 11:17 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39989/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2015, 11:17 p.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 7bac0fea4bcd040307fdfdcd002387d5baee46d1 > src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 > > 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 > >
