On Fri, Mar 19, 2021 at 2:20 AM Mark Michelson <[email protected]> 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 <[email protected]>
Thanks for addressing this. Acked-by: Numan Siddique <[email protected]> @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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
