| Message ID | 20230306110700.2107451-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] northd: prevents sending packet to conntrack for router ports | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, Mar 6, 2023 at 12:07 PM Xavier Simonart <xsimonar@redhat.com> wrote: > As commented in northd.c, we should not use ct() for router ports. > When there are no stateful_acl, this patch prevents sending packet to > conntrack > for router ports. > The patch does this by issuing ct_clear in ls_out_pre_lb stage so that > hints > are not set in ls_out_acl_hint and ls_out_acl stages. > > Note that ct_clear is not added for ingress for router ports as already > done > for patch ports (no change by this patch on this aspect). > > Also, this patch does not change the behavior for ACLs such as > allow-related: > packets are still sent to conntrack, even for router ports. While this does > not work if router ports are distributed, allow-related ACLs work today on > router ports when those ports are handled on the same chassis for ingress > and > egress traffic. This patch does not change that behavior. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431 > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > v2: - handled Dumitru's comments > - handled Ales' comments > - added change to xml documentation > - do not do ct_clear for ingress as already done > --- > northd/northd.c | 24 +++--- > northd/ovn-northd.8.xml | 11 +++ > tests/ovn-northd.at | 11 +-- > tests/system-ovn.at | 166 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 197 insertions(+), 15 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 7ad4cdfad..b356faf64 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5779,20 +5779,24 @@ skip_port_from_conntrack(struct ovn_datapath *od, > struct ovn_port *op, > * know about the connection, as the icmp request went through the > logical > * router on hostA, not hostB. This would only work with distributed > * conntrack state across all chassis. */ > - struct ds match_in = DS_EMPTY_INITIALIZER; > - struct ds match_out = DS_EMPTY_INITIALIZER; > > - ds_put_format(&match_in, "ip && inport == %s", op->json_key); > - ds_put_format(&match_out, "ip && outport == %s", op->json_key); > + const char *ingress_action = "next;"; > + const char *egress_action = od->has_stateful_acl > + ? "next;" > + : "ct_clear; next;"; > + > + char *ingress_match = xasprintf("ip && inport == %s", op->json_key); > + char *egress_match = xasprintf("ip && outport == %s", op->json_key); > + > ovn_lflow_add_with_lport_and_hint(lflows, od, in_stage, priority, > - ds_cstr(&match_in), "next;", > op->key, > - &op->nbsp->header_); > + ingress_match, ingress_action, > + op->key, &op->nbsp->header_); > ovn_lflow_add_with_lport_and_hint(lflows, od, out_stage, priority, > - ds_cstr(&match_out), "next;", > op->key, > - &op->nbsp->header_); > + egress_match, egress_action, > + op->key, &op->nbsp->header_); > > - ds_destroy(&match_in); > - ds_destroy(&match_out); > + free(ingress_match); > + free(egress_match); > } > > static void > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 2eab2c4ae..efadfe808 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -2056,6 +2056,17 @@ output; > db="OVN_Northbound"/> table. > </p> > > + <p> > + This table also has a priority-110 flow with the match > + <code>outport == <var>I</var></code> for all logical switch > + datapaths to move traffic to the next table, and, if there are no > + stateful_acl, clear the ct_state. Where <var>I</var> > + is the peer of a logical router port. This flow is added to > + skip the connection tracking of packets which will be entering > + logical router datapath from logical switch datapath for routing. > + </p> > + > + > <h3>Egress Table 2: Pre-stateful</h3> > > <p> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 3fa02d2b3..d0f6893e9 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -4111,6 +4111,7 @@ check ovn-nbctl lsp-set-options sw0-lr0 > router-port=lr0-sw0 > check ovn-nbctl --wait=sb sync > > check_stateful_flows() { > + action=$1 > ovn-sbctl dump-flows sw0 > sw0flows > AT_CAPTURE_FILE([sw0flows]) > > @@ -4144,12 +4145,12 @@ check_stateful_flows() { > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && > reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = > reg3; }; next;) > ]) > > - AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl > + AT_CHECK_UNQUOTED([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl > table=1 (ls_out_pre_lb ), priority=0 , match=(1), action=(next;) > table=1 (ls_out_pre_lb ), priority=100 , match=(ip), > action=(reg0[[2]] = 1; next;) > table=1 (ls_out_pre_lb ), priority=110 , match=(eth.mcast), > action=(next;) > - table=1 (ls_out_pre_lb ), priority=110 , match=(eth.src == > $svc_monitor_mac), action=(next;) > - table=1 (ls_out_pre_lb ), priority=110 , match=(ip && outport == > "sw0-lr0"), action=(next;) > + table=1 (ls_out_pre_lb ), priority=110 , match=(eth.src == > \$svc_monitor_mac), action=(next;) > + table=1 (ls_out_pre_lb ), priority=110 , match=(ip && outport == > "sw0-lr0"), action=($action) > table=1 (ls_out_pre_lb ), priority=110 , match=(nd || nd_rs || > nd_ra || mldv1 || mldv2), action=(next;) > table=1 (ls_out_pre_lb ), priority=110 , match=(reg0[[16]] == 1), > action=(next;) > ]) > @@ -4169,13 +4170,13 @@ check_stateful_flows() { > ]) > } > > -check_stateful_flows > +check_stateful_flows "ct_clear; next;" > > # Add few ACLs > check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 "ip4 && tcp && > tcp.dst == 80" allow-related > check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 "ip4 && tcp && > tcp.src == 80" drop > > -check_stateful_flows > +check_stateful_flows "next;" > > # Remove load balancers from sw0 > check ovn-nbctl ls-lb-del sw0 lb0 > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 84a459d6a..73636feac 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10706,3 +10706,169 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > /connection dropped.*/d"]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ACL and committing to conntrack]) > +AT_KEYWORDS([acl]) > + > +CHECK_CONNTRACK() > +CHECK_CONNTRACK_NAT() > +ovn_start > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > + > +start_daemon ovn-controller > + > +check ovn-nbctl lr-add r1 > +check ovn-nbctl lrp-add r1 r1_s1 00:de:ad:fe:00:01 173.0.1.1/24 > +check ovn-nbctl lrp-add r1 r1_s2 00:de:ad:fe:00:02 173.0.2.1/24 > + > +check ovn-nbctl ls-add s1 > +check ovn-nbctl lsp-add s1 s1_r1 > +check ovn-nbctl lsp-set-type s1_r1 router > +check ovn-nbctl lsp-set-addresses s1_r1 router > +check ovn-nbctl lsp-set-options s1_r1 router-port=r1_s1 > + > +check ovn-nbctl ls-add s2 > +check ovn-nbctl lsp-add s2 s2_r1 > +check ovn-nbctl lsp-set-type s2_r1 router > +check ovn-nbctl lsp-set-addresses s2_r1 router > +check ovn-nbctl lsp-set-options s2_r1 router-port=r1_s2 > + > +check ovn-nbctl lsp-add s1 vm1 > +check ovn-nbctl lsp-set-addresses vm1 "00:de:ad:01:00:01 173.0.1.2" > + > +check ovn-nbctl lsp-add s2 vm2 > +check ovn-nbctl lsp-set-addresses vm2 "00:de:ad:01:00:02 173.0.2.2" > + > +check ovn-nbctl lsp-add s2 vm3 > +check ovn-nbctl lsp-set-addresses vm3 "00:de:ad:01:00:03 173.0.2.3" > + > +check ovn-nbctl lb-add lb1 30.0.0.1:80 173.0.2.2:80 udp > +check ovn-nbctl lb-add lb2 20.0.0.1:80 173.0.1.2:80 udp > +check ovn-nbctl lb-add lb1 30.0.0.1 173.0.2.2 > +check ovn-nbctl lb-add lb2 173.0.2.250 173.0.1.3 > +check ovn-nbctl ls-lb-add s1 lb1 > +check ovn-nbctl ls-lb-add s2 lb2 > + > +ADD_NAMESPACES(vm1) > +ADD_VETH(vm1, vm1, br-int, "173.0.1.2/24", "00:de:ad:01:00:01", \ > + "173.0.1.1") > +ADD_NAMESPACES(vm2) > +ADD_VETH(vm2, vm2, br-int, "173.0.2.2/24", "00:de:ad:01:00:02", \ > + "173.0.2.1") > +ADD_NAMESPACES(vm3) > +ADD_VETH(vm3, vm3, br-int, "173.0.2.250/24", "00:de:ad:01:00:03", \ > + "173.0.2.1") > + > +check ovn-nbctl acl-add s1 from-lport 1001 "ip" allow > +check ovn-nbctl acl-add s1 to-lport 1002 "ip" allow > +check ovn-nbctl acl-add s2 from-lport 1003 "ip" allow > +check ovn-nbctl acl-add s2 to-lport 1004 "ip" allow > +check ovn-nbctl --wait=hv sync > +AS_BOX([initial ping]) > +# Send ping in background. Same ping, same flow throughout the test > +on_exit 'kill $(pidof ping)' > +NS_EXEC([vm1], [ping -c 10000 -i 0.1 30.0.0.1 > icmp.txt &]) > + > +# Check for conntrack entries > +OVS_WAIT_FOR_OUTPUT([ > + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(173.0.1.2) | \ > + sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > > +icmp,orig=(src=173.0.1.2,dst=173.0.2.2,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared> > > +icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared>,mark=2 > +]) > + > +# Now check for multiple ct_commits > +ovs-appctl dpctl/dump-flows > dp_flows > +zone_id=$(ovn-appctl -t ovn-controller ct-zone-list | grep vm1 | cut -d ' > ' -f2) > +AT_CHECK([test 1 = `cat dp_flows | grep "commit,zone=$zone_id" | wc -l`]) > + > +check ovn-nbctl acl-del s1 from-lport 1001 "ip" > +check ovn-nbctl acl-del s1 to-lport 1002 "ip" > +check ovn-nbctl acl-del s2 from-lport 1003 "ip" > +check ovn-nbctl acl-del s2 to-lport 1004 "ip" > + > +AS_BOX([acl drop echo request]) > +check ovn-nbctl --log --severity=alert --name=drop-flow-s1 acl-add s1 > to-lport 2001 icmp4 drop > +# acl-drop to-lport s1 apply to traffic from s1 to vm1 and s1 to r1. > +check ovn-nbctl --wait=hv sync > + > +# Check that traffic is blocked > +# Wait for some packets to hit the rule to avoid potential race > conditions. Then count packets. > +OVS_WAIT_UNTIL([test `cat ovn-controller.log | grep acl_log | grep -c > drop-flow-s1` -gt "0"]) > +total_icmp_pkts=$(cat icmp.txt | grep ttl | wc -l) > + > +# Wait some time and check whether packets went through. In the worse > race condition, the sleep is too short > +# and this test will still succeed. > +sleep 1 > +OVS_WAIT_UNTIL([ > + total_icmp1_pkts=$(cat icmp.txt | grep ttl | wc -l) > + test "${total_icmp1_pkts}" -eq "${total_icmp_pkts}" > +]) > + > +AS_BOX([acl allow-related echo request]) > +check ovn-nbctl acl-add s1 to-lport 2002 "icmp4 && ip4.src == 173.0.1.2" > allow-related > +# This rule has higher priority than to-lport 2001 icmp4 drop. > +# So traffic from s1 (w/ src=173.0.1.2) to r1 should be accepted > +# (return) traffic from s1 to vm1 should be accepted as return traffic > +check ovn-nbctl --wait=hv sync > +OVS_WAIT_UNTIL([ > + total_icmp1_pkts=$(cat icmp.txt | grep ttl | wc -l) > + test "${total_icmp1_pkts}" -gt "${total_icmp_pkts}" > +]) > + > +# Check we did not break handling acl-drop for existing flows > +AS_BOX([acl drop echo request in s2]) > +check ovn-nbctl acl-del s1 to-lport 2001 icmp4 > +check ovn-nbctl --log --severity=alert --name=drop-flow-s2 acl-add s2 > to-lport 2001 icmp4 drop > +check ovn-nbctl --wait=hv sync > + > +OVS_WAIT_UNTIL([test `cat ovn-controller.log | grep acl_log | grep -c > drop-flow-s2` -gt "0"]) > + > +OVS_WAIT_FOR_OUTPUT([ > + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \ > + sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > + sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl > > +icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared> > +]) > +total_icmp_pkts=$(cat icmp.txt | grep ttl | wc -l) > + > +# Allow ping again > +AS_BOX([acl allow echo request in s2]) > +check ovn-nbctl acl-add s2 to-lport 2005 icmp4 allow > +check ovn-nbctl --wait=hv sync > +OVS_WAIT_FOR_OUTPUT([ > + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \ > + sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > > +icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared>,mark=2 > +]) > +OVS_WAIT_UNTIL([ > + total_icmp1_pkts=$(cat icmp.txt | grep ttl | wc -l) > + test "${total_icmp1_pkts}" -gt "${total_icmp_pkts}" > +]) > + > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > +AT_CLEANUP > +]) > + > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 3/7/23 09:27, Ales Musil wrote: > On Mon, Mar 6, 2023 at 12:07 PM Xavier Simonart <xsimonar@redhat.com> wrote: > >> As commented in northd.c, we should not use ct() for router ports. >> When there are no stateful_acl, this patch prevents sending packet to >> conntrack >> for router ports. >> The patch does this by issuing ct_clear in ls_out_pre_lb stage so that >> hints >> are not set in ls_out_acl_hint and ls_out_acl stages. >> >> Note that ct_clear is not added for ingress for router ports as already >> done >> for patch ports (no change by this patch on this aspect). >> >> Also, this patch does not change the behavior for ACLs such as >> allow-related: >> packets are still sent to conntrack, even for router ports. While this does >> not work if router ports are distributed, allow-related ACLs work today on >> router ports when those ports are handled on the same chassis for ingress >> and >> egress traffic. This patch does not change that behavior. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431 >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> >> --- >> v2: - handled Dumitru's comments >> - handled Ales' comments >> - added change to xml documentation >> - do not do ct_clear for ingress as already done >> --- [...] >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> > Thanks, Xavier and Ales! I applied this to the main branch! Regards, Dumitru
diff --git a/northd/northd.c b/northd/northd.c index 7ad4cdfad..b356faf64 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5779,20 +5779,24 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, * know about the connection, as the icmp request went through the logical * router on hostA, not hostB. This would only work with distributed * conntrack state across all chassis. */ - struct ds match_in = DS_EMPTY_INITIALIZER; - struct ds match_out = DS_EMPTY_INITIALIZER; - ds_put_format(&match_in, "ip && inport == %s", op->json_key); - ds_put_format(&match_out, "ip && outport == %s", op->json_key); + const char *ingress_action = "next;"; + const char *egress_action = od->has_stateful_acl + ? "next;" + : "ct_clear; next;"; + + char *ingress_match = xasprintf("ip && inport == %s", op->json_key); + char *egress_match = xasprintf("ip && outport == %s", op->json_key); + ovn_lflow_add_with_lport_and_hint(lflows, od, in_stage, priority, - ds_cstr(&match_in), "next;", op->key, - &op->nbsp->header_); + ingress_match, ingress_action, + op->key, &op->nbsp->header_); ovn_lflow_add_with_lport_and_hint(lflows, od, out_stage, priority, - ds_cstr(&match_out), "next;", op->key, - &op->nbsp->header_); + egress_match, egress_action, + op->key, &op->nbsp->header_); - ds_destroy(&match_in); - ds_destroy(&match_out); + free(ingress_match); + free(egress_match); } static void diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 2eab2c4ae..efadfe808 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -2056,6 +2056,17 @@ output; db="OVN_Northbound"/> table. </p> + <p> + This table also has a priority-110 flow with the match + <code>outport == <var>I</var></code> for all logical switch + datapaths to move traffic to the next table, and, if there are no + stateful_acl, clear the ct_state. Where <var>I</var> + is the peer of a logical router port. This flow is added to + skip the connection tracking of packets which will be entering + logical router datapath from logical switch datapath for routing. + </p> + + <h3>Egress Table 2: Pre-stateful</h3> <p> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 3fa02d2b3..d0f6893e9 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -4111,6 +4111,7 @@ check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 check ovn-nbctl --wait=sb sync check_stateful_flows() { + action=$1 ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) @@ -4144,12 +4145,12 @@ check_stateful_flows() { table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) ]) - AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl + AT_CHECK_UNQUOTED([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl table=1 (ls_out_pre_lb ), priority=0 , match=(1), action=(next;) table=1 (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[2]] = 1; next;) table=1 (ls_out_pre_lb ), priority=110 , match=(eth.mcast), action=(next;) - table=1 (ls_out_pre_lb ), priority=110 , match=(eth.src == $svc_monitor_mac), action=(next;) - table=1 (ls_out_pre_lb ), priority=110 , match=(ip && outport == "sw0-lr0"), action=(next;) + table=1 (ls_out_pre_lb ), priority=110 , match=(eth.src == \$svc_monitor_mac), action=(next;) + table=1 (ls_out_pre_lb ), priority=110 , match=(ip && outport == "sw0-lr0"), action=($action) table=1 (ls_out_pre_lb ), priority=110 , match=(nd || nd_rs || nd_ra || mldv1 || mldv2), action=(next;) table=1 (ls_out_pre_lb ), priority=110 , match=(reg0[[16]] == 1), action=(next;) ]) @@ -4169,13 +4170,13 @@ check_stateful_flows() { ]) } -check_stateful_flows +check_stateful_flows "ct_clear; next;" # Add few ACLs check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 "ip4 && tcp && tcp.dst == 80" allow-related check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 "ip4 && tcp && tcp.src == 80" drop -check_stateful_flows +check_stateful_flows "next;" # Remove load balancers from sw0 check ovn-nbctl ls-lb-del sw0 lb0 diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 84a459d6a..73636feac 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10706,3 +10706,169 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /connection dropped.*/d"]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ACL and committing to conntrack]) +AT_KEYWORDS([acl]) + +CHECK_CONNTRACK() +CHECK_CONNTRACK_NAT() +ovn_start +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +start_daemon ovn-controller + +check ovn-nbctl lr-add r1 +check ovn-nbctl lrp-add r1 r1_s1 00:de:ad:fe:00:01 173.0.1.1/24 +check ovn-nbctl lrp-add r1 r1_s2 00:de:ad:fe:00:02 173.0.2.1/24 + +check ovn-nbctl ls-add s1 +check ovn-nbctl lsp-add s1 s1_r1 +check ovn-nbctl lsp-set-type s1_r1 router +check ovn-nbctl lsp-set-addresses s1_r1 router +check ovn-nbctl lsp-set-options s1_r1 router-port=r1_s1 + +check ovn-nbctl ls-add s2 +check ovn-nbctl lsp-add s2 s2_r1 +check ovn-nbctl lsp-set-type s2_r1 router +check ovn-nbctl lsp-set-addresses s2_r1 router +check ovn-nbctl lsp-set-options s2_r1 router-port=r1_s2 + +check ovn-nbctl lsp-add s1 vm1 +check ovn-nbctl lsp-set-addresses vm1 "00:de:ad:01:00:01 173.0.1.2" + +check ovn-nbctl lsp-add s2 vm2 +check ovn-nbctl lsp-set-addresses vm2 "00:de:ad:01:00:02 173.0.2.2" + +check ovn-nbctl lsp-add s2 vm3 +check ovn-nbctl lsp-set-addresses vm3 "00:de:ad:01:00:03 173.0.2.3" + +check ovn-nbctl lb-add lb1 30.0.0.1:80 173.0.2.2:80 udp +check ovn-nbctl lb-add lb2 20.0.0.1:80 173.0.1.2:80 udp +check ovn-nbctl lb-add lb1 30.0.0.1 173.0.2.2 +check ovn-nbctl lb-add lb2 173.0.2.250 173.0.1.3 +check ovn-nbctl ls-lb-add s1 lb1 +check ovn-nbctl ls-lb-add s2 lb2 + +ADD_NAMESPACES(vm1) +ADD_VETH(vm1, vm1, br-int, "173.0.1.2/24", "00:de:ad:01:00:01", \ + "173.0.1.1") +ADD_NAMESPACES(vm2) +ADD_VETH(vm2, vm2, br-int, "173.0.2.2/24", "00:de:ad:01:00:02", \ + "173.0.2.1") +ADD_NAMESPACES(vm3) +ADD_VETH(vm3, vm3, br-int, "173.0.2.250/24", "00:de:ad:01:00:03", \ + "173.0.2.1") + +check ovn-nbctl acl-add s1 from-lport 1001 "ip" allow +check ovn-nbctl acl-add s1 to-lport 1002 "ip" allow +check ovn-nbctl acl-add s2 from-lport 1003 "ip" allow +check ovn-nbctl acl-add s2 to-lport 1004 "ip" allow +check ovn-nbctl --wait=hv sync +AS_BOX([initial ping]) +# Send ping in background. Same ping, same flow throughout the test +on_exit 'kill $(pidof ping)' +NS_EXEC([vm1], [ping -c 10000 -i 0.1 30.0.0.1 > icmp.txt &]) + +# Check for conntrack entries +OVS_WAIT_FOR_OUTPUT([ + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(173.0.1.2) | \ + sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl +icmp,orig=(src=173.0.1.2,dst=173.0.2.2,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared> +icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared>,mark=2 +]) + +# Now check for multiple ct_commits +ovs-appctl dpctl/dump-flows > dp_flows +zone_id=$(ovn-appctl -t ovn-controller ct-zone-list | grep vm1 | cut -d ' ' -f2) +AT_CHECK([test 1 = `cat dp_flows | grep "commit,zone=$zone_id" | wc -l`]) + +check ovn-nbctl acl-del s1 from-lport 1001 "ip" +check ovn-nbctl acl-del s1 to-lport 1002 "ip" +check ovn-nbctl acl-del s2 from-lport 1003 "ip" +check ovn-nbctl acl-del s2 to-lport 1004 "ip" + +AS_BOX([acl drop echo request]) +check ovn-nbctl --log --severity=alert --name=drop-flow-s1 acl-add s1 to-lport 2001 icmp4 drop +# acl-drop to-lport s1 apply to traffic from s1 to vm1 and s1 to r1. +check ovn-nbctl --wait=hv sync + +# Check that traffic is blocked +# Wait for some packets to hit the rule to avoid potential race conditions. Then count packets. +OVS_WAIT_UNTIL([test `cat ovn-controller.log | grep acl_log | grep -c drop-flow-s1` -gt "0"]) +total_icmp_pkts=$(cat icmp.txt | grep ttl | wc -l) + +# Wait some time and check whether packets went through. In the worse race condition, the sleep is too short +# and this test will still succeed. +sleep 1 +OVS_WAIT_UNTIL([ + total_icmp1_pkts=$(cat icmp.txt | grep ttl | wc -l) + test "${total_icmp1_pkts}" -eq "${total_icmp_pkts}" +]) + +AS_BOX([acl allow-related echo request]) +check ovn-nbctl acl-add s1 to-lport 2002 "icmp4 && ip4.src == 173.0.1.2" allow-related +# This rule has higher priority than to-lport 2001 icmp4 drop. +# So traffic from s1 (w/ src=173.0.1.2) to r1 should be accepted +# (return) traffic from s1 to vm1 should be accepted as return traffic +check ovn-nbctl --wait=hv sync +OVS_WAIT_UNTIL([ + total_icmp1_pkts=$(cat icmp.txt | grep ttl | wc -l) + test "${total_icmp1_pkts}" -gt "${total_icmp_pkts}" +]) + +# Check we did not break handling acl-drop for existing flows +AS_BOX([acl drop echo request in s2]) +check ovn-nbctl acl-del s1 to-lport 2001 icmp4 +check ovn-nbctl --log --severity=alert --name=drop-flow-s2 acl-add s2 to-lport 2001 icmp4 drop +check ovn-nbctl --wait=hv sync + +OVS_WAIT_UNTIL([test `cat ovn-controller.log | grep acl_log | grep -c drop-flow-s2` -gt "0"]) + +OVS_WAIT_FOR_OUTPUT([ + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \ + sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ + sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl +icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared> +]) +total_icmp_pkts=$(cat icmp.txt | grep ttl | wc -l) + +# Allow ping again +AS_BOX([acl allow echo request in s2]) +check ovn-nbctl acl-add s2 to-lport 2005 icmp4 allow +check ovn-nbctl --wait=hv sync +OVS_WAIT_FOR_OUTPUT([ + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \ + sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl +icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=<cleared>,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=<cleared>,type=0,code=0),zone=<cleared>,mark=2 +]) +OVS_WAIT_UNTIL([ + total_icmp1_pkts=$(cat icmp.txt | grep ttl | wc -l) + test "${total_icmp1_pkts}" -gt "${total_icmp_pkts}" +]) + +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) +AT_CLEANUP +]) +
As commented in northd.c, we should not use ct() for router ports. When there are no stateful_acl, this patch prevents sending packet to conntrack for router ports. The patch does this by issuing ct_clear in ls_out_pre_lb stage so that hints are not set in ls_out_acl_hint and ls_out_acl stages. Note that ct_clear is not added for ingress for router ports as already done for patch ports (no change by this patch on this aspect). Also, this patch does not change the behavior for ACLs such as allow-related: packets are still sent to conntrack, even for router ports. While this does not work if router ports are distributed, allow-related ACLs work today on router ports when those ports are handled on the same chassis for ingress and egress traffic. This patch does not change that behavior. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- v2: - handled Dumitru's comments - handled Ales' comments - added change to xml documentation - do not do ct_clear for ingress as already done --- northd/northd.c | 24 +++--- northd/ovn-northd.8.xml | 11 +++ tests/ovn-northd.at | 11 +-- tests/system-ovn.at | 166 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 15 deletions(-)