[ovs-dev] [PATCH v2 1/2 ovn] External IP based NAT: Add Columns and CLI

Mark Michelson mmichels at redhat.com
Mon Jun 29 18:33:35 UTC 2020


Hi Ankur,

Why is it required to use an address set here? Typically in the 
northbound database, we allow for an arbitrary list of IP addresses to 
be accepted, rather than requiring the use of an address set.

We already use the term "external IP" in NAT documentation, so I think 
the changes in ovn-nb.xml could use some elaboration. If I could think 
of a better name for this new feature, I would :). Mention that for 
SNAT, it refers to the destination address, rather than the NATted 
source address, and that for DNAT, it refers to the source address 
rather than the pre-NATted destination address.

Also see below for a couple more comments.

On 6/28/20 9:34 PM, Ankur Sharma wrote:
> From: Ankur Sharma <ankur.sharma at nutanix.com>
> 
> This patch adds following columns to NAT table.
> 
> a. allowed_external_ip:
>     Represents Address Set of External IPs for which
>     a NAT rule is applicable.
> 
> b. disallowed_external_ip
>     Represents Address Set of External IPs for which
>     a NAT rule is NOT applicable.
> 
> Additionally, patch adds nbctl cli to set these column values.
> ovn-nbctl [--is-allowed] lr-nat-update-ext-ip
> 
> Signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>
> ---
>   ovn-nb.ovsschema      |  14 ++++++-
>   ovn-nb.xml            |  24 ++++++++++++
>   tests/ovn-nbctl.at    |  37 +++++++++++++++++-
>   utilities/ovn-nbctl.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++-
>   4 files changed, 173 insertions(+), 4 deletions(-)
> 
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index da9af71..8688ae0 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
> -    "version": "5.24.0",
> -    "cksum": "1092394564 25961",
> +    "version": "5.24.1",
> +    "cksum": "2114041852 26430",
>       "tables": {
>           "NB_Global": {
>               "columns": {
> @@ -400,6 +400,16 @@
>                                                                "snat",
>                                                                "dnat_and_snat"
>                                                                  ]]}}},
> +                "allowed_external_ip": {"type": {
> +                    "key": {"type": "uuid", "refTable": "Address_Set",
> +                            "refType": "strong"},
> +                    "min": 0,
> +                    "max": 1}},
> +                "disallowed_external_ip": {"type": {
> +                    "key": {"type": "uuid", "refTable": "Address_Set",
> +                            "refType": "strong"},
> +                    "min": 0,
> +                    "max": 1}},
>                   "options": {"type": {"key": "string", "value": "string",
>                                        "min": 0, "max": "unlimited"}},
>                   "external_ids": {
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 6ac178b..d2d0b25 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2658,6 +2658,30 @@
>         </p>
>       </column>
>   
> +    <column name="allowed_external_ip">
> +      It represents Address Set of external ips that NAT rule is applicable to.
> +
> +      <p>
> +        This configuration overrides the default NAT behavior of applying a
> +        rule solely based on internal IP.
> +      </p>
> +    </column>
> +
> +    <column name="disallowed_external_ip">
> +      It represents Address Set of external ips that NAT rule is NOT
> +      applicable to.
> +      <p>
> +        This configuration overrides the default NAT behavior of applying a
> +        rule solely based on internal IP.
> +      </p>
> +
> +      <p>
> +        "allowed_external_ip" and "disallowed_external_ip" are mutually
> +        exclusive to each other. If both Address Sets are set for a rule,
> +        then the NAT rule is not applied.
> +      </p>
> +    </column>
> +
>       <column name="options" key="stateless">
>         Indicates if a dnat_and_snat rule should lead to connection
>         tracking state or not.
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 6d66087..613d297 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -685,7 +685,42 @@ snat             40.0.0.3           21-65535         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])])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat])
> +
> +AT_CHECK([ovn-nbctl lr-nat-del lr0])
> +
> +ovn-nbctl create Address_Set name=allowed_range addresses=\"1.1.1.1\"
> +ovn-nbctl create Address_Set name=disallowed_range addresses=\"2.2.2.2\"
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 40.0.0.3 192.168.1.6])
> +AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 snat 192.168.1.6 disallowed_range])
> +AT_CHECK([ovn-nbctl --is-allowed lr-nat-update-ext-ip lr0 snat 192.168.1.6 allowed_range])
> +AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 snat 192.168.1.6 disallowed_range_tmp], [1], [],
> +[ovn-nbctl: disallowed_range_tmp: Address Set name not found
> +])
> +AT_CHECK([ovn-nbctl --is-allowed lr-nat-update-ext-ip lr0 snat 192.168.1.6 allowed_range_tmp], [1], [],
> +[ovn-nbctl: allowed_range_tmp: Address Set name not found
> +])
> +
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 40.0.0.4 192.168.1.7])
> +AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 dnat 40.0.0.4 disallowed_range])
> +AT_CHECK([ovn-nbctl --is-allowed lr-nat-update-ext-ip lr0 dnat 40.0.0.4 allowed_range])
> +AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 dnat 40.0.0.4 disallowed_range_tmp], [1], [],
> +[ovn-nbctl: disallowed_range_tmp: Address Set name not found
> +])
> +AT_CHECK([ovn-nbctl --is-allowed lr-nat-update-ext-ip lr0 dnat 40.0.0.4 allowed_range_tmp], [1], [],
> +[ovn-nbctl: allowed_range_tmp: Address Set name not found
> +])
> +
> +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat_and_snat 40.0.0.5 192.168.1.8])
> +AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 dnat_and_snat 40.0.0.5 disallowed_range])
> +AT_CHECK([ovn-nbctl --is-allowed lr-nat-update-ext-ip lr0 dnat 40.0.0.4 allowed_range])
> +AT_CHECK([ovn-nbctl lr-nat-update-ext-ip lr0 dnat 40.0.0.4 disallowed_range_tmp], [1], [],
> +[ovn-nbctl: disallowed_range_tmp: Address Set name not found
> +])
> +AT_CHECK([ovn-nbctl --is-allowed lr-nat-update-ext-ip lr0 dnat 40.0.0.4 allowed_range_tmp], [1], [],
> +[ovn-nbctl: allowed_range_tmp: Address Set name not found
> +])
> +AT_CHECK([ovn-nbctl lr-nat-del lr0])])
>   
>   dnl ---------------------------------------------------------------------
>   
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 7578b99..868cfb1 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -838,6 +838,46 @@ lr_by_name_or_uuid(struct ctl_context *ctx, const char *id,
>       return NULL;
>   }
>   
> +/* Find an Address Set given its id. */
> +static char * OVS_WARN_UNUSED_RESULT
> +address_set_by_name_or_uuid(struct ctl_context *ctx,
> +                            const char *id, bool must_exist,
> +                            const struct nbrec_address_set **addr_set_p)
> +{
> +    const struct nbrec_address_set *addr_set = NULL;
> +    bool is_uuid = false;
> +    struct uuid addr_set_uuid;
> +
> +    *addr_set_p = NULL;
> +    if (uuid_from_string(&addr_set_uuid, id)) {
> +        is_uuid = true;
> +        addr_set = nbrec_address_set_get_for_uuid(ctx->idl, &addr_set_uuid);
> +    }
> +
> +    if (!addr_set) {
> +        const struct nbrec_address_set *iter;
> +
> +        NBREC_ADDRESS_SET_FOR_EACH(iter, ctx->idl) {
> +            if (strcmp(iter->name, id)) {
> +                continue;
> +            }
> +            if (addr_set) {
> +                return xasprintf("Multiple Address Sets named '%s'.  "
> +                                 "Use a UUID.", id);
> +            }
> +            addr_set = iter;
> +        }
> +    }
> +
> +    if (!addr_set && must_exist) {
> +        return xasprintf("%s: Address Set %s not found",
> +                         id, is_uuid ? "UUID" : "name");
> +    }
> +
> +    *addr_set_p = addr_set;
> +    return NULL;
> +}
> +
>   static char * OVS_WARN_UNUSED_RESULT
>   ls_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist,
>                      const struct nbrec_logical_switch **ls_p)
> @@ -4503,6 +4543,65 @@ nbctl_lr_nat_list(struct ctl_context *ctx)
>       smap_destroy(&lr_nats);
>   }
>   
> +static void
> +nbctl_lr_nat_set_ext_ips(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    const struct nbrec_address_set *addr_set = NULL;
> +    bool is_allowed = shash_find(&ctx->options, "--is-allowed");
> +    bool nat_found = false;
> +
> +    if (ctx->argc < 5) {
> +        ctl_error(ctx, "Incomplete input");

This message could be improved to print the required arguments.

> +        return;
> +    }
> +
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const char *nat_type = ctx->argv[2];
> +    if (strcmp(nat_type, "dnat") && strcmp(nat_type, "snat")
> +            && strcmp(nat_type, "dnat_and_snat")) {
> +        ctl_error(ctx, "%s: type must be one of \"dnat\", \"snat\" and "
> +                  "\"dnat_and_snat\".", nat_type);
> +        return;
> +    }
> +
> +    error = address_set_by_name_or_uuid(ctx, ctx->argv[4], true, &addr_set);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const char *nat_ip = ctx->argv[3];
> +    int is_snat = !strcmp("snat", nat_type);
> +
> +    /* Update the matching NAT. */
> +    for (size_t i = 0; i < lr->n_nat; i++) {
> +        struct nbrec_nat *nat = lr->nat[i];
> +        if (!strcmp(nat_type, nat->type) &&
> +             !strcmp(nat_ip, is_snat ? nat->logical_ip : nat->external_ip)) {

There has been an effort to ensure IP address comparisons use normalized 
IP address strings.

See series 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=185623 for 
a better idea of what I'm talking about.

> +            nat_found = true;
> +            nbrec_logical_router_verify_nat(lr);
> +            if (is_allowed) {
> +                nbrec_nat_set_allowed_external_ip(nat, addr_set);
> +            } else {
> +                nbrec_nat_set_disallowed_external_ip(nat, addr_set);
> +            }
> +            return;
> +        }
> +    }
> +
> +    if (!nat_found) {
> +        ctl_error(ctx, "%s: Could not locate nat rule for: %s.",
> +                  nat_type, nat_ip);
> +        return;
> +    }
> +}
> +
>   

>   static char * OVS_WARN_UNUSED_RESULT
>   lrp_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist,
> @@ -6413,7 +6512,8 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>       { "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 },
> -
> +    { "lr-nat-update-ext-ip", 4, 4, "ROUTER TYPE IP ADDRESS_SET", NULL,
> +      nbctl_lr_nat_set_ext_ips, NULL, "--is-allowed", RW},
>       /* load balancer commands. */
>       { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]", NULL,
>         nbctl_lb_add, NULL, "--may-exist,--add-duplicate", RW },
> 



More information about the dev mailing list