diff mbox series

[ovs-dev,ovn] ovn-northd: Add ARP responder flows for SNAT entries.

Message ID 20200811073404.2154888-1-numans@ovn.org
State Superseded
Headers show
Series [ovs-dev,ovn] ovn-northd: Add ARP responder flows for SNAT entries. | expand

Commit Message

Numan Siddique Aug. 11, 2020, 7:34 a.m. UTC
From: Numan Siddique <numans@ovn.org>

If the below SNAT entry is added:
ovn-nbctl lr-nat-add snat 172.168.0.120 10.0.0.5

And when the logical port with IP - 10.0.0.5, sends a packet destined to
outside, gets SNATted to 172.168.0.120 as expected, but for the reverse traffic
if the upstream switch sends an ARP request for 172.168.0.120, then OVN doesn't
reply and the reply traffic never reaches the logical port.

This patch fixes this issue by addding ARP responder flows for SNAT entries.

Note that, if 172.168.0.120 happens to be the logical router IP, then it works.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861294
Reported-by: Alexander Constantinescu <aconstan@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/ovn-northd.8.xml | 18 +++++++++---------
 northd/ovn-northd.c     | 14 ++++++++++++--
 tests/ovn-northd.at     |  8 ++++++++
 3 files changed, 29 insertions(+), 11 deletions(-)

Comments

Dumitru Ceara Aug. 11, 2020, 9:32 a.m. UTC | #1
On 8/11/20 9:34 AM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> If the below SNAT entry is added:
> ovn-nbctl lr-nat-add snat 172.168.0.120 10.0.0.5
> 
> And when the logical port with IP - 10.0.0.5, sends a packet destined to
> outside, gets SNATted to 172.168.0.120 as expected, but for the reverse traffic
> if the upstream switch sends an ARP request for 172.168.0.120, then OVN doesn't
> reply and the reply traffic never reaches the logical port.
> 
> This patch fixes this issue by addding ARP responder flows for SNAT entries.
> 
> Note that, if 172.168.0.120 happens to be the logical router IP, then it works.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861294
> Reported-by: Alexander Constantinescu <aconstan@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---

Hi Numan,

This patch looks good to me overall. I do think that it would be good to
have a system test checking that ARP responder works fine for SNAT.
Except that I have one more minor comment inline.

Thanks,
Dumitru

>  northd/ovn-northd.8.xml | 18 +++++++++---------
>  northd/ovn-northd.c     | 14 ++++++++++++--
>  tests/ovn-northd.at     |  8 ++++++++
>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 20fd19450..ee21c825d 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1952,15 +1952,15 @@ nd_na_router {
>        <li>
>          <p>
>            These flows reply to ARP requests or IPv6 neighbor solicitation
> -          for the virtual IP addresses configured in the router for DNAT
> -          or load balancing.
> +          for the virtual IP addresses configured in the router for NAT
> +          (both DNAT and SNAT) or load balancing.
>          </p>
>  
>          <p>
> -          IPv4: For a configured DNAT IP address or a load balancer
> -          IPv4 VIP <var>A</var>, for each router port <var>P</var> with
> -          Ethernet address <var>E</var>, a priority-90 flow matches
> -          <code>arp.op == 1 &amp;&amp; arp.tpa == <var>A</var></code>
> +          IPv4: For a configured NAT (both DNAT and SNAT) IP address or a
> +          load balancer IPv4 VIP <var>A</var>, for each router port
> +          <var>P</var> with Ethernet address <var>E</var>, a priority-90 flow
> +          matches <code>arp.op == 1 &amp;&amp; arp.tpa == <var>A</var></code>
>            (ARP request) with the following actions:
>          </p>
>  
> @@ -1990,9 +1990,9 @@ output;
>          </p>
>  
>          <p>
> -          IPv6: For a configured DNAT IP address or a load balancer
> -          IPv6 VIP <var>A</var>, solicited node address <var>S</var>,
> -          for each router port <var>P</var> with
> +          IPv6: For a configured NAT (both DNAT and SNAT) IP address or a
> +          load balancer IPv6 VIP <var>A</var>, solicited node address
> +          <var>S</var>, for each router port <var>P</var> with
>            Ethernet address <var>E</var>, a priority-90 flow matches
>            <code>inport == <var>P</var> &amp;&amp; nd_ns &amp;&amp;
>            ip6.dst == {<var>A</var>, <var>S</var>} &amp;&amp;
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 99eb582c3..3b0272b00 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -8648,6 +8648,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>           * per logical port but DNAT addresses can be handled per datapath
>           * for non gateway router ports.
>           */
> +        struct sset snat_ips = SSET_INITIALIZER(&snat_ips);
>          for (int i = 0; i < od->nbr->n_nat; i++) {
>              struct ovn_nat *nat_entry = &od->nat_entries[i];
>              const struct nbrec_nat *nat = nat_entry->nb;
> @@ -8657,15 +8658,22 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                  continue;
>              }
>  
> +            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> +            char *ext_addr = nat_entry_is_v6(nat_entry) ?
> +                             ext_addrs->ipv6_addrs[0].addr_s :
> +                             ext_addrs->ipv4_addrs[0].addr_s;
> +
>              if (!strcmp(nat->type, "snat")) {
> -                continue;
> +                if (sset_contains(&snat_ips, ext_addr)) {
> +                    continue;
> +                }
> +                sset_add(&snat_ips, ext_addr);
>              }
>  
>              /* Priority 91 and 92 flows are added for each gateway router
>               * port to handle the special cases. In case we get the packet
>               * on a regular port, just reply with the port's ETH address.
>               */
> -            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
>              if (nat_entry_is_v6(nat_entry)) {
>                  build_lrouter_nd_flow(od, NULL, "nd_na",
>                                        ext_addrs->ipv6_addrs[0].addr_s,
> @@ -8679,6 +8687,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                                         &nat->header_, lflows);
>              }
>          }
> +        sset_destroy(&snat_ips);
> +

Nit: no need for the additional newline here.

>  
>          /* Drop ARP packets (priority 85). ARP request packets for router's own
>           * IPs are handled with priority-90 flows.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 7872d5051..8344c7f5e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1572,6 +1572,8 @@ ovn-nbctl set logical_router lr options:chassis=ch
>  ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.2 42.42.42.2
>  ovn-nbctl lr-nat-add lr dnat 43.43.43.3 42.42.42.3
>  ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.4 42.42.42.4 ls-vm 00:00:00:00:00:02
> +ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.50
> +ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.51
>  
>  ovn-nbctl --wait=sb sync
>  
> @@ -1594,6 +1596,9 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
>  # Ingress router port ETH address is used for ARP reply/NA in lr_in_ip_input.
>  AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> +match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
> +action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
> +  table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> @@ -1648,6 +1653,9 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
>  # Priority 90 flows (per router).
>  AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> +match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
> +action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
> +  table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>
Numan Siddique Aug. 11, 2020, 10:17 a.m. UTC | #2
On Tue, Aug 11, 2020 at 3:02 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 8/11/20 9:34 AM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > If the below SNAT entry is added:
> > ovn-nbctl lr-nat-add snat 172.168.0.120 10.0.0.5
> >
> > And when the logical port with IP - 10.0.0.5, sends a packet destined to
> > outside, gets SNATted to 172.168.0.120 as expected, but for the reverse traffic
> > if the upstream switch sends an ARP request for 172.168.0.120, then OVN doesn't
> > reply and the reply traffic never reaches the logical port.
> >
> > This patch fixes this issue by addding ARP responder flows for SNAT entries.
> >
> > Note that, if 172.168.0.120 happens to be the logical router IP, then it works.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861294
> > Reported-by: Alexander Constantinescu <aconstan@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
>
> Hi Numan,
>
> This patch looks good to me overall. I do think that it would be good to
> have a system test checking that ARP responder works fine for SNAT.
> Except that I have one more minor comment inline.

Thanks Dumitru for the review. I added the system test and submitted v2.
Request to take a look.

Thanks
Numan

>
> Thanks,
> Dumitru
>
> >  northd/ovn-northd.8.xml | 18 +++++++++---------
> >  northd/ovn-northd.c     | 14 ++++++++++++--
> >  tests/ovn-northd.at     |  8 ++++++++
> >  3 files changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 20fd19450..ee21c825d 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -1952,15 +1952,15 @@ nd_na_router {
> >        <li>
> >          <p>
> >            These flows reply to ARP requests or IPv6 neighbor solicitation
> > -          for the virtual IP addresses configured in the router for DNAT
> > -          or load balancing.
> > +          for the virtual IP addresses configured in the router for NAT
> > +          (both DNAT and SNAT) or load balancing.
> >          </p>
> >
> >          <p>
> > -          IPv4: For a configured DNAT IP address or a load balancer
> > -          IPv4 VIP <var>A</var>, for each router port <var>P</var> with
> > -          Ethernet address <var>E</var>, a priority-90 flow matches
> > -          <code>arp.op == 1 &amp;&amp; arp.tpa == <var>A</var></code>
> > +          IPv4: For a configured NAT (both DNAT and SNAT) IP address or a
> > +          load balancer IPv4 VIP <var>A</var>, for each router port
> > +          <var>P</var> with Ethernet address <var>E</var>, a priority-90 flow
> > +          matches <code>arp.op == 1 &amp;&amp; arp.tpa == <var>A</var></code>
> >            (ARP request) with the following actions:
> >          </p>
> >
> > @@ -1990,9 +1990,9 @@ output;
> >          </p>
> >
> >          <p>
> > -          IPv6: For a configured DNAT IP address or a load balancer
> > -          IPv6 VIP <var>A</var>, solicited node address <var>S</var>,
> > -          for each router port <var>P</var> with
> > +          IPv6: For a configured NAT (both DNAT and SNAT) IP address or a
> > +          load balancer IPv6 VIP <var>A</var>, solicited node address
> > +          <var>S</var>, for each router port <var>P</var> with
> >            Ethernet address <var>E</var>, a priority-90 flow matches
> >            <code>inport == <var>P</var> &amp;&amp; nd_ns &amp;&amp;
> >            ip6.dst == {<var>A</var>, <var>S</var>} &amp;&amp;
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 99eb582c3..3b0272b00 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -8648,6 +8648,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >           * per logical port but DNAT addresses can be handled per datapath
> >           * for non gateway router ports.
> >           */
> > +        struct sset snat_ips = SSET_INITIALIZER(&snat_ips);
> >          for (int i = 0; i < od->nbr->n_nat; i++) {
> >              struct ovn_nat *nat_entry = &od->nat_entries[i];
> >              const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8657,15 +8658,22 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                  continue;
> >              }
> >
> > +            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> > +            char *ext_addr = nat_entry_is_v6(nat_entry) ?
> > +                             ext_addrs->ipv6_addrs[0].addr_s :
> > +                             ext_addrs->ipv4_addrs[0].addr_s;
> > +
> >              if (!strcmp(nat->type, "snat")) {
> > -                continue;
> > +                if (sset_contains(&snat_ips, ext_addr)) {
> > +                    continue;
> > +                }
> > +                sset_add(&snat_ips, ext_addr);
> >              }
> >
> >              /* Priority 91 and 92 flows are added for each gateway router
> >               * port to handle the special cases. In case we get the packet
> >               * on a regular port, just reply with the port's ETH address.
> >               */
> > -            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> >              if (nat_entry_is_v6(nat_entry)) {
> >                  build_lrouter_nd_flow(od, NULL, "nd_na",
> >                                        ext_addrs->ipv6_addrs[0].addr_s,
> > @@ -8679,6 +8687,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                                         &nat->header_, lflows);
> >              }
> >          }
> > +        sset_destroy(&snat_ips);
> > +
>
> Nit: no need for the additional newline here.
>
> >
> >          /* Drop ARP packets (priority 85). ARP request packets for router's own
> >           * IPs are handled with priority-90 flows.
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 7872d5051..8344c7f5e 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -1572,6 +1572,8 @@ ovn-nbctl set logical_router lr options:chassis=ch
> >  ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.2 42.42.42.2
> >  ovn-nbctl lr-nat-add lr dnat 43.43.43.3 42.42.42.3
> >  ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.4 42.42.42.4 ls-vm 00:00:00:00:00:02
> > +ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.50
> > +ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.51
> >
> >  ovn-nbctl --wait=sb sync
> >
> > @@ -1594,6 +1596,9 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
> >  # Ingress router port ETH address is used for ARP reply/NA in lr_in_ip_input.
> >  AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
> >    table=3 (lr_in_ip_input     ), priority=90   , dnl
> > +match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
> > +action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
> > +  table=3 (lr_in_ip_input     ), priority=90   , dnl
> >  match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
> >  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
> >    table=3 (lr_in_ip_input     ), priority=90   , dnl
> > @@ -1648,6 +1653,9 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
> >  # Priority 90 flows (per router).
> >  AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
> >    table=3 (lr_in_ip_input     ), priority=90   , dnl
> > +match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
> > +action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
> > +  table=3 (lr_in_ip_input     ), priority=90   , dnl
> >  match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
> >  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
> >    table=3 (lr_in_ip_input     ), priority=90   , dnl
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 20fd19450..ee21c825d 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1952,15 +1952,15 @@  nd_na_router {
       <li>
         <p>
           These flows reply to ARP requests or IPv6 neighbor solicitation
-          for the virtual IP addresses configured in the router for DNAT
-          or load balancing.
+          for the virtual IP addresses configured in the router for NAT
+          (both DNAT and SNAT) or load balancing.
         </p>
 
         <p>
-          IPv4: For a configured DNAT IP address or a load balancer
-          IPv4 VIP <var>A</var>, for each router port <var>P</var> with
-          Ethernet address <var>E</var>, a priority-90 flow matches
-          <code>arp.op == 1 &amp;&amp; arp.tpa == <var>A</var></code>
+          IPv4: For a configured NAT (both DNAT and SNAT) IP address or a
+          load balancer IPv4 VIP <var>A</var>, for each router port
+          <var>P</var> with Ethernet address <var>E</var>, a priority-90 flow
+          matches <code>arp.op == 1 &amp;&amp; arp.tpa == <var>A</var></code>
           (ARP request) with the following actions:
         </p>
 
@@ -1990,9 +1990,9 @@  output;
         </p>
 
         <p>
-          IPv6: For a configured DNAT IP address or a load balancer
-          IPv6 VIP <var>A</var>, solicited node address <var>S</var>,
-          for each router port <var>P</var> with
+          IPv6: For a configured NAT (both DNAT and SNAT) IP address or a
+          load balancer IPv6 VIP <var>A</var>, solicited node address
+          <var>S</var>, for each router port <var>P</var> with
           Ethernet address <var>E</var>, a priority-90 flow matches
           <code>inport == <var>P</var> &amp;&amp; nd_ns &amp;&amp;
           ip6.dst == {<var>A</var>, <var>S</var>} &amp;&amp;
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 99eb582c3..3b0272b00 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -8648,6 +8648,7 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
          * per logical port but DNAT addresses can be handled per datapath
          * for non gateway router ports.
          */
+        struct sset snat_ips = SSET_INITIALIZER(&snat_ips);
         for (int i = 0; i < od->nbr->n_nat; i++) {
             struct ovn_nat *nat_entry = &od->nat_entries[i];
             const struct nbrec_nat *nat = nat_entry->nb;
@@ -8657,15 +8658,22 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                 continue;
             }
 
+            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
+            char *ext_addr = nat_entry_is_v6(nat_entry) ?
+                             ext_addrs->ipv6_addrs[0].addr_s :
+                             ext_addrs->ipv4_addrs[0].addr_s;
+
             if (!strcmp(nat->type, "snat")) {
-                continue;
+                if (sset_contains(&snat_ips, ext_addr)) {
+                    continue;
+                }
+                sset_add(&snat_ips, ext_addr);
             }
 
             /* Priority 91 and 92 flows are added for each gateway router
              * port to handle the special cases. In case we get the packet
              * on a regular port, just reply with the port's ETH address.
              */
-            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
             if (nat_entry_is_v6(nat_entry)) {
                 build_lrouter_nd_flow(od, NULL, "nd_na",
                                       ext_addrs->ipv6_addrs[0].addr_s,
@@ -8679,6 +8687,8 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                                        &nat->header_, lflows);
             }
         }
+        sset_destroy(&snat_ips);
+
 
         /* Drop ARP packets (priority 85). ARP request packets for router's own
          * IPs are handled with priority-90 flows.
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 7872d5051..8344c7f5e 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1572,6 +1572,8 @@  ovn-nbctl set logical_router lr options:chassis=ch
 ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.2 42.42.42.2
 ovn-nbctl lr-nat-add lr dnat 43.43.43.3 42.42.42.3
 ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.4 42.42.42.4 ls-vm 00:00:00:00:00:02
+ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.50
+ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.51
 
 ovn-nbctl --wait=sb sync
 
@@ -1594,6 +1596,9 @@  action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
 # Ingress router port ETH address is used for ARP reply/NA in lr_in_ip_input.
 AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
   table=3 (lr_in_ip_input     ), priority=90   , dnl
+match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
+action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
+  table=3 (lr_in_ip_input     ), priority=90   , dnl
 match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
 action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
   table=3 (lr_in_ip_input     ), priority=90   , dnl
@@ -1648,6 +1653,9 @@  action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
 # Priority 90 flows (per router).
 AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
   table=3 (lr_in_ip_input     ), priority=90   , dnl
+match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
+action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
+  table=3 (lr_in_ip_input     ), priority=90   , dnl
 match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
 action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa; arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
   table=3 (lr_in_ip_input     ), priority=90   , dnl