On 12/6/23 11:19, Ales Musil wrote:
> The ct_commit nat was hardcoded to use DNAT zone
> in router pipeline. Extend it that it accepts
> two new arguments (snat/dnat) which will determine
> the zone for router pipeline. The switch pipeline
> has only one so it resolves to the same for both arguments.
>
> In order to keep backward compatibility the ct_commit_nat
> without any arguments is the same as ct_commit_nat(dnat).
>
> Other arguments for this function are ignored to make
> it more future proof.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> include/ovn/actions.h | 12 ++++++--
> include/ovn/lex.h | 1 +
> lib/actions.c | 68 ++++++++++++++++++++++++++++++++++---------
> lib/lex.c | 15 ++++++++++
> tests/ovn.at | 14 +++++++++
> utilities/ovn-trace.c | 2 +-
> 6 files changed, 95 insertions(+), 17 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 49cfe0624..49fb96fc6 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -75,7 +75,7 @@ struct collector_set_ids;
> OVNACT(CT_LB_MARK, ovnact_ct_lb) \
> OVNACT(SELECT, ovnact_select) \
> OVNACT(CT_CLEAR, ovnact_null) \
> - OVNACT(CT_COMMIT_NAT, ovnact_ct_nat) \
> + OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_nat) \
> OVNACT(CLONE, ovnact_nest) \
> OVNACT(ARP, ovnact_nest) \
> OVNACT(ICMP4, ovnact_nest) \
> @@ -274,7 +274,7 @@ enum ovnact_ct_nat_type {
> OVNACT_CT_NAT_UNSPEC,
> };
>
> -/* OVNACT_CT_DNAT, OVNACT_CT_SNAT, OVNACT_CT_COMMIT_NAT. */
> +/* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
> struct ovnact_ct_nat {
> struct ovnact ovnact;
> int family;
> @@ -296,6 +296,14 @@ struct ovnact_ct_nat {
> uint8_t ltable; /* Logical table ID of next table. */
> };
>
> +/* OVNACT_CT_COMMIT_NAT. */
> +struct ovnact_ct_commit_nat {
> + struct ovnact ovnact;
> +
> + bool dnat_zone;
> + uint8_t ltable;
> +};
> +
> enum ovnact_ct_lb_flag {
> OVNACT_CT_LB_FLAG_NONE,
> OVNACT_CT_LB_FLAG_SKIP_SNAT,
> diff --git a/include/ovn/lex.h b/include/ovn/lex.h
> index 64d33361f..13947fa6d 100644
> --- a/include/ovn/lex.h
> +++ b/include/ovn/lex.h
> @@ -138,6 +138,7 @@ void lexer_init(struct lexer *, const char *input);
> void lexer_destroy(struct lexer *);
>
> enum lex_type lexer_get(struct lexer *);
> +bool lexer_get_until(struct lexer *, enum lex_type);
> enum lex_type lexer_lookahead(const struct lexer *);
> bool lexer_match(struct lexer *, enum lex_type);
> bool lexer_force_match(struct lexer *, enum lex_type);
> diff --git a/lib/actions.c b/lib/actions.c
> index a73fe1a1e..849dbf462 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1024,12 +1024,26 @@ parse_CT_COMMIT_NAT(struct action_context *ctx)
> return;
> }
>
> - struct ovnact_ct_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts);
> - cn->commit = true;
> + struct ovnact_ct_commit_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts);
> cn->ltable = ctx->pp->cur_ltable + 1;
> - cn->family = AF_UNSPEC;
> - cn->type = OVNACT_CT_NAT_UNSPEC;
> - cn->port_range.exists = false;
> + cn->dnat_zone = true;
> +
> + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> + return;
> + }
> +
> + while (lexer_get_until(ctx->lexer, LEX_T_ID)) {
> + if (lexer_match_id(ctx->lexer, "dnat")) {
> + cn->dnat_zone = true;
> + break;
> + } else if (lexer_match_id(ctx->lexer, "snat")) {
> + cn->dnat_zone = false;
> + break;
> + }
> + }
This seems too relaxed IMO. And the test you added shows it too:
ct_commit_nat(snat, dnat, ignore, 1, "ignore");
formats as ct_commit_nat(snat);
encodes as ct(commit,table=19,zone=NXM_NX_REG13[0..15],nat)
has prereqs ip
Moreover it's possible this loops indefinitely, e.g.:
$ echo "ct_commit_nat(ignore);" | tests/ovstest test-ovn parse-actions
This loops for ever.
Why not ensure that at most one of the two "snat" and "dnat" was
provided and nothing else?
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
