[ovs-dev] [PATCH ovn v2 2/2] northd: Remove "reachable" functions and users of them.
Mark Michelson
mmichels at redhat.com
Wed Mar 10 21:15:19 UTC 2021
v2 of this patch was made because patch 2 below had some extra debugging
steps in it that were beyond the scope of the test. v2 removed those
debugging statements.
On 3/10/21 2:55 PM, Mark Michelson wrote:
> 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 | 118 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 126 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..5754b87b8 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -5997,6 +5997,124 @@ 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
> +
> +# 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
>
More information about the dev
mailing list