mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 65313: Refactored authorization logic in the agent.
Date Tue, 13 Mar 2018 15:45:17 GMT


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


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