On 12/8/23 14:51, Daniel Ding wrote:
> On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara <dce...@redhat.com> wrote:
>
>> On 12/6/23 02:56, Daniel Ding wrote:
>>> Hi Dumitru!
>>>
>>> On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara <dce...@redhat.com> wrote:
>>>
>>>> On 12/5/23 13:58, Daniel Ding wrote:
>>>>>
>>>>>
>>>>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara <dce...@redhat.com
>>>>> <mailto:dce...@redhat.com>> wrote:
>>>>>
>>>>> Hi Daniel,
>>>>>
>>>>> Thanks for this new revision but why is it v3? I don't think I saw
>>>> v2
>>>>> posted anywhere but maybe I missed it.
>>>>>
>>>>>
>>>>> Sorry, that is my mistake.
>>>>>
>>>>
>>>> No problem.
>>>>
>>>>>
>>>>> On 12/5/23 08:33, Daniel Ding wrote:
>>>>> > If the router has a snat rule and it's external ip isn't lrp
>>>> address,
>>>>> > when the arp request from other router for this external ip, will
>>>>> > be drop, because of this external ip use same mac address as lrp,
>>>> so
>>>>> > can not forward to MC_FLOOD.
>>>>> >
>>>>> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
>>>>> whenever possible.")
>>>>> > Reported-at: https://github.com/ovn-org/ovn/issues/209
>>>>> <https://github.com/ovn-org/ovn/issues/209>
>>>>> >
>>>>> > Signed-off-by: Daniel Ding <danieldin...@gmail.com
>>>>> <mailto:danieldin...@gmail.com>>
>>>>> > Acked-by: Dumitru Ceara <dce...@redhat.com <mailto:
>>>> dce...@redhat.com>>
>>>>>
>>>>> Please don't add an "Acked-by: ... " if the person never explicitly
>>>>> replied with "Acked-by: ... " on the previous version of the patch
>> or
>>>>> if you didn't have explicit agreement from that person to do so.
>>>>>
>>>>> Quoting from my previous reply to your v1, I said:
>>>>>
>>>>> "I think it makes sense to do what you're suggesting."
>>>>>
>>>>>
>>>>
>> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>>>> <
>>>>
>> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>>>>>
>>>>>
>>>>> That doesn't mean I acked the patch.
>>>>>
>>>>>
>>>>> Got it. Thx!
>>>>>
>>>>
>>>> No worries.
>>>>
>>>>>
>>>>> > ---
>>>>> > northd/northd.c | 18 +++++++++++++++++-
>>>>> > tests/ovn-northd.at <http://ovn-northd.at> | 12 ++++++++++++
>>>>> > 2 files changed, 29 insertions(+), 1 deletion(-)
>>>>> >
>>>>> > diff --git a/northd/northd.c b/northd/northd.c
>>>>> > index e9cb906e2..99fb30f46 100644
>>>>> > --- a/northd/northd.c
>>>>> > +++ b/northd/northd.c
>>>>> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
>>>>> ovn_port *op,
>>>>> > }
>>>>> > }
>>>>> >
>>>>> > + struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
>>>>> > + struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
>>>>> > +
>>>>> > for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>>>>> > struct ovn_nat *nat_entry = &op->od->nat_entries[i];
>>>>> > const struct nbrec_nat *nat = nat_entry->nb;
>>>>> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
>>>>> ovn_port *op,
>>>>> > }
>>>>> >
>>>>> > if (!strcmp(nat->type, "snat")) {
>>>>> > - continue;
>>>>> > + if (nat_entry_is_v6(nat_entry)) {
>>>>> > + if (sset_contains(&snat_ips_v6,
>>>> nat->external_ip)) {
>>>>> > + continue;
>>>>> > + }
>>>>> > + sset_add(&snat_ips_v6, nat->external_ip);
>>>>> > + } else {
>>>>> > + if (sset_contains(&snat_ips_v4,
>>>> nat->external_ip)) {
>>>>> > + continue;
>>>>> > + }
>>>>> > + sset_add(&snat_ips_v4, nat->external_ip);
>>>>> > + }
>>>>>
>>>>> Essentially this just makes sure we don't skip NAT entries and that
>>>> we
>>>>> consider unique external_ips. I'm fine with relaxing the second
>>>> part of
>>>>> the condition in which case, as mentioned on v1, I think we can
>> just
>>>>> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
>>>>>
>>>>> With the following incremental change applied (it removes the
>> block)
>>>>> your test still passes:
>>>>>
>>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>>> index df7d2d60a5..20efd3b74c 100644
>>>>> --- a/northd/northd.c
>>>>> +++ b/northd/northd.c
>>>>> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
>>>>> ovn_port *op,
>>>>> }
>>>>> }
>>>>>
>>>>> - struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
>>>>> - struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
>>>>> -
>>>>> for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>>>>> struct ovn_nat *nat_entry = &op->od->nat_entries[i];
>>>>> const struct nbrec_nat *nat = nat_entry->nb;
>>>>> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
>>>>> ovn_port *op,
>>>>> continue;
>>>>> }
>>>>>
>>>>> - if (!strcmp(nat->type, "snat")) {
>>>>> - if (nat_entry_is_v6(nat_entry)) {
>>>>> - if (sset_contains(&snat_ips_v6,
>> nat->external_ip)) {
>>>>> - continue;
>>>>> - }
>>>>> - sset_add(&snat_ips_v6, nat->external_ip);
>>>>> - } else {
>>>>> - if (sset_contains(&snat_ips_v4,
>> nat->external_ip)) {
>>>>> - continue;
>>>>> - }
>>>>> - sset_add(&snat_ips_v4, nat->external_ip);
>>>>> - }
>>>>> - }
>>>>> -
>>>>> /* Check if the ovn port has a network configured on which
>>>>> we could
>>>>> * expect ARP requests/NS for the DNAT external_ip.
>>>>> */
>>>>> @@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct
>>>>> ovn_port *op,
>>>>> if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
>>>>> build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od,
>>>>> lflows);
>>>>> }
>>>>> -
>>>>> - sset_destroy(&snat_ips_v4);
>>>>> - sset_destroy(&snat_ips_v6);
>>>>> }
>>>>>
>>>>>
>>>>> If the nat_entries has multiple snats with the same external_ip, I
>> think
>>>>> it should check exernal_ip whether in a string hash. In addition, the
>>>>> snat_and_dnat entry is working normally, so exclude it.
>>>>>
>>>>
>>>> I'm not sure I understand, we weren't skipping dnat_and_snat before and
>>>> we wouldn't be skipping it with this either. Regarding the duplicates,
>>>> it really depends how common it is to have duplicates, I guess.
>>>>
>>>>
>>> If I understand correctly, we just remove "if (!strcmp(nat->type,
>> "snat"))
>>> {" block. And keep external checking because we are not sure about too
>> many
>>> snats in a lr.
>>
>> 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
>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
