On 4/17/24 09:04, Ales Musil wrote:
> On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok <[email protected]>
> wrote:
>
>> This change adds a new action 'ct_commit_to_zone' that enables users to
>> commit
>> the flow into a specific zone in the connection tracker.
>>
>> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to
>> avoid
>> issues during upgrade in case the northd is upgraded to a version that
>> supports
>> this action before the controller is upgraded.
>>
>> Note that this action is meaningful only in the context of Logical Router
>> datapath. Logical Switch datapath does not use multiple zones and this
>> action
>> falls back to committing the connection into the default zone for the
>> Logical
>> Switch.
>>
>> Signed-off-by: Martin Kalcok <[email protected]>
>> ---
>>
>
> Hi Martin,
>
Hi Martin, Ales,
> thank you for the followup. I have a couple of comments down below.
>
I have a few of my own. :)
>
>> controller/chassis.c | 8 ++++++
>> include/ovn/actions.h | 1 +
>> include/ovn/features.h | 1 +
>> lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++
>> northd/en-global-config.c | 11 +++++++
>> northd/en-global-config.h | 1 +
>> ovn-sb.xml | 24 ++++++++++++++++
>> tests/ovn.at | 16 +++++++++++
>> utilities/ovn-trace.c | 1 +
>> 9 files changed, 123 insertions(+)
>>
>> diff --git a/controller/chassis.c b/controller/chassis.c
>> index ad75df288..9bb2eba95 100644
>> --- a/controller/chassis.c
>> +++ b/controller/chassis.c
>> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
>> ovs_chassis_cfg *ovs_cfg,
>> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
>> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
>> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
>> + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
>> }
>>
>> /*
>> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
>> ovs_chassis_cfg *ovs_cfg,
>> return true;
>> }
>>
>> + if (!smap_get_bool(&chassis_rec->other_config,
>> + OVN_FEATURE_CT_COMMIT_TO_ZONE,
>> + false)) {
>> + return true;
>> + }
>> +
>> return false;
>> }
>>
>> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported)
>> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
>> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
>> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
>> + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
>> }
>>
>> static void
>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> index 8e794450c..4bcd1f58c 100644
>> --- a/include/ovn/actions.h
>> +++ b/include/ovn/actions.h
>> @@ -68,6 +68,7 @@ struct collector_set_ids;
>> OVNACT(CT_NEXT, ovnact_ct_next) \
>> /* CT_COMMIT_V1 is not supported anymore. */ \
>> OVNACT(CT_COMMIT_V2, ovnact_nest) \
>> + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \
>> OVNACT(CT_DNAT, ovnact_ct_nat) \
>> OVNACT(CT_SNAT, ovnact_ct_nat) \
>> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 08f1d8288..35a5d8ba0 100644
>> --- a/include/ovn/features.h
>> +++ b/include/ovn/features.h
>> @@ -28,6 +28,7 @@
>> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
>> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
>> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
>> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
>>
>> /* OVS datapath supported features. Based on availability OVN might
>> generate
>> * different types of openflows.
>> diff --git a/lib/actions.c b/lib/actions.c
>> index 39bb5bc8a..f26817018 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>> ct = ofpacts->header;
>> ofpact_finish(ofpacts, &ct->ofpact);
>> +}
>> +
>> +static void
>> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx)
>> +{
>> + add_prerequisite(ctx, "ip");
>> +
>> + struct ovnact_ct_commit_nat *ct_commit =
>> + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts);
>>
>
> The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed
> if anything with a flag indicating whether there should be nat or not. By
> doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone can
> be the same differentiated by the flag.
>
Just to be clear, your suggestion is to change the ct_commit_to_zone()
action so that it accepts an optional "nat" argument, right? That would
allow actions like:
ct_commit_to_zone(snat, nat)
Can we do this as a follow up and also update northd to generate flows
with ct_commit_to_zone() instead of ct_commit_nat()?
>
>> +
>> + lexer_force_match(ctx->lexer, LEX_T_LPAREN);
>> +
>> + if (lexer_match_id(ctx->lexer, "dnat")) {
>> + ct_commit->dnat_zone = true;
>> + } else if (lexer_match_id(ctx->lexer, "snat")) {
>> + ct_commit->dnat_zone = false;
>> + } else {
>> + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or
>> 'snat'");
>> + }
>> +
>> + lexer_force_match(ctx->lexer, LEX_T_RPAREN);
>> +}
>> +
>> +static void
>> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit,
>> + struct ds *s)
>> +{
>> + ds_put_format(s, "ct_commit_to_zone(%s);",
>> + ct_commit->dnat_zone ? "dnat" : "snat");
>> +
>> +}
>> +
>> +static void
>> +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit,
>> + const struct ovnact_encode_params *ep,
>> + struct ofpbuf *ofpacts)
>> +{
>> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>> + ct->flags = NX_CT_F_COMMIT;
>> + ct->recirc_table = NX_CT_RECIRC_NONE;
>>
>
> I don't think this action is ever used standalone so we can actually
> include the next table to avoid writing "ct_commit_to_zone(snat); next;".
>
>
We'd have to update ovn-trace too in that case. I'm a bit on the fence
with this one though. We do have "ct_next;" already and that implicitly
advances to the next table. But I think I prefer being explicit in the
logical flow actions (and adding "; next;" in northd). In any case, not
a must, I'm OK with implicit next too.
>> + ct->zone_src.ofs = 0;
>> + ct->zone_src.n_bits = 16;
>> +
>> + if (ep->is_switch) {
>> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
>> + } else {
>> + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone
>> + ? MFF_LOG_DNAT_ZONE
>> + : MFF_LOG_SNAT_ZONE);
>> + }
>> +
>> + size_t set_field_offset = ofpacts->size;
>> + ofpbuf_pull(ofpacts, set_field_offset);
>> + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>> + ct = ofpacts->header;
>> + ofpact_finish(ofpacts, &ct->ofpact);
>>
>
> This is the "manual" way of finishing the action encoding. There is a way
> shorter and more explicit way to do it:
>
> ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
> ofpacts->header = ct;
> ofpact_finish_CT(ofpacts, &ct);
>
>
>
>> +
>> +
Nit: too many empty lines.
>> }
>>
>> static void
>> @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx)
>> parse_CT_NEXT(ctx);
>> } else if (lexer_match_id(ctx->lexer, "ct_commit")) {
>> parse_CT_COMMIT(ctx);
>> + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) {
>> + parse_CT_COMMIT_TO_ZONE(ctx);
>> } else if (lexer_match_id(ctx->lexer, "ct_dnat")) {
>> parse_CT_DNAT(ctx);
>> } else if (lexer_match_id(ctx->lexer, "ct_snat")) {
>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
>> index 34e393b33..842ac5b64 100644
>> --- a/northd/en-global-config.c
>> +++ b/northd/en-global-config.c
>> @@ -370,6 +370,7 @@ northd_enable_all_features(struct
>> ed_type_global_config *data)
>> .fdb_timestamp = true,
>> .ls_dpg_column = true,
>> .ct_commit_nat_v2 = true,
>> + .ct_commit_to_zone = true,
>> };
>> }
>>
>> @@ -439,6 +440,16 @@ build_chassis_features(const struct
>> sbrec_chassis_table *sbrec_chassis_table,
>> chassis_features->ct_commit_nat_v2) {
>> chassis_features->ct_commit_nat_v2 = false;
>> }
>> +
>> + bool ct_commit_to_zone =
>> + smap_get_bool(&chassis->other_config,
>> + OVN_FEATURE_CT_COMMIT_TO_ZONE,
>> + false);
>> + if (!ct_commit_to_zone &&
>> + chassis_features->ct_commit_to_zone) {
>> + chassis_features->ct_commit_to_zone = false;
>> + }
>> +
>> }
>> }
>>
>> diff --git a/northd/en-global-config.h b/northd/en-global-config.h
>> index 38d732808..842bcee70 100644
>> --- a/northd/en-global-config.h
>> +++ b/northd/en-global-config.h
>> @@ -20,6 +20,7 @@ struct chassis_features {
>> bool fdb_timestamp;
>> bool ls_dpg_column;
>> bool ct_commit_nat_v2;
>> + bool ct_commit_to_zone;
>> };
>>
>> struct global_config_tracked_data {
>> diff --git a/ovn-sb.xml b/ovn-sb.xml
>> index 4c26c6714..0d1f640dd 100644
>> --- a/ovn-sb.xml
>> +++ b/ovn-sb.xml
>> @@ -1432,6 +1432,30 @@
>> </p>
>> </dd>
>>
>> + <dt><code>ct_commit_to_zone(dnat);</code></dt>
>> + <dt><code>ct_commit_to_zone(snat);</code></dt>
>> + <dd>
>> + <p>
>> + Commit the flow to the specific zone in the connection
>> tracker.
>> + Similar to the <code>ct_commit</code> action, this action
>> requires
>> + previous call to <code>ct_next</code> to initialize connection
>> + tracking state.
>> + </p>
>>
>
> ct_next implies only the DNAT zone, which is not correct for SNAT.
>
>
>> +
>> + <p>
>> + If you want processing to continue in the next table, you must
>> + execute the <code>next</code> action after
>> + <code>ct_commit_to_zone</code>.
>> + </p>
>>
>
> As mentioned above, I don't think we will want to commit without proceeding.
>
>
>> +
>> + <p>
>> + Note that this action is meaningful only in the Logical Router
>> + Datapath as the Logical Switch Datapath does not use separate
>> + connection tracking zones. Using this action in Logical Switch
>> + Datapath falls back to committing the flow into the switch's
>> + default zone.
This is not accurate. It's actually the logical port's conntrack zone.
>> + </p>
>> + </dd>
>> <dt><code>ct_dnat;</code></dt>
>> <dt><code>ct_dnat(<var>IP</var>);</code></dt>
>> <dd>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index c8cc1d37f..cbb303459 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -1316,6 +1316,22 @@ ct_commit {
>> ct_label=0x181716151413121110090807060504030201; };
>> ct_commit { ip4.dst = 192.168.0.1; };
>> Field ip4.dst is not modifiable.
>>
>> +# ct_commit_to_zone
>> +ct_commit_to_zone(dnat);
>> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]])
>> + has prereqs ip
>> +ct_commit_to_zone(dnat);
>>
>
> s/dnat/snat
>
>
>> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]])
>> + has prereqs ip
>> +ct_commit_to_zone;
>> + Syntax error at `;' expecting `('.
>> +ct_commit_to_zone();
>> + Syntax error at `)' expecting parameter 'dnat' or 'snat'.
>> +ct_commit_to_zone(foo);
>> + Syntax error at `foo' expecting parameter 'dnat' or 'snat'.
>> +ct_commit_to_zone(dnat;
>> + Syntax error at `;' expecting `)'.
>> +
>> # Legact ct_commit_v1 action.
>> ct_commit();
>> Syntax error at `(' expecting `;'.
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index 5e55fbbcc..554682cd0 100644
>> --- a/utilities/ovn-trace.c
>> +++ b/utilities/ovn-trace.c
>> @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, size_t
>> ovnacts_len,
>> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline,
>> super);
>> break;
>>
>> + case OVNACT_CT_COMMIT_TO_ZONE:
>> case OVNACT_CT_COMMIT_V2:
>> /* Nothing to do. */
>> break;
>> --
>> 2.40.1
>>
>>
> Thanks,
> Ales
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
