On 11/22/22 10:29, Ales Musil wrote:
> Add action called ct_commit_nat, that performs
> NAT while committing the connection. This is
> useful for related traffic on which we need
> to perform NAT, mainly ICMP. We need to
> commit due to design decision of OvS[0]:
>
> "Connections identified as rel are separate from
> the originating connection and must be committed separately."
>
> [0] http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt
>
> Reported-at: https://bugzilla.redhat.com/2126083
> Acked-by: Mark Michelson <[email protected]>
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v3: Rebase on current main.
> ---
Hi Ales,
The change looks mostly OK to me. I just have a few comments below.
> include/ovn/actions.h | 3 +++
> lib/actions.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> ovn-sb.xml | 12 ++++++++++++
> tests/ovn.at | 5 +++++
> utilities/ovn-trace.c | 34 ++++++++++++++++++++++++++++++++++
> 5 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index fdb6ab08b..d1776d684 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -74,6 +74,7 @@ struct ovn_extend_table;
> OVNACT(CT_LB_MARK, ovnact_ct_lb) \
> OVNACT(SELECT, ovnact_select) \
> OVNACT(CT_CLEAR, ovnact_null) \
> + OVNACT(CT_COMMIT_NAT, ovnact_ct_nat) \
> OVNACT(CLONE, ovnact_nest) \
> OVNACT(ARP, ovnact_nest) \
> OVNACT(ICMP4, ovnact_nest) \
> @@ -277,6 +278,8 @@ struct ovnact_ct_nat {
Can you please add OVNACT_CT_COMMIT_NAT to the comment just before
'struct ovnact_ct_nat {'?
> uint16_t port_hi;
> } port_range;
>
> + bool commit; /* Explicit commit action. */
> +
> uint8_t ltable; /* Logical table ID of next table. */
> };
>
> diff --git a/lib/actions.c b/lib/actions.c
> index b59f364bf..fb688eeb1 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -920,6 +920,7 @@ parse_ct_nat(struct action_context *ctx, const char *name,
> return;
> }
> cn->ltable = ctx->pp->cur_ltable + 1;
> + cn->commit = false;
>
> if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> if (ctx->lexer->token.type != LEX_T_INTEGER
> @@ -929,9 +930,11 @@ parse_ct_nat(struct action_context *ctx, const char
> *name,
> return;
> }
> if (ctx->lexer->token.format == LEX_F_IPV4) {
> + cn->commit = true;
> cn->family = AF_INET;
> cn->ipv4 = ctx->lexer->token.value.ipv4;
> } else if (ctx->lexer->token.format == LEX_F_IPV6) {
> + cn->commit = true;
> cn->family = AF_INET6;
> cn->ipv6 = ctx->lexer->token.value.ipv6;
> }
> @@ -1004,6 +1007,24 @@ parse_CT_SNAT_IN_CZONE(struct action_context *ctx)
> ovnact_put_CT_SNAT_IN_CZONE(ctx->ovnacts));
> }
>
> +static void
> +parse_CT_COMMIT_NAT(struct action_context *ctx)
> +{
> + add_prerequisite(ctx, "ip");
> +
> + if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> + lexer_error(ctx->lexer,
> + "\"ct_commit_related\" action not allowed in last
> table.");
> + return;
> + }
> +
> + struct ovnact_ct_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts);
> + cn->commit = true;
> + cn->ltable = ctx->pp->cur_ltable + 1;
> + cn->family = AF_UNSPEC;
> + cn->port_range.exists = false;
> +}
> +
> static void
> format_ct_nat(const struct ovnact_ct_nat *cn, const char *name, struct ds *s)
> {
> @@ -1053,6 +1074,12 @@ format_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat
> *cn, struct ds *s)
> format_ct_nat(cn, "ct_snat_in_czone", s);
> }
>
> +static void
> +format_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn OVS_UNUSED, struct ds *s)
> +{
> + ds_put_cstr(s, "ct_commit_nat;");
> +}
> +
> static void
> encode_ct_nat(const struct ovnact_ct_nat *cn,
> const struct ovnact_encode_params *ep,
> @@ -1104,7 +1131,7 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
>
> ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> ct = ofpacts->header;
> - if (cn->family == AF_INET || cn->family == AF_INET6) {
> + if (cn->commit) {
> ct->flags |= NX_CT_F_COMMIT;
> }
> ofpact_finish(ofpacts, &ct->ofpact);
> @@ -1143,6 +1170,17 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn,
> encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts);
> }
>
> +static void
> +encode_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn,
> + const struct ovnact_encode_params *ep,
> + struct ofpbuf *ofpacts)
> +{
> + enum mf_field_id zone = ep->is_switch
> + ? MFF_LOG_CT_ZONE
> + : MFF_LOG_DNAT_ZONE;
> + encode_ct_nat(cn, ep, false, zone, ofpacts);
We only deal with DNAT (third argument is 'false'). I'm pretty sure
there can also be a case when we need to allow and commit related SNATed
traffic. Or am I missing something?
> +}
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
