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 72089: Introduced `provideObjectApprover(...)` authorizer interface.
Date Thu, 20 Feb 2020 11:06:18 GMT

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


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