| Message ID | 1585351104-106546-4-git-send-email-svc.mail.git@nutanix.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NAT port range support | expand |
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 <ankur.sharma@nutanix.com> > > Changes to parse the logical flow, which specifies port_range > for ct_nat action. > > Example logical flow: > ct_snat(10.15.24.135,1-30000) > > Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com> > --- > 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(&cn->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. > +ct_dnat(192.168.1.2, 1000); > + Syntax error at `)' expecting - for port range. The underlying OVS nat action allows for a single port to be specified. Shouldn't we allow it for ct_snat/ct_dnat as well? > > # ct_snat > ct_snat; > @@ -1069,15 +1079,25 @@ ct_snat(192.168.1.2); > ct_snat(fd11::2); > encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=fd11::2)) > has prereqs ip > +ct_snat(192.168.1.2, 1-3000); > + formats as ct_snat(192.168.1.2,1-3000); > + encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000)) > + has prereqs ip > > ct_snat(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_snat(foo); > Syntax error at `foo' expecting IPv4 or IPv6 address. > ct_snat(foo, bar); > Syntax error at `foo' expecting IPv4 or IPv6 address. > ct_snat(); > Syntax error at `)' expecting IPv4 or IPv6 address. > +ct_snat(192.168.1.2, foo); > + Syntax error at `foo' expecting Integer for port range. > +ct_snat(192.168.1.2, 1000-foo); > + Syntax error at `foo' expecting Integer for port range. > +ct_snat(192.168.1.2, 1000); > + Syntax error at `)' expecting - for port range. > > # ct_clear > ct_clear; >
Hi Mark, Thanks a lot for review. Addressed all the comments. Please take a look at V2. Regards, Ankur -----Original Message----- From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Mark Michelson Sent: Monday, March 30, 2020 12:21 PM To: svc.mail.git <svc.mail.git@nutanix.com>; 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 <ankur.sharma@nutanix.com> > > Changes to parse the logical flow, which specifies port_range for > ct_nat action. > > Example logical flow: > ct_snat(10.15.24.135,1-30000) > > Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com> > --- > 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(&cn->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. > +ct_dnat(192.168.1.2, 1000); > + Syntax error at `)' expecting - for port range. The underlying OVS nat action allows for a single port to be specified. Shouldn't we allow it for ct_snat/ct_dnat as well? > > # ct_snat > ct_snat; > @@ -1069,15 +1079,25 @@ ct_snat(192.168.1.2); > ct_snat(fd11::2); > encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=fd11::2)) > has prereqs ip > +ct_snat(192.168.1.2, 1-3000); > + formats as ct_snat(192.168.1.2,1-3000); > + encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000)) > + has prereqs ip > > ct_snat(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_snat(foo); > Syntax error at `foo' expecting IPv4 or IPv6 address. > ct_snat(foo, bar); > Syntax error at `foo' expecting IPv4 or IPv6 address. > ct_snat(); > Syntax error at `)' expecting IPv4 or IPv6 address. > +ct_snat(192.168.1.2, foo); > + Syntax error at `foo' expecting Integer for port range. > +ct_snat(192.168.1.2, 1000-foo); > + Syntax error at `foo' expecting Integer for port range. > +ct_snat(192.168.1.2, 1000); > + Syntax error at `)' expecting - for port range. > > # ct_clear > ct_clear; >
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(&cn->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. +ct_dnat(192.168.1.2, 1000); + Syntax error at `)' expecting - for port range. # ct_snat ct_snat; @@ -1069,15 +1079,25 @@ ct_snat(192.168.1.2); ct_snat(fd11::2); encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=fd11::2)) has prereqs ip +ct_snat(192.168.1.2, 1-3000); + formats as ct_snat(192.168.1.2,1-3000); + encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000)) + has prereqs ip ct_snat(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_snat(foo); Syntax error at `foo' expecting IPv4 or IPv6 address. ct_snat(foo, bar); Syntax error at `foo' expecting IPv4 or IPv6 address. ct_snat(); Syntax error at `)' expecting IPv4 or IPv6 address. +ct_snat(192.168.1.2, foo); + Syntax error at `foo' expecting Integer for port range. +ct_snat(192.168.1.2, 1000-foo); + Syntax error at `foo' expecting Integer for port range. +ct_snat(192.168.1.2, 1000); + Syntax error at `)' expecting - for port range. # ct_clear ct_clear;