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:32:33 UTC
Review Request 72098: Converted ACCEPT to synchronous authorization.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72098/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Greg Mann.
Bugs: MESOS-10023, MESOS-10056 and MESOS-10083
https://issues.apache.org/jira/browse/MESOS-10023
https://issues.apache.org/jira/browse/MESOS-10056
https://issues.apache.org/jira/browse/MESOS-10083
Repository: mesos
Description
-------
This patch converts ACCEPT call to synchronous authorization
(see MESOS-10056), thus fixing race between ACCEPT and REVIVE
(MESOS-10023) and removing potential for other similar races.
It also moves authorization of scheduler API operations after their
validation (thus fixing MESOS-10083) and effectively gets rid of the
concept of a "task pending authorization".
Tests are converted from mocking `Authorizer::authorized(...)`
to mocking `Authorizer::provideObjectApprover(...)` as necessary.
Diffs
-----
src/master/authorization.cpp 77719eb079b2a19e0841573f639437aa3bb0fe54
src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96
src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f
src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e
src/tests/master_authorization_tests.cpp bc8155b97c9078eaa151cc4a3e5bc6ea0d7ac9fa
src/tests/master_tests.cpp 9688f5f0266f7c7142b54d488f2c13b427e542c0
src/tests/reconciliation_tests.cpp cdff370c5871ded0cb10b8b782bd669e092eb741
Diff: https://reviews.apache.org/r/72098/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72098: Converted ACCEPT to synchronous
authorization.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72098/
-----------------------------------------------------------
(Updated Feb. 27, 2020, 6:41 p.m.)
Review request for mesos, Benjamin Mahler and Greg Mann.
Bugs: MESOS-10023, MESOS-10056 and MESOS-10083
https://issues.apache.org/jira/browse/MESOS-10023
https://issues.apache.org/jira/browse/MESOS-10056
https://issues.apache.org/jira/browse/MESOS-10083
Repository: mesos
Description
-------
This patch converts ACCEPT call to synchronous authorization
(see MESOS-10056), thus fixing race between ACCEPT and REVIVE
(MESOS-10023) and removing potential for other similar races.
It also moves authorization of scheduler API operations after their
validation (thus fixing MESOS-10083) and effectively gets rid of the
concept of a "task pending authorization".
Tests are converted from mocking `Authorizer::authorized(...)`
to mocking `Authorizer::provideObjectApprover(...)` as necessary.
Diffs (updated)
-----
src/master/authorization.cpp 77719eb079b2a19e0841573f639437aa3bb0fe54
src/master/framework.cpp a9318a9d33122610960e01a184b568a8ea18b514
src/master/master.hpp d774d77a50597770c6f2d4f5dffcbd79b5f29da3
src/master/master.cpp 36a81ccd24d0156049382fee0d085193cc2867e6
src/tests/master_authorization_tests.cpp bc8155b97c9078eaa151cc4a3e5bc6ea0d7ac9fa
src/tests/master_tests.cpp 9688f5f0266f7c7142b54d488f2c13b427e542c0
src/tests/reconciliation_tests.cpp cdff370c5871ded0cb10b8b782bd669e092eb741
Diff: https://reviews.apache.org/r/72098/diff/5/
Changes: https://reviews.apache.org/r/72098/diff/4-5/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72098: Converted ACCEPT to synchronous
authorization.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72098/#review219620
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Line 4290 (original), 4292 (patched)
<https://reviews.apache.org/r/72098/#comment307816>
Maybe a bit of context for the reader about why `_accept` exists? (given that the reader won't know about the previous state of the code)
src/master/master.cpp
Lines 4497-4499 (patched)
<https://reviews.apache.org/r/72098/#comment307817>
is it possible to not wrap it here and use `authorized_` directly instead?
src/master/master.cpp
Lines 4570-4572 (patched)
<https://reviews.apache.org/r/72098/#comment307818>
Feel free to make the logging consistent and add the agent logging in all of these.
src/master/master.cpp
Lines 5320-5322 (original), 5085-5087 (patched)
<https://reviews.apache.org/r/72098/#comment307819>
This comment still refers to authz futures?
src/master/master.cpp
Lines 5335-5338 (original), 5099-5102 (patched)
<https://reviews.apache.org/r/72098/#comment307820>
Looks like we don't need this anymore?
src/master/master.cpp
Lines 5278-5280 (patched)
<https://reviews.apache.org/r/72098/#comment307821>
different wrapping here than the others?
src/master/master.cpp
Lines 5321-5324 (patched)
<https://reviews.apache.org/r/72098/#comment307822>
Ditto here
- Benjamin Mahler
On Feb. 13, 2020, 4:42 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72098/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2020, 4:42 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Greg Mann.
>
>
> Bugs: MESOS-10023, MESOS-10056 and MESOS-10083
> https://issues.apache.org/jira/browse/MESOS-10023
> https://issues.apache.org/jira/browse/MESOS-10056
> https://issues.apache.org/jira/browse/MESOS-10083
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch converts ACCEPT call to synchronous authorization
> (see MESOS-10056), thus fixing race between ACCEPT and REVIVE
> (MESOS-10023) and removing potential for other similar races.
>
> It also moves authorization of scheduler API operations after their
> validation (thus fixing MESOS-10083) and effectively gets rid of the
> concept of a "task pending authorization".
>
> Tests are converted from mocking `Authorizer::authorized(...)`
> to mocking `Authorizer::provideObjectApprover(...)` as necessary.
>
>
> Diffs
> -----
>
> src/master/authorization.cpp 77719eb079b2a19e0841573f639437aa3bb0fe54
> src/master/framework.cpp a9318a9d33122610960e01a184b568a8ea18b514
> src/master/master.hpp f1aa40fb45c693bd992b50cffca11020a1fe4433
> src/master/master.cpp 6d45c4e56432cb997769f7c6d0c8f71bdc8f8005
> src/tests/master_authorization_tests.cpp bc8155b97c9078eaa151cc4a3e5bc6ea0d7ac9fa
> src/tests/master_tests.cpp 9688f5f0266f7c7142b54d488f2c13b427e542c0
> src/tests/reconciliation_tests.cpp cdff370c5871ded0cb10b8b782bd669e092eb741
>
>
> Diff: https://reviews.apache.org/r/72098/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72098: Converted ACCEPT to synchronous
authorization.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72098/
-----------------------------------------------------------
(Updated Feb. 13, 2020, 4:42 p.m.)
Review request for mesos, Benjamin Mahler and Greg Mann.
Changes
-------
Dropped use of `boost::transform_iterator`: build with bundled `boost` fails (due to `transform_iterator` not bundled), and rebundling doesn't look worth it.
Bugs: MESOS-10023, MESOS-10056 and MESOS-10083
https://issues.apache.org/jira/browse/MESOS-10023
https://issues.apache.org/jira/browse/MESOS-10056
https://issues.apache.org/jira/browse/MESOS-10083
Repository: mesos
Description
-------
This patch converts ACCEPT call to synchronous authorization
(see MESOS-10056), thus fixing race between ACCEPT and REVIVE
(MESOS-10023) and removing potential for other similar races.
It also moves authorization of scheduler API operations after their
validation (thus fixing MESOS-10083) and effectively gets rid of the
concept of a "task pending authorization".
Tests are converted from mocking `Authorizer::authorized(...)`
to mocking `Authorizer::provideObjectApprover(...)` as necessary.
Diffs (updated)
-----
src/master/authorization.cpp 77719eb079b2a19e0841573f639437aa3bb0fe54
src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96
src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f
src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e
src/tests/master_authorization_tests.cpp bc8155b97c9078eaa151cc4a3e5bc6ea0d7ac9fa
src/tests/master_tests.cpp 9688f5f0266f7c7142b54d488f2c13b427e542c0
src/tests/reconciliation_tests.cpp cdff370c5871ded0cb10b8b782bd669e092eb741
Diff: https://reviews.apache.org/r/72098/diff/3/
Changes: https://reviews.apache.org/r/72098/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko