You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <bb...@apache.org> on 2019/11/06 10:10:17 UTC
Review Request 71729: Added authorization handling for reservations
with `source`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/
-----------------------------------------------------------
Review request for mesos and Benno Evers.
Bugs: MESOS-9991
https://issues.apache.org/jira/browse/MESOS-9991
Repository: mesos
Description
-------
This patch adds authorization handling for `RESERVE` operations
containing `source` fields. In order to stay backwards-compatible we add
a dedicated authorization branch for such operations which under the
hood translates each removed reservation to an `UNRESERVE` operation and
every added reservation as a `RESERVE` operation where we fall back to
existing authorization code for authorization.
Diffs
-----
src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d
src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
Diff: https://reviews.apache.org/r/71729/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3810>
> >
> > It doesn't seem ideal to have recursively nested calls to `collectauthorizations()`, even if the logic is sound it seems hard to reason about.
> >
> > Would it be possible to eliminate the branching by setting `source` to be `resources.popReservation()` if `source` is empty?
> Would it be possible to eliminate the branching by setting source to be resources.popReservation() if source is empty?
This would only work if we know that all resources passed to `RESERVE` are indeed reserved. Unfortunately that is not the case in the current implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need to keep support for that behavior as it is part of the APII.
In the patch I put up we go from the narrower extended API (e.g., all resources passed to `RESERVE` must have identical reservations) to the wider existing API so we are good. Going from wider to narrower doesn't work, though.
What I could do for the sake of readibility would be to introduce a dedicated function for the legacy behavior to avoid the self-recursion. I am not sure that would help (and might it even make harder to follow the code).
WDYT?
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3820 (patched)
> > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3820>
> >
> > Shouldn't the first `Unreserve` operation contain the original `source`?
Of course.
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3828 (patched)
> > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3828>
> >
> > Is this the same as `reserve.resources().reservations()`?
Good point, this is indeed the same as `reserve.resources(0).reservations`. Using that seems to be a better idea than going strictly with symmetric approaches for `targetReservations` and `ancestorReservations` as it can help avoid a number of temporaries.
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3834 (patched)
> > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3834>
> >
> > That look more like debug-statements rather than `INFO`-level logging?
Indeed, even explicitly marked up as such with my magic string, yet still missed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218545
-----------------------------------------------------------
On Nov. 7, 2019, noon, Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2019, noon)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56
> src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
>
>
> Diff: https://reviews.apache.org/r/71729/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3810>
> >
> > It doesn't seem ideal to have recursively nested calls to `collectauthorizations()`, even if the logic is sound it seems hard to reason about.
> >
> > Would it be possible to eliminate the branching by setting `source` to be `resources.popReservation()` if `source` is empty?
>
> Benjamin Bannier wrote:
> > Would it be possible to eliminate the branching by setting source to be resources.popReservation() if source is empty?
>
> This would only work if we know that all resources passed to `RESERVE` are indeed reserved. Unfortunately that is not the case in the current implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need to keep support for that behavior as it is part of the APII.
>
> In the patch I put up we go from the narrower extended API (e.g., all resources passed to `RESERVE` must have identical reservations) to the wider existing API so we are good. Going from wider to narrower doesn't work, though.
>
> What I could do for the sake of readibility would be to introduce a dedicated function for the legacy behavior to avoid the self-recursion. I am not sure that would help (and might it even make harder to follow the code).
>
> WDYT?
>
> Benno Evers wrote:
> Intuitively introducing a dedicated function sounds cleaner to me, but the self-recursion should be fine as well if there's no easy way to avoid it. Maybe it would be good to add some of the reasoning above to the comment, though.
I added context to the comment.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218545
-----------------------------------------------------------
On Nov. 8, 2019, 1:48 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2019, 1:48 p.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d
> src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
>
>
> Diff: https://reviews.apache.org/r/71729/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benno Evers <be...@mesosphere.com>.
> On Nov. 6, 2019, 4:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3810>
> >
> > It doesn't seem ideal to have recursively nested calls to `collectauthorizations()`, even if the logic is sound it seems hard to reason about.
> >
> > Would it be possible to eliminate the branching by setting `source` to be `resources.popReservation()` if `source` is empty?
>
> Benjamin Bannier wrote:
> > Would it be possible to eliminate the branching by setting source to be resources.popReservation() if source is empty?
>
> This would only work if we know that all resources passed to `RESERVE` are indeed reserved. Unfortunately that is not the case in the current implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need to keep support for that behavior as it is part of the APII.
>
> In the patch I put up we go from the narrower extended API (e.g., all resources passed to `RESERVE` must have identical reservations) to the wider existing API so we are good. Going from wider to narrower doesn't work, though.
>
> What I could do for the sake of readibility would be to introduce a dedicated function for the legacy behavior to avoid the self-recursion. I am not sure that would help (and might it even make harder to follow the code).
>
> WDYT?
Intuitively introducing a dedicated function sounds cleaner to me, but the self-recursion should be fine as well if there's no easy way to avoid it. Maybe it would be good to add some of the reasoning above to the comment, though.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218545
-----------------------------------------------------------
On Nov. 7, 2019, 11 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2019, 11 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56
> src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
>
>
> Diff: https://reviews.apache.org/r/71729/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218545
-----------------------------------------------------------
src/master/master.cpp
Lines 3810 (patched)
<https://reviews.apache.org/r/71729/#comment306287>
It doesn't seem ideal to have recursively nested calls to `collectauthorizations()`, even if the logic is sound it seems hard to reason about.
Would it be possible to eliminate the branching by setting `source` to be `resources.popReservation()` if `source` is empty?
src/master/master.cpp
Lines 3820 (patched)
<https://reviews.apache.org/r/71729/#comment306284>
Shouldn't the first `Unreserve` operation contain the original `source`?
src/master/master.cpp
Lines 3828 (patched)
<https://reviews.apache.org/r/71729/#comment306285>
Is this the same as `reserve.resources().reservations()`?
src/master/master.cpp
Lines 3834 (patched)
<https://reviews.apache.org/r/71729/#comment306279>
That look more like debug-statements rather than `INFO`-level logging?
- Benno Evers
On Nov. 6, 2019, 10:10 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 10:10 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d
> src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
>
>
> Diff: https://reviews.apache.org/r/71729/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218527
-----------------------------------------------------------
Bad review!
Reviews applied: [71729]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On Nov. 6, 2019, 10:10 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 10:10 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d
> src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
>
>
> Diff: https://reviews.apache.org/r/71729/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218579
-----------------------------------------------------------
Ship it!
Ship It!
- Benno Evers
On Nov. 8, 2019, 12:48 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2019, 12:48 p.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d
> src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
>
>
> Diff: https://reviews.apache.org/r/71729/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/
-----------------------------------------------------------
(Updated Nov. 8, 2019, 1:48 p.m.)
Review request for mesos and Benno Evers.
Bugs: MESOS-9991
https://issues.apache.org/jira/browse/MESOS-9991
Repository: mesos
Description
-------
This patch adds authorization handling for `RESERVE` operations
containing `source` fields. In order to stay backwards-compatible we add
a dedicated authorization branch for such operations which under the
hood translates each removed reservation to an `UNRESERVE` operation and
every added reservation as a `RESERVE` operation where we fall back to
existing authorization code for authorization.
Diffs (updated)
-----
src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d
src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
Diff: https://reviews.apache.org/r/71729/diff/3/
Changes: https://reviews.apache.org/r/71729/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 71729: Added authorization handling for
reservations with `source`.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/
-----------------------------------------------------------
(Updated Nov. 7, 2019, noon)
Review request for mesos and Benno Evers.
Bugs: MESOS-9991
https://issues.apache.org/jira/browse/MESOS-9991
Repository: mesos
Description
-------
This patch adds authorization handling for `RESERVE` operations
containing `source` fields. In order to stay backwards-compatible we add
a dedicated authorization branch for such operations which under the
hood translates each removed reservation to an `UNRESERVE` operation and
every added reservation as a `RESERVE` operation where we fall back to
existing authorization code for authorization.
Diffs (updated)
-----
src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56
src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2
Diff: https://reviews.apache.org/r/71729/diff/2/
Changes: https://reviews.apache.org/r/71729/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier