You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2015/04/23 14:49:49 UTC
Re: Review Request 33295: Added firewall mechanism to control access
on libprocess http endpoints.
-----------------------------------------------------------
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
>
>
Re: Review Request 33295: Added firewall mechanism to control access
on libprocess http endpoints.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On April 23, 2015, 2:49 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/include/process/firewall.hpp, line 37
> > <https://reviews.apache.org/r/33295/diff/1/?file=933067#file933067line37>
> >
> > I forgot the verbally proposed name for this function, but maybe a negated result and the name `reject()` would fit better.
>
> Adam B wrote:
> Somebody suggested just using the () operator, aka the "apply" operator.
As decided in the review meeting, the () operator is not a good idea since the implementors will be called from pointers which results in weird looking syntax, i.e. either `(*foo)(socket, request)` (which looks like a function pointer or a weird looking cast) or `foo->operator()(socket, request)`. Naming was also considered ok in the meeting.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33295/#review80582
-----------------------------------------------------------
On May 1, 2015, 7:12 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 7:12 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 8aab0593f296c7aae71289f9bd6cf3eb3578a721
> 3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/process.hpp 392c74df3e8a122aecd3633dffdeec4bcbd1f097
> 3rdparty/libprocess/src/process.cpp 8ab0dbcd3e9b858244f1e67061897143204eada0
> 3rdparty/libprocess/src/tests/process_tests.cpp b6c6c511cabc525516b55d38fc4f5f596a3d6517
> docs/powered-by-mesos.md 8a0fefd07ed430f2fc81f1f3b350d451b7fafca5
>
> Diff: https://reviews.apache.org/r/33295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33295: Added firewall mechanism to control access
on libprocess http endpoints.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On April 23, 2015, 2:49 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1993
> > <https://reviews.apache.org/r/33295/diff/1/?file=933069#file933069line1993>
> >
> > `<< "': firewall rule forbids connection";` might be a bit more mesos'esque.
It follows the patter of that same function.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33295/#review80582
-----------------------------------------------------------
On April 22, 2015, 4: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, 4: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
>
>
Re: Review Request 33295: Added firewall mechanism to control access
on libprocess http endpoints.
Posted by Adam B <ad...@mesosphere.io>.
> On April 23, 2015, 5:49 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/include/process/firewall.hpp, line 37
> > <https://reviews.apache.org/r/33295/diff/1/?file=933067#file933067line37>
> >
> > I forgot the verbally proposed name for this function, but maybe a negated result and the name `reject()` would fit better.
Somebody suggested just using the () operator, aka the "apply" operator.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33295/#review80582
-----------------------------------------------------------
On April 22, 2015, 7:35 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> -----------------------------------------------------------
>
> (Updated April 22, 2015, 7:35 a.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
>
>