The ICMP related was missing the defrag stage for LBs
with port and protocol configured, because the defrag
checks for that protocol. Add logical flow into defrag
stage that will match on ICMP with action ct_dnat. This
allows us to match on ct_state in the DNAT stage.

Reported-at: https://bugzilla.redhat.com/2126083
Signed-off-by: Ales Musil <[email protected]>
---
 northd/northd.c         |  7 ++++-
 northd/ovn-northd.8.xml |  7 +++++
 tests/ovn-northd.at     | 11 +++++++
 tests/system-ovn.at     | 68 ++++++++++++++++++++++++++++++++---------
 4 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 7c48bb3b4..a16e8a8a7 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -14142,7 +14142,10 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath 
*od, struct hmap *lflows,
     ovn_lflow_add(lflows, od, S_ROUTER_OUT_EGR_LOOP, 0, "1", "next;");
     ovn_lflow_add(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 0, "1", "next;");
 
-    /* Ingress DNAT Table (Priority 50).
+    /* Ingress DNAT and DEFRAG Table (Priority 50).
+     *
+     * The defrag stage needs to have flows for ICMP in order to get
+     * the correct ct_state that can be used by DNAT stage.
      *
      * Allow traffic that is related to an existing conntrack entry.
      * At the same time apply NAT for this traffic.
@@ -14152,6 +14155,8 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath 
*od, struct hmap *lflows,
      * related traffic such as an ICMP Port Unreachable through
      * that's generated from a non-listening UDP port.  */
     if (od->has_lb_vip) {
+        ovn_lflow_add(lflows, od, S_ROUTER_IN_DEFRAG, 50, "icmp || icmp6",
+                      "ct_dnat;");
         ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
                       "ct.rel && !ct.est && !ct.new", "ct_commit_nat;");
     }
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index dffbba96d..4d455e0f3 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3296,6 +3296,13 @@ icmp6 {
       packet de-fragmentation and tracking before sending it to the next table.
     </p>
 
+    <p>
+      If load balancing rules are configured in <code>OVN_Northbound</code>
+      database for a Gateway router, a priority 50 flow that matches
+      <code>icmp || icmp6</code> with an action of <code>ct_dnat;</code>,
+      this allows potentially related ICMP traffic to pass through CT.
+    </p>
+
     <h3>Ingress Table 6: Load balancing affinity check</h3>
 
     <p>
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index ca4263eac..7cc53e367 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3669,6 +3669,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.100 && tcp), action=(reg0 = 10.0.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -3706,6 +3707,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.100 && tcp), action=(reg0 = 10.0.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -3753,6 +3755,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.100 && tcp), action=(reg0 = 10.0.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -3814,6 +3817,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.100 && tcp), action=(reg0 = 10.0.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -3862,6 +3866,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.100 && tcp), action=(reg0 = 10.0.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.20 && tcp), action=(reg0 = 10.0.0.20; reg9[[16..31]] = tcp.dst; ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | grep skip_snat_for_lb | sort], [0], [dnl
@@ -5115,6 +5120,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && udp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = udp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -5185,6 +5191,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && udp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = udp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -5247,6 +5254,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; reg9[[16..31]] = tcp.dst; ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && udp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = udp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -5312,6 +5320,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && udp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = udp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -5390,6 +5399,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && udp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = udp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip6.dst == 
def0::2 && tcp), action=(xxreg0 = def0::2; reg9[[16..31]] = tcp.dst; ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
@@ -5459,6 +5469,7 @@ AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
   table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && tcp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = tcp.dst; 
ct_dnat;)
   table=5 (lr_in_defrag       ), priority=110  , match=(ip && ip4.dst == 
172.168.0.210 && udp), action=(reg0 = 172.168.0.210; reg9[[16..31]] = udp.dst; 
ct_dnat;)
+  table=5 (lr_in_defrag       ), priority=50   , match=(icmp || icmp6), 
action=(ct_dnat;)
 ])
 
 AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index b99578b9e..e3325acae 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -9243,9 +9243,7 @@ check ovn-nbctl lr-add lr
 check ovn-nbctl lrp-add lr lr-ls0 00:00:00:00:10:00 192.168.10.1/24
 check ovn-nbctl lrp-add lr lr-ls1 00:00:00:00:20:00 192.168.20.1/24
 
-check ovn-nbctl lrp-set-gateway-chassis lr-ls0 hv1
-
-check ovn-nbctl lb-add lb0 192.168.20.20 192.168.20.10
+check ovn-nbctl set logical_router lr options:chassis=hv1
 
 ADD_NAMESPACES(client)
 ADD_VETH(client, client, br-int, "192.168.10.10/24", "00:00:00:00:10:10", \
@@ -9271,15 +9269,9 @@ 
icmp_expected=000000002010000000002000080045000038011f0000fe011c41c0a80a0ac0\
 a8140a0304f778000005784500001c000040000911d26cc0a8140ac0a80a0a0002000100080000
 
 test_related_traffic() {
+    check ovn-nbctl --wait=hv sync
 
-    check ovn-nbctl ls-lb-del ls0
-    check ovn-nbctl lr-lb-del lr
-
-    if [[ "$1" == "switch" ]]; then
-        check ovn-nbctl ls-lb-add ls0 lb0
-    else
-        check ovn-nbctl lr-lb-add lr lb0
-    fi
+    check ovs-appctl dpctl/flush-conntrack
 
     NETNS_DAEMONIZE([client], [tcpdump -U -i client -w client.pcap], 
[tcpdump0.pid])
     NETNS_DAEMONIZE([server], [tcpdump -U -i server -w server.pcap], 
[tcpdump1.pid])
@@ -9299,8 +9291,8 @@ test_related_traffic() {
     check ovs-ofctl packet-out br-int 
"in_port=ovs-client,packet=$icmp,actions=resubmit(,0)"
 
     # Check if all packets have arrived
-    WAIT_PACKET([client.pcap], [$client_udp_expected])
     WAIT_PACKET([server.pcap], [$server_udp_expected])
+    WAIT_PACKET([client.pcap], [$client_udp_expected])
     WAIT_PACKET([server.pcap], [$icmp_expected])
 
     kill $(cat tcpdump0.pid) $(cat tcpdump1.pid)
@@ -9309,8 +9301,56 @@ test_related_traffic() {
     rm -f client.pcap server.pcap
 }
 
-test_related_traffic switch
-test_related_traffic router
+AS_BOX([ICMP related on switch, LB without port and protocol])
+check ovn-nbctl lb-add lb0 192.168.20.20 192.168.20.10
+check ovn-nbctl ls-lb-add ls0 lb0
+
+test_related_traffic
+
+check ovn-nbctl ls-lb-del ls0
+check ovn-nbctl lb-del lb0
+
+AS_BOX([ICMP related on switch, LB with port and protocol])
+check ovn-nbctl lb-add lb0 192.168.20.20:2 192.168.20.10:2 udp
+check ovn-nbctl ls-lb-add ls0 lb0
+
+test_related_traffic
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.20.20) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+udp,orig=(src=192.168.10.10,dst=192.168.20.20,sport=<cleared>,dport=<cleared>),reply=(src=192.168.20.10,dst=192.168.10.10,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2
+])
+
+check ovn-nbctl ls-lb-del ls0
+check ovn-nbctl lb-del lb0
+
+AS_BOX([ICMP related on router, LB without port and protocol])
+check ovn-nbctl lb-add lb0 192.168.20.20 192.168.20.10
+check ovn-nbctl lr-lb-add lr lb0
+
+test_related_traffic
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.20.20) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+udp,orig=(src=192.168.10.10,dst=192.168.20.20,sport=<cleared>,dport=<cleared>),reply=(src=192.168.20.10,dst=192.168.10.10,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2
+])
+
+check ovn-nbctl lr-lb-del lr
+check ovn-nbctl lb-del lb0
+
+AS_BOX([ICMP related on switch, LB with port and protocol])
+check ovn-nbctl lb-add lb0 192.168.20.20:2 192.168.20.10:2 udp
+check ovn-nbctl lr-lb-add lr lb0
+
+test_related_traffic
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.20.20) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+udp,orig=(src=192.168.10.10,dst=192.168.20.20,sport=<cleared>,dport=<cleared>),reply=(src=192.168.20.10,dst=192.168.10.10,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2
+])
+
+check ovn-nbctl lr-lb-del lr
+check ovn-nbctl lb-del lb0
 
 OVS_APP_EXIT_AND_WAIT([ovn-controller])
 
-- 
2.38.1

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to