You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/05/13 02:58:25 UTC
Re: 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/#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>.
> 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
>
>