[ovs-dev,v4,1/2,ovn] NAT: Provide port range in input

From: Ankur Sharma <ankur.sharma@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@nutanix.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
---
 ovn-nb.ovsschema      |   5 ++-
 ovn-nb.xml            |  23 +++++++++++
 tests/ovn-nbctl.at    |  46 +++++++++++++++++++++
 utilities/ovn-nbctl.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 169 insertions(+), 13 deletions(-)

Bleep bloop.  Greetings svc.mail.git, 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 100 characters long (recommended limit is 79)
WARNING: Line lacks whitespace around operator
#146 FILE: utilities/ovn-nbctl.c:706:
  lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP [LOGICAL_PORT EXTERNAL_MAC] [EXTERNAL_PORT_RANGE]\n\

Lines checked: 308, Warnings: 2, Errors: 0


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

Thanks,
0-day Robot

On Fri, Apr 3, 2020 at 12:39 AM Ankur Sharma <svc.mail.git@nutanix.com> wrote:
>
> From: Ankur Sharma <ankur.sharma@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@nutanix.com>
> Acked-by: Mark Michelson <mmichels@redhat.com>

Hi Ankur,
Few comments below

compilation is failing for this patch when confgured with
"--enable-Werror --enable-sparse"


> ---
>  ovn-nb.ovsschema      |   5 ++-
>  ovn-nb.xml            |  23 +++++++++++
>  tests/ovn-nbctl.at    |  46 +++++++++++++++++++++
>  utilities/ovn-nbctl.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 169 insertions(+), 13 deletions(-)
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index ea6f4e3..affa0ce 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.20.1",
> -    "cksum": "721375950 25251",
> +    "version": "5.20.2",

Since you're adding a new column the version should be bumped to "5.21.0"


> +    "cksum": "623498453 25310",
>      "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 541ec20..61c1ae8 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..39189fd 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -604,6 +604,52 @@ 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 40.0.0.5 192.168.1.10 1])
> +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 lp0 00:00:00:04:05:06 1-3000])
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 lp0 1-3000], [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 00:00:00:04:05:06 1-3000], [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.7 192.168.1.10 1-], [1], [],
> +[ovn-nbctl: invalid port range 1-.
> +])
> +
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 -300], [1], [],
> +[ovn-nbctl: invalid port range -300.
> +])
> +
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 500-300], [1], [],
> +[ovn-nbctl: invalid port range 500-300.
> +])
> +
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 a-300], [1], [],
> +[ovn-nbctl: invalid port range a-300.
> +])
> +
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 100-b], [1], [],
> +[ovn-nbctl: invalid port range 100-b.
> +])
> +
> +AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 0-10], [1], [],
> +[ovn-nbctl: invalid port range 0-10.
> +])
> +
> +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             40.0.0.5           192.168.1.10
> +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 59abe00..ab37b49 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 [LOGICAL_PORT EXTERNAL_MAC] [EXTERNAL_PORT_RANGE]\n\
>                              add a NAT to ROUTER\n\

Compilation is failing because of this when configured with
"--enable-Werrir --enable-sparse".

/bin/sh ./libtool  --tag=CC   --mode=compile env REAL_CC="gcc"
CHECK="sparse -Wsparse-error -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/include/sparse
-m64 -I /usr/local/include  " cgcc -target=x86_64 -DHAVE_CONFIG_H -I.
-I..   -I ../include  -I ../include -I ../ovn -I ./include -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/include -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc/include -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/lib -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc/lib -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc -I ../lib
-I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
-Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
-Wunused-parameter -Wbad-function-cast -Wcast-align
-Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
-Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
-Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
-Wshift-negative-value -Wduplicated-cond -Wshadow
-Wmultistatement-macros -Wcast-align=strict -Werror -Werror  -g -O2
-MT lib/ovn-nb-idl.lo -MD -MP -MF $depbase.Tpo -c -o lib/ovn-nb-idl.lo
lib/ovn-nb-idl.c &&\
mv -f $depbase.Tpo $depbase.Plo
../utilities/ovn-nbctl.c:752:55: error: string too long (8208 bytes,
8191 bytes max)
make[1]: *** [Makefile:2053: utilities/ovn-nbctl.o] Error 1
make[1]: *** Waiting for unfinished jobs....


Thanks
Numan

>    lr-nat-del ROUTER [TYPE [IP]]\n\
>                              remove NATs from ROUTER\n\
> @@ -3963,6 +3964,56 @@ out:
>      free(nexthop);
>  }
>
> +static bool
> +is_valid_port_range(const char *port_range)
> +{
> +    int range_lo_int, range_hi_int;
> +    bool ret = false;
> +
> +    if (!port_range) {
> +        return false;
> +    }
> +
> +    char *save_ptr = NULL;
> +    char *tokstr = xstrdup(port_range);
> +    char *range_lo = strtok_r(tokstr, "-", &save_ptr);
> +    if (!range_lo) {
> +        goto done;
> +    }
> +    range_lo_int = strtol(range_lo, NULL, 10);
> +    if (errno == EINVAL || range_lo_int <= 0) {
> +        goto done;
> +    }
> +
> +    if (!strchr(port_range, '-')) {
> +        ret = true;
> +        goto done;
> +    }
> +
> +    char *range_hi = strtok_r(NULL, "", &save_ptr);
> +    if (!range_hi) {
> +        goto done;
> +    }
> +    range_hi_int = strtol(range_hi, NULL, 10);
> +    if (errno == EINVAL) {
> +        goto done;
> +    }
> +
> +    if (range_lo_int >= range_hi_int) {
> +        goto done;
> +    }
> +
> +    if (range_lo_int <= 0 || range_hi_int > 65535) {
> +        goto done;
> +    }
> +
> +    ret = true;
> +
> +done:
> +    free(tokstr);
> +    return ret;
> +}
> +
>  static void
>  nbctl_lr_nat_add(struct ctl_context *ctx)
>  {
> @@ -3971,6 +4022,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) {
> @@ -4034,14 +4086,25 @@ 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 (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) {
> +        if (!is_portrange) {
> +            ctl_error(ctx, "lr-nat-add with logical_port "
> +                      "must also specify external_mac.");
> +            free(new_logical_ip);
> +            return;
> +        }
> +        port_range = ctx->argv[5];
> +        if (!is_valid_port_range(port_range)) {
> +            ctl_error(ctx, "invalid port range %s.", port_range);
> +            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\".");
> @@ -4049,6 +4112,13 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>              return;
>          }
>
> +        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[5];
>          const struct nbrec_logical_switch_port *lsp;
>          error = lsp_by_name_or_uuid(ctx, logical_port, true, &lsp);
> @@ -4065,7 +4135,18 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
>              free(new_logical_ip);
>              return;
>          }
> +
> +        if (ctx->argc > 7) {
> +            port_range = ctx->argv[7];
> +            if (!is_valid_port_range(port_range)) {
> +                ctl_error(ctx, "invalid port range %s.", port_range);
> +                free(new_logical_ip);
> +                return;
> +            }
> +        }
> +
>      } else {
> +        port_range = NULL;
>          logical_port = NULL;
>          external_mac = NULL;
>      }
> @@ -4137,6 +4218,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);
>
> @@ -6133,9 +6218,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"
> +      "[LOGICAL_PORT EXTERNAL_MAC] [EXTERNAL_PORT_RANGE]", 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 },
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Hi Numan,

Thanks a lot for the feedback.
Addressed the schema versioning in V5.

Regarding the error seen. I tried with sparse and wError on my machine.
I tried ./.travis/linux-build.sh as well, but compilation went through.

Looking at the error, looks like it is complaining about string length to printf. Tried fixing it in V5.
But i am not able to validate it.

Please let me know, if V5 shows same issue.

Regards,
Ankur