mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rojas" <alexan...@mesosphere.io>
Subject Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.
Date Sat, 13 Jun 2015 07:23:14 GMT


> On June 9, 2015, 2:32 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp, line 1921
> > <https://reviews.apache.org/r/33295/diff/10/?file=980632#file980632line1921>
> >
> >     Just 'pid' will do :)

`pid` conflicts with a name before. However, following the parameter name in the `ProcessBase`
constructor now is called `id`


> On June 9, 2015, 2:32 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp, line 1994
> > <https://reviews.apache.org/r/33295/diff/10/?file=980632#file980632line1994>
> >
> >     Ditto, just "pid" is what we've been calling process ids as variables throughout
the code.

`pid` conflicts with a name before. However, following the parameter name in the `ProcessBase`
constructor now is called `id`


> On June 9, 2015, 2:32 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/firewall.hpp, lines 54-56
> > <https://reviews.apache.org/r/33295/diff/10/?file=980629#file980629line54>
> >
> >     Hm.. I think this interface might prove to be a bit confusing for folks:
> >     
> >     ```
> >     Try<Nothing> apply = rule.apply(...);
> >     
> >     if (apply.isError()) {
> >       // What does this mean?
> >     }
> >     ```
> >     
> >     What does it mean for there the rule application to fail?
> >     
> >     It's subtle, but much like we did for the validation code, it seems clearer
to have the rule application return an optional error. The idea is that rather than trying
to apply and failing, applying the rule always succeeds, but yields an error (ideally a 'Rejection',
but to avoid the unnecessary extra struct):
> >     
> >     ```
> >     Option<Error> apply = rule.apply(...);
> >     
> >     if (apply.isSome()) {
> >       // This rule rejected the request, when applied.
> >     }
> >     ```
> >     
> >     Does that make sense?

As you agreed with Till, whe go for `Option<Error>` and do `Option<Error> rejected
= rule.apply(...)`


- Alexander


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


On June 8, 2015, 12:09 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> -----------------------------------------------------------
> 
> (Updated June 8, 2015, 12:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
>     https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduces the interface `FirewallRule` which will be matched against incoming connections
in order to allow them to be served or being blocked. For details, check the [design doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/Makefile.am f45e7c5c0fad063cc0b34ec7977cef685c2909d3 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 79d1719932a3fdc90b6247d3a77adee123e72435

>   3rdparty/libprocess/src/process.cpp d1b4d469a11abc618c1406bce602300dd9793b58 
>   3rdparty/libprocess/src/tests/process_tests.cpp 7b9ba9e70e1fe7a22b26444b3bd928208fecd491

> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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