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

Han Zhou zhouhan at gmail.com
Mon Jun 25 17:05:43 UTC 2018


On Mon, Jun 25, 2018 at 3:38 AM, Jakub Sitnicki <jkbs at redhat.com> wrote:
>
> 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.
>

Agree.

> >      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.

Oops, I forgot to remove it.

>
> 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
>

Thanks for your review. I sent out v3:
https://patchwork.ozlabs.org/patch/934484/

Thanks,
Han


More information about the dev mailing list