Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-08 Thread Dumitru Ceara 
On 12/8/23 14:51, Daniel Ding wrote:
> On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara  wrote:
> 
>> On 12/6/23 02:56, Daniel Ding wrote:
>>> Hi Dumitru!
>>>
>>> On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:
>>>
 On 12/5/23 13:58, Daniel Ding wrote:
>
>
> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > 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
> 
> >
> > Signed-off-by: Daniel Ding  >
> > Acked-by: Dumitru Ceara >>> 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  | 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(_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >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(_ips_v6,
 nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(_ips_v4,
 nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_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(_ips_v4);
> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>  const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-08 Thread Daniel Ding 
On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara  wrote:

> On 12/6/23 02:56, Daniel Ding wrote:
> > Hi Dumitru!
> >
> > On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:
> >
> >> On 12/5/23 13:58, Daniel Ding wrote:
> >>>
> >>>
> >>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  >>> > 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
> >>> 
> >>> >
> >>> > Signed-off-by: Daniel Ding  >>> >
> >>> > Acked-by: Dumitru Ceara  >> 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  | 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(_ips_v4);
> >>> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> >>> > +
> >>> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >>> >  struct ovn_nat *nat_entry = >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(_ips_v6,
> >> nat->external_ip)) {
> >>> > +continue;
> >>> > +}
> >>> > +sset_add(_ips_v6, nat->external_ip);
> >>> > +} else {
> >>> > +if (sset_contains(_ips_v4,
> >> nat->external_ip)) {
> >>> > +continue;
> >>> > +}
> >>> > +sset_add(_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(_ips_v4);
> >>> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> >>> -
> >>>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >>>  struct ovn_nat *nat_entry = >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,
> >>>

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-07 Thread Dumitru Ceara 
On 12/6/23 02:56, Daniel Ding wrote:
> Hi Dumitru!
> 
> On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:
> 
>> On 12/5/23 13:58, Daniel Ding wrote:
>>>
>>>
>>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara >> > 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
>>> 
>>> >
>>> > Signed-off-by: Daniel Ding >> >
>>> > Acked-by: Dumitru Ceara > 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  | 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(_ips_v4);
>>> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
>>> > +
>>> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>>> >  struct ovn_nat *nat_entry = >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(_ips_v6,
>> nat->external_ip)) {
>>> > +continue;
>>> > +}
>>> > +sset_add(_ips_v6, nat->external_ip);
>>> > +} else {
>>> > +if (sset_contains(_ips_v4,
>> nat->external_ip)) {
>>> > +continue;
>>> > +}
>>> > +sset_add(_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(_ips_v4);
>>> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
>>> -
>>>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>>>  struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
>>> -continue;
>>> -}
>>> -

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Daniel Ding 
Hi Dumitru!

On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:

> On 12/5/23 13:58, Daniel Ding wrote:
> >
> >
> > On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > > 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
> > 
> > >
> > > Signed-off-by: Daniel Ding  > >
> > > Acked-by: Dumitru Ceara  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  | 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(_ips_v4);
> > > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > > +
> > >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > >  struct ovn_nat *nat_entry = >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(_ips_v6,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(_ips_v6, nat->external_ip);
> > > +} else {
> > > +if (sset_contains(_ips_v4,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(_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(_ips_v4);
> > -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > -
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
> > -continue;
> > -}
> > -sset_add(_ips_v6, nat->external_ip);
> > -} else {
> >

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Dumitru Ceara 
On 12/5/23 13:58, Daniel Ding wrote:
> 
> 
> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > 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
> 
> >
> > Signed-off-by: Daniel Ding  >
> > Acked-by: Dumitru Ceara 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
>  
> 
> 
> That doesn't mean I acked the patch.
> 
> 
> Got it. Thx!
>  

No worries.

> 
> > ---
> >  northd/northd.c     | 18 +-
> >  tests/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(_ips_v4);
> > +    struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >      for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >          struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
> > +                    continue;
> > +                }
> > +                sset_add(_ips_v6, nat->external_ip);
> > +            } else {
> > +                if (sset_contains(_ips_v4, nat->external_ip)) {
> > +                    continue;
> > +                }
> > +                sset_add(_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(_ips_v4);
> -    struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>      for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>          struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
> -                    continue;
> -                }
> -                sset_add(_ips_v6, nat->external_ip);
> -            } else {
> -                if (sset_contains(_ips_v4, nat->external_ip)) {
> -                    continue;
> -                }
> -                sset_add(_ips_v4, nat->external_ip);
> -            }
> -        }
> -
>          /* Check if the ovn port has a network

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Daniel Ding 
On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  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.


> 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
> >
> > Signed-off-by: Daniel Ding 
> > Acked-by: Dumitru Ceara 
>
> 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
>
> That doesn't mean I acked the patch.
>
>
Got it. Thx!


> > ---
> >  northd/northd.c | 18 +-
> >  tests/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(_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(_ips_v4, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_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(_ips_v4);
> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
> -continue;
> -}
> -sset_add(_ips_v6, nat->external_ip);
> -} else {
> -if (sset_contains(_ips_v4, nat->external_ip)) {
> -continue;
> -}
> -sset_add(_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(_ips_v4);
> -sset_destroy(_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.

 static void
> ---
>
> Best regards,
> Dumitru
>
> >  }
> >
> >  /* Check if the ovn port has a network configured on which we
> could
> > @@ -9025,6 +9038,9 @@

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-05 Thread Dumitru Ceara 
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.

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
> 
> Signed-off-by: Daniel Ding 
> Acked-by: Dumitru Ceara 

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

That doesn't mean I acked the patch.

> ---
>  northd/northd.c | 18 +-
>  tests/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(_ips_v4);
> +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> +
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
> +continue;
> +}
> +sset_add(_ips_v6, nat->external_ip);
> +} else {
> +if (sset_contains(_ips_v4, nat->external_ip)) {
> +continue;
> +}
> +sset_add(_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(_ips_v4);
-struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
-
 for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
 struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
-continue;
-}
-sset_add(_ips_v6, nat->external_ip);
-} else {
-if (sset_contains(_ips_v4, nat->external_ip)) {
-continue;
-}
-sset_add(_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(_ips_v4);
-sset_destroy(_ips_v6);
 }
 
 static void
---

Best regards,
Dumitru

>  }
>  
>  /* Check if the ovn port has a network configured on which we could
> @@ -9025,6 +9038,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(_ips_v4);
> +sset_destroy(_ips_v6);
>  }
>  
>  static void
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5c9da811f..953e0d829 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
> 's/table=../table=??/' | sort],
>

[ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

2023-12-04 Thread Daniel Ding 
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

Signed-off-by: Daniel Ding 
Acked-by: Dumitru Ceara 
---
 northd/northd.c | 18 +-
 tests/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(_ips_v4);
+struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
+
 for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
 struct ovn_nat *nat_entry = >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(_ips_v6, nat->external_ip)) {
+continue;
+}
+sset_add(_ips_v6, nat->external_ip);
+} else {
+if (sset_contains(_ips_v4, nat->external_ip)) {
+continue;
+}
+sset_add(_ips_v4, nat->external_ip);
+}
 }
 
 /* Check if the ovn port has a network configured on which we could
@@ -9025,6 +9038,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(_ips_v4);
+sset_destroy(_ips_v6);
 }
 
 static void
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5c9da811f..953e0d829 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
's/table=../table=??/' | sort],
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
   table=??(ls_in_l2_lkup  ), priority=75   , match=(eth.src == 
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport 
= "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport = "ls1-ro1"; 
output; }; outport = "_MC_flood_l2"; output;)
+  table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 10.0.0.200), action=(clone {outport = "ls1-ro1"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 192.168.1.1), action=(clone {outport = "ls1-ro1"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport = 
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
 ])
@@ -5098,6 +5099,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 
's/table=../table=??/' | sort],
   table=??(ls_in_l2_lkup  ), priority=75   , match=(eth.src == 
{00:00:00:00:02:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport 
= "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 192.168.2.1), action=(clone {outport = "ls2-ro2"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 20.0.0.100), action=(clone {outport = "ls2-ro2"; 
output; }; outport = "_MC_flood_l2"; output;)
+  table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 20.0.0.200), action=(clone {outport = "ls2-ro2"; 
output; }; outport = "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
nd_ns && nd.target == fe80::200:ff:fe00:201), action=(clone {outport = 
"ls2-ro2"; output; }; outport = "_MC_flood_l2"; output;)
 ])
 
@@ -5118,8 +5120,10 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
's/table=../table=??/' | sort],
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
   table=??(ls_in_l2_lkup  ), priority=75   , match=(eth.src == 
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport 
= "_MC_flood_l2"; output;)
   table=??(ls_in_l2_lkup  ), priority=80   , match=(flags[[1]] == 0 && 
arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport =