Re: Review Request 71729: Added authorization handling for reservations with `source`.

2019-11-08 Thread Benno Evers 

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

2019-11-08 Thread Benjamin Bannier 


> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > 
> >
> > 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`.

2019-11-08 Thread Benjamin Bannier 

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

2019-11-07 Thread Benno Evers 


> On Nov. 6, 2019, 4:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > 
> >
> > 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`.

2019-11-07 Thread Benjamin Bannier 


> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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`.

2019-11-07 Thread Benjamin Bannier 

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

Re: Review Request 71729: Added authorization handling for reservations with `source`.

2019-11-06 Thread Benno Evers 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71729/#review218545
---




src/master/master.cpp
Lines 3810 (patched)


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)


Shouldn't the first `Unreserve` operation contain the original `source`?



src/master/master.cpp
Lines 3828 (patched)


Is this the same as `reserve.resources().reservations()`?



src/master/master.cpp
Lines 3834 (patched)


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

2019-11-06 Thread Mesos Reviewbot 

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

Review Request 71729: Added authorization handling for reservations with `source`.

2019-11-06 Thread Benjamin Bannier 

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