northd: Add logical flow to defrag ICMP traffic

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 <amusil@redhat.com> Acked-by: Mark Michelson <mmichels@redhat.com> Signed-off-by: Dumitru Ceara <dceara@redhat.com> (cherry picked from commit 3f360a4)
ovn-org · Jan 23, 2023 · 9682916 · 9682916
1 parent 06f92e6
commit 9682916
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 15 deletions.
diff --git a/northd/northd.c b/northd/northd.c
@@ -13450,7 +13450,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.
@@ -13460,6 +13463,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
@@ -3120,6 +3120,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
@@ -3584,6 +3584,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
@@ -3621,6 +3622,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
@@ -3668,6 +3670,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
@@ -3729,6 +3732,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
@@ -3777,6 +3781,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
@@ -5028,6 +5033,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
@@ -5098,6 +5104,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
@@ -5160,6 +5167,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
@@ -5225,6 +5233,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
@@ -5303,6 +5312,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
@@ -5372,6 +5382,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
@@ -8668,9 +8668,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", \
@@ -8696,15 +8694,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])
@@ -8724,8 +8716,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)
@@ -8734,8 +8726,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])