On Fri, Apr 19, 2024 at 2:33 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, there are only two things that can be addressed during merge. > controller/chassis.c | 8 ++ > include/ovn/actions.h | 8 +- > include/ovn/features.h | 1 + > lib/actions.c | 155 +++++++++++++++++++++++++------------- > northd/en-global-config.c | 10 +++ > northd/en-global-config.h | 1 + > ovn-sb.xml | 22 +++++- > tests/ovn.at | 16 ++++ > utilities/ovn-trace.c | 45 ++++++++--- > 9 files changed, 199 insertions(+), 67 deletions(-) > > 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..aa5e4ab89 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_to_zone) \ > OVNACT(CT_DNAT, ovnact_ct_nat) \ > OVNACT(CT_SNAT, ovnact_ct_nat) \ > OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ > @@ -76,7 +77,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_commit_nat) \ > + OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_to_zone) \ > OVNACT(CLONE, ovnact_nest) \ > OVNACT(ARP, ovnact_nest) \ > OVNACT(ICMP4, ovnact_nest) \ > @@ -290,11 +291,12 @@ struct ovnact_ct_nat { > uint8_t ltable; /* Logical table ID of next table. */ > }; > > -/* OVNACT_CT_COMMIT_NAT. */ > -struct ovnact_ct_commit_nat { > +/* OVNACT_CT_COMMIT_TO_ZONE, OVNACT_CT_COMMIT_NAT. */ > +struct ovnact_ct_commit_to_zone { > struct ovnact ovnact; > > bool dnat_zone; > + bool do_nat; > uint8_t ltable; > }; > > 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..b9bdcd48d 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -870,6 +870,45 @@ parse_ct_nat(struct action_context *ctx, const char > *name, > } > } > > +static void > +parse_ct_commit_to_zone(struct action_context *ctx, const char *name, > + bool do_nat, bool require_param, > + struct ovnact_ct_commit_to_zone *cn) > +{ > + add_prerequisite(ctx, "ip"); > + > + if (ctx->pp->cur_ltable >= ctx->pp->n_tables) { > + lexer_error(ctx->lexer, > + "\"%s\" action not allowed in last table.", name); > + return; > + } > + > + cn->ltable = ctx->pp->cur_ltable + 1; > + cn->do_nat = do_nat; > + cn->dnat_zone = true; > + > + if (require_param) { > + lexer_force_match(ctx->lexer, LEX_T_LPAREN); > + } else { > + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) { > + return; > + } > + } > + > + if (lexer_match_id(ctx->lexer, "dnat")) { > + cn->dnat_zone = true; > + } else if (lexer_match_id(ctx->lexer, "snat")) { > + cn->dnat_zone = false; > + } else { > + lexer_error(ctx->lexer, "\"%s\" action accepts" > + " only \"dnat\" or \"snat\" parameter.", name); > + return; > + } > + > + lexer_force_match(ctx->lexer, LEX_T_RPAREN); > +} > + > + > static void > parse_CT_DNAT(struct action_context *ctx) > { > @@ -901,33 +940,17 @@ parse_CT_SNAT_IN_CZONE(struct action_context *ctx) > 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_nat\" action not allowed in last > table."); > - return; > - } > - > - struct ovnact_ct_commit_nat *cn = > ovnact_put_CT_COMMIT_NAT(ctx->ovnacts); > - cn->ltable = ctx->pp->cur_ltable + 1; > - cn->dnat_zone = true; > - > - if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) { > - return; > - } > - > - if (lexer_match_id(ctx->lexer, "dnat")) { > - cn->dnat_zone = true; > - } else if (lexer_match_id(ctx->lexer, "snat")) { > - cn->dnat_zone = false; > - } else { > - lexer_error(ctx->lexer, "\"ct_commit_nat\" action accepts" > - " only \"dnat\" or \"snat\" parameter."); > - return; > - } > + parse_ct_commit_to_zone(ctx, "ct_commit_nat", > + true, false, > + ovnact_put_CT_COMMIT_NAT(ctx->ovnacts)); > +} > > - lexer_force_match(ctx->lexer, LEX_T_RPAREN); > +static void > +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) > +{ > + parse_ct_commit_to_zone(ctx, "ct_commit_to_zone", > + false, true, > + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts)); > } > > static void > @@ -980,12 +1003,20 @@ format_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat > *cn, struct ds *s) > } > > static void > -format_CT_COMMIT_NAT(const struct ovnact_ct_commit_nat *cn, struct ds *s) > +format_CT_COMMIT_NAT(const struct ovnact_ct_commit_to_zone *cn, struct ds > *s) > { > ds_put_cstr(s, "ct_commit_nat"); > ds_put_cstr(s, cn->dnat_zone ? "(dnat);" : "(snat);"); > } > > +static void > +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_to_zone *cn, > + struct ds *s) > +{ > + ds_put_cstr(s, "ct_commit_to_zone"); > + ds_put_cstr(s, cn->dnat_zone ? "(dnat);" : "(snat);"); > +} > + > static void > encode_ct_nat(const struct ovnact_ct_nat *cn, > const struct ovnact_encode_params *ep, > @@ -1055,6 +1086,39 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > ofpbuf_push_uninit(ofpacts, ct_offset); > } > > +static void > +encode_ct_commit_to_zone(const struct ovnact_ct_commit_to_zone *cn, > + const struct ovnact_encode_params *ep, > + struct ofpbuf *ofpacts) > +{ > + const size_t ct_offset = ofpacts->size; > + > + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > + ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); > + ct->zone_src.ofs = 0; > + ct->zone_src.n_bits = 16; > + ct->flags = NX_CT_F_COMMIT; > + ct->alg = 0; > + > + if (ep->is_switch) { > + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > + } else { > + ct->zone_src.field = mf_from_id(cn->dnat_zone > + ? MFF_LOG_DNAT_ZONE > + : MFF_LOG_SNAT_ZONE); > + } > + > + if (cn->do_nat) { > + struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); > + nat->range_af = AF_UNSPEC; > + nat->flags = 0; > + } > + > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > +} > + > static void > encode_CT_DNAT(const struct ovnact_ct_nat *cn, > const struct ovnact_encode_params *ep, > @@ -1088,34 +1152,19 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat > *cn, > } > > static void > -encode_CT_COMMIT_NAT(const struct ovnact_ct_commit_nat *cn, > +encode_CT_COMMIT_NAT(const struct ovnact_ct_commit_to_zone *cn, > const struct ovnact_encode_params *ep, > struct ofpbuf *ofpacts) > { > - const size_t ct_offset = ofpacts->size; > - > - struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > - ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); > - ct->zone_src.ofs = 0; > - ct->zone_src.n_bits = 16; > - ct->flags = NX_CT_F_COMMIT; > - ct->alg = 0; > - > - if (ep->is_switch) { > - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > - } else { > - ct->zone_src.field = mf_from_id(cn->dnat_zone > - ? MFF_LOG_DNAT_ZONE > - : MFF_LOG_SNAT_ZONE); > - } > - > - struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); > - nat->range_af = AF_UNSPEC; > - nat->flags = 0; > + encode_ct_commit_to_zone(cn, ep, ofpacts); > +} > > - ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > - ofpacts->header = ct; > - ofpact_finish_CT(ofpacts, &ct); > +static void > +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_to_zone *cn, > + const struct ovnact_encode_params *ep, > + struct ofpbuf *ofpacts) > +{ > + encode_ct_commit_to_zone(cn, ep, ofpacts); > } > > static void > @@ -1124,7 +1173,7 @@ ovnact_ct_nat_free(struct ovnact_ct_nat *ct_nat > OVS_UNUSED) > } > > static void > -ovnact_ct_commit_nat_free(struct ovnact_ct_commit_nat *cn OVS_UNUSED) > +ovnact_ct_commit_to_zone_free(struct ovnact_ct_commit_to_zone *cn > OVS_UNUSED) > { > } > > @@ -5306,6 +5355,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..193deaebc 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,15 @@ 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..ab0c37c8d 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -1432,13 +1432,31 @@ > </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. > + The packet is then automatically sent to the next tables as if > + followed by <code>next;</code> action. The next tables will > + see the changes in the packet caused by the connection > tracker. > + </p> > + > + <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 logical > port's > + conntrack zone. > + </p> > + </dd> > <dt><code>ct_dnat;</code></dt> > <dt><code>ct_dnat(<var>IP</var>);</code></dt> > <dd> > <p> > <code>ct_dnat</code> sends the packet through the DNAT zone in > connection tracking table to unDNAT any packet that was > DNATed in > - the opposite direction. The packet is then automatically > sent to > + the opposite direction. The packet is then automatically sent > nit: Unrelated change > to the next tables as if followed by <code>next;</code> > action. > The next tables will see the changes in the packet caused by > the connection tracker. > @@ -1448,7 +1466,7 @@ > DNAT zone to change the destination IP address of the packet > to > the one provided inside the parentheses and commits the > connection. > The packet is then automatically sent to the next tables as if > - followed by <code>next;</code> action. The next tables will > see > + followed by <code>next;</code> action. The next tables will > see > nit: Same > the changes in the packet caused by the connection tracker. > </p> > </dd> > diff --git a/tests/ovn.at b/tests/ovn.at > index c8cc1d37f..1852457e1 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,table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > +ct_commit_to_zone(snat); > + encodes as ct(commit,table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > +ct_commit_to_zone; > + Syntax error at `;' expecting `('. > +ct_commit_to_zone(); > + "ct_commit_to_zone" action accepts only "dnat" or "snat" parameter. > +ct_commit_to_zone(foo); > + "ct_commit_to_zone" action accepts only "dnat" or "snat" parameter. > +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 ee086a7ae..7aa6e2ca9 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -2463,24 +2463,20 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat, > } > > static void > -execute_ct_commit_nat(const struct ovnact_ct_commit_nat *ct_nat, > - const struct ovntrace_datapath *dp, struct flow > *uflow, > - enum ovnact_pipeline pipeline, struct ovs_list > *super) > +ct_commit_to_zone__(const struct ovnact_ct_commit_to_zone *ct_nat, > + const struct ovntrace_datapath *dp, struct flow > *uflow, > + enum ovnact_pipeline pipeline, struct ovs_list *super, > + struct ds *action) > { > struct flow ct_flow = *uflow; > - struct ds s = DS_EMPTY_INITIALIZER; > - > - ds_put_cstr(&s, "ct_commit_nat /* assuming no" > - " un-nat entry, so no change */"); > > /* ct(nat) implies ct(). */ > if (!(ct_flow.ct_state & CS_TRACKED)) { > - ct_flow.ct_state |= next_ct_state(&s); > + ct_flow.ct_state |= next_ct_state(action); > } > > struct ovntrace_node *node = ovntrace_node_append( > - super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(&s)); > - ds_destroy(&s); > + super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(action)); > > /* Trace the actions in the next table. */ > trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs); > @@ -2490,6 +2486,30 @@ execute_ct_commit_nat(const struct > ovnact_ct_commit_nat *ct_nat, > * flow, not ct_flow. */ > } > > +static void > +execute_ct_commit_nat(const struct ovnact_ct_commit_to_zone *ct_nat, > + const struct ovntrace_datapath *dp, struct flow > *uflow, > + enum ovnact_pipeline pipeline, struct ovs_list > *super) > +{ > + struct ds s = DS_EMPTY_INITIALIZER; > + ds_put_cstr(&s, "ct_commit_nat /* assuming no" > + " un-nat entry, so no change */"); > + ct_commit_to_zone__(ct_nat, dp, uflow, pipeline, super, &s); > + ds_destroy(&s); > +} > + > +static void > +execute_ct_commit_to_zone(const struct ovnact_ct_commit_to_zone > *ct_commit, > + const struct ovntrace_datapath *dp, > + struct flow *uflow, enum ovnact_pipeline > pipeline, > + struct ovs_list *super) > +{ > + struct ds s = DS_EMPTY_INITIALIZER; > + ds_put_format(&s, "ct_commit_to_zone(%s)", > + ct_commit->dnat_zone ? "dnat" : "snat"); > + ct_commit_to_zone__(ct_commit, dp, uflow, pipeline, super, &s); > + ds_destroy(&s); > +} > > static void > execute_ct_lb(const struct ovnact_ct_lb *ct_lb, > @@ -3147,6 +3167,11 @@ trace_actions(const struct ovnact *ovnacts, size_t > ovnacts_len, > flow_clear_conntrack(uflow); > break; > > + case OVNACT_CT_COMMIT_TO_ZONE: > + execute_ct_commit_to_zone(ovnact_get_CT_COMMIT_TO_ZONE(a), dp, > + uflow, pipeline, super); > + break; > + > case OVNACT_CT_COMMIT_NAT: > execute_ct_commit_nat(ovnact_get_CT_COMMIT_NAT(a), dp, uflow, > pipeline, super); > -- > 2.40.1 > > With that addressed: Acked-by: Ales Musil <[email protected]> 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
