You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2018/03/08 12:26:14 UTC
Re: Review Request 65313: Refactored authorization logic in the agent.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/
-----------------------------------------------------------
(Updated March 8, 2018, 1:26 p.m.)
Review request for mesos, Benjamin Hindman and Greg Mann.
Changes
-------
rebase.
Bugs: MESOS-8434
https://issues.apache.org/jira/browse/MESOS-8434
Repository: mesos
Description
-------
This patch makes uses of the new `ObjectApprovers` class which greatly
simplifies the logic for constructing and using authorization.
Diffs (updated)
-----
src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
Diff: https://reviews.apache.org/r/65313/diff/6/
Changes: https://reviews.apache.org/r/65313/diff/5-6/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/
-----------------------------------------------------------
(Updated March 14, 2018, 9:58 a.m.)
Review request for mesos, Benjamin Hindman and Greg Mann.
Bugs: MESOS-8434
https://issues.apache.org/jira/browse/MESOS-8434
Repository: mesos
Description
-------
This patch makes uses of the new `ObjectApprovers` class which greatly
simplifies the logic for constructing and using authorization.
Diffs (updated)
-----
src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
src/slave/slave.cpp 2f4ab157448eafc0f41372ee50255a76129e90db
Diff: https://reviews.apache.org/r/65313/diff/8/
Changes: https://reviews.apache.org/r/65313/diff/7-8/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/#review199091
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/http.cpp
Lines 2199-2218 (original), 2031-2039 (patched)
<https://reviews.apache.org/r/65313/#comment279384>
Not indented enough.
src/slave/http.cpp
Lines 2250-2276 (original), 2071-2080 (patched)
<https://reviews.apache.org/r/65313/#comment279383>
Not indented far enough; here and elsewhere. Could you double-check the indentation on these deferred continuations?
src/slave/http.cpp
Lines 3037-3044 (original), 2857-2864 (patched)
<https://reviews.apache.org/r/65313/#comment279382>
Not indented far enough.
- Greg Mann
On March 9, 2018, 1:06 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 1:06 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Greg Mann.
>
>
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
>
>
> Diffs
> -----
>
> src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
> src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
> src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
>
>
> Diff: https://reviews.apache.org/r/65313/diff/7/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/
-----------------------------------------------------------
(Updated March 9, 2018, 2:06 p.m.)
Review request for mesos, Benjamin Hindman and Greg Mann.
Bugs: MESOS-8434
https://issues.apache.org/jira/browse/MESOS-8434
Repository: mesos
Description
-------
This patch makes uses of the new `ObjectApprovers` class which greatly
simplifies the logic for constructing and using authorization.
Diffs (updated)
-----
src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
Diff: https://reviews.apache.org/r/65313/diff/7/
Changes: https://reviews.apache.org/r/65313/diff/6-7/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 9, 2018, 5:30 a.m., Greg Mann wrote:
> > src/slave/http.cpp
> > Lines 2641-2665 (original), 2431-2469 (patched)
> > <https://reviews.apache.org/r/65313/diff/6/?file=1972598#file1972598line2668>
> >
> > Why did you opt for a new templated method instead of the pre-existing lambda, here and elsewhere? Is it necessary because we pass the action as a template parameter?
>
> Alexander Rojas wrote:
> It has to do more with the `Approvers::approved<T>()` being parametrize, which means that the type needs to be resolved on compilation time, so this is a neat trick for that.
>
> Greg Mann wrote:
> IMO it's unfortunate that we need to do this, since it makes this code harder to read.
>
> Actually, looking again at `ObjectApprovers`, I wonder if the action really needs to be a template parameter. We could also do something like:
> ```
> class ObjectApprovers {
> template <typename... Args>
> bool approved(const authorization::Action& action, const Args&... args);
>
> ...
> ```
>
> Then a callsite would look like
> ```
> if (!approvers->approved(SOME_ACTION, someInfo)) {
> ```
> which seems OK to me. WDYT?
As we discussed in chat, we can get rid of some of these extra continuations if we duplicate some code. Let's get this stuff merged now to avoid more rebasing, then we can clean up later.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/#review198917
-----------------------------------------------------------
On March 9, 2018, 1:06 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 1:06 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Greg Mann.
>
>
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
>
>
> Diffs
> -----
>
> src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
> src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
> src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
>
>
> Diff: https://reviews.apache.org/r/65313/diff/7/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On March 9, 2018, 6:30 a.m., Greg Mann wrote:
> > src/slave/http.cpp
> > Lines 2641-2665 (original), 2431-2469 (patched)
> > <https://reviews.apache.org/r/65313/diff/6/?file=1972598#file1972598line2668>
> >
> > Why did you opt for a new templated method instead of the pre-existing lambda, here and elsewhere? Is it necessary because we pass the action as a template parameter?
It has to do more with the `Approvers::approved<T>()` being parametrize, which means that the type needs to be resolved on compilation time, so this is a neat trick for that.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/#review198917
-----------------------------------------------------------
On March 8, 2018, 1:26 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> -----------------------------------------------------------
>
> (Updated March 8, 2018, 1:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Greg Mann.
>
>
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
>
>
> Diffs
> -----
>
> src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
> src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
> src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
>
>
> Diff: https://reviews.apache.org/r/65313/diff/6/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 9, 2018, 5:30 a.m., Greg Mann wrote:
> > src/slave/http.cpp
> > Lines 2641-2665 (original), 2431-2469 (patched)
> > <https://reviews.apache.org/r/65313/diff/6/?file=1972598#file1972598line2668>
> >
> > Why did you opt for a new templated method instead of the pre-existing lambda, here and elsewhere? Is it necessary because we pass the action as a template parameter?
>
> Alexander Rojas wrote:
> It has to do more with the `Approvers::approved<T>()` being parametrize, which means that the type needs to be resolved on compilation time, so this is a neat trick for that.
IMO it's unfortunate that we need to do this, since it makes this code harder to read.
Actually, looking again at `ObjectApprovers`, I wonder if the action really needs to be a template parameter. We could also do something like:
```
class ObjectApprovers {
template <typename... Args>
bool approved(const authorization::Action& action, const Args&... args);
...
```
Then a callsite would look like
```
if (!approvers->approved(SOME_ACTION, someInfo)) {
```
which seems OK to me. WDYT?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/#review198917
-----------------------------------------------------------
On March 9, 2018, 1:06 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 1:06 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Greg Mann.
>
>
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
>
>
> Diffs
> -----
>
> src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
> src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
> src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
>
>
> Diff: https://reviews.apache.org/r/65313/diff/7/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 65313: Refactored authorization logic in the agent.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65313/#review198917
-----------------------------------------------------------
src/slave/http.hpp
Line 119 (original), 119 (patched)
<https://reviews.apache.org/r/65313/#comment279184>
const ref?
src/slave/http.hpp
Lines 230 (patched)
<https://reviews.apache.org/r/65313/#comment279200>
I'm not completely sure what the proper capitalization would be here, but I think it should be either `Action` or `action` instead of `ACTION`. The ObjectApprovers code uses `action` for this case.
src/slave/http.cpp
Line 297 (original), 316 (patched)
<https://reviews.apache.org/r/65313/#comment279187>
I think this should be indented more?
src/slave/http.cpp
Line 315 (original), 334 (patched)
<https://reviews.apache.org/r/65313/#comment279188>
Indentation?
src/slave/http.cpp
Lines 2641-2665 (original), 2431-2469 (patched)
<https://reviews.apache.org/r/65313/#comment279202>
Why did you opt for a new templated method instead of the pre-existing lambda, here and elsewhere? Is it necessary because we pass the action as a template parameter?
src/slave/http.cpp
Lines 2999-3006 (original), 2810-2817 (patched)
<https://reviews.apache.org/r/65313/#comment279201>
Not indented far enough.
src/slave/http.cpp
Line 3575 (original), 3366 (patched)
<https://reviews.apache.org/r/65313/#comment279203>
Not indented far enough.
src/slave/slave.cpp
Lines 8376-8377 (original), 8377-8378 (patched)
<https://reviews.apache.org/r/65313/#comment279204>
I think we can get rid of these lines?
- Greg Mann
On March 8, 2018, 12:26 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> -----------------------------------------------------------
>
> (Updated March 8, 2018, 12:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Greg Mann.
>
>
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
>
>
> Diffs
> -----
>
> src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b
> src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b
> src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219
>
>
> Diff: https://reviews.apache.org/r/65313/diff/6/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>