----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44322/#review122699 -----------------------------------------------------------
A couple of quick thoughts on your WIP so far. Also, I committed the /weights endpoint, so there'll be 1 more ACL to refactor once you rebase. Sorry. include/mesos/authorizer/authorizer.hpp (line 82) <https://reviews.apache.org/r/44322/#comment184804> Can we get a comment? include/mesos/authorizer/authorizer.proto (lines 34 - 41) <https://reviews.apache.org/r/44322/#comment184803> s/AS/WITH/ s/OF/WITH/ s/FOR/WITH/ src/authorizer/local/authorizer.hpp (line 55) <https://reviews.apache.org/r/44322/#comment184805> Update comment, since it no longer applies to a block of `authorize` methods. src/master/master.cpp (line 2793) <https://reviews.apache.org/r/44322/#comment184806> Someday we may want to return something besides a bool, so that the client can get back a more meaningful error than "Forbidden", e.g. which of the N roles specified was disallowed? Any ideas how we would do that for the operator endpoints? src/master/master.cpp (line 2826) <https://reviews.apache.org/r/44322/#comment184807> When would this ever be empty? Shouldn't we have validated somewhere prior that the Reserve operation is reserving at least one resource/role? I'd think this could be a CHECK. - Adam B On March 8, 2016, 9:07 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44322/ > ----------------------------------------------------------- > > (Updated March 8, 2016, 9:07 p.m.) > > > Review request for mesos, Adam B, Joerg Schad, and Vinod Kone. > > > Bugs: MESOS-2950 > https://issues.apache.org/jira/browse/MESOS-2950 > > > Repository: mesos > > > Description > ------- > > **WIP: Do not commit** > > Implements the later [Generic Authorizer Interface v > 0.3.1](https://docs.google.com/document/d/1-XARWJFUq0r_TgRHz_472NvLZNjbqE4G8c2JL44OSMQ) > proposal. > > It still lacks some parts: > > - [ ] Doxygen in the interface. > - [ ] Updating `authorizer.md` > > Still the basic functionality is there and I don't expect it to change > much. As I mentioned, comments aren't there yet but feel free to point > where they are missed, at this point focusing in the actual content > may not be relevant as the patch may change from its current form. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.hpp > ec6c9928c55c3096c7de634f900419abbdd00886 > include/mesos/authorizer/authorizer.proto PRE-CREATION > src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 > src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 > src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 > src/authorizer/local/authorizer.hpp > 96baf77709cf721caf46b6c2c096a843c1b5d9c0 > src/authorizer/local/authorizer.cpp > 4e5c2c2869823ec957735cebfc80dc850d40f9eb > src/master/http.cpp a3ad57a1c3f8a01aa609b28c12825670bb243387 > src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f > src/master/quota_handler.cpp a41c91f10bc0eedc754425b4de1b3e184c4ffb08 > src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 > src/tests/master_authorization_tests.cpp > 29c89fb11da792c3e71eb880a19657ea225b3cc8 > src/tests/mesos.hpp 9c62833e0a64cfd62fce8cffd04f9cdd933646c8 > src/tests/mesos.cpp 395b23d32b2d03aef446858e197cb9788644eefa > src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 > > Diff: https://reviews.apache.org/r/44322/diff/ > > > Testing > ------- > > make -j4 check > > > Thanks, > > Alexander Rojas > >
