On Mon, Sep 21, 2020 at 7:49 PM Mark Gray <mark.d.g...@redhat.com> wrote:
> On 17/09/2020 03:16, Ankur Sharma wrote:
> > From: Ankur Sharma <ankur.sha...@nutanix.com>
> >
> > This patch has following changes:
> >
> > a. Northd changes to put port range hash in the
> > logical flow based on configuration.
> >
> > b. Changes to parse the logical flow, which specifies
> > port_range_hash along with port_range for ct_nat action.
> >
> > Example logical flow:
> > ct_snat(10.15.24.135,1-30000, hash)
>
> Thanks for the patch.
>
Hi Ankur,
Thanks for the patch. Please see below for a few comments.
>
> >
> > Signed-off-by: Ankur Sharma <ankur.sha...@nutanix.com>
> > ---
> > include/ovn/actions.h | 1 +
> > lib/actions.c | 51 +++++++++++++++++++++++++++++++++++++--
> > northd/ovn-northd.c | 16 +++++++++++++
> > tests/ovn-northd.at | 31 ++++++++++++++++++++++++
> > tests/ovn.at | 66
> ++++++++++++++++++++++++++++++++++++++++++++-------
> > 5 files changed, 155 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 636cb4b..101cd7a 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -235,6 +235,7 @@ struct ovnact_ct_nat {
> > bool exists;
> > uint16_t port_lo;
> > uint16_t port_hi;
> > + char *port_hash;
>
> I wonder could the name be changed to something like 'port_hash_type' or
> 'port_hash_algorthm'. 'port_hash' reads more like the result of the hash
> rather than the type of hash. This would need to be changed across all
> your changes (e.g. 'external_port_hash' -> 'external_port_hash_type', etc)
>
I think instead of 'port_hash' you can have an integer with the name
'nat_flags' ?
And during parsing, you can set the value to 'NX_NAT_F_PROTO_HASH',
'NX_NAT_F_PROTO_RANDOM' or 0.
Right now you are allocating memory for this during parsing, but you're not
freeing it. With integer type, there is no
need to allocate any memory.
> > } port_range;
> >
> > uint8_t ltable; /* Logical table ID of next table. */
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 5fe0a38..c8e0767 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -707,6 +707,8 @@ parse_ct_nat(struct action_context *ctx, const char
> *name,
> >
> > if (lexer_match(ctx->lexer, LEX_T_COMMA)) {
> >
> > + cn->port_range.port_hash = NULL;
> > +
> > if (ctx->lexer->token.type != LEX_T_INTEGER ||
> > ctx->lexer->token.format != LEX_F_DECIMAL) {
> > lexer_syntax_error(ctx->lexer, "expecting Integer for
> port "
> > @@ -733,8 +735,40 @@ parse_ct_nat(struct action_context *ctx, const char
> *name,
> > "greater than range low");
> > }
> > lexer_get(ctx->lexer);
> > +
> > + if (lexer_match(ctx->lexer, LEX_T_COMMA)) {
> > + if (ctx->lexer->token.type != LEX_T_ID) {
> > + lexer_syntax_error(ctx->lexer, "expecting string
> for "
> > + "port hash");
> > + }
> > +
> > + if (strcmp(ctx->lexer->token.s, "hash") &&
> > + strcmp(ctx->lexer->token.s, "random")) {
> > + lexer_syntax_error(ctx->lexer, "Invalid value
> for "
> > + "port hash");
> > + }
> > +
> > + cn->port_range.port_hash =
> xstrdup(ctx->lexer->token.s);
> > + lexer_get(ctx->lexer);
> > + }
> > } else {
> > cn->port_range.port_hi = 0;
> > +
> > + if (lexer_match(ctx->lexer, LEX_T_COMMA)) {
> > + if (ctx->lexer->token.type != LEX_T_ID) {
> > + lexer_syntax_error(ctx->lexer, "expecting string
> for "
> > + "port hash");
> > + }
> > +
> > + if (strcmp(ctx->lexer->token.s, "hash") &&
> > + strcmp(ctx->lexer->token.s, "random")) {
> > + lexer_syntax_error(ctx->lexer, "Invalid value
> for "
> > + "port hash");
> > + }
> > +
> > + cn->port_range.port_hash =
> xstrdup(ctx->lexer->token.s);
> > + lexer_get(ctx->lexer);
> > + }
> > }
> >
> > cn->port_range.exists = true;
> > @@ -777,6 +811,10 @@ format_ct_nat(const struct ovnact_ct_nat *cn, const
> char *name, struct ds *s)
> > if (cn->port_range.port_hi) {
> > ds_put_format(s, "-%d", cn->port_range.port_hi);
> > }
> > +
> > + if (cn->port_range.port_hash) {
> > + ds_put_format(s, ",%s", cn->port_range.port_hash);
> > + }
> > ds_put_char(s, ')');
> > }
> >
> > @@ -843,8 +881,17 @@ 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;
> > + const char *port_hash = cn->port_range.port_hash;
> > + nat->range.proto.min = cn->port_range.port_lo;
> > + nat->range.proto.max = cn->port_range.port_hi;
> > +
> > + if (port_hash) {
> > + if (!strcmp(port_hash, "hash")) {
> > + nat->flags |= NX_NAT_F_PROTO_HASH;
> > + } else {
> > + nat->flags |= NX_NAT_F_PROTO_RANDOM;
> > + }
> > + }
> > }
> >
> > ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index db14909..2d8bc8b 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -9601,6 +9601,10 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> > if (nat->external_port_range[0]) {
> > ds_put_format(&actions, ",%s",
> > nat->external_port_range);
> > + if (nat->external_port_hash[0]) {
> > + ds_put_format(&actions, ",%s",
> > + nat->external_port_hash);
> > + }
> > }
> > ds_put_format(&actions, ");");
> > }
> > @@ -9638,6 +9642,10 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> > if (nat->external_port_range[0]) {
> > ds_put_format(&actions, ",%s",
> > nat->external_port_range);
> > + if (nat->external_port_hash[0]) {
> > + ds_put_format(&actions, ",%s",
> > + nat->external_port_hash);
> > + }
> > }
> > ds_put_format(&actions, ");");
> > }
> > @@ -9755,6 +9763,10 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> > if (nat->external_port_range[0]) {
> > ds_put_format(&actions, ",%s",
> > nat->external_port_range);
> > + if (nat->external_port_hash[0]) {
> > + ds_put_format(&actions, ",%s",
> > + nat->external_port_hash);
> > + }
> > }
> > ds_put_format(&actions, ");");
> > }
> > @@ -9804,6 +9816,10 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> > if (nat->external_port_range[0]) {
> > ds_put_format(&actions, ",%s",
> > nat->external_port_range);
> > + if (nat->external_port_hash[0]) {
> > + ds_put_format(&actions, ",%s",
> > + nat->external_port_hash);
> > + }
> > }
> > ds_put_format(&actions, ");");
> > }
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 99a9204..960ce00 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2010,3 +2010,34 @@ ovn-nbctl --wait=sb set NB_Global .
> options:ignore_lsp_down=true
> > AT_CHECK([ovn-sbctl lflow-list | grep arp | grep 10\.0\.0\.1], [0],
> [ignore])
> >
> > AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- Port Range and Hash in NAT entries])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +
> > +ovn-nbctl lr-add lr0
> > +ovn-nbctl lrp-add lr0 lr0-public 00:00:01:01:02:04 192.168.2.1/24
> > +ovn-nbctl lrp-add lr0 lr0-join 00:00:01:01:02:04 10.10.0.1/24
> > +
> > +ovn-nbctl set logical_router lr0 options:chassis=ch1
> > +
> > +ovn-nbctl --portrange lr-nat-add lr0 snat 192.168.2.1 10.0.0.0/24
> 1-1000 hash
> > +ovn-nbctl --portrange lr-nat-add lr0 dnat_and_snat 192.168.2.4 10.0.0.4
> 1100-2000 random
> > +ovn-nbctl --portrange lr-nat-add lr0 dnat 192.168.2.5 10.0.0.5 2100-3000
> > +
> > +ovn-sbctl dump-flows lr0
> > +
> > +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_out_snat | \
> > +grep "ct_snat(192.168.2.1,1-1000,hash)" | wc -l], [0], [1
> > +])
> > +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_in_dnat | \
> > +grep "ct_dnat(10.0.0.4,1100-2000,random)" | wc -l], [0], [1
> > +])
> > +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_out_snat | \
> > +grep "ct_snat(192.168.2.4,1100-2000,random)" | wc -l], [0], [1
> > +])
> > +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_in_dnat | \
> > +grep "ct_dnat(10.0.0.5,2100-3000)" | wc -l], [0], [1
> > +])
> > +
> > +AT_CLEANUP
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 41fe577..75b79b0 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1123,6 +1123,22 @@ 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, 1000);
> > + formats as ct_dnat(192.168.1.2,1000);
> > + encodes as ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=
> 192.168.1.2:1000))
> > + has prereqs ip
> > +ct_dnat(192.168.1.2, 1-3000, hash);
> > + formats as ct_dnat(192.168.1.2,1-3000,hash);
> > + encodes as
> ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1
> -3000,hash))
> > + has prereqs ip
> > +ct_dnat(192.168.1.2, 1-3000, random);
> > + formats as ct_dnat(192.168.1.2,1-3000,random);
> > + encodes as
> ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1
> -3000,random))
> > + has prereqs ip
> > +ct_dnat(192.168.1.2, 1000, hash);
> > + formats as ct_dnat(192.168.1.2,1000,hash);
> > + encodes as ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=
> 192.168.1.2:1000,hash))
> > + has prereqs ip
> >
> > ct_dnat(192.168.1.2, 192.168.1.3);
> > Syntax error at `192.168.1.3' expecting Integer for port range.
> > @@ -1136,12 +1152,20 @@ 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);
> > - formats as ct_dnat(192.168.1.2,1000);
> > - encodes as ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=
> 192.168.1.2:1000))
> > - has prereqs ip
> > ct_dnat(192.168.1.2, 1000-100);
> > Syntax error at `100' range high should be greater than range low.
> > +ct_dnat(192.168.1.2, hash);
> > + Syntax error at `hash' expecting Integer for port range.
> > +ct_dnat(192.168.1.2, random);
> > + Syntax error at `random' expecting Integer for port range.
> > +ct_dnat(192.168.1.2, 192.168.1.3, hash);
> > + Syntax error at `192.168.1.3' expecting Integer for port range.
> > +ct_dnat(192.168.1.2, foo, hash);
> > + Syntax error at `foo' expecting Integer for port range.
> > +ct_dnat(192.168.1.2, 1000-foo, hash);
> > + Syntax error at `foo' expecting Integer for port range.
> > +ct_dnat(192.168.1.2, 1000-100, hash);
> > + Syntax error at `100' range high should be greater than range low.
> >
> > # ct_snat
> > ct_snat;
> > @@ -1157,6 +1181,22 @@ 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, 1000);
> > + formats as ct_snat(192.168.1.2,1000);
> > + encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=
> 192.168.1.2:1000))
> > + has prereqs ip
> > +ct_snat(192.168.1.2, 1-3000, hash);
> > + formats as ct_snat(192.168.1.2,1-3000,hash);
> > + encodes as
> ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1
> -3000,hash))
> > + has prereqs ip
> > +ct_snat(192.168.1.2, 1-3000, random);
> > + formats as ct_snat(192.168.1.2,1-3000,random);
> > + encodes as
> ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1
> -3000,random))
> > + has prereqs ip
> > +ct_snat(192.168.1.2, 1000, hash);
> > + formats as ct_snat(192.168.1.2,1000,hash);
> > + encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=
> 192.168.1.2:1000,hash))
> > + has prereqs ip
> >
> > ct_snat(192.168.1.2, 192.168.1.3);
> > Syntax error at `192.168.1.3' expecting Integer for port range.
> > @@ -1170,12 +1210,22 @@ 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);
> > - formats as ct_snat(192.168.1.2,1000);
> > - encodes as ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=
> 192.168.1.2:1000))
> > - has prereqs ip
> > ct_snat(192.168.1.2, 1000-100);
> > Syntax error at `100' range high should be greater than range low.
> > +ct_snat(192.168.1.2, hash);
> > + Syntax error at `hash' expecting Integer for port range.
> > +ct_snat(192.168.1.2, random);
> > + Syntax error at `random' expecting Integer for port range.
> > +ct_snat(192.168.1.2, 192.168.1.3, hash);
> > + Syntax error at `192.168.1.3' expecting Integer for port range.
> > +ct_snat(192.168.1.2, foo, hash);
> > + Syntax error at `foo' expecting Integer for port range.
> > +ct_snat(192.168.1.2, 1000-foo, hash);
> > + Syntax error at `foo' expecting Integer for port range.
> > +ct_snat(192.168.1.2, 1000-100, hash);
> > + Syntax error at `100' range high should be greater than range low.
> > +
> > +
> > # ct_clear
> > ct_clear;
> > encodes as ct_clear
>
Can you please add more tests which checks that the proper OF flows are
added and the NAT flags are
set properly ?
Thanks
Numan
>
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
