You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/04/14 01:45:38 UTC
Review Request 20295: Added API for managing ICMP packet filters.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Repository: mesos-git
Description
-------
This is Part 2 of the linux routing library.
Diffs
-----
src/Makefile.am 560b4c7
src/linux/routing/filter/action.hpp PRE-CREATION
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/linux/routing/filter/internal.hpp PRE-CREATION
src/linux/routing/filter/priority.hpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On April 14, 2014, 3:37 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/icmp.hpp, line 69
> > <https://reviews.apache.org/r/20295/diff/1/?file=555698#file555698line69>
> >
> > What happens if two Classifiers have overlapping set of links? The first match wins?
Each filter consists of a "classifier" and an "action".
The "action" of a filter will be taken if a packet satisfies the conditions specified by the "classifier". If a packet satisfies multiple classifiers, the priority decides the preference order.
> On April 14, 2014, 3:37 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/priority.hpp, lines 51-53
> > <https://reviews.apache.org/r/20295/diff/1/?file=555701#file555701line51>
> >
> > should these hex values be replaced by static const variables declared below in private?
The static const variables declared below are "type" part of the priority. The number here are "value" part of the priority (see comments above).
BTW: Previously, I call them "major" and "minor" part of the priority. However, seems that some linux header defines major/minor as a macro, causing some compilation issue.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40233
-----------------------------------------------------------
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On April 14, 2014, 3:37 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/icmp.cpp, line 244
> > <https://reviews.apache.org/r/20295/diff/1/?file=555699#file555699line244>
> >
> > What is the need for this 'internal' indirection?
Because we have many types of filters all need to use this general internal filter API.
> On April 14, 2014, 3:37 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/priority.hpp, lines 51-53
> > <https://reviews.apache.org/r/20295/diff/1/?file=555701#file555701line51>
> >
> > should these hex values be replaced by static const variables declared below in private?
>
> Jie Yu wrote:
> The static const variables declared below are "type" part of the priority. The number here are "value" part of the priority (see comments above).
>
> BTW: Previously, I call them "major" and "minor" part of the priority. However, seems that some linux header defines major/minor as a macro, causing some compilation issue.
I changed the semantics here. Now, we rely on the user to explicitly specify the priority, instead of creating an internal dependency. If a user does not specify a priority, the kernel will just randomly assign a priority.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40233
-----------------------------------------------------------
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40233
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/20295/#comment73173>
where are the header files?
src/linux/routing/filter/action.hpp
<https://reviews.apache.org/r/20295/#comment73176>
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment73175>
s/dstIP/destinationIP/
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment73177>
What happens if two Classifiers have overlapping set of links? The first match wins?
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73178>
A comment on what 'pack' does would be nice.
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73181>
s/valueProtocol/protocol/
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73182>
s/valueProtocolMask/mask/
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73179>
what is this doing? comment?
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73180>
what is this doing? comment?
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73185>
what is this doing? comment?.
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73183>
s/valueProtocol/protocol/
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73184>
s/valueDstIP/destinationIP/
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73186>
s/set/sets/
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73187>
What is the need for this 'internal' indirection?
src/linux/routing/filter/priority.hpp
<https://reviews.apache.org/r/20295/#comment73188>
should these hex values be replaced by static const variables declared below in private?
- Vinod Kone
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On April 15, 2014, 6:17 p.m., Cong Wang wrote:
> > src/linux/routing/filter/internal.hpp, line 213
> > <https://reviews.apache.org/r/20295/diff/1/?file=555700#file555700line213>
> >
> > We should manage it, libnl is supposed to refcount it correctly. I had a patch to fix this bug in libnl.
Added a TODO
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40428
-----------------------------------------------------------
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40428
-----------------------------------------------------------
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73425>
We should manage it, libnl is supposed to refcount it correctly. I had a patch to fix this bug in libnl.
- Cong Wang
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On April 15, 2014, 6:05 p.m., Ian Downes wrote:
> > src/linux/routing/filter/action.hpp, line 39
> > <https://reviews.apache.org/r/20295/diff/1/?file=555697#file555697line39>
> >
> > const?
I need to put then into std containers. Declaring const will make the default assignment operator not working.
> On April 15, 2014, 6:05 p.m., Ian Downes wrote:
> > src/linux/routing/filter/icmp.hpp, line 38
> > <https://reviews.apache.org/r/20295/diff/1/?file=555698#file555698line38>
> >
> > what about dropping the Ip in dstIp since it's clear from the type, i.e., just calling it 'destination'?
> >
> > Can you document what it means for the IP address to be optional?
Changed it to destinationIP. I wanna keep 'IP' here because we may wanna introduce destinationMAC:)
> On April 15, 2014, 6:05 p.m., Ian Downes wrote:
> > src/linux/routing/filter/internal.hpp, line 470
> > <https://reviews.apache.org/r/20295/diff/1/?file=555700#file555700line470>
> >
> > s/oldCls/oldClassifier/?
I have the following convention in the code:
libnl related -> cls, act, etc.
our representation -> classifier, action, etc.
> On April 15, 2014, 6:05 p.m., Ian Downes wrote:
> > src/tests/routing_tests.cpp, line 383
> > <https://reviews.apache.org/r/20295/diff/1/?file=555702#file555702line383>
> >
> > Can we write tests that confirm functionality, e.g., can we ping the interfaces? (see suggestion for IPs on 127/8)
Yeah, will follow up with more sophisticated tests!
> On April 15, 2014, 6:05 p.m., Ian Downes wrote:
> > src/tests/routing_tests.cpp, line 290
> > <https://reviews.apache.org/r/20295/diff/1/?file=555702#file555702line290>
> >
> > we probably shouldn't be using routable IPs. What about 127.1.2.3 and 127.4.5.6 so we're on the local 127.0.0.0/8?
It's OK. I just want to check if filters are setup properly. Since all filters are attached to veth-test, no packet will be flowing around.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40335
-----------------------------------------------------------
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review40335
-----------------------------------------------------------
Looks good but there's a lot going on here! Could you add documentation to the functions so we have an idea of what the libnl code is doing - e.g., the high-level steps involved in creating a filter.
src/linux/routing/filter/action.hpp
<https://reviews.apache.org/r/20295/#comment73336>
const?
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment73338>
what about dropping the Ip in dstIp since it's clear from the type, i.e., just calling it 'destination'?
Can you document what it means for the IP address to be optional?
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment73339>
const?
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment73377>
which set of links?
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment73379>
return Error()?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73383>
can "matches the user provided filter specific classifier" be simplified to "the specified classifier"?
if so, change in all comments.
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73382>
s/any type of filters/any type of filter/
here and everywhere else.
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73384>
s/alloc/allocate/?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73385>
Can you comment on the implications of this?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73386>
this looks to fit on one line. you can bring up the error check too.
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73387>
s/oldCls/oldClassifier/?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73410>
"Each filter type defines its own pack function."?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/20295/#comment73388>
ditto
src/linux/routing/filter/priority.hpp
<https://reviews.apache.org/r/20295/#comment73414>
s/The order/The priority order/
src/linux/routing/filter/priority.hpp
<https://reviews.apache.org/r/20295/#comment73390>
Can you comment on why ARP is before ICMP and IP?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20295/#comment73408>
can we have helpers for creating IP addresses so we can avoid writing them in hex! e.g., parsing a string "1.2.3.4" or 4-tuple of bytes (1, 2, 3, 4)
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20295/#comment73411>
we probably shouldn't be using routable IPs. What about 127.1.2.3 and 127.4.5.6 so we're on the local 127.0.0.0/8?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20295/#comment73418>
Can we write tests that confirm functionality, e.g., can we ping the interfaces? (see suggestion for IPs on 127/8)
- Ian Downes
On April 13, 2014, 11:45 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 2 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am 560b4c7
> src/linux/routing/filter/action.hpp PRE-CREATION
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/linux/routing/filter/internal.hpp PRE-CREATION
> src/linux/routing/filter/priority.hpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review43017
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 28, 2014, 5:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 28, 2014, 5:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 3 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am d2e006d
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On May 13, 2014, 12:58 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/icmp.hpp, lines 77-92
> > <https://reviews.apache.org/r/20295/diff/4/?file=569527#file569527line77>
> >
> > Why not take action::Action and use the dynamic cast trick in an earlier review?
Yeah, this was a deliberate design (as you saw in ARP and IP filters) that we don't expose an interface that we don't need. For example, in the current network isolator, we never add a IP filter that has mirror action, as a result, we don't support adding IP filter that has a mirror action (although it will be very trivial to add given the internal APIs).
The same philosophy applies for "Classifiers". In theory, we could support src/dest MACs, IPs and ports. But we choose to support only a subset of that that we need in the network isolator.
But since this is a library, I think it's fine to expose all of that even if we don't use (we may want to use in the future). So I am gonna do the change you suggested.
Now that we want to be generic, I think the current Classifier construction interface is not quite good and is not quite extensible. Especially, using the constructors and relying on the order specified by the user is not quite reliable. For example, to create an IP classifier:
ip::Classifier(mac, ip, ports1, ports2)
The user may wonder which one is source ports and which one is destination ports. If we add more fields in the future, it's gonna be problematic because we need to check all the use sites to make sure the order is correct after the change.
Here is what I propose for the new Classifier interface:
ip::Classifier()
.setDestinationMAC(mac)
.setDestinationIP(ip)
.setSourcePorts(ports1)
.setDestinationPorts(ports2);
If we want to introduce more fields in the future, we can simply add another 'set' function and no change to the current use sites.
What do you think?
> On May 13, 2014, 12:58 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/icmp.hpp, line 107
> > <https://reviews.apache.org/r/20295/diff/4/?file=569527#file569527line107>
> >
> > No update for Redirect action?
See comments above. We don't need that in the network isolator.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review42794
-----------------------------------------------------------
On April 28, 2014, 5:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 28, 2014, 5:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 3 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am d2e006d
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On May 13, 2014, 12:58 a.m., Vinod Kone wrote:
> > src/linux/routing/filter/icmp.hpp, lines 77-92
> > <https://reviews.apache.org/r/20295/diff/4/?file=569527#file569527line77>
> >
> > Why not take action::Action and use the dynamic cast trick in an earlier review?
>
> Jie Yu wrote:
> Yeah, this was a deliberate design (as you saw in ARP and IP filters) that we don't expose an interface that we don't need. For example, in the current network isolator, we never add a IP filter that has mirror action, as a result, we don't support adding IP filter that has a mirror action (although it will be very trivial to add given the internal APIs).
>
> The same philosophy applies for "Classifiers". In theory, we could support src/dest MACs, IPs and ports. But we choose to support only a subset of that that we need in the network isolator.
>
> But since this is a library, I think it's fine to expose all of that even if we don't use (we may want to use in the future). So I am gonna do the change you suggested.
>
> Now that we want to be generic, I think the current Classifier construction interface is not quite good and is not quite extensible. Especially, using the constructors and relying on the order specified by the user is not quite reliable. For example, to create an IP classifier:
>
> ip::Classifier(mac, ip, ports1, ports2)
>
> The user may wonder which one is source ports and which one is destination ports. If we add more fields in the future, it's gonna be problematic because we need to check all the use sites to make sure the order is correct after the change.
>
> Here is what I propose for the new Classifier interface:
>
> ip::Classifier()
> .setDestinationMAC(mac)
> .setDestinationIP(ip)
> .setSourcePorts(ports1)
> .setDestinationPorts(ports2);
>
> If we want to introduce more fields in the future, we can simply add another 'set' function and no change to the current use sites.
>
> What do you think?
>
>
>
>
Discussed with Vinod offline. We decided to go with the existing interface for now.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review42794
-----------------------------------------------------------
On April 28, 2014, 5:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 28, 2014, 5:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 3 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am d2e006d
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review42794
-----------------------------------------------------------
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment76709>
Why not take action::Action and use the dynamic cast trick in an earlier review?
src/linux/routing/filter/icmp.hpp
<https://reviews.apache.org/r/20295/#comment76710>
No update for Redirect action?
- Vinod Kone
On April 28, 2014, 5:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 28, 2014, 5:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 3 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am d2e006d
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated May 15, 2014, 12:44 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Rebased.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 3 of the linux routing library.
Diffs (updated)
-----
src/Makefile.am 12374c4
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated May 14, 2014, 11:56 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Rebased.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 3 of the linux routing library.
Diffs (updated)
-----
src/Makefile.am d2e006d
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
> On May 13, 2014, 9:56 p.m., Dominic Hamon wrote:
> > src/linux/routing/filter/icmp.cpp, line 193
> > <https://reviews.apache.org/r/20295/diff/4/?file=569528#file569528line193>
> >
> > can you get a protocol match without a destination IP?
Yes.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review42900
-----------------------------------------------------------
On April 28, 2014, 5:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 28, 2014, 5:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 3 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am d2e006d
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/#review42900
-----------------------------------------------------------
src/linux/routing/filter/icmp.cpp
<https://reviews.apache.org/r/20295/#comment76826>
can you get a protocol match without a destination IP?
- Dominic Hamon
On April 28, 2014, 10:02 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20295/
> -----------------------------------------------------------
>
> (Updated April 28, 2014, 10:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 3 of the linux routing library.
>
>
> Diffs
> -----
>
> src/Makefile.am d2e006d
> src/linux/routing/filter/icmp.hpp PRE-CREATION
> src/linux/routing/filter/icmp.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20295/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated April 28, 2014, 5:02 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Adjusted to use the new internal APIs.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description (updated)
-------
This is Part 3 of the linux routing library.
Diffs (updated)
-----
src/Makefile.am d2e006d
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated April 22, 2014, 6:04 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Updated "depends on"
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 2 of the linux routing library.
Diffs
-----
src/Makefile.am a44ea42
src/linux/routing/filter/action.hpp PRE-CREATION
src/linux/routing/filter/filter.hpp PRE-CREATION
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/linux/routing/filter/internal.hpp PRE-CREATION
src/linux/routing/filter/priority.hpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated April 22, 2014, 6:03 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Updated "Bugs".
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 2 of the linux routing library.
Diffs
-----
src/Makefile.am a44ea42
src/linux/routing/filter/action.hpp PRE-CREATION
src/linux/routing/filter/filter.hpp PRE-CREATION
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/linux/routing/filter/internal.hpp PRE-CREATION
src/linux/routing/filter/priority.hpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated April 22, 2014, 5:41 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Added ingress/egress option.
Repository: mesos-git
Description
-------
This is Part 2 of the linux routing library.
Diffs (updated)
-----
src/Makefile.am a44ea42
src/linux/routing/filter/action.hpp PRE-CREATION
src/linux/routing/filter/filter.hpp PRE-CREATION
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/linux/routing/filter/internal.hpp PRE-CREATION
src/linux/routing/filter/priority.hpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20295: Added API for managing ICMP packet filters.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20295/
-----------------------------------------------------------
(Updated April 18, 2014, 7:36 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Review comments. I changed a few internal APIs:
1) made priority setting more explicit and killed hard coded internal priority dependencies
2) left interfaces for adding more actions to a filter in the future (left a TODO)
3) allowed users to get actions attached to a filter if libnl supports that in the future
Repository: mesos-git
Description
-------
This is Part 2 of the linux routing library.
Diffs (updated)
-----
src/Makefile.am 9e715bf
src/linux/routing/filter/action.hpp PRE-CREATION
src/linux/routing/filter/icmp.hpp PRE-CREATION
src/linux/routing/filter/icmp.cpp PRE-CREATION
src/linux/routing/filter/internal.hpp PRE-CREATION
src/linux/routing/filter/priority.hpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20295/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu