mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 71859: Moved creating authorization Object out of `Master::authorizeTask`.
Date Mon, 06 Jan 2020 18:26:23 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message