On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok <martin.kal...@canonical.com>
wrote:
> Action `ct_commit` currently does not allow specifying zone into
> which connection is committed. For example, in LR datapath, the `ct_commit`
> will always use the DNAT zone.
>
> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
> explicitly specify the zone into which the connection will be committed.
>
> Original behavior of `ct_commit` without the arguments remains unchanged.
>
> Signed-off-by: Martin Kalcok <martin.kal...@canonical.com>
>
Hi Martin,
thank you for the followup, I have a few comments down below.
One small nit for the commit subject, we usually specify only the module
name without .c/.h e.g. action, ovn-controller, northd etc.
AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it for
this. Maybe we could remove the old behavior and leave just this? Before
posting v2 we should discuss this with others.
Adding Dumitru and Numan to this discussion.
> ---
> include/ovn/actions.h | 9 +++++++++
> lib/actions.c | 20 +++++++++++++++++++-
> ovn-sb.xml | 9 +++++++++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 49fb96fc6..ce9597cf5 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -259,11 +259,20 @@ struct ovnact_ct_next {
> uint8_t ltable; /* Logical table ID of next table. */
> };
>
> +/* Conntrack zone to be used for commiting CT entries by the action.
> + * DEFAULT uses default zone for given datapath. */
> +enum ovnact_ct_zone {
> + OVNACT_CT_ZONE_DEFAULT,
> + OVNACT_CT_ZONE_SNAT,
> + OVNACT_CT_ZONE_DNAT,
> +};
> +
>
There is no need for this enum, see details below.
> /* OVNACT_CT_COMMIT_V1. */
> struct ovnact_ct_commit_v1 {
> struct ovnact ovnact;
> uint32_t ct_mark, ct_mark_mask;
> ovs_be128 ct_label, ct_label_mask;
> + enum ovnact_ct_zone zone;
>
};
>
> /* Type of NAT used for the particular action.
> diff --git a/lib/actions.c b/lib/actions.c
> index a45874dfb..319e65b6f 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -707,6 +707,7 @@ static void
> parse_ct_commit_v1_arg(struct action_context *ctx,
> struct ovnact_ct_commit_v1 *cc)
> {
> + cc->zone = OVNACT_CT_ZONE_DEFAULT;
> if (lexer_match_id(ctx->lexer, "ct_mark")) {
> if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> return;
> @@ -737,6 +738,10 @@ parse_ct_commit_v1_arg(struct action_context *ctx,
> return;
> }
> lexer_get(ctx->lexer);
> + } else if (lexer_match_id(ctx->lexer, "snat")) {
> + cc->zone = OVNACT_CT_ZONE_SNAT;
> + } else if (lexer_match_id(ctx->lexer, "dnat")) {
> + cc->zone = OVNACT_CT_ZONE_DNAT;
> } else {
> lexer_syntax_error(ctx->lexer, NULL);
> }
> @@ -814,7 +819,20 @@ encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1
> *cc,
> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> ct->flags = NX_CT_F_COMMIT;
> ct->recirc_table = NX_CT_RECIRC_NONE;
> - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> + switch (cc->zone) {
> + case OVNACT_CT_ZONE_SNAT:
> + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE);
> + break;
> +
> + case OVNACT_CT_ZONE_DNAT:
> + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE);
> + break;
> +
> + case OVNACT_CT_ZONE_DEFAULT:
> + default:
> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> + break;
> + }
>
This would actually break potential use in the switch pipeline when someone
would specify ct_commit(snat)/ct_commit(dnat).
The switch pipeline uses MFF_LOG_CT_ZONE only. See ct_commit_nat for
example [0]. There is only indication if it's snat or dnat and check for
switch/router pipeline.
ct->zone_src.ofs = 0;
> ct->zone_src.n_bits = 16;
>
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index ac4e585f2..66cb9747d 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -1405,6 +1405,8 @@
> <dt><code>ct_commit { ct_mark=<var>value[/mask]</var>;
> };</code></dt>
> <dt><code>ct_commit { ct_label=<var>value[/mask]</var>;
> };</code></dt>
> <dt><code>ct_commit { ct_mark=<var>value[/mask]</var>;
> ct_label=<var>value[/mask]</var>; };</code></dt>
> + <dt><code>ct_commit(snat);</code></dt>
> + <dt><code>ct_commit(dnat);</code></dt>
> <dd>
> <p>
> Commit the flow to the connection tracking entry associated
> with it
> @@ -1421,6 +1423,13 @@
> in order to have specific bits set.
> </p>
>
> + <p>
> + Parameters <code>ct_commit(snat)</code> or
> <code>ct_commit(dnat)
> + </code> can be used to explicitly specify into which zone
> should be
> + connection committed. Without this parameter, the connection
> is
> + committed to the default zone for the Datapath.
> + </p>
> +
> <p>
> Note that if you want processing to continue in the next
> table,
> you must execute the <code>next</code> action after
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
There is a test cae missing for this in "action parsing" test [1].
Thanks,
Ales
[0]
https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/lib/actions.c#L1219-L1225
[1]
https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1308
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
