You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/12/03 15:02:25 UTC
Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/
-----------------------------------------------------------
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/common/authorization.hpp 565d5ca6620442803fa80be652ab7382102347f5
src/common/authorization.cpp fa71b0e8e8b9541376a9fd199f4d7b9db56a3f0f
src/master/authorization.hpp PRE-CREATION
src/master/authorization.cpp PRE-CREATION
src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665
src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869
Diff: https://reviews.apache.org/r/71859/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On Dec. 4, 2019, 6:59 p.m., Benjamin Mahler wrote:
> > Another observation from a subsequent review with:
> >
> > ```
> > static std::vector<ActionObject> growVolume(
> > const Offer::Operation::GrowVolume& grow);
> >
> > static std::vector<ActionObject> shrinkVolume(
> > const Offer::Operation::ShrinkVolume& shrink);
> > ```
> >
> > Why doesn't this patch just take the offer operations for launching tasks?
> >
> > ```
> > static std::vector<ActionObject> launch(
> > const Offer::Operation::Launch& launch);
> >
> > static std::vector<ActionObject> launchGroup(
> > const Offer::Operation::LaunchGroup& launchGroup);
> > ```
> >
> > That would look more consistent, and it would pull more logic out of the master code?
Unfortunately, this has to reflect an underlying inconsistency between authorizing LAUNCH operations and all the other operations.
Currently, authorizing any non-LAUNCH operatiuon requires successful authorization for all the needed actions.
However, tasks in LAUNCH operations are authorized independently; failure to authorize one task has no impact on launching other tasks from the same operation.
If we are to change this behaviour, taking Launch operations as an argument would have perfect sense.
Note, however, that switching to launch-all-or-nothing seems to be a major behaviour change.
Even if we preserve the current paradigm, under which frameworks are informed about launch errors via task status updates, we will need to introduce a weird task status reason (`TaskStatus::REASON_TASK_FROM_THE_SAME_OPERATION_NOT_AUTHORIZED`?)
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/#review218918
-----------------------------------------------------------
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
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/#review218918
-----------------------------------------------------------
Another observation from a subsequent review with:
```
static std::vector<ActionObject> growVolume(
const Offer::Operation::GrowVolume& grow);
static std::vector<ActionObject> shrinkVolume(
const Offer::Operation::ShrinkVolume& shrink);
```
Why doesn't this patch just take the offer operations for launching tasks?
```
static std::vector<ActionObject> launch(
const Offer::Operation::Launch& launch);
static std::vector<ActionObject> launchGroup(
const Offer::Operation::LaunchGroup& launchGroup);
```
That would look more consistent, and it would pull more logic out of the master code?
- Benjamin Mahler
On Dec. 3, 2019, 3:02 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71859/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 3:02 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/common/authorization.hpp 565d5ca6620442803fa80be652ab7382102347f5
> src/common/authorization.cpp fa71b0e8e8b9541376a9fd199f4d7b9db56a3f0f
> src/master/authorization.hpp PRE-CREATION
> src/master/authorization.cpp PRE-CREATION
> src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665
> src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869
>
>
> Diff: https://reviews.apache.org/r/71859/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/#review219135
-----------------------------------------------------------
Ship it!
Made the adjustment below, and will commit shortly!
src/master/authorization.hpp
Lines 34-38 (patched)
<https://reviews.apache.org/r/71859/#comment307233>
probably s/getAction/action/ and s/getObject/object/ (and then `action_` and `object_` members) would be more consistent with the rest of our code
- Benjamin Mahler
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
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/71859/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> 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
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/#review218914
-----------------------------------------------------------
I'm really excited to see the synchronous authorization improvement! Thanks for tackling this
Overall looks good, just a couple of copy related issues, and some style related notes.
src/common/authorization.cpp
Lines 58 (patched)
<https://reviews.apache.org/r/71859/#comment306816>
Don't forget those braces on the next line! :)
src/common/authorization.cpp
Lines 59 (patched)
<https://reviews.apache.org/r/71859/#comment306817>
can just be one line?
```
return stream << jsonify(JSON::Protobuf(object));
```
src/master/authorization.hpp
Lines 32 (patched)
<https://reviews.apache.org/r/71859/#comment306819>
Ditto here, brace on next line
src/master/authorization.hpp
Lines 36 (patched)
<https://reviews.apache.org/r/71859/#comment306818>
s/:/: /
Also, the following looks better?
```
ActionObject(Action action_) : action(std::move(action_)) {};
```
But I assume action is an enum? In that case, maybe we don't take it by ref or move it either?
src/master/authorization.cpp
Lines 41-42 (patched)
<https://reviews.apache.org/r/71859/#comment306820>
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;
```
src/master/authorization.cpp
Lines 44 (patched)
<https://reviews.apache.org/r/71859/#comment306821>
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?)
src/master/master.cpp
Line 3733 (original), 3740 (patched)
<https://reviews.apache.org/r/71859/#comment306825>
Ditto here:
```
*request.mutable_subject() = *subject;
```
src/master/master.cpp
Lines 3736-3741 (original), 3743-3751 (patched)
<https://reviews.apache.org/r/71859/#comment306826>
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).
src/master/master.cpp
Lines 3755 (patched)
<https://reviews.apache.org/r/71859/#comment306827>
thanks!
src/master/master.cpp
Lines 3759 (patched)
<https://reviews.apache.org/r/71859/#comment306828>
why the "if one is available"? That seems to imply there still being an async fallback?
src/master/master.cpp
Lines 3743-3748 (original), 3763-3767 (patched)
<https://reviews.apache.org/r/71859/#comment306829>
Why is this optimization up here rather than in collectAuthorizations?
src/master/master.cpp
Lines 4365-4377 (patched)
<https://reviews.apache.org/r/71859/#comment306822>
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..
```
src/master/master.cpp
Lines 4825-4827 (original), 4843-4846 (patched)
<https://reviews.apache.org/r/71859/#comment306823>
Why the change here? Can you leave the old wrapping style as is?
src/master/master.cpp
Lines 4851-4853 (patched)
<https://reviews.apache.org/r/71859/#comment306824>
We tend to avoid wrapping way over to the right, so the following would be acceptable:
```
const Option<Principal> principal = framework->info.has_principal()
? Principal(framework->info.principal())
: Option<Principal>::none();
```
```
const Option<Principal> principal = framework->info.has_principal()
? Principal(framework->info.principal()) : Option<Principal>::none();
```
```
const Option<Principal> principal =
framework->info.has_principal()
? Principal(framework->info.principal())
: Option<Principal>::none();
```
- Benjamin Mahler
On Dec. 3, 2019, 3:02 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71859/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 3:02 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/common/authorization.hpp 565d5ca6620442803fa80be652ab7382102347f5
> src/common/authorization.cpp fa71b0e8e8b9541376a9fd199f4d7b9db56a3f0f
> src/master/authorization.hpp PRE-CREATION
> src/master/authorization.cpp PRE-CREATION
> src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665
> src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869
>
>
> Diff: https://reviews.apache.org/r/71859/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On Dec. 4, 2019, 6:43 p.m., Benjamin Mahler wrote:
> > src/master/authorization.cpp
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/71859/diff/2/?file=2181799#file2181799line38>
> >
> > Actually, come to think of it, this construction style seems quite odd?
> >
> > We're mixing construction and mutation, which means we have a intermediate state where the action object is not valid.
> >
> > I guess the struct-like approach doesn't work because we don't want to default initialize the action field:
> >
> > ```
> > ActionObject actionObject;
> >
> > actionObject.action = authorization::RUN_TASK;
> > actionObject.object = Object();
> > *actionObject.object->mutable_task_info() = task;
> > ```
> >
> > So it seems we should go with something like:
> >
> > ```
> > Object object = ...;
> > ...
> >
> > ActionObject actionObject(
> > actionObject.action = authorization::RUN_TASK,
> > std::move(object));
> > ```
> >
> > Also, might want to make this constructor private for only the helpers to use? Or do we want callers using it?
Good point, thanks!
Converted ActionObject into a class with private constructor.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/#review218916
-----------------------------------------------------------
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
>
>
Re: Review Request 71859: Moved creating authorization Object out of
`Master::authorizeTask`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71859/#review218916
-----------------------------------------------------------
Sorry, one more thing that I originally missed in this one and noticed when looking at the next reviews, but it's most relevant to mention in this initial one:
src/master/authorization.cpp
Lines 38 (patched)
<https://reviews.apache.org/r/71859/#comment306830>
Actually, come to think of it, this construction style seems quite odd?
We're mixing construction and mutation, which means we have a intermediate state where the action object is not valid.
I guess the struct-like approach doesn't work because we don't want to default initialize the action field:
```
ActionObject actionObject;
actionObject.action = authorization::RUN_TASK;
actionObject.object = Object();
*actionObject.object->mutable_task_info() = task;
```
So it seems we should go with something like:
```
Object object = ...;
...
ActionObject actionObject(
actionObject.action = authorization::RUN_TASK,
std::move(object));
```
Also, might want to make this constructor private for only the helpers to use? Or do we want callers using it?
- Benjamin Mahler
On Dec. 3, 2019, 3:02 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71859/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 3:02 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/common/authorization.hpp 565d5ca6620442803fa80be652ab7382102347f5
> src/common/authorization.cpp fa71b0e8e8b9541376a9fd199f4d7b9db56a3f0f
> src/master/authorization.hpp PRE-CREATION
> src/master/authorization.cpp PRE-CREATION
> src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665
> src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869
>
>
> Diff: https://reviews.apache.org/r/71859/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>