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. However, 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 <[email protected]> --- northd/northd.c | 30 +++++--- tests/ovn-northd.at | 17 ++--- tests/system-ovn.at | 167 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+), 18 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 77e105b86..f161ebebc 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5766,20 +5766,30 @@ 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); + char *ingress_action, *egress_action; + + if (od->has_stateful_acl) { + ingress_action = xasprintf("next;"); + egress_action = xasprintf("next;"); + } else { + ingress_action = xasprintf("ct_clear;next;"); + egress_action = xasprintf("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_action); + free(egress_action); + free(ingress_match); + free(egress_match); } static void diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 3fa02d2b3..2e1788d73 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -4111,15 +4111,16 @@ 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]) - AT_CHECK([grep "ls_in_pre_lb" sw0flows | sort | sed 's/table=./table=?/'], [0], [dnl + AT_CHECK_UNQUOTED([grep "ls_in_pre_lb" sw0flows | sort | sed 's/table=./table=?/'], [0], [dnl table=? (ls_in_pre_lb ), priority=0 , match=(1), action=(next;) table=? (ls_in_pre_lb ), priority=100 , match=(ip), action=(reg0[[2]] = 1; next;) - table=? (ls_in_pre_lb ), priority=110 , match=(eth.dst == $svc_monitor_mac), action=(next;) + table=? (ls_in_pre_lb ), priority=110 , match=(eth.dst == \$svc_monitor_mac), action=(next;) table=? (ls_in_pre_lb ), priority=110 , match=(eth.mcast), action=(next;) - table=? (ls_in_pre_lb ), priority=110 , match=(ip && inport == "sw0-lr0"), action=(next;) + table=? (ls_in_pre_lb ), priority=110 , match=(ip && inport == "sw0-lr0"), action=($action) table=? (ls_in_pre_lb ), priority=110 , match=(nd || nd_rs || nd_ra || mldv1 || mldv2), action=(next;) table=? (ls_in_pre_lb ), priority=110 , match=(reg0[[16]] == 1), action=(next;) ]) @@ -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 2ece0f571..1cabf0233 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10285,3 +10285,170 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /connection dropped.*/d"]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ACL and commiting 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
