On 11 September 2015 at 14:42, Jarno Rajahalme <jrajaha...@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)
>> +
>> +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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
