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 <amu...@redhat.com>
Acked-by: Mark Michelson <mmich...@redhat.com>
Signed-off-by: Dumitru Ceara <dce...@redhat.com>
(cherry picked from commit 3f360a49)
---
 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 8ad2277b0..25fa9c697 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13612,7 +13612,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.
@@ -13622,6 +13625,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 a1066020e..ce6ee1157 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3163,6 +3163,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: DNAT</h3>
 
     <p>
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 9431324d3..a77af9722 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3597,6 +3597,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
@@ -3634,6 +3635,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
@@ -3681,6 +3683,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
@@ -3742,6 +3745,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
@@ -3790,6 +3794,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
@@ -5041,6 +5046,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
@@ -5111,6 +5117,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
@@ -5173,6 +5180,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
@@ -5238,6 +5246,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
@@ -5316,6 +5325,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
@@ -5385,6 +5395,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 9671826a0..5f73a034e 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -8728,9 +8728,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", \
@@ -8756,15 +8754,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])
@@ -8784,8 +8776,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)
@@ -8794,8 +8786,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.39.0

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to