On 9/14/20 12:24 PM, [email protected] wrote:
> From: Numan Siddique <[email protected]>
>
> 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 <[email protected]>
> Fixes: e2aa124ff7c2("ovn-northd: Add ARP responder flows for SNAT entries.")
> Signed-off-by: Numan Siddique <[email protected]>
> ---
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
