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