> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote: > > src/master/master.cpp, lines 3037-3049 > > <https://reviews.apache.org/r/39989/diff/7/?file=1125447#file1125447line3037> > > > > 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.
Thanks Jie, this is indeed a problem. I've implemented a solution which simply pushes a failed `Future` onto `futures` so that `_accept()` can handle the failure. The current diff includes this, as well as a new test that explicitly probes the previous bug. I like your idea of putting the validation into `authorizeReserveResources()`, especially since similar validation logic is also found in the HTTP endpoint authorization code. That requires a bit of refactoring, which I'm working on currently. Will post a new patch soon. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39989/#review106534 ----------------------------------------------------------- On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39989/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2015, 5:07 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 39a0e730b230cee73e30d831ee67d9207359ae28 > 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 > >
