northd: Don't skip the unSNAT stage for traffic towards VIPs.

Otherwise, in case there's also a SNAT rule that uses the VIP as external IP, we break sessions initiated from behind the VIP. This partially reverts 832893b ("ovn-northd: Skip unsnat flows for load balancer vips in router ingress pipeline"). That's OK because commit 384a7c6 ("northd: Refactor Logical Flows for routers with DNAT/Load Balancers") addressed the original issue in a better way: In the reply direction, the order of traversal of the tables "lr_in_defrag", "lr_in_unsnat" and "lr_in_dnat" adds incorrect datapath flows that check ct_state in the wrong conntrack zone. This is illustrated below where reply trafic enters the physical host port (6) and traverses DNAT zone (14), SNAT zone (default), back to the DNAT zone and then on to Logical Switch Port zone (22). The third flow is incorrectly checking the state from the SNAT zone instead of the DNAT zone. We also add a system test to ensure traffic initiated from behind a VIP + SNAT is not broken. Another nice side effect is that the northd I-P is slightly simplified because we don't need to track NAT external IPs anymore. Fixes: 832893b ("ovn-northd: Skip unsnat flows for load balancer vips in router ingress pipeline") Reported-at: https://issues.redhat.com/browse/FDP-291 Signed-off-by: Dumitru Ceara <dceara@redhat.com> Acked-by: Numan Siddique <numans@ovn.org> (cherry picked from commit ffe2673)
ovn-org · Mar 6, 2024 · 03db09a · 03db09a
1 parent e60e512
commit 03db09a
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 40 deletions.
diff --git a/northd/northd.c b/northd/northd.c
@@ -10104,14 +10104,6 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
     ds_chomp(&undnat_match, '|');
     ds_chomp(&undnat_match, ' ');
 
-    struct ds unsnat_match = DS_EMPTY_INITIALIZER;
-    ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
-                  ip_match, ip_match, lb_vip->vip_str, lb->proto);
-    if (lb_vip->vip_port) {
-        ds_put_format(&unsnat_match, " && %s.dst == %d", lb->proto,
-                      lb_vip->vip_port);
-    }
-
     const char *force_snat = features->ct_lb_related && !drop
                              ? "; force_snat"
                              : "";
@@ -10129,23 +10121,6 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
             meter = copp_meter_get(COPP_REJECT, od->nbr->copp, meter_groups);
         }
 
-        if (sset_contains(&od->external_ips, lb_vip->vip_str)) {
-            /* The load balancer vip is also present in the NAT entries.
-             * So add a high priority lflow to advance the the packet
-             * destined to the vip (and the vip port if defined)
-             * in the S_ROUTER_IN_UNSNAT stage.
-             * There seems to be an issue with ovs-vswitchd. When the new
-             * connection packet destined for the lb vip is received,
-             * it is dnat'ed in the S_ROUTER_IN_DNAT stage in the dnat
-             * conntrack zone. For the next packet, if it goes through
-             * unsnat stage, the conntrack flags are not set properly, and
-             * it doesn't hit the established state flows in
-             * S_ROUTER_IN_DNAT stage. */
-             ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120,
-                                     ds_cstr(&unsnat_match), "next;",
-                                     &lb->nlb->header_);
-        }
-
         if (od->n_l3dgw_ports &&
             (lb_vip->n_backends || !lb_vip->empty_backend_rej)) {
             new_match_p = xasprintf("%s && is_chassis_resident(%s)",
@@ -10220,7 +10195,6 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
         free(undnat_match_p);
     }
 
-    ds_destroy(&unsnat_match);
     ds_destroy(&undnat_match);
 
     free(skip_snat_new_action);
@@ -13734,12 +13708,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
                                    cidr_bits, is_v6);
 
         /* ARP resolve for NAT IPs. */
-        if (od->is_gw_router) {
-            /* Add the NAT external_ip to the nat_entries for
-             * gateway routers. This is required for adding load balancer
-             * flows.*/
-            sset_add(&nat_entries, nat->external_ip);
-        } else {
+        if (!od->is_gw_router) {
             if (!sset_contains(&nat_entries, nat->external_ip)) {
                 /* Drop packets coming in from external that still has
                  * destination IP equals to the NAT external IP, to avoid loop.

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
@@ -1475,9 +1475,6 @@ AT_CAPTURE_FILE([sbflows])
 # dnat_and_snat or snat entry.
 AT_CHECK([grep "lr_in_unsnat" sbflows | sort], [0], [dnl
   table=4 (lr_in_unsnat       ), priority=0    , match=(1), action=(next;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 192.168.2.1 && tcp && tcp.dst == 8080), action=(next;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 192.168.2.4 && udp && udp.dst == 8080), action=(next;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 192.168.2.5 && tcp && tcp.dst == 8080), action=(next;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 192.168.2.1), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 192.168.2.4), action=(ct_snat;)
 ])
@@ -1508,9 +1505,6 @@ AT_CAPTURE_FILE([sbflows])
 # dnat_and_snat or snat entry.
 AT_CHECK([grep "lr_in_unsnat" sbflows | sort], [0], [dnl
   table=4 (lr_in_unsnat       ), priority=0    , match=(1), action=(next;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 192.168.2.1 && tcp && tcp.dst == 8080), action=(next;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 192.168.2.4 && udp && udp.dst == 8080), action=(next;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 192.168.2.5 && tcp && tcp.dst == 8080), action=(next;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 192.168.2.1), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 192.168.2.4), action=(ct_snat;)
 ])
@@ -5247,7 +5241,6 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
   table=4 (lr_in_unsnat       ), priority=0    , match=(1), action=(next;)
   table=4 (lr_in_unsnat       ), priority=110  , match=(inport == "lr0-public" && ip4.dst == 172.168.0.10), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=110  , match=(inport == "lr0-sw0" && ip4.dst == 10.0.0.1), action=(ct_snat;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 172.168.0.10 && tcp && tcp.dst == 9082), action=(next;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 172.168.0.10), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 172.168.0.20), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 172.168.0.30), action=(ct_snat;)
@@ -5327,7 +5320,6 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
   table=4 (lr_in_unsnat       ), priority=110  , match=(inport == "lr0-public" && ip6.dst == def0::10), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=110  , match=(inport == "lr0-sw0" && ip4.dst == 10.0.0.1), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=110  , match=(inport == "lr0-sw0" && ip6.dst == aef0::1), action=(ct_snat;)
-  table=4 (lr_in_unsnat       ), priority=120  , match=(ip4 && ip4.dst == 172.168.0.10 && tcp && tcp.dst == 9082), action=(next;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 172.168.0.10), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 172.168.0.20), action=(ct_snat;)
   table=4 (lr_in_unsnat       ), priority=90   , match=(ip && ip4.dst == 172.168.0.30), action=(ct_snat;)

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
@@ -2564,6 +2564,86 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([load balancing in gateway router - client behind LB with SNAT])
+AT_SKIP_IF([test $HAVE_NC = no])
+AT_KEYWORDS([lb])
+
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+check 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 lr \
+    -- set logical_router lr options:chassis=hv1
+check ovn-nbctl lrp-add lr lr-ls1 00:00:00:00:01:00 41.41.41.2/24
+check ovn-nbctl lrp-add lr lr-ls2 00:00:00:00:02:00 42.42.42.2/24
+check ovn-nbctl ls-add ls1
+check ovn-nbctl ls-add ls2
+
+check ovn-nbctl lsp-add ls1 ls1-lr
+check ovn-nbctl lsp-set-addresses ls1-lr 00:00:00:00:01:00
+check ovn-nbctl lsp-set-type ls1-lr router
+check ovn-nbctl lsp-set-options ls1-lr router-port=lr-ls1
+check ovn-nbctl lsp-add ls1 vm1
+check ovn-nbctl lsp-set-addresses vm1 00:00:00:00:00:01
+
+check ovn-nbctl lsp-add ls2 ls2-lr
+check ovn-nbctl lsp-set-addresses ls2-lr 00:00:00:00:02:00
+check ovn-nbctl lsp-set-type ls2-lr router
+check ovn-nbctl lsp-set-options ls2-lr router-port=lr-ls2
+check ovn-nbctl lsp-add ls2 vm2
+check ovn-nbctl lsp-set-addresses vm2 00:00:00:00:00:02
+
+dnl LB using the router IP connected to vm2 as VIP.
+check ovn-nbctl lb-add lb-test 42.42.42.2:8080 41.41.41.1:8080 tcp \
+    -- lr-lb-add lr lb-test
+
+dnl SNAT everything coming from vm1 to the router IP (towards vm2).
+check ovn-nbctl lr-nat-add lr snat 42.42.42.2 41.41.41.1
+
+ADD_NAMESPACES(vm1)
+ADD_VETH(vm1, vm1, br-int, "41.41.41.1/24", "00:00:00:00:00:01", "41.41.41.2")
+
+ADD_NAMESPACES(vm2)
+ADD_VETH(vm2, vm2, br-int, "42.42.42.1/24", "00:00:00:00:00:02", "42.42.42.2")
+
+dnl Start a server on vm2.
+NETNS_DAEMONIZE([vm2], [nc -l -k 42.42.42.1 80], [vm2.pid])
+
+dnl Wait for ovn-controller to catch up.
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+dnl Test the connection originating something that uses the same source port
+dnl as the LB VIP.
+NS_CHECK_EXEC([vm1], [nc -z -p 8080 42.42.42.1 80], 0, [ignore], [ignore])
+
+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([ovn-northd])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([multiple gateway routers, load-balancing])
 AT_KEYWORDS([ovnlb])