[ovs-dev] [PATCH ovn 2/2] northd: Remove "reachable" functions and users of them.

Mark Michelson mmichels at redhat.com
Wed Mar 10 14:28:43 UTC 2021


Self-originated ARPs are intended to be sent to the "owning" router for
a given IP address, as well as flooded to non-router ports on a logical
switch.

When trying to determine the owning router for an IP address, we would
iterate through load balancer and NAT addresses, and check if these IP
addresses were "reachable" on this particular router. Reachable here
means that the NAT external IP or load balancer VIP falls within any of
the networks served by this router. If an IP address were determined not
to be reachable, then we would not try to send ARPs for that particular
address to the router.

However, it is possible (and sometimes desired) to configure NAT floating
IPs on a router that are not in any subnet handled by that router.
In this case, OVN refuses to send ARP requests to the router on which the
floating IP has been configured. The result is that internally-generated
traffic that targets the floating IP cannot reach its destination,
since the router on which the floating IP is configured never receives ARPs
for the floating IP.

This patch fixes the issue by removing the reachability checks
altogether. If a router has a NAT external IP or load balancer VIP that
is outside the range of any of its configured subnets, we still should
send ARPs to that router for those requested addresses.

Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1929901

Signed-off-by: Mark Michelson <mmichels at redhat.com>
---
 northd/ovn-northd.c  |  96 +-----------------------------
 northd/ovn_northd.dl |  44 ++------------
 tests/system-ovn.at  | 139 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+), 132 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index cab17f07e..e0a07002e 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -6253,42 +6253,6 @@ build_lrouter_groups(struct hmap *ports, struct ovs_list *lr_list)
     }
 }
 
-/* Returns 'true' if the IPv4 'addr' is on the same subnet with one of the
- * IPs configured on the router port.
- */
-static bool
-lrouter_port_ipv4_reachable(const struct ovn_port *op, ovs_be32 addr)
-{
-    for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
-        struct ipv4_netaddr *op_addr = &op->lrp_networks.ipv4_addrs[i];
-
-        if ((addr & op_addr->mask) == op_addr->network) {
-            return true;
-        }
-    }
-    return false;
-}
-
-/* Returns 'true' if the IPv6 'addr' is on the same subnet with one of the
- * IPs configured on the router port.
- */
-static bool
-lrouter_port_ipv6_reachable(const struct ovn_port *op,
-                            const struct in6_addr *addr)
-{
-    for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
-        struct ipv6_netaddr *op_addr = &op->lrp_networks.ipv6_addrs[i];
-
-        struct in6_addr nat_addr6_masked =
-            ipv6_addr_bitand(addr, &op_addr->mask);
-
-        if (ipv6_addr_equals(&nat_addr6_masked, &op_addr->network)) {
-            return true;
-        }
-    }
-    return false;
-}
-
 /*
  * Ingress table 19: Flows that flood self originated ARP/ND packets in the
  * switching domain.
@@ -6321,22 +6285,6 @@ build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op,
             continue;
         }
 
-        /* Check if the ovn port has a network configured on which we could
-         * expect ARP requests/NS for the DNAT external_ip.
-         */
-        if (nat_entry_is_v6(nat_entry)) {
-            struct in6_addr *addr = &nat_entry->ext_addrs.ipv6_addrs[0].addr;
-
-            if (!lrouter_port_ipv6_reachable(op, addr)) {
-                continue;
-            }
-        } else {
-            ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr;
-
-            if (!lrouter_port_ipv4_reachable(op, addr)) {
-                continue;
-            }
-        }
         sset_add(&all_eth_addrs, nat->external_mac);
     }
 
@@ -6458,35 +6406,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 
     get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6);
 
-    const char *ip_addr;
-    const char *ip_addr_next;
-    SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v4) {
-        ovs_be32 ipv4_addr;
-
-        /* Check if the ovn port has a network configured on which we could
-         * expect ARP requests for the LB VIP.
-         */
-        if (ip_parse(ip_addr, &ipv4_addr) &&
-                lrouter_port_ipv4_reachable(op, ipv4_addr)) {
-            continue;
-        }
-
-        sset_delete(&all_ips_v4, SSET_NODE_FROM_NAME(ip_addr));
-    }
-    SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v6) {
-        struct in6_addr ipv6_addr;
-
-        /* Check if the ovn port has a network configured on which we could
-         * expect NS requests for the LB VIP.
-         */
-        if (ipv6_parse(ip_addr, &ipv6_addr) &&
-                lrouter_port_ipv6_reachable(op, &ipv6_addr)) {
-            continue;
-        }
-
-        sset_delete(&all_ips_v6, SSET_NODE_FROM_NAME(ip_addr));
-    }
-
     for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
         struct ovn_nat *nat_entry = &op->od->nat_entries[i];
         const struct nbrec_nat *nat = nat_entry->nb;
@@ -6499,21 +6418,10 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
             continue;
         }
 
-        /* Check if the ovn port has a network configured on which we could
-         * expect ARP requests/NS for the DNAT external_ip.
-         */
         if (nat_entry_is_v6(nat_entry)) {
-            struct in6_addr *addr = &nat_entry->ext_addrs.ipv6_addrs[0].addr;
-
-            if (lrouter_port_ipv6_reachable(op, addr)) {
-                sset_add(&all_ips_v6, nat->external_ip);
-            }
+            sset_add(&all_ips_v6, nat->external_ip);
         } else {
-            ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr;
-
-            if (lrouter_port_ipv4_reachable(op, addr)) {
-                sset_add(&all_ips_v4, nat->external_ip);
-            }
+            sset_add(&all_ips_v4, nat->external_ip);
         }
     }
 
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 227f38157..5a3bb83d0 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -4055,29 +4055,6 @@ for (SwitchPortStaticAddresses(.port = &SwitchPort{.lsp = lsp, .json_name = json
  * reliable in case this is a VLAN-backed network.
  * Priority: 75.
  */
-
-/* Returns 'true' if the IP 'addr' is on the same subnet with one of the
- * IPs configured on the router port.
- */
-function lrouter_port_ip_reachable(rp: Ref<RouterPort>, addr: v46_ip): bool {
-    match (addr) {
-        IPv4{ipv4} -> {
-            for (na in rp.networks.ipv4_addrs) {
-                if (ip_same_network((ipv4, na.addr), ipv4_netaddr_mask(na))) {
-                    return true
-                }
-            }
-        },
-        IPv6{ipv6} -> {
-            for (na in rp.networks.ipv6_addrs) {
-                if (ipv6_same_network((ipv6, na.addr), ipv6_netaddr_mask(na))) {
-                    return true
-                }
-            }
-        }
-    };
-    false
-}
 UniqueFlow[Flow{.logical_datapath = sw.ls._uuid,
                 .stage            = switch_stage(IN, L2_LKUP),
                 .priority         = 75,
@@ -4090,10 +4067,7 @@ UniqueFlow[Flow{.logical_datapath = sw.ls._uuid,
         var eth_src_set = set_singleton("${rp.networks.ea}");
         for (nat in rp.router.nats) {
             match (nat.nat.external_mac) {
-                Some{mac} ->
-                    if (lrouter_port_ip_reachable(rp, nat.external_ip)) {
-                        eth_src_set.insert(mac)
-                    } else (),
+                Some{mac} -> eth_src_set.insert(mac),
                 _ -> ()
             }
         };
@@ -4119,9 +4093,7 @@ function get_arp_forward_ips(rp: Ref<RouterPort>): (Set<string>, Set<string>) =
          * expect ARP requests for the LB VIP.
          */
         match (ip_parse(a)) {
-            Some{ipv4} -> if (lrouter_port_ip_reachable(rp, IPv4{ipv4})) {
-                all_ips_v4.insert(a)
-            },
+            Some{ipv4} -> all_ips_v4.insert(a),
             _ -> ()
         }
     };
@@ -4130,9 +4102,7 @@ function get_arp_forward_ips(rp: Ref<RouterPort>): (Set<string>, Set<string>) =
          * expect NS requests for the LB VIP.
          */
         match (ipv6_parse(a)) {
-            Some{ipv6} -> if (lrouter_port_ip_reachable(rp, IPv6{ipv6})) {
-                all_ips_v6.insert(a)
-            },
+            Some{ipv6} -> all_ips_v6.insert(a),
             _ -> ()
         }
     };
@@ -4142,11 +4112,9 @@ function get_arp_forward_ips(rp: Ref<RouterPort>): (Set<string>, Set<string>) =
             /* Check if the ovn port has a network configured on which we could
              * expect ARP requests/NS for the DNAT external_ip.
              */
-            if (lrouter_port_ip_reachable(rp, nat.external_ip)) {
-                match (nat.external_ip) {
-                    IPv4{_} -> all_ips_v4.insert(nat.nat.external_ip),
-                    IPv6{_} -> all_ips_v6.insert(nat.nat.external_ip)
-                }
+            match (nat.external_ip) {
+                IPv4{_} -> all_ips_v4.insert(nat.nat.external_ip),
+                IPv6{_} -> all_ips_v6.insert(nat.nat.external_ip)
             }
         }
     };
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 36b469c30..70c3db85e 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -5997,6 +5997,145 @@ NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 172.18.2.10 | FORMAT_PING], \
 [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
+
+kill $(pidof 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(["/.*error receiving.*/d
+/.*terminating with signal 15.*/d"])
+
+AT_CLEANUP
+
+AT_SETUP(ovn -- Floating IP outside router subnet IPv4)
+AT_KEYWORDS(arp 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
+
+# Logical network:
+# Two VMs
+#   * VM1 with IP address 192.168.100.5
+#   * VM2 with IP address 192.168.200.5
+#
+# VM1 connects to logical switch ls1. ls1 connects to logical router lr1.
+# VM2 connects to logical switch ls2. ls2 connects to logical router lr2.
+# lr1 and lr2 both connect to logical switch ls-pub.
+# * lr1's interface that connects to ls-pub has IP address 172.18.2.110/24
+# * lr2's interface that connects to ls-pub has IP address 172.18.1.173/24
+#
+# lr1 has the following attributes:
+#   * It has a DNAT rule that translates 172.18.2.11 to 192.168.100.5 (VM1)
+#
+# lr2 has the following attributes:
+#   * It has a DNAT rule that translates 172.18.2.12 to 192.168.200.5 (VM2)
+#
+# In this test, we want to ensure that a ping from VM1 to IP address 172.18.2.12 reaches VM2.
+# When VM1 sends the ping, it first reaches lr1. lr1 will need to send an ARP in order to
+# create a MAC binding for 172.18.2.12. The ARP must traverse ls-pub to reach lr2. The crux
+# of this test is ensuring that ls-pub passes the ARP to lr2.
+
+ovn-nbctl ls-add ls1
+ovn-nbctl lsp-add ls1 vm1 -- lsp-set-addresses vm1 "00:00:00:00:01:05 192.168.100.5"
+
+ovn-nbctl ls-add ls2
+ovn-nbctl lsp-add ls2 vm2 -- lsp-set-addresses vm2 "00:00:00:00:02:05 192.168.200.5"
+
+ovn-nbctl ls-add ls-pub
+
+ovn-nbctl lr-add lr1
+ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:01:01 192.168.100.1/24
+ovn-nbctl lsp-add ls1 ls1-lr1                      \
+    -- lsp-set-type ls1-lr1 router                 \
+    -- lsp-set-addresses ls1-lr1 router            \
+    -- lsp-set-options ls1-lr1 router-port=lr1-ls1
+
+ovn-nbctl lr-add lr2
+ovn-nbctl lrp-add lr2 lr2-ls2 00:00:00:00:02:01 192.168.200.1/24
+ovn-nbctl lsp-add ls2 ls2-lr2                      \
+    -- lsp-set-type ls2-lr2 router                 \
+    -- lsp-set-addresses ls2-lr2 router            \
+    -- lsp-set-options ls2-lr2 router-port=lr2-ls2
+
+ovn-nbctl lrp-add lr1 lr1-ls-pub 00:00:00:00:03:01 172.18.2.110/24
+ovn-nbctl lrp-set-gateway-chassis lr1-ls-pub hv1
+ovn-nbctl lsp-add ls-pub ls-pub-lr1                      \
+    -- lsp-set-type ls-pub-lr1 router                    \
+    -- lsp-set-addresses ls-pub-lr1 router               \
+    -- lsp-set-options ls-pub-lr1 router-port=lr1-ls-pub
+
+ovn-nbctl lrp-add lr2 lr2-ls-pub 00:00:00:00:03:02 172.18.1.173/24
+ovn-nbctl lrp-set-gateway-chassis lr2-ls-pub hv1
+ovn-nbctl lsp-add ls-pub ls-pub-lr2                      \
+    -- lsp-set-type ls-pub-lr2 router                    \
+    -- lsp-set-addresses ls-pub-lr2 router               \
+    -- lsp-set-options ls-pub-lr2 router-port=lr2-ls-pub
+
+ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.18.2.11 192.168.100.5 vm1 00:00:00:00:03:01
+ovn-nbctl lr-route-add lr1 172.18.2.12 172.18.1.173 lr1-ls-pub
+
+ovn-nbctl lr-nat-add lr2 dnat_and_snat 172.18.2.12 192.168.200.5 vm2 00:00:00:00:03:02
+ovn-nbctl lr-route-add lr2 172.18.2.11 172.18.2.110 lr2-ls-pub
+
+ADD_NAMESPACES(vm1)
+ADD_VETH(vm1, vm1, br-int, "192.168.100.5/24", "00:00:00:00:01:05", \
+         "192.168.100.1")
+
+ADD_NAMESPACES(vm2)
+ADD_VETH(vm2, vm2, br-int, "192.168.200.5/24", "00:00:00:00:02:05", \
+         "192.168.200.1")
+
+OVN_POPULATE_ARP
+ovn-nbctl --wait=hv sync
+
+# Let's take a quick look at the logical flows
+ovn-sbctl lflow-list
+
+# And the OF flows
+ovs-ofctl dump-flows br-int
+
+# Let's check what ovn-trace says...
+ovn-trace ls1 'inport == "vm1" && eth.src == 00:00:00:00:01:05 && ip4.src == 192.168.100.5 && eth.dst == 00:00:00:00:01:01 && ip4.dst == 172.18.2.12 && ip.ttl == 32'
+
+NS_EXEC([vm1], [ping -q -c 1 172.18.2.12])
+
+# Let's check what ovn-trace says again. Maybe a MAC_Binding has been installed and
+# things will be different now.
+ovn-trace ls1 'inport == "vm1" && eth.src == 00:00:00:00:01:05 && ip4.src == 192.168.100.5 && eth.dst == 00:00:00:00:01:01 && ip4.dst == 172.18.2.12 && ip.ttl == 32'
+
+NS_EXEC([vm1], [ping -q -c 1 172.18.2.12])
+
+# Check OF flows again
+ovs-ofctl dump-flows br-int > flows.txt
+AT_CAPTURE_FILE([flows.txt])
+
+# A ping from vm1 should reach ls-pub, who will ARP for 172.18.2.12. lr2 should
+# answer that ARP, and the pings should go through to VM2.
+NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 172.18.2.12 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
 kill $(pidof ovn-controller)
 
 as ovn-sb
-- 
2.29.2



More information about the dev mailing list