On 7/9/20 8:15 PM, Numan Siddique wrote: > > > On Wed, Jul 8, 2020 at 8:34 PM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: > > Logical flows that limit the ARP/NS broadcast domain on a logical switch > should only match on ARP requests/NS for IPs that can actually be > replied to on the connected router port (i.e., an IP on the same network > is configured on the router port). > > Reported-by: Girish Moodalbail <[email protected] > <mailto:[email protected]>> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-June/050287.html > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain > whenever possible.") > Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> > > > Hi Dumitru, > > The patch LGTM. I've few nits. > > Thanks > Numan >
Hi Numan, Thanks for the review, I sent a v3: https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ Thanks, Dumitru > > --- > northd/ovn-northd.c | 164 > ++++++++++++++++++++++++++++++++++++++++++--------- > tests/ovn.at <http://ovn.at> | 74 +++++++++++++++++++++++ > 2 files changed, 210 insertions(+), 28 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 4c23158..e2e875a 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -6091,6 +6091,44 @@ 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 ((op_addr->addr & op_addr->mask) == (addr & > op_addr->mask)) { > > > This can be - > if ((addr & op_addr->mask) == (op_addr->network)) { > > If you see lib/ovn-util.c, network is initialized as - addr & mask. > > > > > + 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 op_addr6_masked = > + ipv6_addr_bitand(&op_addr->addr, &op_addr->mask); > > > Same as above. We don't need to do this and instead we can use - > op_addr->network > below in ipv6_addr_equals() > > > + struct in6_addr nat_addr6_masked = > + ipv6_addr_bitand(addr, &op_addr->mask); > + > + if (ipv6_addr_equals(&op_addr6_masked, &nat_addr6_masked)) { > > + return true; > + } > + } > + return false; > +} > + > /* > * Ingress table 19: Flows that flood self originated ARP/ND > packets in the > * switching domain. > @@ -6101,8 +6139,47 @@ > build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op, > struct ovn_datapath *od, > struct hmap *lflows) > { > - struct ds match = DS_EMPTY_INITIALIZER; > + struct sset all_eth_addrs = SSET_INITIALIZER(&all_eth_addrs); > struct ds eth_src = DS_EMPTY_INITIALIZER; > + struct ds match = DS_EMPTY_INITIALIZER; > + > + sset_add(&all_eth_addrs, op->lrp_networks.ea_s); > + > + 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; > + > + if (!nat_entry_is_valid(nat_entry)) { > + continue; > + } > + > + if (!strcmp(nat->type, "snat")) { > + continue; > + } > + > + if (!nat->external_mac) { > + continue; > + } > + > + /* Check if there's a network configured on op on which we > could > > > s/op/ovn port > or > s/op/'op' > > I couldn't understand what op is. So either putting it in quotes or > referencing it as an > "ovn port" would be easier to understand. > > > > + * 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); > + } > + > > /* Self originated (G)ARP requests/ND need to be flooded as usual. > * Determine that packets are self originated by also matching on > @@ -6110,15 +6187,11 @@ > build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op, > * is a VLAN-backed network. > * Priority: 80. > */ > - ds_put_format(ð_src, "{ %s, ", op->lrp_networks.ea_s); > - for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > - const struct nbrec_nat *nat = op->od->nbr->nat[i]; > - > - if (!nat->external_mac) { > - continue; > - } > + const char *eth_addr; > > - ds_put_format(ð_src, "%s, ", nat->external_mac); > + ds_put_cstr(ð_src, "{"); > + SSET_FOR_EACH (eth_addr, &all_eth_addrs) { > + ds_put_format(ð_src, "%s, ", eth_addr); > } > ds_chomp(ð_src, ' '); > ds_chomp(ð_src, ','); > @@ -6130,8 +6203,9 @@ > build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op, > ds_cstr(&match), > "outport = \""MC_FLOOD"\"; output;"); > > - ds_destroy(&match); > + sset_destroy(&all_eth_addrs); > ds_destroy(ð_src); > + ds_destroy(&match); > } > > /* > @@ -6222,38 +6296,72 @@ build_lswitch_rport_arp_req_flows(struct > ovn_port *op, > struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); > struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); > > - for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > - sset_add(&all_ips_v4, op->lrp_networks.ipv4_addrs[i].addr_s); > - } > - for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > - sset_add(&all_ips_v6, op->lrp_networks.ipv6_addrs[i].addr_s); > + 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 there's a network configured on op 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; > > - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); > + /* Check if there's a network configured on op 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++) { > - const struct nbrec_nat *nat = op->od->nbr->nat[i]; > + struct ovn_nat *nat_entry = &op->od->nat_entries[i]; > + const struct nbrec_nat *nat = nat_entry->nb; > + > + if (!nat_entry_is_valid(nat_entry)) { > + continue; > + } > > if (!strcmp(nat->type, "snat")) { > continue; > } > > - ovs_be32 ip; > - ovs_be32 mask; > - struct in6_addr ipv6; > - struct in6_addr mask_v6; > + /* Check if there's a network configured on op 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; > > - char *error = ip_parse_masked(nat->external_ip, &ip, &mask); > - if (error) { > - free(error); > - error = ipv6_parse_masked(nat->external_ip, &ipv6, > &mask_v6); > - if (!error) { > + if (lrouter_port_ipv6_reachable(op, addr)) { > sset_add(&all_ips_v6, nat->external_ip); > } > } else { > - sset_add(&all_ips_v4, nat->external_ip); > + 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); > + } > } > - free(error); > + } > + > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + sset_add(&all_ips_v4, op->lrp_networks.ipv4_addrs[i].addr_s); > + } > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > + sset_add(&all_ips_v6, op->lrp_networks.ipv6_addrs[i].addr_s); > } > > if (!sset_is_empty(&all_ips_v4)) { > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> > index 24d93bc..88700e8 100644 > --- a/tests/ovn.at <http://ovn.at> > +++ b/tests/ovn.at <http://ovn.at> > @@ -19249,12 +19249,14 @@ ovn-nbctl --wait=hv sync > > sw_dp_uuid=$(ovn-sbctl --bare --columns _uuid list datapath_binding > sw-agg) > sw_dp_key=$(ovn-sbctl --bare --columns tunnel_key list > datapath_binding sw-agg) > +sw1_dp_key=$(ovn-sbctl --bare --columns tunnel_key list > datapath_binding sw1) > r1_dp_key=$(ovn-sbctl --bare --columns tunnel_key list > datapath_binding rtr1) > > r1_tnl_key=$(ovn-sbctl --bare --columns tunnel_key list > port_binding sw-rtr1) > r2_tnl_key=$(ovn-sbctl --bare --columns tunnel_key list > port_binding sw-rtr2) > > match_sw_metadata="metadata=0x${sw_dp_key}" > +match_sw1_metadata="metadata=0x${sw1_dp_key}" > match_r1_metadata="metadata=0x${r1_dp_key}" > > # Inject ARP request for first router owned IP address. > @@ -19370,6 +19372,78 @@ ovn-nbctl lr-nat-add rtr1 dnat_and_snat > 10.0.0.122 20.0.0.12 sw1-p2 00:00:00:02: > ovn-nbctl lr-nat-add rtr1 dnat_and_snat 10::122 20::12 sw1-p2 > 00:00:00:02:00:00 > ovn-nbctl --wait=hv sync > > +# Check that broadcast domain limiting flows match only on IPs that are > +# directly reachable via the router port. > +# For sw-rtr1: > +# - 10.0.0.1, 10::1, fe80::200:ff:fe00:100 - 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. > +# For sw-rtr2: > +# - 10.0.0.2, 10::2, fe80::200:ff:fe00:200 - interface IPs. > +# - 10.0.0.22, 10::22 - LB VIPs. > +# - 10.0.0.222, 10::222 - DNAT IPs. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw_metadata}" | grep -oE "arp_tpa=[[0-9.]]+" > | sort], [0], [dnl > +arp_tpa=10.0.0.1 > +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=10.0.0.2 > +arp_tpa=10.0.0.22 > +arp_tpa=10.0.0.222 > +]) > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw_metadata}" | grep -oE > "nd_target=[[0-9a-f:]]+" | sort], [0], [dnl > +nd_target=10::1 > +nd_target=10::11 > +nd_target=10::111 > +nd_target=10::121 > +nd_target=10::122 > +nd_target=10::2 > +nd_target=10::22 > +nd_target=10::222 > +nd_target=fe80::200:ff:fe00:100 > +nd_target=fe80::200:ff:fe00:200 > +]) > + > +# For sw1-rtr1: > +# - 20.0.0.1, 20::1, fe80::200:1ff:fe00:0 - interface IPs. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw1_metadata}" | grep -oE "arp_tpa=[[0-9.]]+" > | sort], [0], [dnl > +arp_tpa=20.0.0.1 > +]) > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw1_metadata}" | grep -oE > "nd_target=[[0-9a-f:]]+" | sort], [0], [dnl > +nd_target=20::1 > +nd_target=fe80::200:1ff:fe00:0 > +]) > + > +# Self originated ARP/NS with SMACs owned by rtr1-sw and rtr2-sw > should be > +# flooded: > +# - 00:00:00:00:01:00 - interface MAC (rtr1-sw). > +# - 00:00:00:00:02:00 - interface MAC (rtr2-sw). > +# - 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=80,.*${match_sw_metadata}.*arp_op=1" | grep -oE > "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl > +dl_src=00:00:00:00:01:00 > +dl_src=00:00:00:00:02:00 > +dl_src=00:00:00:01:00:00 > +dl_src=00:00:00:02:00:00 > +]) > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=80,.*${match_sw_metadata}.*icmp_type=135" | grep -oE > "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl > +dl_src=00:00:00:00:01:00 > +dl_src=00:00:00:00:02:00 > +dl_src=00:00:00:01:00:00 > +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. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=80,.*${match_sw1_metadata}.*arp_op=1" | grep -oE > "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl > +dl_src=00:00:01:00:00:00 > +]) > + > # Inject ARP request for first router owned NAT address. > send_arp_request 1 0 ${src_mac} $(ip_to_hex 10 0 0 254) $(ip_to_hex > 10 0 0 111) > > > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
