> On Dec. 4, 2019, 6:21 p.m., Benjamin Mahler wrote: > > src/master/authorization.cpp > > Lines 41-42 (patched) > > <https://reviews.apache.org/r/71859/diff/2/?file=2181799#file2181799line41> > > > > We try in new code to stick to the following assignment style, since it > > works automatically with moves and is also easier to read: > > > > ``` > > *actionObject.object->mutable_task_info() = task; > > *actionObject.object->mutable_framework_info() = framework; > > ```
OK, will treat this as a new code. > On Dec. 4, 2019, 6:21 p.m., Benjamin Mahler wrote: > > src/master/authorization.cpp > > Lines 44 (patched) > > <https://reviews.apache.org/r/71859/diff/2/?file=2181799#file2181799line44> > > > > Can you avoid the copy of the action object here? > > > > I'm guessing in the subsequent patches it will be more obvious why we > > return a vector here (e.g. other action object creators require return > > multiple values and we want a uniform interface?) After looking at all these patches once again, realized that the unifirm interface (return vector everywhere) probably introduces more problems than it solves. Reimplemented some methods to return a single ActionObject. Now there are two overloads of master::authorize() overall: one for a single ActionObject, another, built on top of that, for a vector (introdiced in next patches). > On Dec. 4, 2019, 6:21 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3736-3741 (original), 3743-3751 (patched) > > <https://reviews.apache.org/r/71859/diff/2/?file=2181801#file2181801line3744> > > > > Hm.. can you reply to this comment with an example of the logging > > before and after? I'm curious if it's looking worse or we have less > > information (e.g. task id). Now (note that I added some custom cases after your initial review) the difference looks like this. Note that there is no 1:1 correspondence, as now the logging is per-object. Before this chain of patches: (logged is the entity for which Master will choose some Action and build some Object) ``` I0102 14:22:38.512053 60314 master.cpp:3743] Authorizing framework principal 'test-principal' to launch task d8b9a39c-b105-4546-9197-90d2ba5e74cd I0102 14:22:38.512280 60314 master.cpp:3818] Authorizing principal 'test-principal' to reserve resources '[{"allocation_info":{"role":"role"},"name":"cpus","reservations":[{"principal":"test-principal","role":"role","type":"DYNAMIC"}],"scalar":{"value":1.0},"type":"SCALAR"},{"allocation_info":{"role":"role"},"name":"mem","reservations":[{"principal":"test-principal","role":"role","type":"DYNAMIC"}],"scalar":{"value":32.0},"type":"SCALAR"}]' II0102 14:22:38.512471 60314 master.cpp:3919] Authorizing principal 'test-principal' to unreserve resources '[{"allocation_info":{"role":"role"},"name":"cpus","reservations":[{"role":"role","type":"DYNAMIC"}],"scalar":{"value":1.0},"type":"SCALAR"},{"allocation_info":{"role":"role"},"name":"mem","reservations":[{"role":"role","type":"DYNAMIC"}],"scalar":{"value":32.0},"type":"SCALAR"}]' I0102 14:22:38.512611 60314 master.cpp:3982] Authorizing principal 'test-principal' to create volumes '[{"allocation_info":{"role":"role"},"disk":{"persistence":{"id":"3a5fc7d5-39a1-47d2-a964-f1d2e7143c91","principal":"test-principal"},"volume":{"container_path":"path","mode":"RW"}},"name":"disk","reservations":[{"role":"role","type":"STATIC"}],"scalar":{"value":32.0},"type":"SCALAR"},{"allocation_info":{"role":"role"},"disk":{"persistence":{"id":"3902efa1-86f2-4afb-887e-c4c96130e629","principal":"test-principal"},"volume":{"container_path":"path","mode":"RW"}},"name":"disk","reservations":[{"role":"role","type":"STATIC"}],"scalar":{"value":32.0},"type":"SCALAR"}]' ``` After the whole chain: (logged are the actions and objects) ``` I0102 14:14:56.210851 58963 master.cpp:3713] Authorizing principal 'test-principal' to launch task e7a54ee1-71cd-4981-8f75-520e6c66936f of framework 65d5ce9b-f0ad-4978-b18c-9c67cb407913-0000 I0102 14:14:56.211055 58963 master.cpp:3713] Authorizing principal 'test-principal' to perform action RESERVE_RESOURCES on object {"value":"role","resource":{"name":"cpus","type":"SCALAR","scalar":{"value":1.0},"allocation_info":{"role":"role"},"reservations":[{"type":"DYNAMIC","role":"role","principal":"test-principal"}]}} I0102 14:14:56.211151 58963 master.cpp:3713] Authorizing principal 'test-principal' to perform action RESERVE_RESOURCES on object {"value":"role","resource":{"name":"cpus","type":"SCALAR","scalar":{"value":1.0},"allocation_info":{"role":"role"},"reservations":[{"type":"DYNAMIC","role":"role","principal":"test-principal"}]}} I0102 14:14:56.211235 58963 master.cpp:3713] Authorizing principal 'test-principal' to perform action UNRESERVE_RESOURCES on ANY object I0102 14:14:56.211968 58963 master.cpp:3713] Authorizing principal 'test-principal' to perform action CREATE_MOUNT_DISK on object {"value":"*","resource":{"provider_id":{"value":"provider"},"name":"disk","type":"SCALAR","scalar":{"value":32.0},"allocation_info":{"role":"role"},"disk":{"source":{"type":"RAW","profile":"profile"}}}} ``` Actually, I'm wondering if this was (and is) the proper place for this logging. - Ideally, authorizer should log what is being fed into it, with all the details on which the authorization decistion happens. (Local authorizer doesn't do that.) - If we depend on logging of operations in `Master::authorzie*(...)` methods as a first time where the operation is logged, then it ideally should be moved outwards (as it is not related to authorization. > On Dec. 4, 2019, 6:21 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3759 (patched) > > <https://reviews.apache.org/r/71859/diff/2/?file=2181801#file2181801line3760> > > > > why the "if one is available"? That seems to imply there still being an > > async fallback? Now (after discussing possible approaches to that and e-mailing dev@ in a search for custom Authorizers) I'm more certain that there will be no fallback; removed that. > On Dec. 4, 2019, 6:21 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 4365-4377 (patched) > > <https://reviews.apache.org/r/71859/diff/2/?file=2181801#file2181801line4367> > > > > This patch has introduced an extra copy of all the tasks! Notice that > > before the lambda just returned a reference to the right field, now since > > it's not returning a reference we're copying it. > > > > Can you leave it as it was as a lambda without the copying? I guess > > since we need to call it twice now we move the declaration up: > > > > ``` > > const RepeatedPtrField<TaskInfo>& tasks = [&]() { > > if (operation.type() == Offer::Operation::LAUNCH) { > > return operation.launch().task_infos(); > > } else if (operation.type() == > > Offer::Operation::LAUNCH_GROUP) { > > return operation.launch_group().task_group().tasks(); > > } > > UNREACHABLE(); > > }(); > > > > // Use `tasks` twice below.. > > ``` Thanks for catching that! - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71859/#review218914 ----------------------------------------------------------- On Jan. 2, 2020, 8 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71859/ > ----------------------------------------------------------- > > (Updated Jan. 2, 2020, 8 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Bugs: MESOS-10023 and MESOS-10056 > https://issues.apache.org/jira/browse/MESOS-10023 > https://issues.apache.org/jira/browse/MESOS-10056 > > > Repository: mesos > > > Description > ------- > > This is the first patch in the chain that extract Master code > generating Action-Object pairs into a dedicated ActionObject class, > thus seperating authz Object creation from feeding them into authorizer. > > This is a prerequisite to using ObjectApprover interface for > synchronous authorization of Scheduler API calls. > > > Diffs > ----- > > src/CMakeLists.txt ef9382dc77d37fed344b7267119f3251acd3088a > src/Makefile.am 111c156c8a7abc5ece04779e8ac8879a30c22dbf > src/master/authorization.hpp PRE-CREATION > src/master/authorization.cpp PRE-CREATION > src/master/master.hpp f97b085ae908278731acd326df68f9f381f09483 > src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 > > > Diff: https://reviews.apache.org/r/71859/diff/3/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
