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

Ben Pfaff blp at ovn.org
Wed Mar 10 22:07:12 UTC 2021


Please do take a look at the (minor) feedback I gave for v1 of patch 1.

On Wed, Mar 10, 2021 at 04:15:19PM -0500, Mark Michelson wrote:
> 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
> > 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list