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

From: Ankur Sharma <ankur.sharma@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@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(-)

Bleep bloop.  Greetings Ankur Sharma, 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:
ERROR: Improper whitespace around control block
#168 FILE: utilities/ovn-nbctl.c:860:
        NBREC_ADDRESS_SET_FOR_EACH(iter, ctx->idl) {

Lines checked: 271, Warnings: 0, Errors: 1


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

Thanks,
0-day Robot

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@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@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 },
>

Hi Mark,

Thanks a lot for the feedback.

a. Address Set is used to make sure that we don't have to configure common set of endpoint ips again and again. In a deployment, peered physical subnets will be common across all the logical routers, hence using address set looked better. As more IPs are allowed/disallowed, we dont have to update multiple logical routers.
However, i am fine with moving to arbitrary list, if that is preferred, please let me know your thoughts.

b. Agree with better naming feedback ?. Let me know if you have some preference, else i will try to come up with something better in V3.

Will address rest of the comments in V3.

Regards,
Ankur

Hi Mark,

Submitted V3.

a. As of now, i have kept the usage of Address Set (based on the use case called out). However, please feel free to let me know, if you prefer moving to an ip address list.
b. Renamed  the columns to "applied_ext_ips" and "exempted_ext_ips".
c. Improved the documentation and addressed the review comments.

Please take a look and let me know, if additional changes are needed.

Thanks a lot for review.

Regards,
Ankur