diff mbox series

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

Message ID CALLnitDhFcEtzAPKHJtddyX1CX_2cYS2OtqScrhpv6Jse0MV6g@mail.gmail.com
State Changes Requested
Delegated to: Dumitru Ceara
Headers show
Series [ovs-dev] northd: forward arp request to lrp snat on. | expand

Commit Message

Daniel Ding Oct. 24, 2023, 7:39 a.m. UTC 
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 <danieldin186@gmail.com>
---
 northd/northd.c     | 18 +++++++++++++++++-
 tests/ovn-northd.at | 12 ++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

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 = "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 &&
arp.op == 1 && arp.tpa == 30.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 == 30.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 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
 ])

@@ -5133,7 +5137,9 @@ 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 &&
arp.op == 1 && arp.tpa == 40.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 == 40.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;)
 ])

@@ -5151,9 +5157,11 @@ 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 &&
arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
 ])

@@ -5169,9 +5177,11 @@ 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 &&
arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
 ])

@@ -5193,9 +5203,11 @@ 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 &&
arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
 ])

--
2.39.0

Comments

Dumitru Ceara Nov. 10, 2023, 2:51 p.m. UTC | #1 
On 10/24/23 09:39, 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 <danieldin186@gmail.com>
> ---

Hi Daniel,

Thanks for the patch!  I think it makes sense to do what you're
suggesting.  The patch itself, however, is malformed.

Could you please use git format-patch and git send-email?

https://github.com/ovn-org/ovn/blob/3bf67405faed9fc5c2d07024b0775e0d363651a6/Documentation/internals/contributing/submitting-patches.rst?plain=1#L90

I fixed the patch formatting so I can apply it locally for review; it
was mostly lines being truncated to shorten their lengths.

>  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(&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);
> +            }
>          }

Can't we just remove the whole if?  And not skip all snats?

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(&snat_ips_v4);
> +    sset_destroy(&snat_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 = "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 &&
> arp.op == 1 && arp.tpa == 30.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 == 30.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 &&
> nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
>  ])
> 
> @@ -5133,7 +5137,9 @@ 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 &&
> arp.op == 1 && arp.tpa == 40.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 == 40.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;)
>  ])
> 
> @@ -5151,9 +5157,11 @@ 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 &&
> arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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 &&
> nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
>  ])
> 
> @@ -5169,9 +5177,11 @@ 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 &&
> arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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 &&
> nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
>  ])
> 
> @@ -5193,9 +5203,11 @@ 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 &&
> arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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 &&
> nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
>  ])
> 
> --
> 2.39.0
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Daniel Ding Dec. 5, 2023, 3:18 a.m. UTC | #2 
On Fri, 10 Nov 2023 at 22:51, Dumitru Ceara <dceara@redhat.com> wrote:

> On 10/24/23 09:39, 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 <danieldin186@gmail.com>
> > ---
>
> Hi Daniel,
>
> Thanks for the patch!  I think it makes sense to do what you're
> suggesting.  The patch itself, however, is malformed.
>
> Could you please use git format-patch and git send-email?
>
>
> https://github.com/ovn-org/ovn/blob/3bf67405faed9fc5c2d07024b0775e0d363651a6/Documentation/internals/contributing/submitting-patches.rst?plain=1#L90
>
> I fixed the patch formatting so I can apply it locally for review; it
> was mostly lines being truncated to shorten their lengths.
>

Thanks! I will resubmit it with v3.


> >  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(&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);
> > +            }
> >          }
>
> Can't we just remove the whole if?  And not skip all snats?
>

Can't remove the whole if. The address of snats is not added to op->od->
lb_ips.  and we don't consider lb_ips to snat_ips.


>
> 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(&snat_ips_v4);
> > +    sset_destroy(&snat_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 =
> "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
> &&
> > arp.op == 1 && arp.tpa == 30.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 == 30.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
> &&
> > nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> > "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
> >  ])
> >
> > @@ -5133,7 +5137,9 @@ 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
> &&
> > arp.op == 1 && arp.tpa == 40.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 == 40.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;)
> >  ])
> >
> > @@ -5151,9 +5157,11 @@ 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
> &&
> > arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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
> &&
> > nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> > "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
> >  ])
> >
> > @@ -5169,9 +5177,11 @@ 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
> &&
> > arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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
> &&
> > nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> > "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
> >  ])
> >
> > @@ -5193,9 +5203,11 @@ 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
> &&
> > arp.op == 1 && arp.tpa == 192.168.1.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 == 30.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 == 30.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
> &&
> > nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
> > "ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
> >  ])
> >
> > --
> > 2.39.0
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
diff mbox series

Patch

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);
+            }
         }

         /* 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(&snat_ips_v4);
+    sset_destroy(&snat_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 &&