> 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). > > Andrei Sekretenko wrote: > 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.
Yeah, my opinion is that probably it should be the responsibility of the authorizer, and I would expect most authorizers to keep a separate authz log (that's probably how auth logging tends to work? not sure..) - Benjamin ----------------------------------------------------------- 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/4/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
