[ovs-dev] [PATCH ovn v2] ovn-northd: Fix multiple ARP replies for SNAT entries configured on a distributed router.

[Dumitru Ceara]

On 9/14/20 12:24 PM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> The commit in the Fixes tag, while addressing the issue to send ARP replies for
> the SNAT entries, didn't take into account the gateway router port scenario.
> Because of this, all the chassis which have bridge mappings configured reply
> to ARP request for SNAT entries. This patch fixes the issue by adding
> "is_chassis_resident()" condition for such flows so that only the gateway chassis
> which claims the gateway router port responds to the ARP request.
> 
> Note: This patch doesn't require any changes to ovn-northd.8.xml as it was already
> documented with the desired flows for SNAT entries.
> 
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050679.html
> Reported-by: Chris <kklimonda at syntaxhighlighted.com>
> Fixes: e2aa124ff7c2("ovn-northd: Add ARP responder flows for SNAT entries.")
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
> v1 -> v2
> --- 
>  * Address review comments from Dumitru.
> 
>  northd/ovn-northd.c |  18 ++-
>  tests/ovn-northd.at |   8 +-
>  tests/ovn.at        | 260 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 279 insertions(+), 7 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index f394f17c3..b37ccef2a 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -8856,10 +8856,13 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                               ext_addrs->ipv4_addrs[0].addr_s;
>  
>              if (!strcmp(nat->type, "snat")) {
> -                if (sset_contains(&snat_ips, ext_addr)) {
> +                /* If its a router with distributed gateway port then don't
> +                 * add the ARP flow for SNAT entries. Otherwise all the
> +                 * chassis (which have ovn-bridge-mappings) configured will
> +                 * reply for the ARP request to the snat external_ip. */
> +                if (od->l3dgw_port || !sset_add(&snat_ips, ext_addr)) {

Hi Numan,

Thinking more about it, we don't need "if (od->l3dgw_port" here, right?
This is where we generate priority-90 flows. Priority 91, 92 (for
distributed gw ports) are generated below. That should be enough and
we'd maintain the same logic as for other NAT types.

As a matter of fact, the tests pass just fine if I change this statement to:

if (!sset_add(&snat_ips, ext_addr)) {

On the same refactoring note as on v1, we have this kind of check "if
od->l3dgw_port then do this, otherwise to something completely
different" in a lot of places in the code. Maybe it's worth dedicating
separate functions for logical routers with distributed gateway ports vs
logical routers without. What do you think?

Similar as we discussed on the v1 patch, we should do this as a follow
up. But should we write them down somewhere? Would this be worth an
entry in the TODO.rst file?

>                      continue;
>                  }
> -                sset_add(&snat_ips, ext_addr);
>              }
>  
>              /* Priority 91 and 92 flows are added for each gateway router
> @@ -9237,6 +9240,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>              continue;
>          }
>  
> +        struct sset sset_snat_ips = SSET_INITIALIZER(&sset_snat_ips);

Nit (also for the follow up): this is very confusing. In the same
function, build_lrouter_flows(), we have "struct sset snat_ips", "struct
v46_ip *snat_ips", "struct sset sset_snat_ips". We should probably just
prepopulate a sset as part of struct ovn_datapath and use it everywhere,
like we do for other things like "nat_entries".

Regards,
Dumitru