mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 71859: Moved creating authorization Object out of `Master::authorizeTask`.
Date Thu, 02 Jan 2020 20:06:24 GMT


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


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