diff mbox

[ovs-dev,CudaMailTagged,1/4] ovn: Support DNAT with port.

Message ID 1477965641-95206-1-git-send-email-nickcooper-zhangtonghao@opencloud.tech
State Not Applicable
Delegated to: Guru Shetty
Headers show

Commit Message

nickcooper-zhangtonghao Nov. 1, 2016, 2 a.m. UTC
When the type of NAT is dnat, the externally visible IP address
and port number can be DNATted to the IP address and port number
in the logical space. Adds a protocol column for the NAT. Valid
protocols  are  tcp  or  udp.  This column is useful when a port
number is provided as part of the external_ip column and the type
is set as dnat. If this column is empty and a port number is
provided as part of external_ip column when the type is set as dnat,
OVN assumes the protocol to be tcp.

For example :
ovn-nbctl -- --id=@nat create nat type="dnat" protocol="tcp" \
	logical_ip="192.168.1.2\:8080" external_ip="30.0.0.2\:80" \
	-- add logical_router R1 nat @nat

This patch improves the "tests/test-l7.py" which can support the
"--port" argument. This patch also provides the unit tests and
the documentation.

Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtonghao@opencloud.tech>
---
 include/ovn/actions.h       |   1 +
 ovn/lib/actions.c           |  13 +++-
 ovn/northd/ovn-northd.8.xml |  10 +++
 ovn/northd/ovn-northd.c     | 149 ++++++++++++++++++++++++++++++++++----------
 ovn/ovn-nb.ovsschema        |   8 ++-
 ovn/ovn-nb.xml              |  22 +++++--
 tests/system-ovn.at         | 113 +++++++++++++++++++++++++++++++++
 tests/test-l7.py            |  13 +++-
 8 files changed, 287 insertions(+), 42 deletions(-)

Comments

Gurucharan Shetty Nov. 11, 2016, 10:07 p.m. UTC | #1
On 31 October 2016 at 19:00, nickcooper-zhangtonghao <
nickcooper-zhangtonghao@opencloud.tech> wrote:

> When the type of NAT is dnat, the externally visible IP address
> and port number can be DNATted to the IP address and port number
> in the logical space. Adds a protocol column for the NAT. Valid
> protocols  are  tcp  or  udp.  This column is useful when a port
> number is provided as part of the external_ip column and the type
> is set as dnat. If this column is empty and a port number is
> provided as part of external_ip column when the type is set as dnat,
> OVN assumes the protocol to be tcp.
>
> For example :
> ovn-nbctl -- --id=@nat create nat type="dnat" protocol="tcp" \
>         logical_ip="192.168.1.2\:8080" external_ip="30.0.0.2\:80" \
>         -- add logical_router R1 nat @nat
>
> This patch improves the "tests/test-l7.py" which can support the
> "--port" argument. This patch also provides the unit tests and
> the documentation.
>
> Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtonghao@
> opencloud.tech>
>

Sorry for the delay in the reviews.
I understand that we can extend the current DNAT feature to include
DNAT:port. But is there is a use case where you want to use this? Any
extensions of the NAT table can be better designed if we understand the end
use-case for it. If not, I will just take a look at the first version of
the series.


> ---
>  include/ovn/actions.h       |   1 +
>  ovn/lib/actions.c           |  13 +++-
>  ovn/northd/ovn-northd.8.xml |  10 +++
>  ovn/northd/ovn-northd.c     | 149 ++++++++++++++++++++++++++++++
> ++++----------
>  ovn/ovn-nb.ovsschema        |   8 ++-
>  ovn/ovn-nb.xml              |  22 +++++--
>  tests/system-ovn.at         | 113 +++++++++++++++++++++++++++++++++
>  tests/test-l7.py            |  13 +++-
>  8 files changed, 287 insertions(+), 42 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 0bf6145..ce8aca5 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -170,6 +170,7 @@ struct ovnact_ct_commit {
>  struct ovnact_ct_nat {
>      struct ovnact ovnact;
>      ovs_be32 ip;
> +    uint16_t port;
>      uint8_t ltable;             /* Logical table ID of next table. */
>  };
>
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index df526c0..90c19f3 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -697,13 +697,21 @@ parse_ct_nat(struct action_context *ctx, const char
> *name,
>
>      if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
>          if (ctx->lexer->token.type != LEX_T_INTEGER
> -            || ctx->lexer->token.format != LEX_F_IPV4) {
> +            || mf_subvalue_width(&ctx->lexer->token.value) > 32) {
>              lexer_syntax_error(ctx->lexer, "expecting IPv4 address");
>              return;
>          }
>          cn->ip = ctx->lexer->token.value.ipv4;
>          lexer_get(ctx->lexer);
>
> +        /* Parse optional port. */
> +        uint16_t port = 0;
> +        if (lexer_match(ctx->lexer, LEX_T_COLON)
> +            && !action_parse_port(ctx, &port)) {
> +            return;
> +        }
> +        cn->port = port;
> +
>          if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
>              return;
>          }
> @@ -779,6 +787,9 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
>          if (snat) {
>              nat->flags |= NX_NAT_F_SRC;
>          } else {
> +            if (cn->port) {
> +                nat->range.proto.min = cn->port;
> +            }
>              nat->flags |= NX_NAT_F_DST;
>          }
>      }
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index df53d4c..f141462 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -1191,6 +1191,16 @@ icmp4 {
>
>        <li>
>          For each configuration in the OVN Northbound database, that asks
> +        to change the destination IP address <var>A</var> with a port
> +        <var>P1</var> of protocol <var>P</var> to IP address <var>B</var>
> +        and port <var>P2</var>, a priority-100 flow matches <code>ip
> &amp;&amp;
> +        ip4.dst == <var>A</var> &amp;&amp; <var>P</var> &amp;&amp;
> +        <var>P</var>.dst == <var>P1</var></code> with an action
> +        <code>flags.loopback = 1; ct_dnat(<var>B</var>:<var>P2</
> var>);</code>.
> +      </li>
> +
> +      <li>
> +        For each configuration in the OVN Northbound database, that asks
>          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>A</var></code> with an action
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index ad0739c..03bd9e4 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2252,7 +2252,7 @@ build_pre_acls(struct ovn_datapath *od, struct hmap
> *lflows)
>   * 'ip_address'.  The caller must free() the memory allocated for
>   * 'ip_address'. */
>  static void
> -ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> +ip_address_and_port(const char *key, char **ip_address,
>                                  uint16_t *port)
>  {
>      char *ip_str, *start, *next;
> @@ -2262,8 +2262,6 @@ ip_address_and_port_from_lb_key(const char *key,
> char **ip_address,
>      next = start = xstrdup(key);
>      ip_str = strsep(&next, ":");
>      if (!ip_str || !ip_str[0]) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_WARN_RL(&rl, "bad ip address for load balancer key %s", key);
>          free(start);
>          return;
>      }
> @@ -2271,8 +2269,6 @@ ip_address_and_port_from_lb_key(const char *key,
> char **ip_address,
>      ovs_be32 ip, mask;
>      char *error = ip_parse_masked(ip_str, &ip, &mask);
>      if (error || mask != OVS_BE32_MAX) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_WARN_RL(&rl, "bad ip address for load balancer key %s", key);
>          free(start);
>          free(error);
>          return;
> @@ -2281,8 +2277,6 @@ ip_address_and_port_from_lb_key(const char *key,
> char **ip_address,
>      int l4_port = 0;
>      if (next && next[0]) {
>          if (!str_to_int(next, 0, &l4_port) || l4_port < 0 || l4_port >
> 65535) {
> -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -            VLOG_WARN_RL(&rl, "bad ip port for load balancer key %s",
> key);
>              free(start);
>              return;
>          }
> @@ -2313,8 +2307,11 @@ build_pre_lb(struct ovn_datapath *od, struct hmap
> *lflows)
>              /* node->key contains IP:port or just IP. */
>              char *ip_address = NULL;
>              uint16_t port;
> -            ip_address_and_port_from_lb_key(node->key, &ip_address,
> &port);
> +            ip_address_and_port(node->key, &ip_address, &port);
>              if (!ip_address) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl, "bad ip address or port for load
> balancer "
> +                             "key %s", node->key);
>                  continue;
>              }
>
> @@ -2719,8 +2716,11 @@ build_stateful(struct ovn_datapath *od, struct hmap
> *lflows)
>
>              /* node->key contains IP:port or just IP. */
>              char *ip_address = NULL;
> -            ip_address_and_port_from_lb_key(node->key, &ip_address,
> &port);
> +            ip_address_and_port(node->key, &ip_address, &port);
>              if (!ip_address) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl, "bad ip address or port for load
> balancer "
> +                             "key %s", node->key);
>                  continue;
>              }
>
> @@ -3613,8 +3613,12 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                  char *ip_address = NULL;
>                  uint16_t port;
>
> -                ip_address_and_port_from_lb_key(node->key, &ip_address,
> &port);
> +                ip_address_and_port(node->key, &ip_address, &port);
>                  if (!ip_address) {
> +                    static struct vlog_rate_limit rl =
> +                        VLOG_RATE_LIMIT_INIT(5, 1);
> +                    VLOG_WARN_RL(&rl, "bad ip address or port for load "
> +                                 "balancer key %s", node->key);
>                      continue;
>                  }
>
> @@ -3669,10 +3673,25 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>
>              ovs_be32 ip;
>              if (!ip_parse(nat->external_ip, &ip) || !ip) {
> -                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
> -                VLOG_WARN_RL(&rl, "bad ip address %s in nat configuration
> "
> -                             "for router %s", nat->external_ip, op->key);
> -                continue;
> +                bool is_external_port = false;
> +                if (!strcmp(nat->type, "dnat")) {
> +                    ovs_be16 port;
> +                    char *error = ip_parse_port(nat->external_ip, &ip,
> &port);
> +                    if (error) {
> +                        free(error);
> +                    } else {
> +                        is_external_port = true;
> +                    }
> +                }
> +
> +                if (!is_external_port) {
> +                    static struct vlog_rate_limit rl =
> +                        VLOG_RATE_LIMIT_INIT(5, 1);
> +                    VLOG_WARN_RL(&rl, "bad external ip address %s in nat "
> +                                 "configuration for router %s",
> +                                 nat->external_ip, op->key);
> +                    continue;
> +                }
>              }
>
>              if (!strcmp(nat->type, "snat")) {
> @@ -3845,8 +3864,12 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>
>                  /* node->key contains IP:port or just IP. */
>                  char *ip_address = NULL;
> -                ip_address_and_port_from_lb_key(node->key, &ip_address,
> &port);
> +                ip_address_and_port(node->key, &ip_address, &port);
>                  if (!ip_address) {
> +                    static struct vlog_rate_limit rl =
> +                        VLOG_RATE_LIMIT_INIT(5, 1);
> +                    VLOG_WARN_RL(&rl, "bad ip address or port for load "
> +                                 "balancer key %s", node->key);
>                      continue;
>                  }
>
> @@ -3905,37 +3928,78 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>
>              nat = od->nbr->nat[i];
>
> -            ovs_be32 ip, mask;
> -
> -            char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
> -            if (error || mask != OVS_BE32_MAX) {
> +            uint16_t external_port = 0;
> +            char *external_ip = NULL;
> +            ip_address_and_port(nat->external_ip,
> +                                &external_ip, &external_port);
> +            if (!external_ip) {
>                  static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
>                  VLOG_WARN_RL(&rl, "bad external ip %s for nat",
>                               nat->external_ip);
> -                free(error);
> +                continue;
> +            }
> +
> +            if (strcmp(nat->type, "dnat") && external_port) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl, "the external ip (%s) with port should
> not "
> +                             "be configured for snat or dnat_and_snat",
> +                             nat->external_ip);
> +                free(external_ip);
>                  continue;
>              }
>
>              /* Check the validity of nat->logical_ip. 'logical_ip' can
>               * be a subnet when the type is "snat". */
> -            error = ip_parse_masked(nat->logical_ip, &ip, &mask);
> -            if (!strcmp(nat->type, "snat")) {
> -                if (error) {
> +            uint16_t logical_port = 0;
> +            char *error, *logical_ip = NULL;
> +            ovs_be32 ip, mask = OVS_BE32_MAX;
> +            ip_address_and_port(nat->logical_ip,
> +                                &logical_ip, &logical_port);
> +            if (!logical_ip) {
> +                if (!strcmp(nat->type, "snat")) {
> +                    error = ip_parse_masked(nat->logical_ip, &ip, &mask);
> +                    if (error) {
> +                        static struct vlog_rate_limit rl =
> +                            VLOG_RATE_LIMIT_INIT(5, 1);
> +                        VLOG_WARN_RL(&rl, "bad logical ip network or ip
> %s "
> +                                     "for snat in router "UUID_FMT"",
> +                                     nat->logical_ip,
> UUID_ARGS(&od->key));
> +                        free(external_ip);
> +                        free(error);
> +                        continue;
> +                    }
> +                } else {
>                      static struct vlog_rate_limit rl =
>                          VLOG_RATE_LIMIT_INIT(5, 1);
> -                    VLOG_WARN_RL(&rl, "bad ip network or ip %s for snat "
> -                                 "in router "UUID_FMT"",
> +                    VLOG_WARN_RL(&rl, "bad logical ip %s for dnat or "
> +                                 "dnat_and_snat in router "UUID_FMT"",
>                                   nat->logical_ip, UUID_ARGS(&od->key));
> -                    free(error);
> +                    free(external_ip);
> +                    continue;
> +                }
> +            }
> +
> +            if (!strcmp(nat->type, "dnat")) {
> +                if ((external_port != 0 && logical_port == 0)
> +                    || (external_port == 0 && logical_port != 0)) {
> +                    static struct vlog_rate_limit rl =
> +                        VLOG_RATE_LIMIT_INIT(5, 1);
> +                    VLOG_WARN_RL(&rl, "the external/logical ip (%s, %s)
> with "
> +                                 "port should be configured at same time
> for "
> +                                 "dnat", nat->external_ip,
> nat->logical_ip);
> +                    free(external_ip);
> +                    free(logical_ip);
>                      continue;
>                  }
>              } else {
> -                if (error || mask != OVS_BE32_MAX) {
> +                if (logical_port) {
>                      static struct vlog_rate_limit rl =
>                          VLOG_RATE_LIMIT_INIT(5, 1);
> -                    VLOG_WARN_RL(&rl, "bad ip %s for dnat in router "
> -                        ""UUID_FMT"", nat->logical_ip,
> UUID_ARGS(&od->key));
> -                    free(error);
> +                    VLOG_WARN_RL(&rl, "the logical ip (%s) with port
> should "
> +                                 "not be configured for snat or
> dnat_and_snat",
> +                                 nat->logical_ip);
> +                    free(external_ip);
> +                    free(logical_ip);
>                      continue;
>                  }
>              }
> @@ -3966,10 +4030,27 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                   * We need to zero the inport because the router can
>                   * send the packet back through the same interface. */
>                  ds_clear(&match);
> -                ds_put_format(&match, "ip && ip4.dst == %s",
> nat->external_ip);
>                  ds_clear(&actions);
> -                ds_put_format(&actions,"flags.loopback = 1;
> ct_dnat(%s);",
> -                              nat->logical_ip);
> +                if (external_port && logical_port) {
> +                    /* The external_port and logical port should be
> configured
> +                     * only for DNAT.*/
> +                    char *protocol = "tcp";
> +                    if (nat->protocol && !strcmp(nat->protocol, "udp")) {
> +                       protocol = "udp";
> +                    }
> +                    ds_put_format(&match, "ip && ip4.dst == %s "
> +                                  "&& %s && %s.dst == %d",
> +                                  external_ip, protocol,
> +                                  protocol, external_port);
> +                    ds_put_format(&actions,"flags.loopback = 1; "
> +                                  "ct_dnat(%s:%d);",
> +                                  logical_ip, logical_port);
> +                } else {
> +                    ds_put_format(&match, "ip && ip4.dst == %s",
> +                                  external_ip);
> +                    ds_put_format(&actions,"flags.loopback = 1;
> ct_dnat(%s);",
> +                                  logical_ip);
> +                }
>                  ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 100,
>                                ds_cstr(&match), ds_cstr(&actions));
>              }
> @@ -3991,6 +4072,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                count_1bits(ntohl(mask)) + 1,
>                                ds_cstr(&match), ds_cstr(&actions));
>              }
> +            free(external_ip);
> +            free(logical_ip);
>          }
>
>          /* Re-circulate every packet through the DNAT zone.
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index 865dd34..a290c67 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.4.0",
> -    "cksum": "4176761817 11225",
> +    "version": "5.4.1",
> +    "cksum": "944843976 11424",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -203,6 +203,10 @@
>              "columns": {
>                  "external_ip": {"type": "string"},
>                  "logical_ip": {"type": "string"},
> +                "protocol": {
> +                    "type": {"key": {"type": "string",
> +                             "enum": ["set", ["tcp", "udp"]]},
> +                             "min": 0, "max": 1}},
>                  "type": {"type": {"key": {"type": "string",
>                                             "enum": ["set", ["dnat",
>                                                               "snat",
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index e1b3136..ec13278 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -1116,8 +1116,10 @@
>        <ul>
>          <li>
>            When <ref column="type"/> is <code>dnat</code>, the externally
> -          visible IP address <ref column="external_ip"/> is DNATted to
> the IP
> -          address <ref column="logical_ip"/> in the logical space.
> +          visible IP address (and an optional port number with
> <code>:</code>
> +          as a separator) <ref column="external_ip"/> is DNATted to the IP
> +          address (and an optional port number with <code>:</code> as a
> +          separator) <ref column="logical_ip"/> in the logical space.
>          </li>
>          <li>
>            When <ref column="type"/> is <code>snat</code>, IP packets
> @@ -1138,11 +1140,23 @@
>      </column>
>
>      <column name="external_ip">
> -      An IPv4 address.
> +     An IPv4 address (and an optional port number with <code>:</code>
> +     as a separator).
>      </column>
>
>      <column name="logical_ip">
> -      An IPv4 network (e.g 192.168.1.0/24) or an IPv4 address.
> +     An IPv4 network (e.g 192.168.1.0/24) or an IPv4 address (and an
> optional
> +     port number with <code>:</code> as a separator).
> +    </column>
> +
> +    <column name="protocol">
> +     Valid protocols are <code>tcp</code> or <code>udp</code>.  This
> column
> +     is useful when a port number is provided as part of the
> +     <code>external_ip</code> column and the <code>type</code> is set
> +     as <code>dnat</code>.  If this column is empty and a port number
> +     is provided as part of <code>external_ip</code> column when
> +     the <code>type</code> is set as <code>dnat</code>, OVN assumes the
> +     <code>protocol</code> to be <code>tcp</code>.
>      </column>
>    </table>
>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 21226d9..ba69df6 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -279,6 +279,119 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> patch-.*/d
>  /connection dropped.*/d"])
>  AT_CLEANUP
>
> +AT_SETUP([ovn -- 2 LRs connected via LS, gateway router, DNAT with port])
> +AT_KEYWORDS([ovnnat])
> +
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +# Set external-ids in br-int needed for ovn-controller
> +ovs-vsctl \
> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> +        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock
> \
> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +        -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +# Logical network:
> +# Two LRs - R1 and R2 that are connected to each other via LS "join"
> +# in 20.0.0.0/24 network. R1 has switchess foo (192.168.1.0/24) connected
> +# to it.  R2 has alice (172.16.1.0/24) connected to it.
> +# R2 is a gateway router on which we add NAT rules.
> +#
> +#    foo -- R1 -- join - R2 -- alice
> +
> +ovn-nbctl lr-add R1
> +ovn-nbctl lr-add R2 -- set Logical_Router R2 options:chassis=hv1
> +
> +ovn-nbctl ls-add foo
> +ovn-nbctl ls-add alice
> +ovn-nbctl ls-add join
> +
> +ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
> +ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
> +ovn-nbctl lrp-add R1 R1_join 00:00:04:01:02:03 20.0.0.1/24
> +ovn-nbctl lrp-add R2 R2_join 00:00:04:01:02:04 20.0.0.2/24
> +
> +# Connect foo to R1
> +ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
> +    type=router options:router-port=foo addresses=\"00:00:01:01:02:03\"
> +
> +# Connect alice to R2
> +ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
> +    type=router options:router-port=alice addresses=\"00:00:02:01:02:03\"
> +
> +# Connect R1 to join
> +ovn-nbctl lsp-add join r1-join -- set Logical_Switch_Port r1-join \
> +    type=router options:router-port=R1_join addresses='"00:00:04:01:02:03"
> '
> +
> +# Connect R2 to join
> +ovn-nbctl lsp-add join r2-join -- set Logical_Switch_Port r2-join \
> +    type=router options:router-port=R2_join addresses='"00:00:04:01:02:04"
> '
> +
> +# Static routes.
> +ovn-nbctl lr-route-add R1 172.16.1.0/24 20.0.0.2
> +ovn-nbctl lr-route-add R2 192.168.0.0/16 20.0.0.1
> +
> +# Logical port 'foo1' in switch 'foo'.
> +ADD_NAMESPACES(foo1)
> +ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> +         "192.168.1.1")
> +ovn-nbctl lsp-add foo foo1 \
> +-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
> +
> +# Logical port 'alice1' in switch 'alice'.
> +ADD_NAMESPACES(alice1)
> +ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24", "f0:00:00:01:02:04", \
> +         "172.16.1.1")
> +ovn-nbctl lsp-add alice alice1 \
> +-- lsp-set-addresses alice1 "f0:00:00:01:02:04 172.16.1.2"
> +
> +# Add a DNAT with port rule
> +ovn-nbctl -- --id=@nat create nat type="dnat" protocol="tcp"
> logical_ip="192.168.1.2\:8080" \
> +    external_ip="30.0.0.2\:80" -- add logical_router R2 nat @nat
> +ovn-nbctl -- --id=@nat create nat type="dnat" protocol="udp"
> logical_ip="192.168.1.2\:8181" \
> +    external_ip="30.0.0.3\:81" -- add logical_router R2 nat @nat
> +
> +OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep ct\( | grep nat])
> +
> +# South-North DNAT
> +NETNS_DAEMONIZE([foo1], [[$PYTHON $srcdir/test-l7.py --port 8080]],
> [http0.pid])
> +NS_CHECK_EXEC([alice1], [wget 30.0.0.2 -t 3 -T 1 --retry-connrefused -v
> -o wget0.log])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br-int ovs-alice1 normal '
> 000002010203F0000001020408004500001C000100004011AFBBAC100102
> 1E0000030035005100083443'])
> +
> +# We verify that DNAT indeed happened via 'dump-conntrack' command.
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2) | \
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> +tcp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,
> dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,
> sport=<cleared>,dport=<cleared>),zone=<cleared>,
> protoinfo=(state=<cleared>)
> +])
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.3) | \
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
> +udp,orig=(src=172.16.1.2,dst=30.0.0.3,sport=<cleared>,
> dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,
> sport=<cleared>,dport=<cleared>),zone=<cleared>
> +])
> +
> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +
>  AT_SETUP([ovn -- load-balancing])
>  AT_KEYWORDS([ovnlb])
>
> diff --git a/tests/test-l7.py b/tests/test-l7.py
> index aed34f4..c6aae4e 100755
> --- a/tests/test-l7.py
> +++ b/tests/test-l7.py
> @@ -61,8 +61,14 @@ def main():
>      protocols = [srv for srv in SERVERS]
>      parser = argparse.ArgumentParser(
>              description='Run basic application servers.')
> -    parser.add_argument('proto', default='http', nargs='?',
> -            help='protocol to serve (%s)' % protocols)
> +    parser.add_argument('proto',
> +                        default='http',
> +                        nargs='?',
> +                        help='protocol to serve (%s)' % protocols)
> +    parser.add_argument('-o','--port',
> +                        type=int,
> +                        action='store',
> +                        help='Port to connect on')
>      args = parser.parse_args()
>
>      if args.proto not in SERVERS:
> @@ -72,6 +78,9 @@ def main():
>      constructor = SERVERS[args.proto][0]
>      handler = SERVERS[args.proto][1]
>      port = SERVERS[args.proto][2]
> +    if args.port:
> +        port = args.port
> +
>      srv = constructor(('', port), handler)
>      srv.serve_forever()
>
> --
> 1.8.3.1
>
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
nickcooper-zhangtonghao Nov. 14, 2016, 9:59 a.m. UTC | #2
Hi Guru Shetty

Because of public ip restrictions , I have different private IPs sharing a single outbound internet IP.
The VMs with different private IPs will run different services(e.g. web, email, dns service).

I hope OVN will support that feature. The patches I submitted may be not good enough. If you have any idea,
let me know.


Thanks.
Nick

> On Nov 12, 2016, at 6:07 AM, Guru Shetty <guru@ovn.org> wrote:
> 
> Sorry for the delay in the reviews.
> I understand that we can extend the current DNAT feature to include DNAT:port. But is there is a use case where you want to use this? Any extensions of the NAT table can be better designed if we understand the end use-case for it. If not, I will just take a look at the first version of the series.
>
Gurucharan Shetty Nov. 14, 2016, 3:56 p.m. UTC | #3
On 14 November 2016 at 01:59, nickcooper-zhangtonghao <nic@opencloud.tech>
wrote:

> Hi Guru Shetty
>
> Because of public ip restrictions , I have different private IPs sharing a
> single outbound internet IP.
> The VMs with different private IPs will run different services(e.g. web,
> email, dns service).
>
> I hope OVN will support that feature. The patches I submitted may be not
> good enough. If you have any idea,
> let me know.
>

We do support it right now via LB right? i.e. one VIP:port =
PrivateIP:Port. With just one target, it acts the same as a DNAT.


>
>
> Thanks.
> Nick
>
> On Nov 12, 2016, at 6:07 AM, Guru Shetty <guru@ovn.org> wrote:
>
> Sorry for the delay in the reviews.
> I understand that we can extend the current DNAT feature to include
> DNAT:port. But is there is a use case where you want to use this? Any
> extensions of the NAT table can be better designed if we understand the end
> use-case for it. If not, I will just take a look at the first version of
> the series.
>
>
>
nickcooper-zhangtonghao Nov. 15, 2016, 2:30 p.m. UTC | #4
Hi Guru Shetty.
LB may support DNAT with port. But in some case, there is only a public ip address set on the gateway router, which is connected to multiple VMs. Thus this public ip address need to be projected to multiple private ip address. They denote a one to many relationship not one to one.

I don't have many public ip, so it is not possible to do VIP:port = PrivateIP:port. I hope that we can do VIP:port1 = PrivateIP1:port1, 
VIP:port2 = PrivateIP2:port2 ...

Thanks.
Nick

> On Nov 14, 2016, at 11:56 PM, Guru Shetty <guru@ovn.org> wrote:
> 
> We do support it right now via LB right? i.e. one VIP:port = PrivateIP:Port. With just one target, it acts the same as a DNAT.
Gurucharan Shetty Nov. 15, 2016, 3:58 p.m. UTC | #5
On 15 November 2016 at 06:30, nickcooper-zhangtonghao <nic@opencloud.tech>
wrote:

> Hi Guru Shetty.
> LB may support DNAT with port. But in some case, there is only a public ip
> address set on the gateway router, which is connected to multiple VMs. Thus
> this public ip address need to be projected to multiple private ip address.
> They denote a one to many relationship not one to one.
>
> I don't have many public ip, so it is not possible to do VIP:port =
> PrivateIP:port. I hope that we can do VIP:port1 = PrivateIP1:port1,
> VIP:port2 = PrivateIP2:port2 ...
>

We can do that with LB.



>
> Thanks.
> Nick
>
> On Nov 14, 2016, at 11:56 PM, Guru Shetty <guru@ovn.org> wrote:
>
> We do support it right now via LB right? i.e. one VIP:port =
> PrivateIP:Port. With just one target, it acts the same as a DNAT.
>
>
>
nickcooper-zhangtonghao Nov. 15, 2016, 10:21 p.m. UTC | #6
I got it. Thank you explaining this. Can you review the v1 of NAT command,
and other patches ? Thanks very much.

http://patchwork.ozlabs.org/patch/680778/ <http://patchwork.ozlabs.org/patch/680778/>
http://patchwork.ozlabs.org/patch/680777/ <http://patchwork.ozlabs.org/patch/680777/>
http://patchwork.ozlabs.org/patch/689737/ <http://patchwork.ozlabs.org/patch/689737/>

Thanks.
Nick

> On Nov 15, 2016, at 11:58 PM, Guru Shetty <guru@ovn.org> wrote:
> 
> We can do that with LB.
diff mbox

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 0bf6145..ce8aca5 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -170,6 +170,7 @@  struct ovnact_ct_commit {
 struct ovnact_ct_nat {
     struct ovnact ovnact;
     ovs_be32 ip;
+    uint16_t port;
     uint8_t ltable;             /* Logical table ID of next table. */
 };
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index df526c0..90c19f3 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -697,13 +697,21 @@  parse_ct_nat(struct action_context *ctx, const char *name,
 
     if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
         if (ctx->lexer->token.type != LEX_T_INTEGER
-            || ctx->lexer->token.format != LEX_F_IPV4) {
+            || mf_subvalue_width(&ctx->lexer->token.value) > 32) {
             lexer_syntax_error(ctx->lexer, "expecting IPv4 address");
             return;
         }
         cn->ip = ctx->lexer->token.value.ipv4;
         lexer_get(ctx->lexer);
 
+        /* Parse optional port. */
+        uint16_t port = 0;
+        if (lexer_match(ctx->lexer, LEX_T_COLON)
+            && !action_parse_port(ctx, &port)) {
+            return;
+        }
+        cn->port = port;
+
         if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
             return;
         }
@@ -779,6 +787,9 @@  encode_ct_nat(const struct ovnact_ct_nat *cn,
         if (snat) {
             nat->flags |= NX_NAT_F_SRC;
         } else {
+            if (cn->port) {
+                nat->range.proto.min = cn->port;
+            }
             nat->flags |= NX_NAT_F_DST;
         }
     }
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index df53d4c..f141462 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1191,6 +1191,16 @@  icmp4 {
 
       <li>
         For each configuration in the OVN Northbound database, that asks
+        to change the destination IP address <var>A</var> with a port
+        <var>P1</var> of protocol <var>P</var> to IP address <var>B</var>
+        and port <var>P2</var>, a priority-100 flow matches <code>ip &amp;&amp;
+        ip4.dst == <var>A</var> &amp;&amp; <var>P</var> &amp;&amp;
+        <var>P</var>.dst == <var>P1</var></code> with an action
+        <code>flags.loopback = 1; ct_dnat(<var>B</var>:<var>P2</var>);</code>.
+      </li>
+
+      <li>
+        For each configuration in the OVN Northbound database, that asks
         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>A</var></code> with an action
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index ad0739c..03bd9e4 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2252,7 +2252,7 @@  build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
  * 'ip_address'.  The caller must free() the memory allocated for
  * 'ip_address'. */
 static void
-ip_address_and_port_from_lb_key(const char *key, char **ip_address,
+ip_address_and_port(const char *key, char **ip_address,
                                 uint16_t *port)
 {
     char *ip_str, *start, *next;
@@ -2262,8 +2262,6 @@  ip_address_and_port_from_lb_key(const char *key, char **ip_address,
     next = start = xstrdup(key);
     ip_str = strsep(&next, ":");
     if (!ip_str || !ip_str[0]) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "bad ip address for load balancer key %s", key);
         free(start);
         return;
     }
@@ -2271,8 +2269,6 @@  ip_address_and_port_from_lb_key(const char *key, char **ip_address,
     ovs_be32 ip, mask;
     char *error = ip_parse_masked(ip_str, &ip, &mask);
     if (error || mask != OVS_BE32_MAX) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "bad ip address for load balancer key %s", key);
         free(start);
         free(error);
         return;
@@ -2281,8 +2277,6 @@  ip_address_and_port_from_lb_key(const char *key, char **ip_address,
     int l4_port = 0;
     if (next && next[0]) {
         if (!str_to_int(next, 0, &l4_port) || l4_port < 0 || l4_port > 65535) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-            VLOG_WARN_RL(&rl, "bad ip port for load balancer key %s", key);
             free(start);
             return;
         }
@@ -2313,8 +2307,11 @@  build_pre_lb(struct ovn_datapath *od, struct hmap *lflows)
             /* node->key contains IP:port or just IP. */
             char *ip_address = NULL;
             uint16_t port;
-            ip_address_and_port_from_lb_key(node->key, &ip_address, &port);
+            ip_address_and_port(node->key, &ip_address, &port);
             if (!ip_address) {
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+                VLOG_WARN_RL(&rl, "bad ip address or port for load balancer "
+                             "key %s", node->key);
                 continue;
             }
 
@@ -2719,8 +2716,11 @@  build_stateful(struct ovn_datapath *od, struct hmap *lflows)
 
             /* node->key contains IP:port or just IP. */
             char *ip_address = NULL;
-            ip_address_and_port_from_lb_key(node->key, &ip_address, &port);
+            ip_address_and_port(node->key, &ip_address, &port);
             if (!ip_address) {
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+                VLOG_WARN_RL(&rl, "bad ip address or port for load balancer "
+                             "key %s", node->key);
                 continue;
             }
 
@@ -3613,8 +3613,12 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                 char *ip_address = NULL;
                 uint16_t port;
 
-                ip_address_and_port_from_lb_key(node->key, &ip_address, &port);
+                ip_address_and_port(node->key, &ip_address, &port);
                 if (!ip_address) {
+                    static struct vlog_rate_limit rl =
+                        VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "bad ip address or port for load "
+                                 "balancer key %s", node->key);
                     continue;
                 }
 
@@ -3669,10 +3673,25 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
 
             ovs_be32 ip;
             if (!ip_parse(nat->external_ip, &ip) || !ip) {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad ip address %s in nat configuration "
-                             "for router %s", nat->external_ip, op->key);
-                continue;
+                bool is_external_port = false;
+                if (!strcmp(nat->type, "dnat")) {
+                    ovs_be16 port;
+                    char *error = ip_parse_port(nat->external_ip, &ip, &port);
+                    if (error) {
+                        free(error);
+                    } else {
+                        is_external_port = true;
+                    }
+                }
+
+                if (!is_external_port) {
+                    static struct vlog_rate_limit rl =
+                        VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "bad external ip address %s in nat "
+                                 "configuration for router %s",
+                                 nat->external_ip, op->key);
+                    continue;
+                }
             }
 
             if (!strcmp(nat->type, "snat")) {
@@ -3845,8 +3864,12 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
 
                 /* node->key contains IP:port or just IP. */
                 char *ip_address = NULL;
-                ip_address_and_port_from_lb_key(node->key, &ip_address, &port);
+                ip_address_and_port(node->key, &ip_address, &port);
                 if (!ip_address) {
+                    static struct vlog_rate_limit rl =
+                        VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "bad ip address or port for load "
+                                 "balancer key %s", node->key);
                     continue;
                 }
 
@@ -3905,37 +3928,78 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
 
             nat = od->nbr->nat[i];
 
-            ovs_be32 ip, mask;
-
-            char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
-            if (error || mask != OVS_BE32_MAX) {
+            uint16_t external_port = 0;
+            char *external_ip = NULL;
+            ip_address_and_port(nat->external_ip,
+                                &external_ip, &external_port);
+            if (!external_ip) {
                 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
                 VLOG_WARN_RL(&rl, "bad external ip %s for nat",
                              nat->external_ip);
-                free(error);
+                continue;
+            }
+
+            if (strcmp(nat->type, "dnat") && external_port) {
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+                VLOG_WARN_RL(&rl, "the external ip (%s) with port should not "
+                             "be configured for snat or dnat_and_snat",
+                             nat->external_ip);
+                free(external_ip);
                 continue;
             }
 
             /* Check the validity of nat->logical_ip. 'logical_ip' can
              * be a subnet when the type is "snat". */
-            error = ip_parse_masked(nat->logical_ip, &ip, &mask);
-            if (!strcmp(nat->type, "snat")) {
-                if (error) {
+            uint16_t logical_port = 0;
+            char *error, *logical_ip = NULL;
+            ovs_be32 ip, mask = OVS_BE32_MAX;
+            ip_address_and_port(nat->logical_ip,
+                                &logical_ip, &logical_port);
+            if (!logical_ip) {
+                if (!strcmp(nat->type, "snat")) {
+                    error = ip_parse_masked(nat->logical_ip, &ip, &mask);
+                    if (error) {
+                        static struct vlog_rate_limit rl =
+                            VLOG_RATE_LIMIT_INIT(5, 1);
+                        VLOG_WARN_RL(&rl, "bad logical ip network or ip %s "
+                                     "for snat in router "UUID_FMT"",
+                                     nat->logical_ip, UUID_ARGS(&od->key));
+                        free(external_ip);
+                        free(error);
+                        continue;
+                    }
+                } else {
                     static struct vlog_rate_limit rl =
                         VLOG_RATE_LIMIT_INIT(5, 1);
-                    VLOG_WARN_RL(&rl, "bad ip network or ip %s for snat "
-                                 "in router "UUID_FMT"",
+                    VLOG_WARN_RL(&rl, "bad logical ip %s for dnat or "
+                                 "dnat_and_snat in router "UUID_FMT"",
                                  nat->logical_ip, UUID_ARGS(&od->key));
-                    free(error);
+                    free(external_ip);
+                    continue;
+                }
+            }
+
+            if (!strcmp(nat->type, "dnat")) {
+                if ((external_port != 0 && logical_port == 0)
+                    || (external_port == 0 && logical_port != 0)) {
+                    static struct vlog_rate_limit rl =
+                        VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "the external/logical ip (%s, %s) with "
+                                 "port should be configured at same time for "
+                                 "dnat", nat->external_ip, nat->logical_ip);
+                    free(external_ip);
+                    free(logical_ip);
                     continue;
                 }
             } else {
-                if (error || mask != OVS_BE32_MAX) {
+                if (logical_port) {
                     static struct vlog_rate_limit rl =
                         VLOG_RATE_LIMIT_INIT(5, 1);
-                    VLOG_WARN_RL(&rl, "bad ip %s for dnat in router "
-                        ""UUID_FMT"", nat->logical_ip, UUID_ARGS(&od->key));
-                    free(error);
+                    VLOG_WARN_RL(&rl, "the logical ip (%s) with port should "
+                                 "not be configured for snat or dnat_and_snat",
+                                 nat->logical_ip);
+                    free(external_ip);
+                    free(logical_ip);
                     continue;
                 }
             }
@@ -3966,10 +4030,27 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                  * We need to zero the inport because the router can
                  * send the packet back through the same interface. */
                 ds_clear(&match);
-                ds_put_format(&match, "ip && ip4.dst == %s", nat->external_ip);
                 ds_clear(&actions);
-                ds_put_format(&actions,"flags.loopback = 1; ct_dnat(%s);",
-                              nat->logical_ip);
+                if (external_port && logical_port) {
+                    /* The external_port and logical port should be configured
+                     * only for DNAT.*/
+                    char *protocol = "tcp";
+                    if (nat->protocol && !strcmp(nat->protocol, "udp")) {
+                       protocol = "udp";
+                    }
+                    ds_put_format(&match, "ip && ip4.dst == %s "
+                                  "&& %s && %s.dst == %d",
+                                  external_ip, protocol,
+                                  protocol, external_port);
+                    ds_put_format(&actions,"flags.loopback = 1; "
+                                  "ct_dnat(%s:%d);",
+                                  logical_ip, logical_port);
+                } else {
+                    ds_put_format(&match, "ip && ip4.dst == %s",
+                                  external_ip);
+                    ds_put_format(&actions,"flags.loopback = 1; ct_dnat(%s);",
+                                  logical_ip);
+                }
                 ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 100,
                               ds_cstr(&match), ds_cstr(&actions));
             }
@@ -3991,6 +4072,8 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                               count_1bits(ntohl(mask)) + 1,
                               ds_cstr(&match), ds_cstr(&actions));
             }
+            free(external_ip);
+            free(logical_ip);
         }
 
         /* Re-circulate every packet through the DNAT zone.
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 865dd34..a290c67 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "5.4.0",
-    "cksum": "4176761817 11225",
+    "version": "5.4.1",
+    "cksum": "944843976 11424",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -203,6 +203,10 @@ 
             "columns": {
                 "external_ip": {"type": "string"},
                 "logical_ip": {"type": "string"},
+                "protocol": {
+                    "type": {"key": {"type": "string",
+                             "enum": ["set", ["tcp", "udp"]]},
+                             "min": 0, "max": 1}},
                 "type": {"type": {"key": {"type": "string",
                                            "enum": ["set", ["dnat",
                                                              "snat",
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index e1b3136..ec13278 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -1116,8 +1116,10 @@ 
       <ul>
         <li>
           When <ref column="type"/> is <code>dnat</code>, the externally
-          visible IP address <ref column="external_ip"/> is DNATted to the IP
-          address <ref column="logical_ip"/> in the logical space.
+          visible IP address (and an optional port number with <code>:</code>
+          as a separator) <ref column="external_ip"/> is DNATted to the IP
+          address (and an optional port number with <code>:</code> as a
+          separator) <ref column="logical_ip"/> in the logical space.
         </li>
         <li>
           When <ref column="type"/> is <code>snat</code>, IP packets
@@ -1138,11 +1140,23 @@ 
     </column>
 
     <column name="external_ip">
-      An IPv4 address.
+     An IPv4 address (and an optional port number with <code>:</code>
+     as a separator).
     </column>
 
     <column name="logical_ip">
-      An IPv4 network (e.g 192.168.1.0/24) or an IPv4 address.
+     An IPv4 network (e.g 192.168.1.0/24) or an IPv4 address (and an optional
+     port number with <code>:</code> as a separator).
+    </column>
+
+    <column name="protocol">
+     Valid protocols are <code>tcp</code> or <code>udp</code>.  This column
+     is useful when a port number is provided as part of the
+     <code>external_ip</code> column and the <code>type</code> is set
+     as <code>dnat</code>.  If this column is empty and a port number
+     is provided as part of <code>external_ip</code> column when
+     the <code>type</code> is set as <code>dnat</code>, OVN assumes the
+     <code>protocol</code> to be <code>tcp</code>.
     </column>
   </table>
 
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 21226d9..ba69df6 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -279,6 +279,119 @@  OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
 /connection dropped.*/d"])
 AT_CLEANUP
 
+AT_SETUP([ovn -- 2 LRs connected via LS, gateway router, DNAT with port])
+AT_KEYWORDS([ovnnat])
+
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+# Logical network:
+# Two LRs - R1 and R2 that are connected to each other via LS "join"
+# in 20.0.0.0/24 network. R1 has switchess foo (192.168.1.0/24) connected
+# to it.  R2 has alice (172.16.1.0/24) connected to it.
+# R2 is a gateway router on which we add NAT rules.
+#
+#    foo -- R1 -- join - R2 -- alice
+
+ovn-nbctl lr-add R1
+ovn-nbctl lr-add R2 -- set Logical_Router R2 options:chassis=hv1
+
+ovn-nbctl ls-add foo
+ovn-nbctl ls-add alice
+ovn-nbctl ls-add join
+
+ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
+ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
+ovn-nbctl lrp-add R1 R1_join 00:00:04:01:02:03 20.0.0.1/24
+ovn-nbctl lrp-add R2 R2_join 00:00:04:01:02:04 20.0.0.2/24
+
+# Connect foo to R1
+ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
+    type=router options:router-port=foo addresses=\"00:00:01:01:02:03\"
+
+# Connect alice to R2
+ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
+    type=router options:router-port=alice addresses=\"00:00:02:01:02:03\"
+
+# Connect R1 to join
+ovn-nbctl lsp-add join r1-join -- set Logical_Switch_Port r1-join \
+    type=router options:router-port=R1_join addresses='"00:00:04:01:02:03"'
+
+# Connect R2 to join
+ovn-nbctl lsp-add join r2-join -- set Logical_Switch_Port r2-join \
+    type=router options:router-port=R2_join addresses='"00:00:04:01:02:04"'
+
+# Static routes.
+ovn-nbctl lr-route-add R1 172.16.1.0/24 20.0.0.2
+ovn-nbctl lr-route-add R2 192.168.0.0/16 20.0.0.1
+
+# Logical port 'foo1' in switch 'foo'.
+ADD_NAMESPACES(foo1)
+ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
+         "192.168.1.1")
+ovn-nbctl lsp-add foo foo1 \
+-- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
+
+# Logical port 'alice1' in switch 'alice'.
+ADD_NAMESPACES(alice1)
+ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24", "f0:00:00:01:02:04", \
+         "172.16.1.1")
+ovn-nbctl lsp-add alice alice1 \
+-- lsp-set-addresses alice1 "f0:00:00:01:02:04 172.16.1.2"
+
+# Add a DNAT with port rule
+ovn-nbctl -- --id=@nat create nat type="dnat" protocol="tcp" logical_ip="192.168.1.2\:8080" \
+    external_ip="30.0.0.2\:80" -- add logical_router R2 nat @nat
+ovn-nbctl -- --id=@nat create nat type="dnat" protocol="udp" logical_ip="192.168.1.2\:8181" \
+    external_ip="30.0.0.3\:81" -- add logical_router R2 nat @nat
+
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep ct\( | grep nat])
+
+# South-North DNAT
+NETNS_DAEMONIZE([foo1], [[$PYTHON $srcdir/test-l7.py --port 8080]], [http0.pid])
+NS_CHECK_EXEC([alice1], [wget 30.0.0.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br-int ovs-alice1 normal '000002010203F0000001020408004500001C000100004011AFBBAC1001021E0000030035005100083443'])
+
+# We verify that DNAT indeed happened via 'dump-conntrack' command.
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+])
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.3) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+udp,orig=(src=172.16.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>
+])
+
+OVS_APP_EXIT_AND_WAIT([ovn-controller])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([ovn-northd])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+AT_CLEANUP
+
 AT_SETUP([ovn -- load-balancing])
 AT_KEYWORDS([ovnlb])
 
diff --git a/tests/test-l7.py b/tests/test-l7.py
index aed34f4..c6aae4e 100755
--- a/tests/test-l7.py
+++ b/tests/test-l7.py
@@ -61,8 +61,14 @@  def main():
     protocols = [srv for srv in SERVERS]
     parser = argparse.ArgumentParser(
             description='Run basic application servers.')
-    parser.add_argument('proto', default='http', nargs='?',
-            help='protocol to serve (%s)' % protocols)
+    parser.add_argument('proto',
+                        default='http',
+                        nargs='?',
+                        help='protocol to serve (%s)' % protocols)
+    parser.add_argument('-o','--port',
+                        type=int,
+                        action='store',
+                        help='Port to connect on')
     args = parser.parse_args()
 
     if args.proto not in SERVERS:
@@ -72,6 +78,9 @@  def main():
     constructor = SERVERS[args.proto][0]
     handler = SERVERS[args.proto][1]
     port = SERVERS[args.proto][2]
+    if args.port:
+        port = args.port
+
     srv = constructor(('', port), handler)
     srv.serve_forever()