| Message ID | 20240419123306.46145-1-martin.kalcok@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v4,1/2] actions: New action ct_commit_to_zone. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Fri, Apr 19, 2024 at 2:33 PM Martin Kalcok <martin.kalcok@canonical.com> 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 <martin.kalcok@canonical.com> > --- > 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 <amusil@redhat.com> Thanks, Ales
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 <amusil@redhat.com> wrote: > > > On Fri, Apr 19, 2024 at 2:33 PM Martin Kalcok <martin.kalcok@canonical.com> > 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 <martin.kalcok@canonical.com> >> --- >> > > 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 <amusil@redhat.com> > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> >
On 4/22/24 12:34, Martin Kalcok wrote: > 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 <amusil@redhat.com> wrote: > [...] >>> 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 >> While the "to to" typo should be fixed the double space is not an issue. It's something commonly used in the code base to clearly separate sentences. The coding style specifies it for comments: https://github.com/ovn-org/ovn/blob/main/Documentation/internals/contributing/coding-style.rst#comments >> >>> 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 >> >> Two spaces are fine. I'll revert these changes before applying the patch. But a patch to fix the "to to" is welcome! Thanks, Dumitru
On 4/22/24 10:49, Ales Musil wrote: > On Fri, Apr 19, 2024 at 2:33 PM Martin Kalcok <martin.kalcok@canonical.com> > 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 <martin.kalcok@canonical.com> >> --- >> > > 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 <amusil@redhat.com> > > Thanks, > Ales > Thanks Ales and Martin! Applied to main and 24.03 with the changes suggested by Ales. Regards, Dumitru
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 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 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);
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 <martin.kalcok@canonical.com> --- 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(-)