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://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 },
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
