[ovs-dev,v2] ovn.at: Add stateful test for ACL on port groups.

Message ID 1529945790-20901-1-git-send-email-hzhou8@ebay.com
State New
Headers show
Series
  • [ovs-dev,v2] ovn.at: Add stateful test for ACL on port groups.
Related show

Commit Message

Han Zhou June 25, 2018, 4:56 p.m.
A bug was reported on the feature of applying ACLs on port groups [1].
This bug was not detected by the original test case, because it didn't
test the return traffic and so didn't ensure the stateful feature is
working. The fix [2] causes the original test case fail, because
once the conntrack is enabled, the test packets are dropped because
the checksum in those packets are invalid and so marked with "invalid"
state by conntrack. To avoid the test case failure, the fix [2] changed
it to test stateless acl only, which leaves the scenario untested,
although it is fixed. This patch adds back the stateful ACL in the
test, and replaced the dummy/receive with inject-pkt to send the test
packets, so that checksums can be properly filled in, and it also
adds tests for the return traffic, which ensures the stateful is
working.

[1] https://mail.openvswitch.org/pipermail/ovs-discuss/2018-June/046927.html

[2] https://patchwork.ozlabs.org/patch/931913/

Signed-off-by: Han Zhou <hzhou8@ebay.com>
---
Note: this patch depends on Daniel's patch [2] which is not merged yet.
v1->v2: 
    - Addressed Jacub's comments - simplified packet expr and removed
      debug information.
    - Renamed test_ip to test_icmp.

 tests/ovn.at | 60 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 16 deletions(-)

Comments

0-day Robot June 25, 2018, 7:57 p.m. | #1
Bleep bloop.  Greetings Han Zhou, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
== Checking 6cbdf56ce88d ("datapath-windows: Compute ct hash based on 5-tuple and zone") ==
ERROR: Too many signoffs; are you missing Co-authored-by lines?
Lines checked: 320, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Aaron Conole June 25, 2018, 8:03 p.m. | #2
0-day Robot <robot@bytheb.org> writes:

> Bleep bloop.  Greetings Han Zhou, I am a robot and I have tried out your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> == Checking 6cbdf56ce88d ("datapath-windows: Compute ct hash based on
> 5-tuple and zone") ==
> ERROR: Too many signoffs; are you missing Co-authored-by lines?
> Lines checked: 320, Warnings: 0, Errors: 1

Sorry for the noise.  I'll debug this error.

> Please check this out.  If you feel there has been an error, please
> email aconole@bytheb.org
>
> Thanks,
> 0-day Robot

Patch

diff --git a/tests/ovn.at b/tests/ovn.at
index 93644b0..7317d39 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9981,7 +9981,7 @@  ovn-nbctl create Port_Group name=pg2 ports="$pg2_ports"
 # create ACLs on pg1 to drop traffic from pg2 to pg1
 ovn-nbctl acl-add pg1 to-lport 1001 'outport == @pg1' drop
 ovn-nbctl --type=port-group acl-add pg1 to-lport 1002 \
-        'outport == @pg1 && ip4.src == $pg2_ip4' allow
+        'outport == @pg1 && ip4.src == $pg2_ip4' allow-related
 
 # Physical network:
 #
@@ -10043,7 +10043,7 @@  OVN_POPULATE_ARP
 # XXX This should be more systematic.
 sleep 1
 
-# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT...
+# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP ICMP_TYPE OUTPORT...
 #
 # This shell function causes a packet to be received on INPORT.  The packet's
 # content has Ethernet destination DST and source SRC (each exactly 12 hex
@@ -10057,26 +10057,42 @@  for i in 1 2 3; do
         done
     done
 done
-test_ip() {
-    # This packet has bad checksums but logical L3 routing doesn't check.
-    local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
-    local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000
-    shift; shift; shift; shift; shift
+
+lsp_to_mac() {
+    echo f0:00:00:00:0${1:0:1}:${1:1:2}
+}
+
+lrp_to_mac() {
+    echo 00:00:00:00:ff:$1
+}
+
+test_icmp() {
+    local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 icmp_type=$6
+    local packet="inport==\"lp$inport\" && eth.src==$src_mac &&
+                  eth.dst==$dst_mac && ip.ttl==64 && ip4.src==$src_ip
+                  && ip4.dst==$dst_ip && icmp4.type==$icmp_type &&
+                  icmp4.code==0"
+    shift; shift; shift; shift; shift; shift
     hv=hv`vif_to_hv $inport`
-    as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
-    #as $hv ovs-appctl ofproto/trace br-int in_port=$inport $packet
+    as $hv ovs-appctl -t ovn-controller inject-pkt "$packet"
     in_ls=`vif_to_ls $inport`
     in_lrp=`vif_to_lrp $inport`
     for outport; do
         out_ls=`vif_to_ls $outport`
         if test $in_ls = $out_ls; then
             # Ports on the same logical switch receive exactly the same packet.
-            echo $packet
+            echo $packet | ovstest test-ovn expr-to-packets
         else
             # Routing decrements TTL and updates source and dest MAC
             # (and checksum).
             out_lrp=`vif_to_lrp $outport`
-            echo f00000000${outport}00000000ff${out_lrp}08004500001c00000000"3f1101"00${src_ip}${dst_ip}0035111100080000
+            exp_smac=`lrp_to_mac $out_lrp`
+            exp_dmac=`lsp_to_mac $outport`
+            exp_packet="eth.src==$exp_smac && eth.dst==$exp_dmac &&
+                ip.ttl==63 && ip4.src==$src_ip && ip4.dst==$dst_ip &&
+                icmp4.type==$icmp_type && icmp4.code==0"
+            echo $exp_packet | ovstest test-ovn expr-to-packets
+
         fi >> $outport.expected
     done
 }
@@ -10099,14 +10115,17 @@  for is in 1 2 3; do
     for ks in 1 2 3; do
       bcast=
       s=$is$js$ks
-      smac=f00000000$s
-      sip=`ip_to_hex 192 168 $is$js $ks`
+      slsp_mac=`lsp_to_mac $s`
+      slrp_mac=`lrp_to_mac $is$js`
+      sip=192.168.$is$js.$ks
       for id in 1 2 3; do
           for jd in 1 2 3; do
               for kd in 1 2 3; do
                 d=$id$jd$kd
-                dip=`ip_to_hex 192 168 $id$jd $kd`
-                if test $is = $id; then dmac=f00000000$d; else dmac=00000000ff$is$js; fi
+                dlsp_mac=`lsp_to_mac $d`
+                dlrp_mac=`lrp_to_mac $id$jd`
+                dip=192.168.$id$jd.$kd
+                if test $is = $id; then dmac=$dlsp_mac; else dmac=$slrp_mac; fi
                 if test $d != $s; then unicast=$d; else unicast=; fi
 
                 # packets matches ACL1 but not ACL2 should be dropped
@@ -10115,7 +10134,16 @@  for is in 1 2 3; do
                         unicast=
                     fi
                 fi
-                test_ip $s $smac $dmac $sip $dip $unicast #1
+                # icmp request (type = 8)
+                test_icmp $s $slsp_mac $dmac $sip $dip 8 $unicast
+
+                # if packets are not dropped, test the return traffic (icmp echo)
+                # to make sure stateful works, too.
+                if test x$unicast != x; then
+                    if test $is = $id; then dmac=$slsp_mac; else dmac=$dlrp_mac; fi
+                    # icmp echo (type = 0)
+                    test_icmp $unicast $dlsp_mac $dmac $dip $sip 0 $s
+                fi
               done
           done
         done