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 <[email protected]>
> Acked-by: Dumitru Ceara <[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
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(&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);
}
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(&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;)
> ])
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
