[ovs-dev] [PATCH v1 1/3 ovn] NAT: Provide port range in input

Mark Michelson mmichels at redhat.com
Mon Mar 30 19:21:18 UTC 2020


On 3/27/20 7:18 PM, Ankur Sharma wrote:
> From: Ankur Sharma <ankur.sharma at nutanix.com>
> 
> This patch enhances the NB OVSSCHEMA to
> add an additional comuln in NAT table.
> 
> external_port_range: Specifies the range of port numbers
>                       to translate source port to.
> 
> Changes also add corresponding ovn-nbctl cli.
> 
> Signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>
> ---
>   ovn-nb.ovsschema      |  5 +++--
>   ovn-nb.xml            | 23 +++++++++++++++++++++++
>   tests/ovn-nbctl.at    | 20 ++++++++++++++++++++
>   utilities/ovn-nbctl.c | 50 +++++++++++++++++++++++++++++++++++++-------------
>   4 files changed, 83 insertions(+), 15 deletions(-)
> 
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 843e979..c7c28fd 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
> -    "version": "5.20.0",
> -    "cksum": "2846067333 25243",
> +    "version": "5.20.1",
> +    "cksum": "3966465842 25302",
>       "tables": {
>           "NB_Global": {
>               "columns": {
> @@ -379,6 +379,7 @@
>                   "external_ip": {"type": "string"},
>                   "external_mac": {"type": {"key": "string",
>                                             "min": 0, "max": 1}},
> +                "external_port_range": {"type": "string"},
>                   "logical_ip": {"type": "string"},
>                   "logical_port": {"type": {"key": "string",
>                                             "min": 0, "max": 1}},
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index f4ecc1c..70e0c65 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2552,6 +2552,29 @@
>         </p>
>       </column>
>   
> +    <column name="external_port_range">
> +      <p>
> +        L4 source port range
> +      </p>
> +
> +      <p>
> +        Range of port, from which a port number will be picked that will
> +        replace the source port of to be NATed packet. This is basically
> +        PAT (port address translation).
> +      </p>
> +
> +      <p>
> +        Value of the column is in the format, port_lo-port_hi.
> +        For example:
> +        external_port_range : "1-30000"
> +      </p>
> +
> +      <p>
> +        Valid range of ports is 1-65535.
> +      </p>
> +
> +    </column>
> +
>       <column name="logical_ip">
>         An IPv4 network (e.g 192.168.1.0/24) or an IPv4 address.
>       </column>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index fcb6ad7..34d75b7 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -604,6 +604,26 @@ snat             fd01::1            fd11::/64
>   ])
>   
>   AT_CHECK([ovn-nbctl lr-nat-del lr0])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 snat 40.0.0.3 192.168.1.6 1-3000])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat 40.0.0.4 192.168.1.7 1-3000])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.5 192.168.1.8 1-3000])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 1-3000 lp0 00:00:00:04:05:06])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 1-3000 lp0], [1], [],
> +[ovn-nbctl: lr-nat-add with logical_port must also specify external_mac.
> +])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 1-3000 00:00:00:04:05:06], [1], [],
> +[ovn-nbctl: lr-nat-add with logical_port must also specify external_mac.
> +])
> +
> +AT_CHECK([ovn-nbctl lr-nat-list lr0], [0], [dnl
> +TYPE             EXTERNAL_IP        LOGICAL_IP            EXTERNAL_MAC         LOGICAL_PORT
> +dnat             40.0.0.4           192.168.1.7
> +dnat_and_snat    40.0.0.5           192.168.1.8
> +dnat_and_snat    40.0.0.6           192.168.1.9           00:00:00:04:05:06    lp0
> +snat             40.0.0.3           192.168.1.6
> +])
> +
> +AT_CHECK([ovn-nbctl lr-nat-del lr0])
>   AT_CHECK([ovn-nbctl lr-nat-list lr0], [0], [])
>   AT_CHECK([ovn-nbctl lr-nat-del lr0])
>   AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat])])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index e80058e..e84e1fc 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -702,7 +702,8 @@ Policy commands:\n\
>   \n\
>   NAT commands:\n\
>     [--stateless]\n\
> -  lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP [LOGICAL_PORT EXTERNAL_MAC]\n\
> +  [--portrange]\n\
> +  lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP [PORT_RANGE][LOGICAL_PORT EXTERNAL_MAC]\n\

A couple of suggestions:
* Refer to this as "EXTERNAL_PORT_RANGE" to be more clear.
* Why is the port range  this point in the command? I suggest either 
placing it directly after the EXTERNAL_IP argument or as the last 
argument. Placing it after EXTERNAL_IP groups it nicely with the other 
external option. Placing it at the end makes the code changes in 
ovn-nbctl much simpler. Placing it at the end would also get rid of the 
need for the --portrange option.

>                               add a NAT to ROUTER\n\
>     lr-nat-del ROUTER [TYPE [IP]]\n\
>                               remove NATs from ROUTER\n\
> @@ -3969,6 +3970,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>       const char *external_ip = ctx->argv[3];
>       const char *logical_ip = ctx->argv[4];
>       char *new_logical_ip = NULL;
> +    bool is_portrange = shash_find(&ctx->options, "--portrange") != NULL;
>   
>       char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
>       if (error) {
> @@ -4032,14 +4034,23 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>           }
>       }
>   
> -    const char *logical_port;
> -    const char *external_mac;
> +    const char *logical_port = NULL;
> +    const char *external_mac = NULL;
> +    const char *port_range = NULL;
> +
> +    if (is_portrange) {
> +        port_range = ctx->argv[5];
> +    }

You should perform some validation of the supplied port range here. If 
the port range is not valid (e.g. 9000-8000 or hello-goodbye), then 
print an error and do not set anything.

> +
>       if (ctx->argc == 6) {
> -        ctl_error(ctx, "lr-nat-add with logical_port "
> -                  "must also specify external_mac.");
> -        free(new_logical_ip);
> -        return;
> -    } else if (ctx->argc == 7) {
> +

This blank line looks weird once the patch is applied.

> +        if (!is_portrange) {
> +            ctl_error(ctx, "lr-nat-add with logical_port "
> +                      "must also specify external_mac.");
> +            free(new_logical_ip);
> +            return;
> +        }
> +    } else if (ctx->argc >= 7) {
>           if (strcmp(nat_type, "dnat_and_snat")) {
>               ctl_error(ctx, "logical_port and external_mac are only valid when "
>                         "type is \"dnat_and_snat\".");
> @@ -4047,7 +4058,14 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>               return;
>           }
>   
> -        logical_port = ctx->argv[5];
> +        if (ctx->argc == 7 && is_portrange) {
> +            ctl_error(ctx, "lr-nat-add with logical_port "
> +                      "must also specify external_mac.");
> +            free(new_logical_ip);
> +            return;
> +        }
> +
> +        logical_port = ctx->argv[is_portrange ? 6: 5];
>           const struct nbrec_logical_switch_port *lsp;
>           error = lsp_by_name_or_uuid(ctx, logical_port, true, &lsp);
>           if (error) {
> @@ -4056,7 +4074,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>               return;
>           }
>   
> -        external_mac = ctx->argv[6];
> +        external_mac = ctx->argv[is_portrange? 7: 6];
>           struct eth_addr ea;
>           if (!eth_addr_from_string(external_mac, &ea)) {
>               ctl_error(ctx, "invalid mac address %s.", external_mac);
> @@ -4064,6 +4082,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>               return;
>           }
>       } else {
> +        port_range = NULL;
>           logical_port = NULL;
>           external_mac = NULL;
>       }
> @@ -4135,6 +4154,10 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>           nbrec_nat_set_external_mac(nat, external_mac);
>       }
>   
> +    if (port_range) {
> +        nbrec_nat_set_external_port_range(nat, port_range);
> +    }
> +
>       smap_add(&nat_options, "stateless", stateless ? "true":"false");
>       nbrec_nat_set_options(nat, &nat_options);
>   
> @@ -6131,9 +6154,10 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>          "", RO },
>   
>       /* NAT commands. */
> -    { "lr-nat-add", 4, 6,
> -      "ROUTER TYPE EXTERNAL_IP LOGICAL_IP [LOGICAL_PORT EXTERNAL_MAC]", NULL,
> -      nbctl_lr_nat_add, NULL, "--may-exist,--stateless", RW },
> +    { "lr-nat-add", 4, 7,
> +      "ROUTER TYPE EXTERNAL_IP LOGICAL_IP [PORT_RANGE]"
> +      "[LOGICAL_PORT EXTERNAL_MAC]", NULL,
> +      nbctl_lr_nat_add, NULL, "--may-exist,--stateless,--portrange", RW },
>       { "lr-nat-del", 1, 3, "ROUTER [TYPE [IP]]", NULL,
>           nbctl_lr_nat_del, NULL, "--if-exists", RW },
>       { "lr-nat-list", 1, 1, "ROUTER", NULL, nbctl_lr_nat_list, NULL, "", RO },
> 



More information about the dev mailing list