| Message ID | 1529634821-10548-1-git-send-email-hzhou8@ebay.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovn.at: Add stateful test for ACL on port groups. | expand |
Hi Han, On Thu, 21 Jun 2018 19:33:41 -0700 Han Zhou <zhouhan@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@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
On Mon, Jun 25, 2018 at 3:38 AM, Jakub Sitnicki <jkbs@redhat.com> wrote: > > Hi Han, > > On Thu, 21 Jun 2018 19:33:41 -0700 > Han Zhou <zhouhan@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@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
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 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 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
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. tests/ovn.at | 57 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 15 deletions(-)