Hi Dumitru!

On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara <dce...@redhat.com> wrote:

> On 12/5/23 13:58, Daniel Ding wrote:
> >
> >
> > On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara <dce...@redhat.com
> > <mailto:dce...@redhat.com>> 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 <danieldin...@gmail.com
> >     <mailto:danieldin...@gmail.com>>
> >     > Acked-by: Dumitru Ceara <dce...@redhat.com <mailto:
> dce...@redhat.com>>
> >
> >     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.

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
>
>

-- 
Regards,
Daniel Ding
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to