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, thank you for the followup. I have a couple of comments down below. > 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. > + > + 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;". > + 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); > + > + > } > > 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. > + </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 -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
