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
________________________________
From: Mark Michelson <mmich...@redhat.com>
Sent: Monday, June 29, 2020 11:33 AM
To: svc.mail.git <svc.mail....@nutanix.com>; ovs-dev@openvswitch.org
<ovs-dev@openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v2 1/2 ovn] External IP based NAT: Add Columns
and CLI
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.sha...@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.sha...@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://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_project_openvswitch_list_-3Fseries-3D185623&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=gdsHrMKI-VOHoN1ziUyqlIGzgVzIjMMz-NHIsocZYPE&m=mIhp3u3VnA8VgRTwXI7aUiC2wpKcWazR7pSdQUpgCoQ&s=SFjyFiER6NYS1yXsO1TYbJxXmKvoZJhgiFLOr1hrsTI&e=
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 },
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
