mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.
Date Tue, 09 Jun 2015 00:32:05 GMT

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


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