[ovs-dev,v2] northd: Add support for NAT with multiple DGP

Currently, if multiple distributed gateway ports (DGP) are configured
on a logical router, NAT is disabled as part of commit 15348b7b
(northd: Multiple distributed gateway port support.)

This patch updates the behavior by selectively applying NAT rules at DGPs.
A NAT rule is applied on matching packets entering or leaving a specific
DGP only if the external_ip of the rule belongs to the same subnet as the
DGP.

This patch also updates ovn-nbctl to accept multiple NAT rules of type
`snat` with the same logical_ip but different external_ip for a logical
router.

Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
---
 NEWS                      |   1 +
 northd/northd.c           | 210 +++++++++++++++++++++++++-------------
 northd/ovn-northd.8.xml   |  27 +++--
 ovn-architecture.7.xml    |   6 +-
 ovn-nb.xml                |   4 +-
 tests/ovn-nbctl.at        |  40 +++++++-
 tests/ovn-northd.at       | 200 +++++++++++++++++++++++++++++++++---
 utilities/ovn-nbctl.8.xml |  18 +++-
 utilities/ovn-nbctl.c     | 157 ++++++++++++++++++++++++++--
 9 files changed, 547 insertions(+), 116 deletions(-)

References:  <20220105100833.152201-1-sangana.abhiram@nutanix.com>
 

Bleep bloop.  Greetings Abhiram Sangana, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 84 characters long (recommended limit is 79)
#180 FILE: northd/northd.c:3488:
                                                    : op->peer->od->l3dgw_ports[0]);

WARNING: Line is 141 characters long (recommended limit is 79)
#1008 FILE: utilities/ovn-nbctl.8.xml:1205:
      <dt>[<code>--if-exists</code>] <code>lr-nat-del</code> <var>router</var> [<var>type</var> [<var>ip</var> [<var>logical_ip</var>]]]</dt>

WARNING: Line lacks whitespace around operator
#1045 FILE: utilities/ovn-nbctl.c:384:
  lr-nat-del ROUTER [TYPE [IP [LOGICAL_IP]]]\n\

WARNING: Line is 80 characters long (recommended limit is 79)
#1227 FILE: utilities/ovn-nbctl.c:4974:
                 * need to iterate over all the rules and delete all nat entries

Lines checked: 1268, Warnings: 4, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

Hi,

I haven't done a full review of this patch, but I have noticed a problem 
pretty early on when I started looking.

The new ip_in_lrp_networks() function that is added here is intended to 
determine which l3dgw port to use for a particular NAT external address. 
The problem is that there are configurations, especially from OpenStack, 
where this will cause problems.

In current OpenStack configurations, they might set up a router with a 
gateway port that serves the 10.0.0.0/8 network. But they might set up a 
DNAT with external address 192.168.0.1 on that router. They expect that 
traffic sent to 192.168.0.1 will be NATted correctly on that router, 
even though the router port network does not include that IP address.

If you run `make check-system-userspace` with your series, you'll find 
that the "Floating IP outside router subnet IPv4" test fails.

How could this be fixed?

One idea is to configure the external port explicitly either on the NAT 
itself or on the router. This way, even if the IP address is outside the 
subnet of the DGP, you can still know which DGP is the "correct" one.

Another idea would be to differentiate behavior between SNAT and DNAT. 
For DNAT, you could treat all DGPs as equals, so it doesn't matter on 
which DGP the traffic is received, it will get translated properly. For 
SNAT or DNAT-and-SNAT, you'd probably need to explicitly specify which 
DGP to use, though.

On 1/5/22 05:08, Abhiram Sangana wrote:
> Currently, if multiple distributed gateway ports (DGP) are configured
> on a logical router, NAT is disabled as part of commit 15348b7b
> (northd: Multiple distributed gateway port support.)
> 
> This patch updates the behavior by selectively applying NAT rules at DGPs.
> A NAT rule is applied on matching packets entering or leaving a specific
> DGP only if the external_ip of the rule belongs to the same subnet as the
> DGP.
> 
> This patch also updates ovn-nbctl to accept multiple NAT rules of type
> `snat` with the same logical_ip but different external_ip for a logical
> router.
> 
> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com>
> ---
>   NEWS                      |   1 +
>   northd/northd.c           | 210 +++++++++++++++++++++++++-------------
>   northd/ovn-northd.8.xml   |  27 +++--
>   ovn-architecture.7.xml    |   6 +-
>   ovn-nb.xml                |   4 +-
>   tests/ovn-nbctl.at        |  40 +++++++-
>   tests/ovn-northd.at       | 200 +++++++++++++++++++++++++++++++++---
>   utilities/ovn-nbctl.8.xml |  18 +++-
>   utilities/ovn-nbctl.c     | 157 ++++++++++++++++++++++++++--
>   9 files changed, 547 insertions(+), 116 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 53f9718b1..a27f89150 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,6 @@
>   Post v21.12.0
>   -------------
> +  - Support NAT with multiple distributed gateway ports on a logical router.
>   
>   OVN v21.12.0 - xx xxx xxxx
>   --------------------------
> diff --git a/northd/northd.c b/northd/northd.c
> index c714227b2..7f766158d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -604,11 +604,11 @@ struct ovn_datapath {
>   
>       /* Applies to only logical router datapath.
>        * True if logical router is a gateway router. i.e options:chassis is set.
> -     * If this is true, then 'l3dgw_port' will be ignored. */
> +     * If this is true, then 'l3dgw_ports' will be ignored. */
>       bool is_gw_router;
>   
> -    /* OVN northd only needs to know about the logical router gateway port for
> -     * NAT on a distributed router.  The "distributed gateway ports" are
> +    /* OVN northd only needs to know about logical router gateway ports for
> +     * NAT/LB on a distributed router.  The "distributed gateway ports" are
>        * populated only when there is a gateway chassis or ha chassis group
>        * specified for some of the ports on the logical router. Otherwise this
>        * will be NULL. */
> @@ -761,16 +761,6 @@ init_nat_entries(struct ovn_datapath *od)
>           return;
>       }
>   
> -    if (od->n_l3dgw_ports > 1) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -        VLOG_WARN_RL(&rl, "NAT is configured on logical router %s, which has %"
> -                     PRIuSIZE" distributed gateway ports. NAT is not supported"
> -                     " yet when there is more than one distributed gateway "
> -                     "port on the router.",
> -                     od->nbr->name, od->n_l3dgw_ports);
> -        return;
> -    }
> -
>       od->nat_entries = xmalloc(od->nbr->n_nat * sizeof *od->nat_entries);
>   
>       for (size_t i = 0; i < od->nbr->n_nat; i++) {
> @@ -1631,6 +1621,49 @@ is_cr_port(const struct ovn_port *op)
>       return op->l3dgw_port;
>   }
>   
> +/* Checks if the IP address (assumed valid) represented by string 'address' is
> + * in one of the networks of the logical router port 'op'. */
> +static bool
> +ip_in_lrp_networks(const struct ovn_port *op, const char *address) {
> +    ovs_be32 ip, mask;
> +    struct in6_addr ipv6, mask_v6;
> +
> +    char *error = ip_parse_masked(address, &ip, &mask);
> +    bool is_v6 = false;
> +
> +    if (error || mask != OVS_BE32_MAX) {
> +        free(error);
> +        ipv6_parse_masked(address, &ipv6, &mask_v6);
> +        is_v6 = true;
> +    }
> +
> +    struct lport_addresses lrp_networks;
> +    extract_lrp_networks(op->nbrp, &lrp_networks);
> +
> +    bool ip_in_net = false;
> +    if (is_v6) {
> +        for (int i = 0; i < lrp_networks.n_ipv6_addrs; i++) {
> +            struct ipv6_netaddr *lrp6_addr = &(lrp_networks.ipv6_addrs[i]);
> +            struct in6_addr ip6_mask = ipv6_addr_bitand(&lrp6_addr->mask,
> +                                                        &ipv6);
> +
> +            if (ipv6_addr_equals(&ip6_mask, &(lrp6_addr->network))) {
> +                ip_in_net = true;
> +            }
> +        }
> +    } else {
> +        for (int i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
> +            struct ipv4_netaddr *lrp4_addr = &(lrp_networks.ipv4_addrs[i]);
> +
> +            if ((ip & lrp4_addr->mask) == lrp4_addr->network) {
> +                ip_in_net = true;
> +            }
> +        }
> +    }
> +    destroy_lport_addresses(&lrp_networks);
> +    return ip_in_net;
> +}
> +
>   static void
>   destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
>   {
> @@ -2705,8 +2738,9 @@ join_logical_ports(struct northd_input *input_data,
>    * port, followed by 'is_chassis_resident("LPORT_NAME")', where the
>    * LPORT_NAME is the name of the L3 redirect port or the name of the
>    * logical_port specified in a NAT rule.  These strings include the
> - * external IP addresses of all NAT rules defined on that router, and all
> - * of the IP addresses used in load balancer VIPs defined on that router.
> + * external IP addresses of NAT rules defined on that router which are in the
> + * same network as the router port 'op', and all of the IP addresses used in
> + * load balancer VIPs defined on that router.
>    *
>    * The caller must free each of the n returned strings with free(),
>    * and must free the returned array when it is no longer needed. */
> @@ -2717,8 +2751,7 @@ get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
>       struct eth_addr mac;
>       if (!op || !op->nbrp || !op->od || !op->od->nbr
>           || (!op->od->nbr->n_nat && !op->od->has_lb_vip)
> -        || !eth_addr_from_string(op->nbrp->mac, &mac)
> -        || op->od->n_l3dgw_ports > 1) {
> +        || !eth_addr_from_string(op->nbrp->mac, &mac)) {
>           *n = n_nats;
>           return NULL;
>       }
> @@ -2747,6 +2780,10 @@ get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
>               continue;
>           }
>   
> +        if (!ip_in_lrp_networks(op, nat->external_ip)) {
> +            continue;
> +        }
> +
>           /* Determine whether this NAT rule satisfies the conditions for
>            * distributed NAT processing. */
>           if (op->od->n_l3dgw_ports && !strcmp(nat->type, "dnat_and_snat")
> @@ -2815,9 +2852,9 @@ get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
>       if (central_ip_address) {
>           /* Gratuitous ARP for centralized NAT rules on distributed gateway
>            * ports should be restricted to the gateway chassis. */
> -        if (op->od->n_l3dgw_ports) {
> +        if (is_l3dgw_port(op)) {
>               ds_put_format(&c_addresses, " is_chassis_resident(%s)",
> -                          op->od->l3dgw_ports[0]->cr_port->json_key);
> +                          op->cr_port->json_key);
>           }
>   
>           addresses[n_nats++] = ds_steal_cstr(&c_addresses);
> @@ -3446,9 +3483,11 @@ ovn_port_update_sbrec(struct northd_input *input_data,
>                   }
>   
>                   if (op->peer->od->n_l3dgw_ports) {
> +                        const struct ovn_port *l3dgw_port = (
> +                            is_l3dgw_port(op->peer) ? op->peer
> +                                                    : op->peer->od->l3dgw_ports[0]);
>                       ds_put_format(&garp_info, " is_chassis_resident(%s)",
> -                                  op->peer->od->l3dgw_ports[0]
> -                                  ->cr_port->json_key);
> +                                  l3dgw_port->cr_port->json_key);
>                   }
>   
>                   n_nats++;
> @@ -10212,6 +10251,12 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op,
>       const struct nbrec_nat *nat = nat_entry->nb;
>       struct ds match = DS_EMPTY_INITIALIZER;
>   
> +    /* ARP/ND should be sent from router port that is in the same subnet as
> +     * the NAT external IP. */
> +    if (!ip_in_lrp_networks(op, nat->external_ip)) {
> +        return;
> +    }
> +
>       /* Mac address to use when replying to ARP/NS. */
>       const char *mac_s = REG_INPORT_ETH_ADDR;
>       struct eth_addr mac;
> @@ -10235,10 +10280,9 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op,
>            * upstream MAC learning points to the gateway chassis.
>            * Also need to avoid generation of multiple ARP responses
>            * from different chassis. */
> -        if (op->od->n_l3dgw_ports) {
> -            ds_put_format(&match, "is_chassis_resident(%s)",
> -                          op->od->l3dgw_ports[0]->cr_port->json_key);
> -        }
> +        ovs_assert(is_l3dgw_port(op));
> +        ds_put_format(&match, "is_chassis_resident(%s)",
> +                      op->cr_port->json_key);
>       }
>   
>       /* Respond to ARP/NS requests on the chassis that binds the gw
> @@ -11925,7 +11969,7 @@ build_ipv6_input_flows_for_lrouter_port(
>           struct ds *match, struct ds *actions,
>           const struct shash *meter_groups)
>   {
> -    if (op->nbrp && (!op->l3dgw_port)) {
> +    if (op->nbrp && !is_cr_port(op)) {
>           /* No ingress packets are accepted on a chassisredirect
>            * port, so no need to program flows for that port. */
>           if (op->lrp_networks.n_ipv6_addrs) {
> @@ -12054,7 +12098,7 @@ build_ipv6_input_flows_for_lrouter_port(
>               ds_clear(match);
>               ds_clear(actions);
>               ds_clear(&ip_ds);
> -            if (op->od->n_l3dgw_ports && op->od->l3dgw_ports[0] == op) {
> +            if (is_l3dgw_port(op)) {
>                   ds_put_cstr(&ip_ds, "ip6.dst <-> ip6.src");
>               } else {
>                   ds_put_format(&ip_ds, "ip6.dst = ip6.src; ip6.src = %s",
> @@ -12144,7 +12188,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
>   {
>       /* No ingress packets are accepted on a chassisredirect
>        * port, so no need to program flows for that port. */
> -    if (op->nbrp && (!op->l3dgw_port)) {
> +    if (op->nbrp && !is_cr_port(op)) {
>           if (op->lrp_networks.n_ipv4_addrs) {
>               /* L3 admission control: drop packets that originate from an
>                * IPv4 address owned by the router or a broadcast address
> @@ -12186,7 +12230,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
>               ds_clear(match);
>               ds_clear(actions);
>               ds_clear(&ip_ds);
> -            if (op->od->n_l3dgw_ports && op->od->l3dgw_ports[0] == op) {
> +            if (is_l3dgw_port(op)) {
>                   ds_put_cstr(&ip_ds, "ip4.dst <-> ip4.src");
>               } else {
>                   ds_put_format(&ip_ds, "ip4.dst = ip4.src; ip4.src = %s",
> @@ -12431,7 +12475,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
>   static void
>   build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>                                const struct nbrec_nat *nat, struct ds *match,
> -                             struct ds *actions, bool distributed, bool is_v6)
> +                             struct ds *actions, bool distributed, bool is_v6,
> +                             struct ovn_port *l3dgw_port)
>   {
>       /* Ingress UNSNAT table: It is for already established connections'
>       * reverse traffic. i.e., SNAT has already been done in egress
> @@ -12470,12 +12515,12 @@ build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>           ds_clear(actions);
>           ds_put_format(match, "ip && ip%s.dst == %s && inport == %s && "
>                         "flags.loopback == 0", is_v6 ? "6" : "4",
> -                      nat->external_ip, od->l3dgw_ports[0]->json_key);
> +                      nat->external_ip, l3dgw_port->json_key);
>           if (!distributed && od->n_l3dgw_ports) {
>               /* Flows for NAT rules that are centralized are only
>               * programmed on the gateway chassis. */
>               ds_put_format(match, " && is_chassis_resident(%s)",
> -                          od->l3dgw_ports[0]->cr_port->json_key);
> +                          l3dgw_port->cr_port->json_key);
>           }
>   
>           if (!strcmp(nat->type, "dnat_and_snat") && stateless) {
> @@ -12495,12 +12540,12 @@ build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>               ds_put_format(match, "ip && ip%s.dst == %s && inport == %s && "
>                             "flags.loopback == 1 && flags.use_snat_zone == 1",
>                             is_v6 ? "6" : "4", nat->external_ip,
> -                          od->l3dgw_ports[0]->json_key);
> +                          l3dgw_port->json_key);
>               if (!distributed && od->n_l3dgw_ports) {
>                   /* Flows for NAT rules that are centralized are only
>                   * programmed on the gateway chassis. */
>                   ds_put_format(match, " && is_chassis_resident(%s)",
> -                            od->l3dgw_ports[0]->cr_port->json_key);
> +                            l3dgw_port->cr_port->json_key);
>               }
>               ds_put_cstr(actions, "ct_snat;");
>               ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT,
> @@ -12514,7 +12559,8 @@ static void
>   build_lrouter_in_dnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>                              const struct nbrec_nat *nat, struct ds *match,
>                              struct ds *actions, bool distributed,
> -                           ovs_be32 mask, bool is_v6)
> +                           ovs_be32 mask, bool is_v6,
> +                           struct ovn_port *l3dgw_port)
>   {
>       /* Ingress DNAT table: Packets enter the pipeline with destination
>       * IP address that needs to be DNATted from a external IP address
> @@ -12566,12 +12612,12 @@ build_lrouter_in_dnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>               ds_clear(match);
>               ds_put_format(match, "ip && ip%s.dst == %s && inport == %s",
>                             is_v6 ? "6" : "4", nat->external_ip,
> -                          od->l3dgw_ports[0]->json_key);
> +                          l3dgw_port->json_key);
>               if (!distributed && od->n_l3dgw_ports) {
>                   /* Flows for NAT rules that are centralized are only
>                   * programmed on the gateway chassis. */
>                   ds_put_format(match, " && is_chassis_resident(%s)",
> -                              od->l3dgw_ports[0]->cr_port->json_key);
> +                              l3dgw_port->cr_port->json_key);
>               }
>               ds_clear(actions);
>               if (nat->allowed_ext_ips || nat->exempted_ext_ips) {
> @@ -12601,7 +12647,8 @@ static void
>   build_lrouter_out_undnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>                                 const struct nbrec_nat *nat, struct ds *match,
>                                 struct ds *actions, bool distributed,
> -                              struct eth_addr mac, bool is_v6)
> +                              struct eth_addr mac, bool is_v6,
> +                              struct ovn_port *l3dgw_port)
>   {
>       /* Egress UNDNAT table: It is for already established connections'
>       * reverse traffic. i.e., DNAT has already been done in ingress
> @@ -12618,12 +12665,12 @@ build_lrouter_out_undnat_flow(struct hmap *lflows, struct ovn_datapath *od,
>       ds_clear(match);
>       ds_put_format(match, "ip && ip%s.src == %s && outport == %s",
>                     is_v6 ? "6" : "4", nat->logical_ip,
> -                  od->l3dgw_ports[0]->json_key);
> +                  l3dgw_port->json_key);
>       if (!distributed && od->n_l3dgw_ports) {
>           /* Flows for NAT rules that are centralized are only
>           * programmed on the gateway chassis. */
>           ds_put_format(match, " && is_chassis_resident(%s)",
> -                      od->l3dgw_ports[0]->cr_port->json_key);
> +                      l3dgw_port->cr_port->json_key);
>       }
>       ds_clear(actions);
>       if (distributed) {
> @@ -12649,7 +12696,7 @@ static void
>   build_lrouter_out_is_dnat_local(struct hmap *lflows, struct ovn_datapath *od,
>                                   const struct nbrec_nat *nat, struct ds *match,
>                                   struct ds *actions, bool distributed,
> -                                bool is_v6)
> +                                bool is_v6, struct ovn_port *l3dgw_port)
>   {
>       /* Note that this only applies for NAT on a distributed router.
>        */
> @@ -12664,7 +12711,7 @@ build_lrouter_out_is_dnat_local(struct hmap *lflows, struct ovn_datapath *od,
>           ds_put_format(match, "is_chassis_resident(\"%s\")", nat->logical_port);
>       } else {
>           ds_put_format(match, "is_chassis_resident(%s)",
> -                      od->l3dgw_ports[0]->cr_port->json_key);
> +                      l3dgw_port->cr_port->json_key);
>       }
>   
>       ds_clear(actions);
> @@ -12680,7 +12727,8 @@ build_lrouter_out_snat_flow(struct hmap *lflows, struct ovn_datapath *od,
>                               const struct nbrec_nat *nat, struct ds *match,
>                               struct ds *actions, bool distributed,
>                               struct eth_addr mac, ovs_be32 mask,
> -                            int cidr_bits, bool is_v6)
> +                            int cidr_bits, bool is_v6,
> +                            struct ovn_port *l3dgw_port)
>   {
>       /* Egress SNAT table: Packets enter the egress pipeline with
>       * source ip address that needs to be SNATted to a external ip
> @@ -12727,7 +12775,7 @@ build_lrouter_out_snat_flow(struct hmap *lflows, struct ovn_datapath *od,
>           ds_clear(match);
>           ds_put_format(match, "ip && ip%s.src == %s && outport == %s",
>                         is_v6 ? "6" : "4", nat->logical_ip,
> -                      od->l3dgw_ports[0]->json_key);
> +                      l3dgw_port->json_key);
>           if (od->n_l3dgw_ports) {
>               if (distributed) {
>                   ovs_assert(nat->logical_port);
> @@ -12739,7 +12787,7 @@ build_lrouter_out_snat_flow(struct hmap *lflows, struct ovn_datapath *od,
>                   * programmed on the gateway chassis. */
>                   priority += 128;
>                   ds_put_format(match, " && is_chassis_resident(%s)",
> -                              od->l3dgw_ports[0]->cr_port->json_key);
> +                              l3dgw_port->cr_port->json_key);
>               }
>           }
>           ds_clear(actions);
> @@ -12798,13 +12846,13 @@ build_lrouter_ingress_nat_check_pkt_len(struct hmap *lflows,
>                                           const struct nbrec_nat *nat,
>                                           struct ovn_datapath *od, bool is_v6,
>                                           struct ds *match, struct ds *actions,
> -                                        int mtu,
> +                                        int mtu, struct ovn_port *l3dgw_port,
>                                           const struct shash *meter_groups)
>   {
>           ds_clear(match);
>           ds_put_format(match, "inport == %s && "REGBIT_PKT_LARGER
>                         " && "REGBIT_EGRESS_LOOPBACK" == 0",
> -                      od->l3dgw_ports[0]->json_key);
> +                      l3dgw_port->json_key);
>   
>           ds_clear(actions);
>           if (!is_v6) {
> @@ -12825,7 +12873,7 @@ build_lrouter_ingress_nat_check_pkt_len(struct hmap *lflows,
>                   "outport = %s; flags.loopback = 1; output; };",
>                   nat->external_mac,
>                   nat->external_ip,
> -                mtu, od->l3dgw_ports[0]->json_key);
> +                mtu, l3dgw_port->json_key);
>               ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_IP_INPUT, 160,
>                                         ds_cstr(match), ds_cstr(actions),
>                                         NULL,
> @@ -12852,7 +12900,7 @@ build_lrouter_ingress_nat_check_pkt_len(struct hmap *lflows,
>                   "outport = %s; flags.loopback = 1; output; };",
>                   nat->external_mac,
>                   nat->external_ip,
> -                mtu, od->l3dgw_ports[0]->json_key);
> +                mtu, l3dgw_port->json_key);
>               ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_IP_INPUT, 160,
>                                         ds_cstr(match), ds_cstr(actions),
>                                         NULL,
> @@ -12869,13 +12917,14 @@ build_lrouter_ingress_flow(struct hmap *lflows, struct ovn_datapath *od,
>                              const struct nbrec_nat *nat, struct ds *match,
>                              struct ds *actions, struct eth_addr mac,
>                              bool distributed, bool is_v6,
> +                           struct ovn_port *l3dgw_port,
>                              const struct shash *meter_groups)
>   {
>       if (od->n_l3dgw_ports && !strcmp(nat->type, "snat")) {
>           ds_clear(match);
>           ds_put_format(
>               match, "inport == %s && %s == %s",
> -            od->l3dgw_ports[0]->json_key,
> +            l3dgw_port->json_key,
>               is_v6 ? "ip6.src" : "ip4.src", nat->external_ip);
>           ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_INPUT,
>                                   120, ds_cstr(match), "next;",
> @@ -12891,24 +12940,24 @@ build_lrouter_ingress_flow(struct hmap *lflows, struct ovn_datapath *od,
>           * This will save us from having to match on inport further
>           * down in the pipeline.
>           */
> -        int gw_mtu = smap_get_int(&od->l3dgw_ports[0]->nbrp->options,
> +        int gw_mtu = smap_get_int(&l3dgw_port->nbrp->options,
>                                     "gateway_mtu", 0);
>           ds_clear(match);
>           ds_put_format(match,
>                         "eth.dst == "ETH_ADDR_FMT" && inport == %s"
>                         " && is_chassis_resident(\"%s\")",
>                         ETH_ADDR_ARGS(mac),
> -                      od->l3dgw_ports[0]->json_key,
> +                      l3dgw_port->json_key,
>                         nat->logical_port);
> -        build_gateway_mtu_flow(lflows, od->l3dgw_ports[0],
> +        build_gateway_mtu_flow(lflows, l3dgw_port,
>                                  S_ROUTER_IN_ADMISSION, 50, 55,
>                                  match, actions, &nat->header_,
>                                  REG_INPORT_ETH_ADDR " = %s; next;",
> -                               od->l3dgw_ports[0]->lrp_networks.ea_s);
> +                               l3dgw_port->lrp_networks.ea_s);
>           if (gw_mtu) {
>               build_lrouter_ingress_nat_check_pkt_len(lflows, nat, od, is_v6,
>                                                       match, actions, gw_mtu,
> -                                                    meter_groups);
> +                                                    l3dgw_port, meter_groups);
>           }
>       }
>   }
> @@ -12916,7 +12965,8 @@ build_lrouter_ingress_flow(struct hmap *lflows, struct ovn_datapath *od,
>   static int
>   lrouter_check_nat_entry(struct ovn_datapath *od, const struct nbrec_nat *nat,
>                           ovs_be32 *mask, bool *is_v6, int *cidr_bits,
> -                        struct eth_addr *mac, bool *distributed)
> +                        struct eth_addr *mac, bool *distributed,
> +                        struct ovn_port **nat_l3dgw_port)
>   {
>       struct in6_addr ipv6, mask_v6, v6_exact = IN6ADDR_EXACT_INIT;
>       ovs_be32 ip;
> @@ -12950,6 +13000,24 @@ lrouter_check_nat_entry(struct ovn_datapath *od, const struct nbrec_nat *nat,
>           *is_v6 = true;
>       }
>   
> +    /* Get the l3dgw port (if present) corresponding to the external IP
> +     * of the NAT rule. */
> +    *nat_l3dgw_port = NULL;
> +
> +    for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
> +        if (ip_in_lrp_networks(od->l3dgw_ports[i], nat->external_ip)) {
> +            *nat_l3dgw_port = od->l3dgw_ports[i];
> +        }
> +    }
> +
> +    if (od->n_l3dgw_ports && *nat_l3dgw_port == NULL) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL(&rl, "Could not map NAT external ip: %s to a "
> +                     "distributed gateway port in router "UUID_FMT"",
> +                     nat->external_ip, UUID_ARGS(&od->key));
> +        return -EINVAL;
> +    }
> +
>       /* Check the validity of nat->logical_ip. 'logical_ip' can
>       * be a subnet when the type is "snat". */
>       if (*is_v6) {
> @@ -13049,7 +13117,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>       ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120, "nd_ns", "next;");
>   
>       /* NAT rules are only valid on Gateway routers and routers with
> -     * l3dgw_port (router has a port with gateway chassis
> +     * l3dgw_ports (router has port(s) with gateway chassis
>        * specified). */
>       if (!od->is_gw_router && !od->n_l3dgw_ports) {
>           return;
> @@ -13068,18 +13136,19 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>           bool is_v6, distributed;
>           ovs_be32 mask;
>           int cidr_bits;
> +        struct ovn_port *l3dgw_port;
>   
>           if (lrouter_check_nat_entry(od, nat, &mask, &is_v6, &cidr_bits,
> -                                    &mac, &distributed) < 0) {
> +                                    &mac, &distributed, &l3dgw_port) < 0) {
>               continue;
>           }
>   
>           /* S_ROUTER_IN_UNSNAT */
>           build_lrouter_in_unsnat_flow(lflows, od, nat, match, actions, distributed,
> -                                     is_v6);
> +                                     is_v6, l3dgw_port);
>           /* S_ROUTER_IN_DNAT */
>           build_lrouter_in_dnat_flow(lflows, od, nat, match, actions, distributed,
> -                                   mask, is_v6);
> +                                   mask, is_v6, l3dgw_port);
>   
>           /* ARP resolve for NAT IPs. */
>           if (od->is_gw_router) {
> @@ -13092,14 +13161,14 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>                   ds_clear(match);
>                   ds_put_format(
>                       match, "outport == %s && %s == %s",
> -                    od->l3dgw_ports[0]->json_key,
> +                    l3dgw_port->json_key,
>                       is_v6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4,
>                       nat->external_ip);
>                   ds_clear(actions);
>                   ds_put_format(
>                       actions, "eth.dst = %s; next;",
>                       distributed ? nat->external_mac :
> -                    od->l3dgw_ports[0]->lrp_networks.ea_s);
> +                    l3dgw_port->lrp_networks.ea_s);
>                   ovn_lflow_add_with_hint(lflows, od,
>                                           S_ROUTER_IN_ARP_RESOLVE,
>                                           100, ds_cstr(match),
> @@ -13111,18 +13180,19 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>   
>           /* S_ROUTER_OUT_DNAT_LOCAL */
>           build_lrouter_out_is_dnat_local(lflows, od, nat, match, actions,
> -                                        distributed, is_v6);
> +                                        distributed, is_v6, l3dgw_port);
>   
>           /* S_ROUTER_OUT_UNDNAT */
>           build_lrouter_out_undnat_flow(lflows, od, nat, match, actions, distributed,
> -                                      mac, is_v6);
> +                                      mac, is_v6, l3dgw_port);
>           /* S_ROUTER_OUT_SNAT */
>           build_lrouter_out_snat_flow(lflows, od, nat, match, actions, distributed,
> -                                    mac, mask, cidr_bits, is_v6);
> +                                    mac, mask, cidr_bits, is_v6, l3dgw_port);
>   
>           /* S_ROUTER_IN_ADMISSION - S_ROUTER_IN_IP_INPUT */
> -        build_lrouter_ingress_flow(lflows, od, nat, match, actions,
> -                                   mac, distributed, is_v6, meter_groups);
> +        build_lrouter_ingress_flow(lflows, od, nat, match, actions, mac,
> +                                   distributed, is_v6, l3dgw_port,
> +                                   meter_groups);
>   
>           /* Ingress Gateway Redirect Table: For NAT on a distributed
>            * router, add flows that are specific to a NAT rule.  These
> @@ -13139,7 +13209,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>               ds_put_format(match,
>                             "ip%s.src == %s && outport == %s",
>                             is_v6 ? "6" : "4", nat->logical_ip,
> -                          od->l3dgw_ports[0]->json_key);
> +                          l3dgw_port->json_key);
>               /* Add a rule to drop traffic from a distributed NAT if
>                * the virtual port has not claimed yet becaused otherwise
>                * the traffic will be centralized misconfiguring the TOR switch.
> @@ -13172,10 +13242,10 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>               ds_put_format(match, "ip%s.dst == %s && outport == %s",
>                             is_v6 ? "6" : "4",
>                             nat->external_ip,
> -                          od->l3dgw_ports[0]->json_key);
> +                          l3dgw_port->json_key);
>               if (!distributed) {
>                   ds_put_format(match, " && is_chassis_resident(%s)",
> -                              od->l3dgw_ports[0]->cr_port->json_key);
> +                              l3dgw_port->cr_port->json_key);
>               } else {
>                   ds_put_format(match, " && is_chassis_resident(\"%s\")",
>                                 nat->logical_port);
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 79f35bc16..42894a413 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2888,7 +2888,8 @@ icmp6 {
>                 <code>ip &amp;&amp;
>                 ip6.dst == <var>B</var> &amp;&amp; inport == <var>GW</var>
>                 &amp;&amp; flags.loopback == 0</code>
> -              where <var>GW</var> is the logical router gateway port, with an
> +              where <var>GW</var> is the logical router gateway port
> +              corresponding to IP <var>B</var>, with an
>                 action <code>ct_snat_in_czone;</code> to unSNAT in the common
>                 zone.  If the NAT rule is of type dnat_and_snat and has
>                 <code>stateless=true</code> in the options, then the action
> @@ -2913,7 +2914,8 @@ icmp6 {
>                 ip6.dst == <var>B</var> &amp;&amp; inport == <var>GW</var>
>                 &amp;&amp; flags.loopback == 0 &amp;&amp;
>                 flags.use_snat_zone == 1</code>
> -              where <var>GW</var> is the logical router gateway port, with an
> +              where <var>GW</var> is the logical router gateway port
> +              corresponding to IP <var>B</var>, with an
>                 action <code>ct_snat;</code> to unSNAT in the snat zone. If the
>                 NAT rule is of type dnat_and_snat and has
>                 <code>stateless=true</code> in the options, then the action
> @@ -3194,9 +3196,10 @@ icmp6 {
>             to change the destination IP address of a packet from <var>A</var> to
>             <var>B</var>, a priority-100 flow matches <code>ip &amp;&amp;
>             ip4.dst == <var>B</var> &amp;&amp; inport == <var>GW</var></code>,
> -          where <var>GW</var> is the logical router gateway port, with an
> -          action <code>ct_dnat(<var>B</var>);</code>.  The match will
> -          include <code>ip6.dst == <var>B</var></code> in the IPv6 case.
> +          where <var>GW</var> is the logical router gateway port corresponding
> +          to IP <var>A</var>, with an action
> +          <code>ct_dnat(<var>B</var>);</code>.  The match will include
> +          <code>ip6.dst == <var>B</var></code> in the IPv6 case.
>             If the NAT rule is of type dnat_and_snat and has
>             <code>stateless=true</code> in the options, then the action
>             would be <code>ip4/6.dst=(<var>B</var>)</code>.
> @@ -4006,10 +4009,11 @@ icmp6 {
>           flow with match <code>ip4.src == <var>B</var> &amp;&amp;
>           outport == <var>GW</var></code> &amp;&amp;
>           is_chassis_resident(<var>P</var>), where <var>GW</var> is
> -        the logical router distributed gateway port and <var>P</var>
> -        is the NAT logical port. IP traffic matching the above rule
> -        will be managed locally setting <code>reg1</code> to <var>C</var>
> -        and <code>eth.src</code> to <var>D</var>, where <var>C</var> is NAT
> +        the logical router distributed gateway port corresponding to the
> +        NAT external IP and <var>P</var> is the NAT logical port. IP traffic
> +        matching the above rule will be managed locally setting
> +        <code>reg1</code> to <var>C</var> and
> +        <code>eth.src</code> to <var>D</var>, where <var>C</var> is NAT
>           external ip and <var>D</var> is NAT external mac.
>         </li>
>   
> @@ -4476,8 +4480,9 @@ nd_ns {
>             outport == <var>GW</var> &amp;&amp;
>             is_chassis_resident(<var>P</var>)</code>, where <var>E</var> is the
>             external IP address specified in the NAT rule, <var>GW</var>
> -          is the logical router distributed gateway port. For dnat_and_snat
> -          NAT rule, <var>P</var> is the logical port specified in the NAT rule.
> +          is the logical router distributed gateway port corresponding to the
> +          NAT external IP. For dnat_and_snat NAT rule, <var>P</var> is the
> +          logical port specified in the NAT rule.
>             If <ref column="logical_port"
>             table="NAT" db="OVN_Northbound"/> column of
>             <ref table="NAT" db="OVN_Northbound"/> table is NOT set, then
> diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
> index ef8d669a2..a48757761 100644
> --- a/ovn-architecture.7.xml
> +++ b/ovn-architecture.7.xml
> @@ -742,9 +742,9 @@
>   
>     <p>
>       A logical router can have multiple distributed gateway ports, each
> -    connecting different external networks. However, some features, such as NAT
> -    and load balancers, are not supported yet for logical routers with more
> -    than one distributed gateway port configured.
> +    connecting different external networks. Load balancing is not yet
> +    supported for logical routers with more than one distributed gateway
> +    port configured.
>     </p>
>   
>     <h4>Physical VLAN MTU Issues</h4>
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 6a6972856..b0226973c 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2552,8 +2552,8 @@
>         <p>
>           There can be more than one distributed gateway ports configured
>           on each logical router, each connecting to different L2 segments.
> -        However, features such as NAT and load-balancer are not supported
> -        on logical routers with more than one distributed gateway ports.
> +        Load-balancing is not yet supported on logical routers with more
> +        than one distributed gateway ports.
>         </p>
>   
>         <p>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index a43a1ce8f..b914abc82 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -745,7 +745,45 @@ AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 snat 192.168.16 allowed_range], [1]
>   [ovn-nbctl: 192.168.16: Invalid IP address or CIDR
>   ])
>   
> -AT_CHECK([ovn-nbctl lr-nat-del lr0])])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0])
> +
> +AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lrp1 00:00:00:01:02:04 172.64.1.1/24])
> +AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1])
> +AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp1 chassis2])
> +
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.1.10 20.0.0.10])
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 172.64.1.10 20.0.0.10])
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.1.20 20.0.0.10], [1], [],
> +[ovn-nbctl: a NAT with this type (snat) and logical_ip (20.0.0.10) already exists
> +])
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 172.64.1.20 20.0.0.20])
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 172.64.1.20 20.0.0.30], [1], [],
> +[ovn-nbctl: a NAT with this type (dnat) and external_ip (172.64.1.20) already exists
> +])
> +AT_CHECK([ovn-nbctl lr-nat-list lr0], [0], [dnl
> +TYPE             EXTERNAL_IP        EXTERNAL_PORT    LOGICAL_IP            EXTERNAL_MAC         LOGICAL_PORT
> +dnat             172.64.1.20                         20.0.0.20
> +snat             172.64.1.10                         20.0.0.10
> +snat             192.168.1.10                        20.0.0.10
> +])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0 snat 172.64.1.10 20.0.0.10])
> +AT_CHECK([ovn-nbctl lr-nat-list lr0], [0], [dnl
> +TYPE             EXTERNAL_IP        EXTERNAL_PORT    LOGICAL_IP            EXTERNAL_MAC         LOGICAL_PORT
> +dnat             172.64.1.20                         20.0.0.20
> +snat             192.168.1.10                        20.0.0.10
> +])
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 172.64.1.10 20.0.0.10])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0 snat 20.0.0.10])
> +AT_CHECK([ovn-nbctl lr-nat-list lr0], [0], [dnl
> +TYPE             EXTERNAL_IP        EXTERNAL_PORT    LOGICAL_IP            EXTERNAL_MAC         LOGICAL_PORT
> +dnat             172.64.1.20                         20.0.0.20
> +])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0 snat 20.0.0.10], [1], [],
> +[ovn-nbctl: no matching NAT with the type (snat) and logical_ip (20.0.0.10)
> +])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0])
> +])
>   
>   dnl ---------------------------------------------------------------------
>   
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 652903761..80da5e37c 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -847,7 +847,7 @@ ovn_start
>   ovn-sbctl chassis-add gw1 geneve 127.0.0.1
>   
>   ovn-nbctl lr-add R1
> -ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24 3000::a/64
>   
>   ovn-nbctl ls-add S1
>   ovn-nbctl lsp-add S1 S1-R1
> @@ -888,13 +888,13 @@ ovn-nbctl lr-nat-del R1 dnat_and_snat  172.16.1.1
>   
>   echo
>   echo "IPv6: stateful"
> -ovn-nbctl --wait=sb lr-nat-add R1 dnat_and_snat fd01::1 fd11::2
> +ovn-nbctl --wait=sb lr-nat-add R1 dnat_and_snat 3000::c 1000::3
>   check_flow_match_sets 3 4 2 0 0 0 0
> -ovn-nbctl lr-nat-del R1 dnat_and_snat  fd01::1
> +ovn-nbctl lr-nat-del R1 dnat_and_snat 3000::c
>   
>   echo
>   echo "IPv6: stateless"
> -ovn-nbctl --wait=sb --stateless lr-nat-add R1 dnat_and_snat fd01::1 fd11::2
> +ovn-nbctl --wait=sb --stateless lr-nat-add R1 dnat_and_snat 3000::c 1000::3
>   check_flow_match_sets 2 0 0 0 0 2 2
>   
>   AT_CLEANUP
> @@ -4234,18 +4234,16 @@ check ovn-nbctl lsp-set-type lrp1-attachment router
>   check ovn-nbctl lsp-set-addresses lrp1-attachment 00:00:00:00:ff:02
>   check ovn-nbctl lsp-set-options lrp1-attachment router-port=lrp1
>   
> -check ovn-nbctl lr-nat-add lr0 dnat 42.42.42.42 192.168.0.2
> +check ovn-nbctl lr-nat-add lr0 dnat 11.0.0.42 192.168.0.2
>   check ovn-nbctl --wait=sb sync
>   
> -ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip4.dst == 42.42.42.42 && ip4.src == 11.0.0.2 && ip.ttl == 64'
> -
> -AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip4.dst == 42.42.42.42 && ip4.src == 11.0.0.2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [0], [ignore])
> +AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip4.dst == 11.0.0.42 && ip4.src == 11.0.0.2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [0], [ignore])
>   
>   dnl If we remove the DNAT entry we will be unable to trace to the DNAT address
> -check ovn-nbctl lr-nat-del lr0 dnat 42.42.42.42
> +check ovn-nbctl lr-nat-del lr0 dnat 11.0.0.42
>   check ovn-nbctl --wait=sb sync
>   
> -AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip4.dst == 42.42.42.42 && ip4.src == 11.0.0.2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [1], [ignore])
> +AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip4.dst == 11.0.0.42 && ip4.src == 11.0.0.2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [1], [ignore])
>   
>   AT_CLEANUP
>   ])
> @@ -4275,16 +4273,16 @@ check ovn-nbctl lsp-set-type lrp1-attachment router
>   check ovn-nbctl lsp-set-addresses lrp1-attachment 00:00:00:00:ff:02
>   check ovn-nbctl lsp-set-options lrp1-attachment router-port=lrp1
>   
> -check ovn-nbctl lr-nat-add lr0 dnat fd42::42 fd68::2
> +check ovn-nbctl lr-nat-add lr0 dnat fd11::42 fd68::2
>   check ovn-nbctl --wait=sb sync
>   
> -AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip6.dst == fd42::42 && ip6.src == fd11::2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [0], [ignore])
> +AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip6.dst == fd11::42 && ip6.src == fd11::2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [0], [ignore])
>   
>   dnl If we remove the DNAT entry we will be unable to trace to the DNAT address
> -check ovn-nbctl lr-nat-del lr0 dnat fd42::42
> +check ovn-nbctl lr-nat-del lr0 dnat fd11::42
>   check ovn-nbctl --wait=sb sync
>   
> -AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip6.dst == fd42::42 && ip6.src == fd11::2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [1], [ignore])
> +AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip6.dst == fd11::42 && ip6.src == fd11::2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [1], [ignore])
>   
>   AT_CLEANUP
>   ])
> @@ -5890,3 +5888,177 @@ AT_CHECK([grep -e "(lr_in_ip_routing   ).*outport" lr0flows | sed 's/table=../ta
>   
>   AT_CLEANUP
>   ])
> +
> +AT_SETUP([ovn-northd -- lr multiple gw ports NAT])
> +AT_KEYWORDS([multiple-l3dgw-ports])
> +ovn_start
> +
> +# Logical network:
> +# 1 Logical Router, 3 bridged Logical Switches,
> +# 1 gateway chassis attached to each corresponding LRP.
> +#
> +#                | S1 (gw1)
> +#                |
> +#      ls  ----  DR -- S3 (gw3)
> +# (20.0.0.0/24)  |
> +#                | S2 (gw2)
> +#
> +# Validate SNAT, DNAT and DNAT_AND_SNAT behavior with multiple
> +# distributed gateway LRPs.
> +
> +check ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> +check ovn-sbctl chassis-add gw2 geneve 128.0.0.1
> +check ovn-sbctl chassis-add gw3 geneve 129.0.0.1
> +
> +check ovn-nbctl lr-add DR
> +check ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24
> +check ovn-nbctl lrp-add DR DR-S2 03:ac:10:01:00:01 10.0.0.1/24
> +check ovn-nbctl lrp-add DR DR-S3 04:ac:10:01:00:01 192.168.0.1/24
> +check ovn-nbctl lrp-add DR DR-ls 05:ac:10:01:00:01 20.0.0.1/24
> +
> +check ovn-nbctl ls-add S1
> +check ovn-nbctl lsp-add S1 S1-DR
> +check ovn-nbctl lsp-set-type S1-DR router
> +check ovn-nbctl lsp-set-addresses S1-DR router
> +check ovn-nbctl --wait=sb lsp-set-options S1-DR router-port=DR-S1
> +
> +check ovn-nbctl ls-add S2
> +check ovn-nbctl lsp-add S2 S2-DR
> +check ovn-nbctl lsp-set-type S2-DR router
> +check ovn-nbctl lsp-set-addresses S2-DR router
> +check ovn-nbctl --wait=sb lsp-set-options S2-DR router-port=DR-S2
> +
> +check ovn-nbctl ls-add S3
> +check ovn-nbctl lsp-add S3 S3-DR
> +check ovn-nbctl lsp-set-type S3-DR router
> +check ovn-nbctl lsp-set-addresses S3-DR router
> +check ovn-nbctl --wait=sb lsp-set-options S3-DR router-port=DR-S3
> +
> +check ovn-nbctl ls-add  ls
> +check ovn-nbctl lsp-add ls ls-DR
> +check ovn-nbctl lsp-set-type ls-DR router
> +check ovn-nbctl lsp-set-addresses ls-DR router
> +check ovn-nbctl --wait=sb lsp-set-options ls-DR router-port=DR-ls
> +
> +check ovn-nbctl lrp-set-gateway-chassis DR-S1 gw1
> +check ovn-nbctl lrp-set-gateway-chassis DR-S2 gw2
> +check ovn-nbctl lrp-set-gateway-chassis DR-S3 gw3
> +
> +check ovn-nbctl --wait=sb sync
> +
> +# Configure SNAT
> +check ovn-nbctl lr-nat-add DR snat  172.16.1.10    20.0.0.10
> +check ovn-nbctl lr-nat-add DR snat  10.0.0.10      20.0.0.10
> +check ovn-nbctl lr-nat-add DR snat  192.168.0.10   20.0.0.10
> +check ovn-nbctl lr-nat-add DR snat  192.168.123.10 20.0.0.10
> +
> +ovn-sbctl dump-flows DR > lrflows
> +AT_CAPTURE_FILE([lrflows])
> +
> +check_lr_in_arp_nat_flows() {
> +    AT_CHECK([grep lr_in_ip_input lrflows | grep arp | grep -e 172.16.1.10 -e 10.0.0.10 -e 192.168.0.10 -e 192.168.123.10 | sed 's/table=../table=??/' | sort], [0], [dnl
> +  table=??(lr_in_ip_input     ), priority=90   , match=(arp.op == 1 && arp.tpa == 10.0.0.10), 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; outport = inport; flags.loopback = 1; output;)
> +  table=??(lr_in_ip_input     ), priority=90   , match=(arp.op == 1 && arp.tpa == 172.16.1.10), 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; outport = inport; flags.loopback = 1; output;)
> +  table=??(lr_in_ip_input     ), priority=90   , match=(arp.op == 1 && arp.tpa == 192.168.0.10), 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; outport = inport; flags.loopback = 1; output;)
> +  table=??(lr_in_ip_input     ), priority=90   , match=(arp.op == 1 && arp.tpa == 192.168.123.10), 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; outport = inport; flags.loopback = 1; output;)
> +  table=??(lr_in_ip_input     ), priority=91   , match=(inport == "DR-S1" && arp.op == 1 && arp.tpa == 172.16.1.10), action=(drop;)
> +  table=??(lr_in_ip_input     ), priority=91   , match=(inport == "DR-S2" && arp.op == 1 && arp.tpa == 10.0.0.10), action=(drop;)
> +  table=??(lr_in_ip_input     ), priority=91   , match=(inport == "DR-S3" && arp.op == 1 && arp.tpa == 192.168.0.10), action=(drop;)
> +  table=??(lr_in_ip_input     ), priority=92   , match=(inport == "DR-S1" && arp.op == 1 && arp.tpa == 172.16.1.10 && is_chassis_resident("cr-DR-S1")), 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; outport = inport; flags.loopback = 1; output;)
> +  table=??(lr_in_ip_input     ), priority=92   , match=(inport == "DR-S2" && arp.op == 1 && arp.tpa == 10.0.0.10 && is_chassis_resident("cr-DR-S2")), 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; outport = inport; flags.loopback = 1; output;)
> +  table=??(lr_in_ip_input     ), priority=92   , match=(inport == "DR-S3" && arp.op == 1 && arp.tpa == 192.168.0.10 && is_chassis_resident("cr-DR-S3")), 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; outport = inport; flags.loopback = 1; output;)
> +])
> +}
> +
> +check_lr_in_unsnat_flows() {
> +    AT_CHECK([grep lr_in_unsnat lrflows | grep ct_snat | sed 's/table=../table=??/' | sort], [0], [dnl
> +  table=??(lr_in_unsnat       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && inport == "DR-S2" && flags.loopback == 0 && is_chassis_resident("cr-DR-S2")), action=(ct_snat_in_czone;)
> +  table=??(lr_in_unsnat       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && inport == "DR-S2" && flags.loopback == 1 && flags.use_snat_zone == 1 && is_chassis_resident("cr-DR-S2")), action=(ct_snat;)
> +  table=??(lr_in_unsnat       ), priority=100  , match=(ip && ip4.dst == 172.16.1.10 && inport == "DR-S1" && flags.loopback == 0 && is_chassis_resident("cr-DR-S1")), action=(ct_snat_in_czone;)
> +  table=??(lr_in_unsnat       ), priority=100  , match=(ip && ip4.dst == 172.16.1.10 && inport == "DR-S1" && flags.loopback == 1 && flags.use_snat_zone == 1 && is_chassis_resident("cr-DR-S1")), action=(ct_snat;)
> +  table=??(lr_in_unsnat       ), priority=100  , match=(ip && ip4.dst == 192.168.0.10 && inport == "DR-S3" && flags.loopback == 0 && is_chassis_resident("cr-DR-S3")), action=(ct_snat_in_czone;)
> +  table=??(lr_in_unsnat       ), priority=100  , match=(ip && ip4.dst == 192.168.0.10 && inport == "DR-S3" && flags.loopback == 1 && flags.use_snat_zone == 1 && is_chassis_resident("cr-DR-S3")), action=(ct_snat;)
> +])
> +}
> +
> +check_lr_out_snat_flows() {
> +    AT_CHECK([grep lr_out_snat lrflows | grep ct_snat | sed 's/table=../table=??/' | sort], [0], [dnl
> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1")), action=(ct_snat_in_czone(172.16.1.10);)
> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S2" && is_chassis_resident("cr-DR-S2")), action=(ct_snat_in_czone(10.0.0.10);)
> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S3" && is_chassis_resident("cr-DR-S3")), action=(ct_snat_in_czone(192.168.0.10);)
> +  table=??(lr_out_snat        ), priority=162  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") && reg9[[4]] == 1), action=(reg9[[4]] = 0; ct_snat(172.16.1.10);)
> +  table=??(lr_out_snat        ), priority=162  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S2" && is_chassis_resident("cr-DR-S2") && reg9[[4]] == 1), action=(reg9[[4]] = 0; ct_snat(10.0.0.10);)
> +  table=??(lr_out_snat        ), priority=162  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S3" && is_chassis_resident("cr-DR-S3") && reg9[[4]] == 1), action=(reg9[[4]] = 0; ct_snat(192.168.0.10);)
> +])
> +}
> +
> +check_lr_in_unsnat_flows
> +check_lr_out_snat_flows
> +check_lr_in_arp_nat_flows
> +
> +check ovn-nbctl lr-nat-del DR snat 20.0.0.10
> +AT_CHECK([ovn-sbctl dump-flows DR | grep -e lr_in_unsnat -e lr_out_snat | grep ct_snat | wc -l], [0], [0
> +])
> +
> +# Configure DNAT
> +check ovn-nbctl lr-nat-add DR dnat  172.16.1.10    20.0.0.10
> +check ovn-nbctl lr-nat-add DR dnat  10.0.0.10      20.0.0.10
> +check ovn-nbctl lr-nat-add DR dnat  192.168.0.10   20.0.0.10
> +check ovn-nbctl lr-nat-add DR dnat  192.168.123.10 20.0.0.10
> +
> +ovn-sbctl dump-flows DR > lrflows
> +AT_CAPTURE_FILE([lrflows])
> +
> +check_lr_in_dnat_flows() {
> +    AT_CHECK([grep lr_in_dnat lrflows | grep ct_dnat | sed 's/table=../table=??/' | sort], [0], [dnl
> +  table=??(lr_in_dnat         ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && inport == "DR-S2" && is_chassis_resident("cr-DR-S2")), action=(ct_dnat_in_czone(20.0.0.10);)
> +  table=??(lr_in_dnat         ), priority=100  , match=(ip && ip4.dst == 172.16.1.10 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1")), action=(ct_dnat_in_czone(20.0.0.10);)
> +  table=??(lr_in_dnat         ), priority=100  , match=(ip && ip4.dst == 192.168.0.10 && inport == "DR-S3" && is_chassis_resident("cr-DR-S3")), action=(ct_dnat_in_czone(20.0.0.10);)
> +])
> +}
> +
> +check_lr_out_undnat_flows() {
> +    AT_CHECK([grep lr_out_undnat lrflows | grep ct_dnat | sed 's/table=../table=??/' | sort], [0], [dnl
> +  table=??(lr_out_undnat      ), priority=100  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1")), action=(ct_dnat_in_czone;)
> +  table=??(lr_out_undnat      ), priority=100  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S2" && is_chassis_resident("cr-DR-S2")), action=(ct_dnat_in_czone;)
> +  table=??(lr_out_undnat      ), priority=100  , match=(ip && ip4.src == 20.0.0.10 && outport == "DR-S3" && is_chassis_resident("cr-DR-S3")), action=(ct_dnat_in_czone;)
> +])
> +}
> +
> +check_lr_in_dnat_flows
> +check_lr_out_undnat_flows
> +check_lr_in_arp_nat_flows
> +
> +check ovn-nbctl lr-nat-del DR dnat  172.16.1.10
> +check ovn-nbctl lr-nat-del DR dnat  10.0.0.10
> +check ovn-nbctl lr-nat-del DR dnat  192.168.0.10
> +check ovn-nbctl lr-nat-del DR dnat  192.168.123.10
> +
> +AT_CHECK([ovn-sbctl dump-flows DR | grep -e lr_in_dnat -e lr_out_undnat | grep ct_dnat | wc -l], [0], [0
> +])
> +
> +# Configure DNAT_AND_SNAT
> +check ovn-nbctl lr-nat-add DR dnat_and_snat  172.16.1.10    20.0.0.10
> +check ovn-nbctl lr-nat-add DR dnat_and_snat  10.0.0.10      20.0.0.10
> +check ovn-nbctl lr-nat-add DR dnat_and_snat  192.168.0.10   20.0.0.10
> +check ovn-nbctl lr-nat-add DR dnat_and_snat  192.168.123.10 20.0.0.10
> +
> +ovn-sbctl dump-flows DR > lrflows
> +AT_CAPTURE_FILE([lrflows])
> +
> +check_lr_in_unsnat_flows
> +check_lr_out_snat_flows
> +check_lr_in_dnat_flows
> +check_lr_out_undnat_flows
> +check_lr_in_arp_nat_flows
> +
> +check ovn-nbctl lr-nat-del DR dnat_and_snat  172.16.1.10
> +check ovn-nbctl lr-nat-del DR dnat_and_snat  10.0.0.10
> +check ovn-nbctl lr-nat-del DR dnat_and_snat  192.168.0.10
> +check ovn-nbctl lr-nat-del DR dnat_and_snat  192.168.123.10
> +
> +AT_CHECK([ovn-sbctl dump-flows DR | grep -e lr_in_unsnat -e lr_out_snat -e lr_in_dnat -e lr_out_undnat | grep ct_snat| wc -l], [0], [0
> +])
> +
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 80a564660..698a7b3b9 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1202,18 +1202,28 @@
>           </p>
>         </dd>
>   
> -      <dt>[<code>--if-exists</code>] <code>lr-nat-del</code> <var>router</var> [<var>type</var> [<var>ip</var>]]</dt>
> +      <dt>[<code>--if-exists</code>] <code>lr-nat-del</code> <var>router</var> [<var>type</var> [<var>ip</var> [<var>logical_ip</var>]]]</dt>
>         <dd>
>           <p>
>             Deletes NATs from <var>router</var>.  If only <var>router</var>
>             is supplied, all the NATs from the logical router are
>             deleted.  If <var>type</var> is also specified, then all the
>             NATs that match the <var>type</var> will be deleted from the logical
> -          router.  If all the fields are given, then a single NAT rule
> -          that matches all the fields will be deleted.  When <var>type</var>
> -          is <code>snat</code>, the <var>ip</var> should be logical_ip.
> +          router.  If <var>ip</var> is also specified, then NAT rules matching
> +          the given fields will be deleted. When <var>type</var> is
> +          <code>snat</code>, the <var>ip</var> should be logical_ip.
>             When <var>type</var> is <code>dnat</code> or
>             <code>dnat_and_snat</code>, the <var>ip</var> shoud be external_ip.
> +          When <var>type</var> is <code>dnat</code> or
> +          <code>dnat_and_snat</code>, a single NAT rule is expected to match
> +          all the fields but this is not necessarily true when <var>type</var>
> +          is <code>snat</code>. To match a single NAT rule of <var>type</var>
> +          <code>snat</code> when multiple rules have the same logical_ip,
> +          external_ip of the rule also needs to be specified.
> +          <var>logical_ip</var> field can only be specified if
> +          <var>type</var> is <code>snat</code>. In this case, the <var>ip</var>
> +          should be external_ip of the rule and a single NAT rule that
> +          matches all the fields will be deleted.
>           </p>
>   
>           <p>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 55b0f5124..bf0d2723b 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -381,7 +381,7 @@ NAT commands:\n\
>     lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP [LOGICAL_PORT EXTERNAL_MAC]\n\
>                               [EXTERNAL_PORT_RANGE]\n\
>                               add a NAT to ROUTER\n\
> -  lr-nat-del ROUTER [TYPE [IP]]\n\
> +  lr-nat-del ROUTER [TYPE [IP [LOGICAL_IP]]]\n\
>                               remove NATs from ROUTER\n\
>     lr-nat-list ROUTER        print NATs for ROUTER\n\
>   \n\
> @@ -4482,6 +4482,92 @@ nbctl_lr_route_del(struct ctl_context *ctx)
>       free(nexthop);
>   }
>   
> +static bool
> +is_snat_rule_conflict(const struct nbrec_logical_router *lr,
> +                      char *new_external_ip, char *old_external_ip, bool is_v6)
> +{
> +    int num_l3dgw_ports = 0;
> +    bool is_conflict = false;
> +
> +    struct lport_addresses old_external_ip_addr, new_external_ip_addr;
> +
> +    if (!extract_ip_addresses(new_external_ip, &new_external_ip_addr) ||
> +        !extract_ip_addresses(old_external_ip, &old_external_ip_addr)) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +        VLOG_WARN_RL(&rl, "Extract addresses failed.");
> +        return true;
> +    }
> +
> +    if (is_v6) {
> +        ovs_assert(new_external_ip_addr.n_ipv6_addrs == 1);
> +        ovs_assert(old_external_ip_addr.n_ipv6_addrs == 1);
> +    } else {
> +        ovs_assert(new_external_ip_addr.n_ipv4_addrs == 1);
> +        ovs_assert(old_external_ip_addr.n_ipv4_addrs == 1);
> +    }
> +
> +    for (size_t i = 0; i < lr->n_ports; i++) {
> +        const struct nbrec_logical_router_port *lrp = lr->ports[i];
> +        const struct nbrec_logical_router_port *old_port = NULL;
> +        const struct nbrec_logical_router_port *new_port = NULL;
> +        if (lrp->n_gateway_chassis) {
> +            num_l3dgw_ports++;
> +            struct lport_addresses lrp_addrs;
> +            if (!extract_lrp_networks(lrp, &lrp_addrs)) {
> +                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +                VLOG_WARN_RL(&rl, "Extract addresses failed.");
> +                continue;
> +            }
> +
> +            if (is_v6) {
> +                for (int j = 0; j < lrp_addrs.n_ipv6_addrs; j++) {
> +                    struct ipv6_netaddr *lrp6_addr = &lrp_addrs.ipv6_addrs[j];
> +                    struct in6_addr new_ip6_mask, old_ip6_mask;
> +                    new_ip6_mask = ipv6_addr_bitand(
> +                        &lrp6_addr->mask,
> +                        &new_external_ip_addr.ipv6_addrs[0].addr);
> +                    old_ip6_mask = ipv6_addr_bitand(
> +                        &lrp6_addr->mask,
> +                        &old_external_ip_addr.ipv6_addrs[0].addr);
> +                    if (ipv6_addr_equals(&new_ip6_mask,
> +                                         &(lrp6_addr->network))) {
> +                        new_port = lrp;
> +                    }
> +                    if (ipv6_addr_equals(&old_ip6_mask,
> +                                         &(lrp6_addr->network))) {
> +                        old_port = lrp;
> +                    }
> +                }
> +            } else {
> +                for (int j = 0; j < lrp_addrs.n_ipv4_addrs; j++) {
> +                    uint32_t nw_addr = ntohl(lrp_addrs.ipv4_addrs[j].network);
> +                    uint32_t mask = ntohl(lrp_addrs.ipv4_addrs[j].mask);
> +                    uint32_t new_ip, old_ip;
> +                    new_ip = ntohl(new_external_ip_addr.ipv4_addrs[0].addr);
> +                    old_ip = ntohl(old_external_ip_addr.ipv4_addrs[0].addr);
> +                    if ((new_ip & mask) == nw_addr) {
> +                        new_port = lrp;
> +                    }
> +                    if ((old_ip & mask) == nw_addr) {
> +                        old_port = lrp;
> +                    }
> +                }
> +            }
> +            if ((old_port || new_port) && (old_port == new_port)) {
> +                is_conflict = true;
> +            }
> +            destroy_lport_addresses(&lrp_addrs);
> +        }
> +    }
> +    destroy_lport_addresses(&old_external_ip_addr);
> +    destroy_lport_addresses(&new_external_ip_addr);
> +
> +    if (num_l3dgw_ports > 1 && !is_conflict) {
> +        return false;
> +    }
> +    return true;
> +}
> +
>   static bool
>   is_valid_port_range(const char *port_range)
>   {
> @@ -4542,6 +4628,7 @@ nbctl_pre_lr_nat_add(struct ctl_context *ctx)
>   {
>       ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
>       ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_nat);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_ports);
>   
>       ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
>   
> @@ -4551,6 +4638,11 @@ nbctl_pre_lr_nat_add(struct ctl_context *ctx)
>       ovsdb_idl_add_column(ctx->idl, &nbrec_nat_col_logical_port);
>       ovsdb_idl_add_column(ctx->idl, &nbrec_nat_col_external_mac);
>       ovsdb_idl_add_column(ctx->idl, &nbrec_nat_col_options);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_networks);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_mac);
> +    ovsdb_idl_add_column(ctx->idl,
> +                         &nbrec_logical_router_port_col_gateway_chassis);
>   }
>   
>   static void
> @@ -4720,12 +4812,16 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>                               should_return = true;
>                           }
>                   } else {
> -                    ctl_error(ctx, "a NAT with this type (%s) and %s (%s) "
> -                              "already exists",
> -                              nat_type,
> -                              is_snat ? "logical_ip" : "external_ip",
> -                              is_snat ? new_logical_ip : new_external_ip);
> -                    should_return = true;
> +                    if (!is_snat ||
> +                        is_snat_rule_conflict(lr, new_external_ip,
> +                                              old_external_ip, is_v6)) {
> +                        ctl_error(ctx, "a NAT with this type (%s) and %s (%s) "
> +                                  "already exists",
> +                                  nat_type,
> +                                  is_snat ? "logical_ip" : "external_ip",
> +                                  is_snat ? new_logical_ip : new_external_ip);
> +                        should_return = true;
> +                    }
>                   }
>               }
>           }
> @@ -4835,6 +4931,21 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
>       }
>   
>       int is_snat = !strcmp("snat", nat_type);
> +    char *snat_external_ip = NULL;
> +    if (ctx->argc == 5) {
> +        if (is_snat) {
> +            snat_external_ip = nat_ip;
> +            nat_ip = normalize_prefix_str(ctx->argv[4]);
> +            if (!snat_external_ip) {
> +                ctl_error(ctx, "%s: Invalid IP address or CIDR", ctx->argv[4]);
> +            }
> +        } else {
> +            ctl_error(ctx, "%s type takes a maximum of one ip address",
> +                      nat_type);
> +        }
> +    }
> +    bool is_exist = false;
> +
>       /* Remove the matching NAT. */
>       for (size_t i = 0; i < lr->n_nat; i++) {
>           struct nbrec_nat *nat = lr->nat[i];
> @@ -4846,8 +4957,29 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
>               continue;
>           }
>           if (!strcmp(nat_type, nat->type) && !strcmp(nat_ip, old_ip)) {
> -            nbrec_logical_router_update_nat_delvalue(lr, nat);
> -            should_return = true;
> +            if (snat_external_ip != NULL) {
> +                char *old_external_ip = normalize_prefix_str(nat->external_ip);
> +                if (!old_external_ip) {
> +                    continue;
> +                }
> +                if (!strcmp(snat_external_ip, old_external_ip)) {
> +                    nbrec_logical_router_update_nat_delvalue(lr, nat);
> +                    free(old_external_ip);
> +                    is_exist = true;
> +                    should_return = true;
> +                }
> +            } else {
> +                nbrec_logical_router_update_nat_delvalue(lr, nat);
> +                /* When nat_type is snat and external_ip is not specified, we
> +                 * need to iterate over all the rules and delete all nat entries
> +                 * matching the logical ip. Hence don't set should_return for
> +                 * snat case.
> +                 */
> +                if (!is_snat) {
> +                    should_return = true;
> +                }
> +                is_exist = true;
> +            }
>           }
>           free(old_ip);
>           if (should_return) {
> @@ -4855,13 +4987,16 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
>           }
>       }
>   
> -    if (must_exist) {
> +    if (must_exist && !is_exist) {
>           ctl_error(ctx, "no matching NAT with the type (%s) and %s (%s)",
>                     nat_type, is_snat ? "logical_ip" : "external_ip", nat_ip);
>       }
>   
>   cleanup:
>       free(nat_ip);
> +    if (snat_external_ip != NULL) {
> +        free(snat_external_ip);
> +    }
>   }
>   
>   static void
> @@ -7109,7 +7244,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>         "[LOGICAL_PORT EXTERNAL_MAC] [EXTERNAL_PORT_RANGE]",
>         nbctl_pre_lr_nat_add, nbctl_lr_nat_add,
>         NULL, "--may-exist,--stateless,--portrange,--add-route", RW },
> -    { "lr-nat-del", 1, 3, "ROUTER [TYPE [IP]]",
> +    { "lr-nat-del", 1, 4, "ROUTER [TYPE [IP [LOGICAL_IP]]]",
>         nbctl_pre_lr_nat_del, nbctl_lr_nat_del, NULL, "--if-exists", RW },
>       { "lr-nat-list", 1, 1, "ROUTER", nbctl_pre_lr_nat_list,
>         nbctl_lr_nat_list, NULL, "", RO },
>

Hi Mark,

Thanks for looking into this patch.

> On 5 Jan 2022, at 22:01, Mark Michelson <mmichels@redhat.com> wrote:
> 
> Hi,
> 
> I haven't done a full review of this patch, but I have noticed a problem pretty early on when I started looking.
> 
> The new ip_in_lrp_networks() function that is added here is intended to determine which l3dgw port to use for a particular NAT external address. The problem is that there are configurations, especially from OpenStack, where this will cause problems.
> 
> In current OpenStack configurations, they might set up a router with a gateway port that serves the 10.0.0.0/8 network. But they might set up a DNAT with external address 192.168.0.1 on that router. They expect that traffic sent to 192.168.0.1 will be NATted correctly on that router, even though the router port network does not include that IP address.

Yeah, this makes sense. I was thinking about this case for Load balancer rules but it’s true for DNAT in general.

> If you run `make check-system-userspace` with your series, you'll find that the "Floating IP outside router subnet IPv4" test fails.
> 
> How could this be fixed?
> 
> One idea is to configure the external port explicitly either on the NAT itself or on the router. This way, even if the IP address is outside the subnet of the DGP, you can still know which DGP is the "correct" one.
> 
> Another idea would be to differentiate behavior between SNAT and DNAT. For DNAT, you could treat all DGPs as equals, so it doesn't matter on which DGP the traffic is received, it will get translated properly. For SNAT or DNAT-and-SNAT, you'd probably need to explicitly specify which DGP to use, though.

I was thinking of the second approach for Load balancer rules. I could do the same for DNAT rules. The first approach gives more control but expects the user to configure correctly. Which approach would you suggest?

Thanks,
Abhiram

On 1/7/22 09:26, Abhiram Sangana wrote:
> Hi Mark,
> 
> Thanks for looking into this patch.
> 
>> On 5 Jan 2022, at 22:01, Mark Michelson <mmichels@redhat.com> wrote:
>>
>> Hi,
>>
>> I haven't done a full review of this patch, but I have noticed a problem pretty early on when I started looking.
>>
>> The new ip_in_lrp_networks() function that is added here is intended to determine which l3dgw port to use for a particular NAT external address. The problem is that there are configurations, especially from OpenStack, where this will cause problems.
>>
>> In current OpenStack configurations, they might set up a router with a gateway port that serves the 10.0.0.0/8 network. But they might set up a DNAT with external address 192.168.0.1 on that router. They expect that traffic sent to 192.168.0.1 will be NATted correctly on that router, even though the router port network does not include that IP address.
> 
> Yeah, this makes sense. I was thinking about this case for Load balancer rules but it’s true for DNAT in general.
> 
>> If you run `make check-system-userspace` with your series, you'll find that the "Floating IP outside router subnet IPv4" test fails.
>>
>> How could this be fixed?
>>
>> One idea is to configure the external port explicitly either on the NAT itself or on the router. This way, even if the IP address is outside the subnet of the DGP, you can still know which DGP is the "correct" one.
>>
>> Another idea would be to differentiate behavior between SNAT and DNAT. For DNAT, you could treat all DGPs as equals, so it doesn't matter on which DGP the traffic is received, it will get translated properly. For SNAT or DNAT-and-SNAT, you'd probably need to explicitly specify which DGP to use, though.
> 
> I was thinking of the second approach for Load balancer rules. I could do the same for DNAT rules. The first approach gives more control but expects the user to configure correctly. Which approach would you suggest?

I think the second approach should be good as well.

> 
> Thanks,
> Abhiram
>

On Mon, Jan 10, 2022 at 5:32 AM Mark Michelson <mmichels@redhat.com> wrote:
>
> On 1/7/22 09:26, Abhiram Sangana wrote:
> > Hi Mark,
> >
> > Thanks for looking into this patch.
> >
> >> On 5 Jan 2022, at 22:01, Mark Michelson <mmichels@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> I haven't done a full review of this patch, but I have noticed a
problem pretty early on when I started looking.
> >>
> >> The new ip_in_lrp_networks() function that is added here is intended
to determine which l3dgw port to use for a particular NAT external address.
The problem is that there are configurations, especially from OpenStack,
where this will cause problems.
> >>
> >> In current OpenStack configurations, they might set up a router with a
gateway port that serves the 10.0.0.0/8 network. But they might set up a
DNAT with external address 192.168.0.1 on that router. They expect that
traffic sent to 192.168.0.1 will be NATted correctly on that router, even
though the router port network does not include that IP address.
> >
> > Yeah, this makes sense. I was thinking about this case for Load
balancer rules but it’s true for DNAT in general.
> >
> >> If you run `make check-system-userspace` with your series, you'll find
that the "Floating IP outside router subnet IPv4" test fails.
> >>
> >> How could this be fixed?
> >>
> >> One idea is to configure the external port explicitly either on the
NAT itself or on the router. This way, even if the IP address is outside
the subnet of the DGP, you can still know which DGP is the "correct" one.
> >>
> >> Another idea would be to differentiate behavior between SNAT and DNAT.
For DNAT, you could treat all DGPs as equals, so it doesn't matter on which
DGP the traffic is received, it will get translated properly. For SNAT or
DNAT-and-SNAT, you'd probably need to explicitly specify which DGP to use,
though.
> >
> > I was thinking of the second approach for Load balancer rules. I could
do the same for DNAT rules. The first approach gives more control but
expects the user to configure correctly. Which approach would you suggest?
>
> I think the second approach should be good as well.
>

Sorry for my ~late response.
For the second approach, I wonder if there would be scale problems: when
there are many DGPs, all DNAT external IPs need to be advertised (GARP)
from all DGPs. Would it be better to allow the configuration to specify
which DGP to use for DNAT (and for all the NAT types), and only if it is
not specified, then advertise on all DGPs as the default behavior?

Thanks,
Han

> >
> > Thanks,
> > Abhiram
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev