> 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. > > Greg Mann wrote: > 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 Mann wrote: > FYI, I'm having trouble with the HTTP endpoint callbacks > `Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying > things like the following to get `Master::authorizeReserveResources()` to > pass the full `Future<bool>` return value to `_operation()` so that the > validation error can be caught there: > > ```cpp > return master->authorizeUnreserveResources(operation, principal, NULL) > .onAny(defer(master->self(), > _operation, > slaveId, > resources, > operation, > lambda::_1)); > ``` > > > or > > > ```cpp > lambda::function<Future<Response>(Future<bool>)> _reserve = > lambda::bind( > &Master::Http::_operation, > *this, > slaveId, > resources.flatten(), > operation, > lambda::_1); > > return master->authorizeUnreserveResources(operation, principal, NULL) > .onAny(defer(master->self(), _reserve)) > ``` > > > where `_operation` is now accepting the entire offer operation as well as > a third parameter of type `Framework*` so that the operation can be dropped > if necessary.
But getting errors in the instantiation of `defer()` in the former case, or `lambda::bind()` in the latter... - 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 > >
