Thanks for the review Ales and sorry about those unrelated changes. I just noticed those two typos and thought I'll sneak in the fix.
On Mon, Apr 22, 2024 at 10:49 AM Ales Musil <[email protected]> wrote: > > > 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> > -- Best Regards, Martin Kalcok. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
