Re: [ovs-dev] [PATCH v1 3/3 ovn] NAT: Parsing port_range options in ct_*nat actions
Hi Mark,
Thanks a lot for review.
Addressed all the comments.
Please take a look at V2.
Regards,
Ankur
-Original Message-
From: dev On Behalf Of Mark Michelson
Sent: Monday, March 30, 2020 12:21 PM
To: svc.mail.git ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1 3/3 ovn] NAT: Parsing port_range options in
ct_*nat actions
This and patch 2 should be combined into one patch. Patch 2 in isolation adds
invalid ct_snat and ct_dnat commands since the code for encoding them is not
added until patch 3.
See below for additional in-line comments.
On 3/27/20 7:18 PM, Ankur Sharma wrote:
> From: Ankur Sharma
>
> Changes to parse the logical flow, which specifies port_range for
> ct_nat action.
>
> Example logical flow:
> ct_snat(10.15.24.135,1-3)
>
> Signed-off-by: Ankur Sharma
> ---
> include/ovn/actions.h | 7 +++
> include/ovn/lex.h | 1 +
> lib/actions.c | 39 +++
> lib/lex.c | 5 -
> tests/ovn.at | 24 ++--
> 5 files changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h index
> 9b01492..2cec369 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -233,6 +233,13 @@ struct ovnact_ct_nat {
> struct in6_addr ipv6;
> ovs_be32 ipv4;
> };
> +
> +struct {
> + bool exists;
> + uint16_t port_lo;
> + uint16_t port_hi;
> +} port_range;
> +
> uint8_t ltable; /* Logical table ID of next table. */
> };
>
> diff --git a/include/ovn/lex.h b/include/ovn/lex.h index
> 8d55857..1da6ccc 100644
> --- a/include/ovn/lex.h
> +++ b/include/ovn/lex.h
> @@ -63,6 +63,7 @@ enum lex_type {
> LEX_T_EXCHANGE, /* <-> */
> LEX_T_DECREMENT,/* -- */
> LEX_T_COLON,/* : */
> +LEX_T_HYPHEN, /* - */
> };
>
> /* Subtype for LEX_T_INTEGER and LEX_T_MASKED_INTEGER tokens.
> diff --git a/lib/actions.c b/lib/actions.c index f22acdd..07fbed8
> 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -770,6 +770,33 @@ parse_ct_nat(struct action_context *ctx, const char
> *name,
> }
> lexer_get(ctx->lexer);
>
> +if (lexer_match(ctx->lexer, LEX_T_COMMA)) {
> +
> + if (ctx->lexer->token.type != LEX_T_INTEGER ||
> + ctx->lexer->token.format != LEX_F_DECIMAL) {
> + lexer_syntax_error(ctx->lexer, "expecting Integer for port "
> + "range");
> + }
> +
> + cn->port_range.port_lo = ntohll(ctx->lexer->token.value.integer);
> + lexer_get(ctx->lexer);
> +
> + if (!lexer_match(ctx->lexer, LEX_T_HYPHEN)) {
> + lexer_syntax_error(ctx->lexer, "expecting - for port range");
> + return;
> + }
> +
> + if (ctx->lexer->token.type != LEX_T_INTEGER) {
> + lexer_syntax_error(ctx->lexer, "expecting Integer for port "
> + "range");
> + }
> +
> + cn->port_range.port_hi =
> + ntohll(ctx->lexer->token.value.integer);
You should probably ensure that cn->port_range.port_hi is greater than
cn->port_range.port_lo.
> + lexer_get(ctx->lexer);
> +
> + cn->port_range.exists = true;
> +}
> +
> if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
> return;
> }
> @@ -799,6 +826,13 @@ format_ct_nat(const struct ovnact_ct_nat *cn, const char
> *name, struct ds *s)
> ipv6_format_addr(>ipv6, s);
> ds_put_char(s, ')');
> }
> +
> +if (cn->port_range.exists) {
> +ds_chomp(s, ')');
> +ds_put_format(s, ",%d-%d)", cn->port_range.port_lo,
> + cn->port_range.port_hi);
> +}
> +
> ds_put_char(s, ';');
> }
>
> @@ -861,6 +895,11 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
> }
> }
>
> +if (cn->port_range.exists) {
> + nat->range.proto.min = cn->port_range.port_lo;
> + nat->range.proto.max = cn->port_range.port_hi;
> +}
> +
> ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> ct = ofpacts->header;
> if (cn->family == AF_INET || cn->family == AF_INET6) { diff
> --git a/lib/lex.c b/lib/lex.c index 7a2ab41..94f6c77 100644
> --- a/lib/lex.c
> +++ b/lib/lex.c
Re: [ovs-dev] [PATCH v1 3/3 ovn] NAT: Parsing port_range options in ct_*nat actions
This and patch 2 should be combined into one patch. Patch 2 in isolation
adds invalid ct_snat and ct_dnat commands since the code for encoding
them is not added until patch 3.
See below for additional in-line comments.
On 3/27/20 7:18 PM, Ankur Sharma wrote:
From: Ankur Sharma
Changes to parse the logical flow, which specifies port_range
for ct_nat action.
Example logical flow:
ct_snat(10.15.24.135,1-3)
Signed-off-by: Ankur Sharma
---
include/ovn/actions.h | 7 +++
include/ovn/lex.h | 1 +
lib/actions.c | 39 +++
lib/lex.c | 5 -
tests/ovn.at | 24 ++--
5 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 9b01492..2cec369 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -233,6 +233,13 @@ struct ovnact_ct_nat {
struct in6_addr ipv6;
ovs_be32 ipv4;
};
+
+struct {
+ bool exists;
+ uint16_t port_lo;
+ uint16_t port_hi;
+} port_range;
+
uint8_t ltable; /* Logical table ID of next table. */
};
diff --git a/include/ovn/lex.h b/include/ovn/lex.h
index 8d55857..1da6ccc 100644
--- a/include/ovn/lex.h
+++ b/include/ovn/lex.h
@@ -63,6 +63,7 @@ enum lex_type {
LEX_T_EXCHANGE, /* <-> */
LEX_T_DECREMENT,/* -- */
LEX_T_COLON,/* : */
+LEX_T_HYPHEN, /* - */
};
/* Subtype for LEX_T_INTEGER and LEX_T_MASKED_INTEGER tokens.
diff --git a/lib/actions.c b/lib/actions.c
index f22acdd..07fbed8 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -770,6 +770,33 @@ parse_ct_nat(struct action_context *ctx, const char *name,
}
lexer_get(ctx->lexer);
+if (lexer_match(ctx->lexer, LEX_T_COMMA)) {
+
+ if (ctx->lexer->token.type != LEX_T_INTEGER ||
+ ctx->lexer->token.format != LEX_F_DECIMAL) {
+ lexer_syntax_error(ctx->lexer, "expecting Integer for port "
+ "range");
+ }
+
+ cn->port_range.port_lo = ntohll(ctx->lexer->token.value.integer);
+ lexer_get(ctx->lexer);
+
+ if (!lexer_match(ctx->lexer, LEX_T_HYPHEN)) {
+ lexer_syntax_error(ctx->lexer, "expecting - for port range");
+ return;
+ }
+
+ if (ctx->lexer->token.type != LEX_T_INTEGER) {
+ lexer_syntax_error(ctx->lexer, "expecting Integer for port "
+ "range");
+ }
+
+ cn->port_range.port_hi = ntohll(ctx->lexer->token.value.integer);
You should probably ensure that cn->port_range.port_hi is greater than
cn->port_range.port_lo.
+ lexer_get(ctx->lexer);
+
+ cn->port_range.exists = true;
+}
+
if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
return;
}
@@ -799,6 +826,13 @@ format_ct_nat(const struct ovnact_ct_nat *cn, const char
*name, struct ds *s)
ipv6_format_addr(>ipv6, s);
ds_put_char(s, ')');
}
+
+if (cn->port_range.exists) {
+ds_chomp(s, ')');
+ds_put_format(s, ",%d-%d)", cn->port_range.port_lo,
+ cn->port_range.port_hi);
+}
+
ds_put_char(s, ';');
}
@@ -861,6 +895,11 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
}
}
+if (cn->port_range.exists) {
+ nat->range.proto.min = cn->port_range.port_lo;
+ nat->range.proto.max = cn->port_range.port_hi;
+}
+
ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
ct = ofpacts->header;
if (cn->family == AF_INET || cn->family == AF_INET6) {
diff --git a/lib/lex.c b/lib/lex.c
index 7a2ab41..94f6c77 100644
--- a/lib/lex.c
+++ b/lib/lex.c
@@ -301,6 +301,9 @@ lex_token_format(const struct lex_token *token, struct ds
*s)
case LEX_T_COLON:
ds_put_char(s, ':');
break;
+case LEX_T_HYPHEN:
+ds_put_char(s, '-');
+break;
default:
OVS_NOT_REACHED();
}
@@ -757,7 +760,7 @@ next:
token->type = LEX_T_DECREMENT;
p++;
} else {
-lex_error(token, "`-' is only valid as part of `--'.");
+ token->type = LEX_T_HYPHEN;
}
break;
diff --git a/tests/ovn.at b/tests/ovn.at
index 9a44f0a..d76db6f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1049,15 +1049,25 @@ ct_dnat(192.168.1.2);
ct_dnat(fd11::2);
encodes as ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=fd11::2))
has prereqs ip
+ct_dnat(192.168.1.2, 1-3000);
+formats as ct_dnat(192.168.1.2,1-3000);
+encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000))
+has prereqs ip
ct_dnat(192.168.1.2, 192.168.1.3);
-Syntax error at `,' expecting `)'.
+Syntax error at `192.168.1.3' expecting
[ovs-dev] [PATCH v1 3/3 ovn] NAT: Parsing port_range options in ct_*nat actions
From: Ankur Sharma
Changes to parse the logical flow, which specifies port_range
for ct_nat action.
Example logical flow:
ct_snat(10.15.24.135,1-3)
Signed-off-by: Ankur Sharma
---
include/ovn/actions.h | 7 +++
include/ovn/lex.h | 1 +
lib/actions.c | 39 +++
lib/lex.c | 5 -
tests/ovn.at | 24 ++--
5 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 9b01492..2cec369 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -233,6 +233,13 @@ struct ovnact_ct_nat {
struct in6_addr ipv6;
ovs_be32 ipv4;
};
+
+struct {
+ bool exists;
+ uint16_t port_lo;
+ uint16_t port_hi;
+} port_range;
+
uint8_t ltable; /* Logical table ID of next table. */
};
diff --git a/include/ovn/lex.h b/include/ovn/lex.h
index 8d55857..1da6ccc 100644
--- a/include/ovn/lex.h
+++ b/include/ovn/lex.h
@@ -63,6 +63,7 @@ enum lex_type {
LEX_T_EXCHANGE, /* <-> */
LEX_T_DECREMENT,/* -- */
LEX_T_COLON,/* : */
+LEX_T_HYPHEN, /* - */
};
/* Subtype for LEX_T_INTEGER and LEX_T_MASKED_INTEGER tokens.
diff --git a/lib/actions.c b/lib/actions.c
index f22acdd..07fbed8 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -770,6 +770,33 @@ parse_ct_nat(struct action_context *ctx, const char *name,
}
lexer_get(ctx->lexer);
+if (lexer_match(ctx->lexer, LEX_T_COMMA)) {
+
+ if (ctx->lexer->token.type != LEX_T_INTEGER ||
+ ctx->lexer->token.format != LEX_F_DECIMAL) {
+ lexer_syntax_error(ctx->lexer, "expecting Integer for port "
+ "range");
+ }
+
+ cn->port_range.port_lo = ntohll(ctx->lexer->token.value.integer);
+ lexer_get(ctx->lexer);
+
+ if (!lexer_match(ctx->lexer, LEX_T_HYPHEN)) {
+ lexer_syntax_error(ctx->lexer, "expecting - for port range");
+ return;
+ }
+
+ if (ctx->lexer->token.type != LEX_T_INTEGER) {
+ lexer_syntax_error(ctx->lexer, "expecting Integer for port "
+ "range");
+ }
+
+ cn->port_range.port_hi = ntohll(ctx->lexer->token.value.integer);
+ lexer_get(ctx->lexer);
+
+ cn->port_range.exists = true;
+}
+
if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
return;
}
@@ -799,6 +826,13 @@ format_ct_nat(const struct ovnact_ct_nat *cn, const char
*name, struct ds *s)
ipv6_format_addr(>ipv6, s);
ds_put_char(s, ')');
}
+
+if (cn->port_range.exists) {
+ds_chomp(s, ')');
+ds_put_format(s, ",%d-%d)", cn->port_range.port_lo,
+ cn->port_range.port_hi);
+}
+
ds_put_char(s, ';');
}
@@ -861,6 +895,11 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
}
}
+if (cn->port_range.exists) {
+ nat->range.proto.min = cn->port_range.port_lo;
+ nat->range.proto.max = cn->port_range.port_hi;
+}
+
ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
ct = ofpacts->header;
if (cn->family == AF_INET || cn->family == AF_INET6) {
diff --git a/lib/lex.c b/lib/lex.c
index 7a2ab41..94f6c77 100644
--- a/lib/lex.c
+++ b/lib/lex.c
@@ -301,6 +301,9 @@ lex_token_format(const struct lex_token *token, struct ds
*s)
case LEX_T_COLON:
ds_put_char(s, ':');
break;
+case LEX_T_HYPHEN:
+ds_put_char(s, '-');
+break;
default:
OVS_NOT_REACHED();
}
@@ -757,7 +760,7 @@ next:
token->type = LEX_T_DECREMENT;
p++;
} else {
-lex_error(token, "`-' is only valid as part of `--'.");
+ token->type = LEX_T_HYPHEN;
}
break;
diff --git a/tests/ovn.at b/tests/ovn.at
index 9a44f0a..d76db6f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1049,15 +1049,25 @@ ct_dnat(192.168.1.2);
ct_dnat(fd11::2);
encodes as ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=fd11::2))
has prereqs ip
+ct_dnat(192.168.1.2, 1-3000);
+formats as ct_dnat(192.168.1.2,1-3000);
+encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000))
+has prereqs ip
ct_dnat(192.168.1.2, 192.168.1.3);
-Syntax error at `,' expecting `)'.
+Syntax error at `192.168.1.3' expecting Integer for port range.
ct_dnat(foo);
Syntax error at `foo' expecting IPv4 or IPv6 address.
ct_dnat(foo, bar);
Syntax error at `foo' expecting IPv4 or IPv6 address.
ct_dnat();
Syntax error at `)' expecting IPv4 or IPv6 address.
+ct_dnat(192.168.1.2, foo);
+Syntax error at `foo' expecting Integer for port range.
+ct_dnat(192.168.1.2, 1000-foo);
+Syntax error at `foo' expecting Integer for port range.
