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

Numan Siddique numans at ovn.org
Tue Mar 23 16:05:06 UTC 2021


On Fri, Mar 19, 2021 at 2:20 AM Mark Michelson <mmichels at redhat.com> 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>

Thanks for addressing this.

Acked-by: Numan Siddique <numans at ovn.org>

@Dumitru - Since you had added the code to limit ARPs.  Can you please
also take a look at this patch ?

Thanks
Numan

> ---
> v2 -> v3: Fixed failing ARP/ND request broadcast limiting test
> ---
>  northd/ovn-northd.c  |  96 +----------------------------------
>  northd/ovn_northd.dl |  44 +++-------------
>  tests/ovn.at         |  16 +++++-
>  tests/system-ovn.at  | 118 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 141 insertions(+), 133 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 6639cf31f..ccc5fd2e8 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 7bc202498..25cedfada 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -4075,29 +4075,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,
> @@ -4110,10 +4087,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),
>                  _ -> ()
>              }
>          };
> @@ -4139,9 +4113,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),
>              _ -> ()
>          }
>      };
> @@ -4150,9 +4122,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),
>              _ -> ()
>          }
>      };
> @@ -4162,11 +4132,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/ovn.at b/tests/ovn.at
> index b751d6db2..38f9b88d4 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -20434,12 +20434,22 @@ nd_target=fe80::200:ff:fe00:200
>  ])
>
>  # For sw1-rtr1:
> -# - 20.0.0.1, 20::1, fe80::200:1ff:fe00:0 - interface IPs.
> +# - 20.0.0.1, 20::1, fe80::200:1ff:fe00:0 - interface IPs
> +# - 10.0.0.11, 10::11 - LB VIPs.
> +# - 10.0.0.111, 10.0.0.121, 10.0.0.122, 10::111, 10::121, 10:122 - DNAT IPs.
>  as hv1
>  AT_CHECK([ovs-ofctl dump-flows br-int | grep -E "priority=80,.*${match_sw1_metadata}" | grep -oE "arp_tpa=[[0-9.]]+" | sort], [0], [dnl
> +arp_tpa=10.0.0.11
> +arp_tpa=10.0.0.111
> +arp_tpa=10.0.0.121
> +arp_tpa=10.0.0.122
>  arp_tpa=20.0.0.1
>  ])
>  AT_CHECK([ovs-ofctl dump-flows br-int | grep -E "priority=80,.*${match_sw1_metadata}" | grep -oE "nd_target=[[0-9a-f:]]+" | sort], [0], [dnl
> +nd_target=10::11
> +nd_target=10::111
> +nd_target=10::121
> +nd_target=10::122
>  nd_target=20::1
>  nd_target=fe80::200:1ff:fe00:0
>  ])
> @@ -20466,8 +20476,12 @@ dl_src=00:00:00:02:00:00
>
>  # Self originated ARP/NS with SMACs owned by rtr1-sw1 should be flooded:
>  # - 00:00:01:00:00:00 - interface MAC.
> +# - 00:00:00:01:00:00 - dnat_and_snat external MAC.
> +# - 00:00:00:02:00:00 - dnat_and_snat external MAC.
>  as hv1
>  AT_CHECK([ovs-ofctl dump-flows br-int | grep -E "priority=75,.*${match_sw1_metadata}.*arp_op=1" | grep -oE "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl
> +dl_src=00:00:00:01:00:00
> +dl_src=00:00:00:02:00:00
>  dl_src=00:00:01:00:00:00
>  ])
>
> 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
> --
> 2.29.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list