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

Mark Michelson mmichels at redhat.com
Thu Mar 18 20:47:11 UTC 2021


On 3/16/21 6:36 AM, Numan Siddique wrote:
> On Thu, Mar 11, 2021 at 3:39 AM Ben Pfaff <blp at ovn.org> wrote:
>>
>> 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>
> 
> Hi Mark,
> 
> The test case - 136: ovn -- ARP/ND request broadcast limiting  is
> failing with this patch.
> 
> I'd also suggest adding tests in ovn-northd.at to make sure that
> ovn-northd and ovn-northd-ddlog
> program the same flows.

Fixing that test actually killed two birds with one stone. That test was 
failing because northd was generating different flows from what it used 
to. And by fixing it, it ensures that ovn-northd an ovn-northd-ddlog are 
generating the same flows.

> 
> Thanks
> Numan
> 
>>>> ---
>>>>    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
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> 



More information about the dev mailing list