[ovs-dev] [PATCH v1 1/3 ovn] NAT: Provide port range in input
Ankur Sharma
ankur.sharma at nutanix.com
Thu Apr 2 03:26:27 UTC 2020
Hi Mark,
Thanks a lot for review.
Please find my replies inline.
Regards,
Ankur
-----Original Message-----
From: dev <ovs-dev-bounces at openvswitch.org> On Behalf Of Mark Michelson
Sent: Monday, March 30, 2020 12:21 PM
To: svc.mail.git <svc.mail.git at nutanix.com>; ovs-dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1 1/3 ovn] NAT: Provide port range in input
On 3/27/20 7:18 PM, Ankur Sharma wrote:
> From: Ankur Sharma <ankur.sharma at 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 at nutanix.com>
> ---
> ovn-nb.ovsschema | 5 +++--
> ovn-nb.xml | 23 +++++++++++++++++++++++
> tests/ovn-nbctl.at | 20 ++++++++++++++++++++
> utilities/ovn-nbctl.c | 50 +++++++++++++++++++++++++++++++++++++-------------
> 4 files changed, 83 insertions(+), 15 deletions(-)
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index
> 843e979..c7c28fd 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> - "version": "5.20.0",
> - "cksum": "2846067333 25243",
> + "version": "5.20.1",
> + "cksum": "3966465842 25302",
> "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 f4ecc1c..70e0c65 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..34d75b7 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -604,6 +604,26 @@ 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_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 1-3000 lp0 00:00:00:04:05:06]) AT_CHECK([ovn-nbctl
> +--portrange lr-nat-add lr0 dnat_and_snat 40.0.0.6 192.168.1.9 1-3000
> +lp0], [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 1-3000 00:00:00:04:05:06], [1], [],
> +[ovn-nbctl: lr-nat-add with logical_port must also specify external_mac.
> +])
> +
> +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_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 e80058e..e84e1fc
> 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
> + [PORT_RANGE][LOGICAL_PORT EXTERNAL_MAC]\n\
A couple of suggestions:
* Refer to this as "EXTERNAL_PORT_RANGE" to be more clear.
[ANKUR]: Sure, v2 will have this change.
* Why is the port range this point in the command? I suggest either placing it directly after the EXTERNAL_IP argument or as the last argument. Placing it after EXTERNAL_IP groups it nicely with the other external option. Placing it at the end makes the code changes in ovn-nbctl much simpler. Placing it at the end would also get rid of the need for the --portrange option.
[ANKUR]: Port range is specified here, since all the optional arguments start after LOGICAL_IP. I thought of having it after EXTERNAL_IP, but did not do so, ideally LOGICAL_IP should be before EXTERNAL IP 😊 (to keep all the external arguments together), plus now we would have an optional argument b/w mandatory ones and that would complicate the implementation quite a bit. However, I agree that another option is placing it in the end. V2 will have this change. "--portrange" would be still needed, since all the arguments after LOGICAL_IP are optional. If there is only 1 argument after LOGICAL_IP, then --portrange helps in classifying if next argument is port_range or it is an invalid input.
> add a NAT to ROUTER\n\
> lr-nat-del ROUTER [TYPE [IP]]\n\
> remove NATs from ROUTER\n\ @@ -3969,6
> +3970,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) {
> @@ -4032,14 +4034,23 @@ 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 (is_portrange) {
> + port_range = ctx->argv[5];
> + }
You should perform some validation of the supplied port range here. If the port range is not valid (e.g. 9000-8000 or hello-goodbye), then print an error and do not set anything.
[ANKUR]: Sure, V2 will have this change.
> +
> 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) {
> +
This blank line looks weird once the patch is applied.
[ANKUR]: Sure, V2 will have this change.
> + if (!is_portrange) {
> + 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 (strcmp(nat_type, "dnat_and_snat")) {
> ctl_error(ctx, "logical_port and external_mac are only valid when "
> "type is \"dnat_and_snat\"."); @@ -4047,7
> +4058,14 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
> return;
> }
>
> - logical_port = ctx->argv[5];
> + 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[is_portrange ? 6: 5];
> const struct nbrec_logical_switch_port *lsp;
> error = lsp_by_name_or_uuid(ctx, logical_port, true, &lsp);
> if (error) {
> @@ -4056,7 +4074,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
> return;
> }
>
> - external_mac = ctx->argv[6];
> + external_mac = ctx->argv[is_portrange? 7: 6];
> struct eth_addr ea;
> if (!eth_addr_from_string(external_mac, &ea)) {
> ctl_error(ctx, "invalid mac address %s.", external_mac);
> @@ -4064,6 +4082,7 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
> return;
> }
> } else {
> + port_range = NULL;
> logical_port = NULL;
> external_mac = NULL;
> }
> @@ -4135,6 +4154,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);
>
> @@ -6131,9 +6154,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 [PORT_RANGE]"
> + "[LOGICAL_PORT EXTERNAL_MAC]", 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 },
>
_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=afd2cBkgXMqD1nfS4x2EJi0-wRcYWwiW4Z16wlDB-5k&s=IfJ8Pdxk0Ag9vhoumK94cIrXXX2lXdP7I_MrFbT-aCc&e=
More information about the dev
mailing list