mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Till Toenshoff" <toensh...@me.com>
Subject Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.
Date Thu, 23 Apr 2015 12:49:49 GMT

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


This review was pending a couple of days in my outbox, hence it may be partially or entirely
invalid by now given that you had updated the RR - sry for that. Feel free to drop any outdated
comments without further ado.


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

    That is the wrong header, we have to use the "Apache License V.2" header for stout and
libprocess. The "ASF" header you are using is for mesos only.
    
    The correct header would be:
    ```
    /**
     * Licensed under the Apache License, Version 2.0 (the "License");
     * you may not use this file except in compliance with the License.
     * You may obtain a copy of the License at
     *
     *     http://www.apache.org/licenses/LICENSE-2.0
     *
     * Unless required by applicable law or agreed to in writing, software
     * distributed under the License is distributed on an "AS IS" BASIS,
     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
     * See the License for the specific language governing permissions and
     * limitations under the License
     */
    ```



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

    I forgot the verbally proposed name for this function, but maybe a negated result and
the name `reject()` would fit better.



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

    Even though this is implementation is tiny, would it still make sense to move it into
a cpp-file?



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

    Could you elaborate on the term "__top__ firewall rule"?



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

    The doxygen rendered output IMHO looks better when using a capital letter on the start
of description -- however, it seems the other argument descriptions start with a lower case.



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

    `<< "': firewall rule forbids connection";` might be a bit more mesos'esque.



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

    const string



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

    const string



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

    Given that you simply take a copy of the firewallRule instance var, the "right" name would
be `firewallRule_`, no?


- Till Toenshoff


On April 22, 2015, 2:35 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, 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 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 392c74df3e8a122aecd3633dffdeec4bcbd1f097

>   3rdparty/libprocess/src/process.cpp 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp eb38edce2c483ba7f963a826893a15a075238618

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