Hi Mark, Thanks a lot for review. Addressed all the comments. Please take a look at V2.
Regards, Ankur -----Original Message----- From: dev <[email protected]> On Behalf Of Mark Michelson Sent: Monday, March 30, 2020 12:21 PM To: svc.mail.git <[email protected]>; [email protected] 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 <[email protected]> > > 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 <[email protected]> > --- > 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; > _______________________________________________ dev mailing list [email protected] https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=Fj-Itn4XNVF7nR6Bu1FBhdmydg5ylOgLh4IxQjV4jDk&s=EZO4XXmQbfKEIglAUKp-9xosW5LMWFrk84OXthwEj98&e= _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
