[ovs-dev] [PATCH 3/8] Add support for connection tracking.

Jarno Rajahalme jrajahalme at nicira.com
Fri Sep 11 23:37:30 UTC 2015


Here is an provisional ACK, I trust you to address the comments to my satisfaction :-)

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

  Jarno

> On Sep 11, 2015, at 4:15 PM, Joe Stringer <joestringer at nicira.com> wrote:
> 
> On 11 September 2015 at 14:42, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index 7dbed68..de6b016 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -139,3 +139,472 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PI
>>> 
>>> OVS_TRAFFIC_VSWITCHD_STOP
>>> AT_CLEANUP
>>> +
>>> +AT_SETUP([conntrack - controller])
>>> +CHECK_CONNTRACK()
>>> +OVS_TRAFFIC_VSWITCHD_START(
>>> +   [set-fail-mode br0 standalone -- ])
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1)
>>> +
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>>> +
>>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
>>> +AT_DATA([flows.txt], [dnl
>>> +priority=1,action=drop
>>> +priority=10,arp,action=normal
>>> +in_port=1,udp,action=ct(commit),controller
>>> +in_port=2,ct_state=-trk,udp,action=ct(table=0)
>>> +in_port=2,ct_state=+trk+est-new,udp,action=controller
>> 
>> ct_state=+est should be sufficient here?
> 
> True, +est-new is a little redundant.
> 
>>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
>>> +AT_DATA([flows.txt], [dnl
>>> +priority=1,action=drop
>>> +priority=10,arp,action=normal
>>> +priority=10,icmp,action=normal
>>> +in_port=1,tcp,action=ct(commit),2
>>> +in_port=2,ct_state=-trk,tcp,action=ct(table=0)
>>> +in_port=2,ct_state=+trk+est-new,tcp,action=1
>> 
>> Having explicit priorities here as well would be helpful. The comment above about “+est” should apply here as well.
> 
> I can update any/all tests that have priorities to either all provide
> specific priorities, or use no priorities.
> 
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>>> +
>>> +dnl Basic connectivity check.
>>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 >/dev/null])
>>> +
>>> +dnl HTTP requests from ns0->ns1 should work fine.
>>> +NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid])
>>> +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log])
>> 
>> 
>> wget was trying to resolve the proxy I had configured in the “http_proxy” environment variable. The resolution from the netns failed, making the test case fail. I had to clear “http_proxy" to make the test work.
> 
> Hmm, I don't have a good solution for this. Personally I always push
> code to a remote test VM that I use specifically for the purpose of
> running these tests, so any configuration I might have in my local
> desktop environment wouldn't cause issues such as this. (This is
> equivalent to how the Vagrant tests run)

Maybe a comment in some doc describing how to run check-kernel?



> 
>>> +
>>> +AT_CHECK([conntrack -L 2>&1 | FORMAT_CT(10.1.1.2)], [0], [dnl
>>> +TIME_WAIT src=10.1.1.1 dst=10.1.1.2 sport=<cleared> dport=<cleared> src=10.1.1.2 dst=10.1.1.1 sport=<cleared> dport=<cleared> [[ASSURED]] mark=0 use=1
>> 
>> Do you know how/if [ASSURED] maps to ct_state flags?
> 
> I can look into it. My understanding is that it's equivalent to
> "established", although it may be more aligned with our concept of
> "committed".
> 
>>> +AT_SETUP([conntrack - commit, recirc])
>>> +CHECK_CONNTRACK()
>>> +OVS_TRAFFIC_VSWITCHD_START(
>>> +   [set-fail-mode br0 standalone -- ])
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3)
>>> +
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
>>> +ADD_VETH(p3, at_ns3, br0, "10.1.1.4/24")
>>> +
>>> +dnl Allow any traffic from ns0->ns1, ns2->ns3.
>>> +AT_DATA([flows.txt], [dnl
>>> +priority=1,action=drop
>>> +priority=10,arp,action=normal
>>> +priority=10,icmp,action=normal
>>> +in_port=1,tcp,ct_state=-trk,action=ct(commit,table=0)
>> 
>> Maybe some of the test cases should use non-zero table?
> 
> Agreed, there's several directions that we can increase the coverage
> in the testsuite. I'll add this to the list.
> 
>>> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3)
>>> +
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
>>> +ADD_VETH(p3, at_ns3, br0, "10.1.1.4/24")
>>> +
>>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
>> 
>> Update comment?
> 
> For ns2/ns3 as well? Sure.
> 
>>> +AT_DATA([flows.txt], [dnl
>>> +priority=1,action=drop
>>> +priority=10,arp,action=normal
>>> +priority=10,icmp,action=normal
>>> +in_port=1,tcp,action=ct(),2
>>> +in_port=2,ct_state=-trk,tcp,action=ct(table=0)
>>> +in_port=2,ct_state=+trk+new,tcp,action=1
>> 
>> Here “+new” should be sufficient, in general, “+trk” is not needed if any of the other bits are matched for being set, right?
> 
> For several of these I have made the rule more explicit than strictly
> necessary, so that if there is an unexpected combination of bits, then
> those flows will hit the "drop" rule rather than silently matching the
> accept rule.
> 
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
>>> +ADD_VETH(p3, at_ns3, br0, "10.1.1.4/24")
>>> +
>>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
>> 
>> Does this comment need an update?
> 
> Yes, I'll update it. I can see the fallacy of copy-paste test crafting
> here: The comments are initially added to improve readability, but
> when they're copied several times it's easy to miss the fact that
> they're out of date. It wouldn't be a problem if there were no
> comments, but then it would be more difficult to grok what's going on.
> 
>>> +dnl Allow any traffic from ns0->ns1, allow established in reverse.
>>> +AT_DATA([flows-br0.txt], [dnl
>>> +priority=1,action=drop
>>> +priority=10,arp,action=normal
>>> +priority=10,icmp,action=normal
>>> +in_port=2,tcp,ct_state=-trk,action=ct(commit,zone=1),1
>>> +in_port=1,tcp,ct_state=-trk,action=ct(table=0,zone=1)
>>> +in_port=1,tcp,ct_state=+trk+est,ct_zone=1,action=2
>>> +])
>>> +
>>> +dnl Allow any traffic from ns0->ns1, allow established in reverse.
>> 
>> Should this comment be different from the one above??
> 
> I think it's saying the same thing, it just wasn't copy/pasted from
> the earlier one. I'll review each of these comments.
> 
>>> +AT_SETUP([conntrack - ICMP related 2])
>>> +CHECK_CONNTRACK()
>>> +OVS_TRAFFIC_VSWITCHD_START(
>>> +   [set-fail-mode br0 standalone -- ])
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1)
>>> +
>>> +ADD_VETH(p0, at_ns0, br0, "172.16.0.1/24")
>>> +ADD_VETH(p1, at_ns1, br0, "172.16.0.2/24")
>>> +
>>> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
>>> +AT_DATA([flows.txt], [dnl
>>> +priority=1,action=drop
>>> +priority=10,arp,action=normal
>>> +in_port=1,ct_state=-trk,udp,action=ct(commit,table=0)
>>> +in_port=1,ct_state=+trk,actions=controller
>>> +in_port=2,ct_state=-trk,action=ct(table=0)
>>> +in_port=2,ct_state=+trk-inv-new,action=controller
>> 
>> Could there be a match on +rel here?
> 
> Yes, we could expand the flows to be more explicit about the exact
> packets seen (looks like there's a "new" and a "related, reply"
> packets)
> 
> Thanks for the thorough review,
> 
>>> diff --git a/tests/test-odp.c b/tests/test-odp.c
>>> index 3e7939e..cb245d6 100644
>>> --- a/tests/test-odp.c
>>> +++ b/tests/test-odp.c
>>> @@ -57,7 +57,10 @@ parse_keys(bool wc_keys)
>>>            struct odp_flow_key_parms odp_parms = {
>>>                .flow = &flow,
>>>                .support = {
>>> +                    .max_mpls_depth = SIZE_MAX,
>> 
>> Why this?
> 
> I think this was intended to be in a separate patch. From memory, if
> any of the .support fields are false, then we don't test ODP parsing
> of that feature.
> 
> Cheers,
> Joe




More information about the dev mailing list