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



3rdparty/libprocess/include/process/firewall.hpp
<https://reviews.apache.org/r/33295/#comment139403>

    This says "connection", but this actually operates at the Request level 
AFAICT..?



3rdparty/libprocess/include/process/firewall.hpp
<https://reviews.apache.org/r/33295/#comment139401>

    The comments after the ';' seem to just re-iterate what the interface 
specifies below, so it seems likely to go out of date and I'm not sure it's 
adding much value, do you need it?



3rdparty/libprocess/include/process/firewall.hpp
<https://reviews.apache.org/r/33295/#comment139398>

    Missing include for Try?



3rdparty/libprocess/include/process/firewall.hpp
<https://reviews.apache.org/r/33295/#comment139402>

    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?



3rdparty/libprocess/include/process/firewall.hpp
<https://reviews.apache.org/r/33295/#comment139399>

    Missing include for Error?



3rdparty/libprocess/include/process/firewall.hpp
<https://reviews.apache.org/r/33295/#comment139400>

    Missing include for Nothing?



3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/33295/#comment139406>

    newline?



3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/33295/#comment139414>

    How about just "used to forbid" incoming requests.



3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/33295/#comment139407>

    Again, this says connections, but these seem to operate at the level of 
requests?



3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/33295/#comment139409>

    Can you do a sweep for where you're saying connection? Seems to not match 
the code :(



3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/33295/#comment139413>

    How about the following for the second setence onward:
    
    ```
    The rules will be applied in the provided order to each incoming request. 
If any rule forbids the request, a Forbidden response will be returned 
containaing the reason from the rule.
    ```



3rdparty/libprocess/include/process/process.hpp
<https://reviews.apache.org/r/33295/#comment139415>

    Include for vector?
    Include for Owned?



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/33295/#comment139421>

    We've generally avoided looping using references, although I see why you 
did here. Was there a reason that you needed apply to be non-const?
    
    ```
    foreach (const Owned<FirewallRule>& rule, firewallRules) {
    
    }
    ```



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/33295/#comment139420>

    The convention has been s/applied/apply/ for this kind of code:
    
    ```
    Try<Nothing> apply = rule->apply(...):
    ```



3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/33295/#comment139416>

    Newline here?



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139432>

    Just 'pid' will do :)



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139424>

    Any plan to add initializer list support for hashset?



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139423>

    Missing includes for vector, Owned, std::move, can you do a sweep?
    
    Also, can you add a using clause for vector to avoid the std:: prefixing?



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139427>

    Any reason you could't use initializer list construction for this?



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139425>

    Can you scope this 'move' to be safe?
    
    ```
    {
      std::vector<Owned<FirewallRule>> rules;
      rules.emplace_back(new DisabledEndpointsFirewallRule(endpoints));
      process::firewall::install(std::move(rules));
    }
    ```
    
    Or avoid it entirely?
    
    ```
    process::firewall::install(
      { Owned<FirewallRule>(new DisabledEndpointsFirewallRule(endpoints)) });
    ```



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139431>

    Ditto, just "pid" is what we've been calling process ids as variables 
throughout the code.



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139430>

    Ditto above.



3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/33295/#comment139429>

    Is it possible to just pass `{}` here for empty vector?


- Ben Mahler


On June 8, 2015, 10:09 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> -----------------------------------------------------------
> 
> (Updated June 8, 2015, 10:09 a.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
> 
>

Reply via email to