On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara <[email protected]> wrote:
> On 12/6/23 02:56, Daniel Ding wrote: > > Hi Dumitru! > > > > On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara <[email protected]> wrote: > > > >> On 12/5/23 13:58, Daniel Ding wrote: > >>> > >>> > >>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara <[email protected] > >>> <mailto:[email protected]>> wrote: > >>> > >>> Hi Daniel, > >>> > >>> Thanks for this new revision but why is it v3? I don't think I saw > >> v2 > >>> posted anywhere but maybe I missed it. > >>> > >>> > >>> Sorry, that is my mistake. > >>> > >> > >> No problem. > >> > >>> > >>> On 12/5/23 08:33, Daniel Ding wrote: > >>> > If the router has a snat rule and it's external ip isn't lrp > >> address, > >>> > when the arp request from other router for this external ip, will > >>> > be drop, because of this external ip use same mac address as lrp, > >> so > >>> > can not forward to MC_FLOOD. > >>> > > >>> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain > >>> whenever possible.") > >>> > Reported-at: https://github.com/ovn-org/ovn/issues/209 > >>> <https://github.com/ovn-org/ovn/issues/209> > >>> > > >>> > Signed-off-by: Daniel Ding <[email protected] > >>> <mailto:[email protected]>> > >>> > Acked-by: Dumitru Ceara <[email protected] <mailto: > >> [email protected]>> > >>> > >>> Please don't add an "Acked-by: ... " if the person never explicitly > >>> replied with "Acked-by: ... " on the previous version of the patch > or > >>> if you didn't have explicit agreement from that person to do so. > >>> > >>> Quoting from my previous reply to your v1, I said: > >>> > >>> "I think it makes sense to do what you're suggesting." > >>> > >>> > >> > https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934 > >> < > >> > https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934 > >>> > >>> > >>> That doesn't mean I acked the patch. > >>> > >>> > >>> Got it. Thx! > >>> > >> > >> No worries. > >> > >>> > >>> > --- > >>> > northd/northd.c | 18 +++++++++++++++++- > >>> > tests/ovn-northd.at <http://ovn-northd.at> | 12 ++++++++++++ > >>> > 2 files changed, 29 insertions(+), 1 deletion(-) > >>> > > >>> > diff --git a/northd/northd.c b/northd/northd.c > >>> > index e9cb906e2..99fb30f46 100644 > >>> > --- a/northd/northd.c > >>> > +++ b/northd/northd.c > >>> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct > >>> ovn_port *op, > >>> > } > >>> > } > >>> > > >>> > + struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4); > >>> > + struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6); > >>> > + > >>> > 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; > >>> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct > >>> ovn_port *op, > >>> > } > >>> > > >>> > if (!strcmp(nat->type, "snat")) { > >>> > - continue; > >>> > + if (nat_entry_is_v6(nat_entry)) { > >>> > + if (sset_contains(&snat_ips_v6, > >> nat->external_ip)) { > >>> > + continue; > >>> > + } > >>> > + sset_add(&snat_ips_v6, nat->external_ip); > >>> > + } else { > >>> > + if (sset_contains(&snat_ips_v4, > >> nat->external_ip)) { > >>> > + continue; > >>> > + } > >>> > + sset_add(&snat_ips_v4, nat->external_ip); > >>> > + } > >>> > >>> Essentially this just makes sure we don't skip NAT entries and that > >> we > >>> consider unique external_ips. I'm fine with relaxing the second > >> part of > >>> the condition in which case, as mentioned on v1, I think we can > just > >>> remove the whole "if (!strcmp(nat->type, "snat")) {" block. > >>> > >>> With the following incremental change applied (it removes the > block) > >>> your test still passes: > >>> > >>> diff --git a/northd/northd.c b/northd/northd.c > >>> index df7d2d60a5..20efd3b74c 100644 > >>> --- a/northd/northd.c > >>> +++ b/northd/northd.c > >>> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct > >>> ovn_port *op, > >>> } > >>> } > >>> > >>> - struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4); > >>> - struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6); > >>> - > >>> 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; > >>> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct > >>> ovn_port *op, > >>> continue; > >>> } > >>> > >>> - if (!strcmp(nat->type, "snat")) { > >>> - if (nat_entry_is_v6(nat_entry)) { > >>> - if (sset_contains(&snat_ips_v6, > nat->external_ip)) { > >>> - continue; > >>> - } > >>> - sset_add(&snat_ips_v6, nat->external_ip); > >>> - } else { > >>> - if (sset_contains(&snat_ips_v4, > nat->external_ip)) { > >>> - continue; > >>> - } > >>> - sset_add(&snat_ips_v4, nat->external_ip); > >>> - } > >>> - } > >>> - > >>> /* Check if the ovn port has a network configured on which > >>> we could > >>> * expect ARP requests/NS for the DNAT external_ip. > >>> */ > >>> @@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct > >>> ovn_port *op, > >>> if (sw_od->n_router_ports != sw_od->nbs->n_ports) { > >>> build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, > >>> lflows); > >>> } > >>> - > >>> - sset_destroy(&snat_ips_v4); > >>> - sset_destroy(&snat_ips_v6); > >>> } > >>> > >>> > >>> If the nat_entries has multiple snats with the same external_ip, I > think > >>> it should check exernal_ip whether in a string hash. In addition, the > >>> snat_and_dnat entry is working normally, so exclude it. > >>> > >> > >> I'm not sure I understand, we weren't skipping dnat_and_snat before and > >> we wouldn't be skipping it with this either. Regarding the duplicates, > >> it really depends how common it is to have duplicates, I guess. > >> > >> > > If I understand correctly, we just remove "if (!strcmp(nat->type, > "snat")) > > {" block. And keep external checking because we are not sure about too > many > > snats in a lr. > > If we're worried about duplicate SNAT IPs, we should probably just use > the precomputed op->od->snat_ips shash. It already consists only of > unique external IPs used for SNAT on the router. > > So no need for the additional ssets you're using. We can just walk the > elements of the "nat_ips" shash directly. > Hi, Dumitru! I'm using od->snat_ips shash to ensure unique external IPs for SNAT. And please help to review again. Thank you! diff --git a/northd/northd.c b/northd/northd.c index 65328434a..08b09da02 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7170,6 +7170,37 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, } } + struct shash_node *snat_snode; + SHASH_FOR_EACH (snat_snode, &op->od->snat_ips) { + struct ovn_snat_ip *snat_ip = snat_snode->data; + + if (ovs_list_is_empty(&snat_ip->snat_entries)) { + continue; + } + + struct ovn_nat *nat_entry = + CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries), + struct ovn_nat, ext_addr_list_node); + const struct nbrec_nat *nat = nat_entry->nb; + + /* 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)) { + if (!sset_contains(&op->od->lb_ips_v6, nat->external_ip)) { + build_lswitch_rport_arp_req_flow( + nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows, + stage_hint); + } + } else { + if (!sset_contains(&op->od->lb_ips_v4, nat->external_ip)) { + build_lswitch_rport_arp_req_flow( + nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows, + stage_hint); + } + } + } + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { build_lswitch_rport_arp_req_flow( op->lrp_networks.ipv4_addrs[i].addr_s, AF_INET, sw_op, sw_od, 80, > > > > > diff --git a/northd/northd.c b/northd/northd.c > > index e9cb906e2..d93870e25 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port > *op, > > } > > } > > > > + struct sset nat_ips_v4 = SSET_INITIALIZER(&nat_ips_v4); > > + struct sset nat_ips_v6 = SSET_INITIALIZER(&nat_ips_v6); > > + > > 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; > > @@ -8982,8 +8985,16 @@ build_lswitch_rport_arp_req_flows(struct ovn_port > > *op, > > continue; > > } > > > > - if (!strcmp(nat->type, "snat")) { > > - continue; > > + if (nat_entry_is_v6(nat_entry)) { > > + if (sset_contains(&nat_ips_v6, nat->external_ip)) { > > + continue; > > + } > > + sset_add(&nat_ips_v6, nat->external_ip); > > + } else { > > + if (sset_contains(&nat_ips_v4, nat->external_ip)) { > > + continue; > > + } > > + sset_add(&nat_ips_v4, nat->external_ip); > > } > > > > /* Check if the ovn port has a network configured on which we > could > > @@ -9025,6 +9036,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port > *op, > > if (sw_od->n_router_ports != sw_od->nbs->n_ports) { > > build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, > lflows); > > } > > + > > + sset_destroy(&nat_ips_v4); > > + sset_destroy(&nat_ips_v6); > > } > > > > Regards, > Dumitru > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
