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

Jakub Sitnicki jkbs at redhat.com
Mon Jun 25 10:38:10 UTC 2018


Hi Han,

On Thu, 21 Jun 2018 19:33:41 -0700
Han Zhou <zhouhan at gmail.com> wrote:

> 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 at ebay.com>
> ---
> Note: this patch depends on Daniel's patch [2] which is not merged yet.

Great readability improvement overall with the switch to packet
expressions that we can feed to 'inject-pkt' and 'expr-to-packets'.

I will definitely be using it from now on where possible.

> 
>  tests/ovn.at | 57 ++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 93644b0..e0b784b 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
> +
> +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_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
> +    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 && ip4 && ip.ttl==64 && ip4.src==$src_ip
> +                  && ip4.dst==$dst_ip && icmp4 && icmp4.type==$icmp_type &&
> +                  icmp4.code==0"
> +    shift; shift; shift; shift; shift; shift

Packet expression could be simplified because both 'expr-to-packets' and
'inject-pkt' commands understand prerequisites.

>      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 && ip4 &&
> +                ip.ttl==63 && ip4.src==$src_ip && ip4.dst==$dst_ip && icmp4 &&
> +                icmp4.type==$icmp_type && icmp4.code==0"
> +            echo $exp_packet | ovstest test-ovn expr-to-packets
> +
>          fi >> $outport.expected
>      done
>  }
> @@ -10099,14 +10115,18 @@ 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
> +                echo "d=$d dmac=$dmac" >> /tmp/temp_packet

Is this logging to /tmp/temp_packet a left over from debugging? Doesn't
seem to be used anywhere else.

Thanks,
Jakub

>                  if test $d != $s; then unicast=$d; else unicast=; fi
>  
>                  # packets matches ACL1 but not ACL2 should be dropped
> @@ -10115,7 +10135,14 @@ for is in 1 2 3; do
>                          unicast=
>                      fi
>                  fi
> -                test_ip $s $smac $dmac $sip $dip $unicast #1
> +                test_ip $s $slsp_mac $dmac $sip $dip 8 $unicast #1
> +
> +                # 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
> +                    test_ip $unicast $dlsp_mac $dmac $dip $sip 0 $s
> +                fi
>                done
>            done
>          done



More information about the dev mailing list