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 2020/02/07 17:31:40 UTC
Review Request 72089: Introduced `provideObjectApprover(...)`
authorizer interface.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72089/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Greg Mann.
Bugs: MESOS-10056
https://issues.apache.org/jira/browse/MESOS-10056
Repository: mesos
Description
-------
This patch introduces a breaking change in the Authorizer interface:
`getObjectApprover(...)` method that returnes ObjectApprover
which should not be stored for a long time is replaced with
`provideObjectApprover(...)` method that returns ObjectApprover
that must be kept valid (by authorizer implementation) throughout its
whoile lifetime.
This unblocks way to synchronous (without dispatch to another actor)
authorization in cases where principal is known to be long-lived;
examples are the scheduler API (see MESOS-10056) and v1 operator API
events (see MESOS-10057).
The local authorizer is modified accordingly.
NOTE: This patch breaks compatibility with custom authorizers which
do not implement this method!
Diffs
-----
docs/authorization.md 698e485fca481d1398594f743141d1cd0af830be
include/mesos/authorizer/authorizer.hpp a86a6eeb592adfc267dcf3faef40e8da3471feaf
src/authorizer/local/authorizer.hpp 2516a37d2019c097dea4e6dbf75a7efbef3853f0
src/authorizer/local/authorizer.cpp 16c0ffa9c315e0a2b4127c2d325232733f0e4e75
src/common/http.hpp 5fc19fdd16138eb4c7d14fd29b1a56a53f6323a9
src/common/http.cpp c5b2a91958c870e272895520ba04fc5287891c3c
src/tests/api_tests.cpp 87550168d950f7c423c57627b0349d99b39881ca
src/tests/master_load_tests.cpp 6bbc1c061684e0c55edde6ab31ef51542d0be980
src/tests/mesos.hpp 73b18663d4dbf0ee179c298ea77b548d5de40921
src/tests/mesos.cpp 664c3027fd5bdfb1e81a4d9966fe93b2181479e4
Diff: https://reviews.apache.org/r/72089/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72089: Introduced `getApprover(...)` authorizer
interface.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72089/
-----------------------------------------------------------
(Updated Feb. 27, 2020, 6:28 p.m.)
Review request for mesos, Benjamin Mahler and Greg Mann.
Changes
-------
Renamed `provideObjectApprover(...)` into `getApprover(...)`; extended comments, filed MESOS-10099 for Operator API authz errors followup.
Summary (updated)
-----------------
Introduced `getApprover(...)` authorizer interface.
Bugs: MESOS-10056
https://issues.apache.org/jira/browse/MESOS-10056
Repository: mesos
Description (updated)
-------
This patch introduces a breaking change in the Authorizer interface:
`getObjectApprover(...)` method that returnes ObjectApprover
which should not be stored for a long time is replaced with
`getApprover(...)` method that returns ObjectApprover
that must be kept valid (by authorizer implementation) throughout its
whole lifetime.
This unblocks way to synchronous (without dispatch to another actor)
authorization in cases where principal is known to be long-lived;
examples are the scheduler API (see MESOS-10056) and v1 operator API
events (see MESOS-10057).
The local authorizer is modified accordingly.
NOTE: This patch breaks compatibility with custom authorizers which
do not implement this method!
Diffs (updated)
-----
docs/authorization.md 698e485fca481d1398594f743141d1cd0af830be
include/mesos/authorizer/authorizer.hpp a86a6eeb592adfc267dcf3faef40e8da3471feaf
src/authorizer/local/authorizer.hpp 2516a37d2019c097dea4e6dbf75a7efbef3853f0
src/authorizer/local/authorizer.cpp 16c0ffa9c315e0a2b4127c2d325232733f0e4e75
src/common/http.hpp 4a0f4a8c2ee9f07032d082ed039c4ea3bba6137a
src/common/http.cpp c5b2a91958c870e272895520ba04fc5287891c3c
src/tests/api_tests.cpp 87550168d950f7c423c57627b0349d99b39881ca
src/tests/master_load_tests.cpp 6bbc1c061684e0c55edde6ab31ef51542d0be980
src/tests/mesos.hpp 73b18663d4dbf0ee179c298ea77b548d5de40921
src/tests/mesos.cpp 664c3027fd5bdfb1e81a4d9966fe93b2181479e4
Diff: https://reviews.apache.org/r/72089/diff/5/
Changes: https://reviews.apache.org/r/72089/diff/4-5/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72089: Introduced `provideObjectApprover(...)`
authorizer interface.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72089/#review219615
-----------------------------------------------------------
Fix it, then Ship it!
Per offline discussion, seems like we need to do follow up about the ignored error in the ObjectApprovers, mainly for the event stream subscribers case where we would now hold on to an object approver and it could enter a failed state (and I guess this is already a problem, so it's not like we're changing the nature of it here: MESOS-10085). For the filtering, I suppose it's less of an issue, but it does seem to make debugging difficult if we're seeing filtered things that shouldn't be filtered, but they're getting filtered due to authz errors (don't know if there's a ticket for that..).
docs/authorization.md
Lines 911-918 (original), 911-918 (patched)
<https://reviews.apache.org/r/72089/#comment307799>
Might be worth mentioning the lifetime stuff with the approver (I guess this should have mentioned it before that it shouldn't be held onto for an undetermined amount of time)
include/mesos/authorizer/authorizer.hpp
Lines 36-38 (original), 36-38 (patched)
<https://reviews.apache.org/r/72089/#comment307800>
Ditto here?
include/mesos/authorizer/authorizer.hpp
Lines 217-218 (original), 217-218 (patched)
<https://reviews.apache.org/r/72089/#comment307801>
Perhaps a bit of context could be added here to make the errors make sense to the reader? i.e. given that this is managed and kept-up-to date by the authorizer, it seems then seems clear that things go go wrong w.r.t. that
src/authorizer/local/authorizer.cpp
Lines 1960 (patched)
<https://reviews.apache.org/r/72089/#comment307802>
why copy it here?
src/common/http.hpp
Lines 280 (patched)
<https://reviews.apache.org/r/72089/#comment307803>
I guess we need another one for the http endpoint filtering issue that this causes? e.g. the state endpoint?
- Benjamin Mahler
On Feb. 7, 2020, 5:31 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72089/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2020, 5:31 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Greg Mann.
>
>
> Bugs: MESOS-10056
> https://issues.apache.org/jira/browse/MESOS-10056
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch introduces a breaking change in the Authorizer interface:
> `getObjectApprover(...)` method that returnes ObjectApprover
> which should not be stored for a long time is replaced with
> `provideObjectApprover(...)` method that returns ObjectApprover
> that must be kept valid (by authorizer implementation) throughout its
> whole lifetime.
>
> This unblocks way to synchronous (without dispatch to another actor)
> authorization in cases where principal is known to be long-lived;
> examples are the scheduler API (see MESOS-10056) and v1 operator API
> events (see MESOS-10057).
>
> The local authorizer is modified accordingly.
>
> NOTE: This patch breaks compatibility with custom authorizers which
> do not implement this method!
>
>
> Diffs
> -----
>
> docs/authorization.md 698e485fca481d1398594f743141d1cd0af830be
> include/mesos/authorizer/authorizer.hpp a86a6eeb592adfc267dcf3faef40e8da3471feaf
> src/authorizer/local/authorizer.hpp 2516a37d2019c097dea4e6dbf75a7efbef3853f0
> src/authorizer/local/authorizer.cpp 16c0ffa9c315e0a2b4127c2d325232733f0e4e75
> src/common/http.hpp 5fc19fdd16138eb4c7d14fd29b1a56a53f6323a9
> src/common/http.cpp c5b2a91958c870e272895520ba04fc5287891c3c
> src/tests/api_tests.cpp 87550168d950f7c423c57627b0349d99b39881ca
> src/tests/master_load_tests.cpp 6bbc1c061684e0c55edde6ab31ef51542d0be980
> src/tests/mesos.hpp 73b18663d4dbf0ee179c298ea77b548d5de40921
> src/tests/mesos.cpp 664c3027fd5bdfb1e81a4d9966fe93b2181479e4
>
>
> Diff: https://reviews.apache.org/r/72089/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>