Hi Han, On Thu, 21 Jun 2018 19:33:41 -0700 Han Zhou <[email protected]> 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 <[email protected]> > --- > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
