| Message ID | 20240415121414.1024577-1-martin.kalcok@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3,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 | fail | github build: failed |
On Mon, Apr 15, 2024 at 2:15 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, thank you for the followup. I have a couple of comments down below. > controller/chassis.c | 8 ++++++ > include/ovn/actions.h | 1 + > include/ovn/features.h | 1 + > lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++ > northd/en-global-config.c | 11 +++++++ > northd/en-global-config.h | 1 + > ovn-sb.xml | 24 ++++++++++++++++ > tests/ovn.at | 16 +++++++++++ > utilities/ovn-trace.c | 1 + > 9 files changed, 123 insertions(+) > > diff --git a/controller/chassis.c b/controller/chassis.c > index ad75df288..9bb2eba95 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -371,6 +371,7 @@ chassis_build_other_config(const struct > ovs_chassis_cfg *ovs_cfg, > smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); > smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); > smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); > + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); > } > > /* > @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct > ovs_chassis_cfg *ovs_cfg, > return true; > } > > + if (!smap_get_bool(&chassis_rec->other_config, > + OVN_FEATURE_CT_COMMIT_TO_ZONE, > + false)) { > + return true; > + } > + > return false; > } > > @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) > sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); > sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); > sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); > + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); > } > > static void > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 8e794450c..4bcd1f58c 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -68,6 +68,7 @@ struct collector_set_ids; > OVNACT(CT_NEXT, ovnact_ct_next) \ > /* CT_COMMIT_V1 is not supported anymore. */ \ > OVNACT(CT_COMMIT_V2, ovnact_nest) \ > + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ > OVNACT(CT_DNAT, ovnact_ct_nat) \ > OVNACT(CT_SNAT, ovnact_ct_nat) \ > OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ > diff --git a/include/ovn/features.h b/include/ovn/features.h > index 08f1d8288..35a5d8ba0 100644 > --- a/include/ovn/features.h > +++ b/include/ovn/features.h > @@ -28,6 +28,7 @@ > #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" > #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" > #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" > +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" > > /* OVS datapath supported features. Based on availability OVN might > generate > * different types of openflows. > diff --git a/lib/actions.c b/lib/actions.c > index 39bb5bc8a..f26817018 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > ct = ofpacts->header; > ofpact_finish(ofpacts, &ct->ofpact); > +} > + > +static void > +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) > +{ > + add_prerequisite(ctx, "ip"); > + > + struct ovnact_ct_commit_nat *ct_commit = > + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed if anything with a flag indicating whether there should be nat or not. By doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone can be the same differentiated by the flag. > + > + lexer_force_match(ctx->lexer, LEX_T_LPAREN); > + > + if (lexer_match_id(ctx->lexer, "dnat")) { > + ct_commit->dnat_zone = true; > + } else if (lexer_match_id(ctx->lexer, "snat")) { > + ct_commit->dnat_zone = false; > + } else { > + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or > 'snat'"); > + } > + > + lexer_force_match(ctx->lexer, LEX_T_RPAREN); > +} > + > +static void > +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, > + struct ds *s) > +{ > + ds_put_format(s, "ct_commit_to_zone(%s);", > + ct_commit->dnat_zone ? "dnat" : "snat"); > + > +} > + > +static void > +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, > + const struct ovnact_encode_params *ep, > + struct ofpbuf *ofpacts) > +{ > + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > + ct->flags = NX_CT_F_COMMIT; > + ct->recirc_table = NX_CT_RECIRC_NONE; > I don't think this action is ever used standalone so we can actually include the next table to avoid writing "ct_commit_to_zone(snat); next;". > + ct->zone_src.ofs = 0; > + ct->zone_src.n_bits = 16; > + > + if (ep->is_switch) { > + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > + } else { > + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone > + ? MFF_LOG_DNAT_ZONE > + : MFF_LOG_SNAT_ZONE); > + } > + > + size_t set_field_offset = ofpacts->size; > + ofpbuf_pull(ofpacts, set_field_offset); > + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > + ct = ofpacts->header; > + ofpact_finish(ofpacts, &ct->ofpact); > This is the "manual" way of finishing the action encoding. There is a way shorter and more explicit way to do it: ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); ofpacts->header = ct; ofpact_finish_CT(ofpacts, &ct); > + > + > } > > static void > @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) > parse_CT_NEXT(ctx); > } else if (lexer_match_id(ctx->lexer, "ct_commit")) { > parse_CT_COMMIT(ctx); > + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { > + parse_CT_COMMIT_TO_ZONE(ctx); > } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { > parse_CT_DNAT(ctx); > } else if (lexer_match_id(ctx->lexer, "ct_snat")) { > diff --git a/northd/en-global-config.c b/northd/en-global-config.c > index 34e393b33..842ac5b64 100644 > --- a/northd/en-global-config.c > +++ b/northd/en-global-config.c > @@ -370,6 +370,7 @@ northd_enable_all_features(struct > ed_type_global_config *data) > .fdb_timestamp = true, > .ls_dpg_column = true, > .ct_commit_nat_v2 = true, > + .ct_commit_to_zone = true, > }; > } > > @@ -439,6 +440,16 @@ build_chassis_features(const struct > sbrec_chassis_table *sbrec_chassis_table, > chassis_features->ct_commit_nat_v2) { > chassis_features->ct_commit_nat_v2 = false; > } > + > + bool ct_commit_to_zone = > + smap_get_bool(&chassis->other_config, > + OVN_FEATURE_CT_COMMIT_TO_ZONE, > + false); > + if (!ct_commit_to_zone && > + chassis_features->ct_commit_to_zone) { > + chassis_features->ct_commit_to_zone = false; > + } > + > } > } > > diff --git a/northd/en-global-config.h b/northd/en-global-config.h > index 38d732808..842bcee70 100644 > --- a/northd/en-global-config.h > +++ b/northd/en-global-config.h > @@ -20,6 +20,7 @@ struct chassis_features { > bool fdb_timestamp; > bool ls_dpg_column; > bool ct_commit_nat_v2; > + bool ct_commit_to_zone; > }; > > struct global_config_tracked_data { > diff --git a/ovn-sb.xml b/ovn-sb.xml > index 4c26c6714..0d1f640dd 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -1432,6 +1432,30 @@ > </p> > </dd> > > + <dt><code>ct_commit_to_zone(dnat);</code></dt> > + <dt><code>ct_commit_to_zone(snat);</code></dt> > + <dd> > + <p> > + Commit the flow to the specific zone in the connection > tracker. > + Similar to the <code>ct_commit</code> action, this action > requires > + previous call to <code>ct_next</code> to initialize connection > + tracking state. > + </p> > ct_next implies only the DNAT zone, which is not correct for SNAT. > + > + <p> > + If you want processing to continue in the next table, you must > + execute the <code>next</code> action after > + <code>ct_commit_to_zone</code>. > + </p> > As mentioned above, I don't think we will want to commit without proceeding. > + > + <p> > + Note that this action is meaningful only in the Logical Router > + Datapath as the Logical Switch Datapath does not use separate > + connection tracking zones. Using this action in Logical Switch > + Datapath falls back to committing the flow into the switch's > + default zone. > + </p> > + </dd> > <dt><code>ct_dnat;</code></dt> > <dt><code>ct_dnat(<var>IP</var>);</code></dt> > <dd> > diff --git a/tests/ovn.at b/tests/ovn.at > index c8cc1d37f..cbb303459 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1316,6 +1316,22 @@ ct_commit { > ct_label=0x181716151413121110090807060504030201; }; > ct_commit { ip4.dst = 192.168.0.1; }; > Field ip4.dst is not modifiable. > > +# ct_commit_to_zone > +ct_commit_to_zone(dnat); > + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > +ct_commit_to_zone(dnat); > s/dnat/snat > + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > +ct_commit_to_zone; > + Syntax error at `;' expecting `('. > +ct_commit_to_zone(); > + Syntax error at `)' expecting parameter 'dnat' or 'snat'. > +ct_commit_to_zone(foo); > + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. > +ct_commit_to_zone(dnat; > + Syntax error at `;' expecting `)'. > + > # Legact ct_commit_v1 action. > ct_commit(); > Syntax error at `(' expecting `;'. > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 5e55fbbcc..554682cd0 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, size_t > ovnacts_len, > execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, > super); > break; > > + case OVNACT_CT_COMMIT_TO_ZONE: > case OVNACT_CT_COMMIT_V2: > /* Nothing to do. */ > break; > -- > 2.40.1 > > Thanks, Ales
On 4/17/24 09:04, Ales Musil wrote: > On Mon, Apr 15, 2024 at 2:15 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, > Hi Martin, Ales, > thank you for the followup. I have a couple of comments down below. > I have a few of my own. :) > >> controller/chassis.c | 8 ++++++ >> include/ovn/actions.h | 1 + >> include/ovn/features.h | 1 + >> lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++ >> northd/en-global-config.c | 11 +++++++ >> northd/en-global-config.h | 1 + >> ovn-sb.xml | 24 ++++++++++++++++ >> tests/ovn.at | 16 +++++++++++ >> utilities/ovn-trace.c | 1 + >> 9 files changed, 123 insertions(+) >> >> diff --git a/controller/chassis.c b/controller/chassis.c >> index ad75df288..9bb2eba95 100644 >> --- a/controller/chassis.c >> +++ b/controller/chassis.c >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct >> ovs_chassis_cfg *ovs_cfg, >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); >> + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); >> } >> >> /* >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct >> ovs_chassis_cfg *ovs_cfg, >> return true; >> } >> >> + if (!smap_get_bool(&chassis_rec->other_config, >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >> + false)) { >> + return true; >> + } >> + >> return false; >> } >> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); >> + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); >> } >> >> static void >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index 8e794450c..4bcd1f58c 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -68,6 +68,7 @@ struct collector_set_ids; >> OVNACT(CT_NEXT, ovnact_ct_next) \ >> /* CT_COMMIT_V1 is not supported anymore. */ \ >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >> + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ >> OVNACT(CT_DNAT, ovnact_ct_nat) \ >> OVNACT(CT_SNAT, ovnact_ct_nat) \ >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ >> diff --git a/include/ovn/features.h b/include/ovn/features.h >> index 08f1d8288..35a5d8ba0 100644 >> --- a/include/ovn/features.h >> +++ b/include/ovn/features.h >> @@ -28,6 +28,7 @@ >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" >> >> /* OVS datapath supported features. Based on availability OVN might >> generate >> * different types of openflows. >> diff --git a/lib/actions.c b/lib/actions.c >> index 39bb5bc8a..f26817018 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >> ct = ofpacts->header; >> ofpact_finish(ofpacts, &ct->ofpact); >> +} >> + >> +static void >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) >> +{ >> + add_prerequisite(ctx, "ip"); >> + >> + struct ovnact_ct_commit_nat *ct_commit = >> + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); >> > > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed > if anything with a flag indicating whether there should be nat or not. By > doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone can > be the same differentiated by the flag. > Just to be clear, your suggestion is to change the ct_commit_to_zone() action so that it accepts an optional "nat" argument, right? That would allow actions like: ct_commit_to_zone(snat, nat) Can we do this as a follow up and also update northd to generate flows with ct_commit_to_zone() instead of ct_commit_nat()? > >> + >> + lexer_force_match(ctx->lexer, LEX_T_LPAREN); >> + >> + if (lexer_match_id(ctx->lexer, "dnat")) { >> + ct_commit->dnat_zone = true; >> + } else if (lexer_match_id(ctx->lexer, "snat")) { >> + ct_commit->dnat_zone = false; >> + } else { >> + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or >> 'snat'"); >> + } >> + >> + lexer_force_match(ctx->lexer, LEX_T_RPAREN); >> +} >> + >> +static void >> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, >> + struct ds *s) >> +{ >> + ds_put_format(s, "ct_commit_to_zone(%s);", >> + ct_commit->dnat_zone ? "dnat" : "snat"); >> + >> +} >> + >> +static void >> +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, >> + const struct ovnact_encode_params *ep, >> + struct ofpbuf *ofpacts) >> +{ >> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >> + ct->flags = NX_CT_F_COMMIT; >> + ct->recirc_table = NX_CT_RECIRC_NONE; >> > > I don't think this action is ever used standalone so we can actually > include the next table to avoid writing "ct_commit_to_zone(snat); next;". > > We'd have to update ovn-trace too in that case. I'm a bit on the fence with this one though. We do have "ct_next;" already and that implicitly advances to the next table. But I think I prefer being explicit in the logical flow actions (and adding "; next;" in northd). In any case, not a must, I'm OK with implicit next too. >> + ct->zone_src.ofs = 0; >> + ct->zone_src.n_bits = 16; >> + >> + if (ep->is_switch) { >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >> + } else { >> + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone >> + ? MFF_LOG_DNAT_ZONE >> + : MFF_LOG_SNAT_ZONE); >> + } >> + >> + size_t set_field_offset = ofpacts->size; >> + ofpbuf_pull(ofpacts, set_field_offset); >> + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >> + ct = ofpacts->header; >> + ofpact_finish(ofpacts, &ct->ofpact); >> > > This is the "manual" way of finishing the action encoding. There is a way > shorter and more explicit way to do it: > > ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > ofpacts->header = ct; > ofpact_finish_CT(ofpacts, &ct); > > > >> + >> + Nit: too many empty lines. >> } >> >> static void >> @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) >> parse_CT_NEXT(ctx); >> } else if (lexer_match_id(ctx->lexer, "ct_commit")) { >> parse_CT_COMMIT(ctx); >> + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { >> + parse_CT_COMMIT_TO_ZONE(ctx); >> } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { >> parse_CT_DNAT(ctx); >> } else if (lexer_match_id(ctx->lexer, "ct_snat")) { >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >> index 34e393b33..842ac5b64 100644 >> --- a/northd/en-global-config.c >> +++ b/northd/en-global-config.c >> @@ -370,6 +370,7 @@ northd_enable_all_features(struct >> ed_type_global_config *data) >> .fdb_timestamp = true, >> .ls_dpg_column = true, >> .ct_commit_nat_v2 = true, >> + .ct_commit_to_zone = true, >> }; >> } >> >> @@ -439,6 +440,16 @@ build_chassis_features(const struct >> sbrec_chassis_table *sbrec_chassis_table, >> chassis_features->ct_commit_nat_v2) { >> chassis_features->ct_commit_nat_v2 = false; >> } >> + >> + bool ct_commit_to_zone = >> + smap_get_bool(&chassis->other_config, >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >> + false); >> + if (!ct_commit_to_zone && >> + chassis_features->ct_commit_to_zone) { >> + chassis_features->ct_commit_to_zone = false; >> + } >> + >> } >> } >> >> diff --git a/northd/en-global-config.h b/northd/en-global-config.h >> index 38d732808..842bcee70 100644 >> --- a/northd/en-global-config.h >> +++ b/northd/en-global-config.h >> @@ -20,6 +20,7 @@ struct chassis_features { >> bool fdb_timestamp; >> bool ls_dpg_column; >> bool ct_commit_nat_v2; >> + bool ct_commit_to_zone; >> }; >> >> struct global_config_tracked_data { >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> index 4c26c6714..0d1f640dd 100644 >> --- a/ovn-sb.xml >> +++ b/ovn-sb.xml >> @@ -1432,6 +1432,30 @@ >> </p> >> </dd> >> >> + <dt><code>ct_commit_to_zone(dnat);</code></dt> >> + <dt><code>ct_commit_to_zone(snat);</code></dt> >> + <dd> >> + <p> >> + Commit the flow to the specific zone in the connection >> tracker. >> + Similar to the <code>ct_commit</code> action, this action >> requires >> + previous call to <code>ct_next</code> to initialize connection >> + tracking state. >> + </p> >> > > ct_next implies only the DNAT zone, which is not correct for SNAT. > > >> + >> + <p> >> + If you want processing to continue in the next table, you must >> + execute the <code>next</code> action after >> + <code>ct_commit_to_zone</code>. >> + </p> >> > > As mentioned above, I don't think we will want to commit without proceeding. > > >> + >> + <p> >> + Note that this action is meaningful only in the Logical Router >> + Datapath as the Logical Switch Datapath does not use separate >> + connection tracking zones. Using this action in Logical Switch >> + Datapath falls back to committing the flow into the switch's >> + default zone. This is not accurate. It's actually the logical port's conntrack zone. >> + </p> >> + </dd> >> <dt><code>ct_dnat;</code></dt> >> <dt><code>ct_dnat(<var>IP</var>);</code></dt> >> <dd> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index c8cc1d37f..cbb303459 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -1316,6 +1316,22 @@ ct_commit { >> ct_label=0x181716151413121110090807060504030201; }; >> ct_commit { ip4.dst = 192.168.0.1; }; >> Field ip4.dst is not modifiable. >> >> +# ct_commit_to_zone >> +ct_commit_to_zone(dnat); >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >> + has prereqs ip >> +ct_commit_to_zone(dnat); >> > > s/dnat/snat > > >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >> + has prereqs ip >> +ct_commit_to_zone; >> + Syntax error at `;' expecting `('. >> +ct_commit_to_zone(); >> + Syntax error at `)' expecting parameter 'dnat' or 'snat'. >> +ct_commit_to_zone(foo); >> + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. >> +ct_commit_to_zone(dnat; >> + Syntax error at `;' expecting `)'. >> + >> # Legact ct_commit_v1 action. >> ct_commit(); >> Syntax error at `(' expecting `;'. >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >> index 5e55fbbcc..554682cd0 100644 >> --- a/utilities/ovn-trace.c >> +++ b/utilities/ovn-trace.c >> @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, size_t >> ovnacts_len, >> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, >> super); >> break; >> >> + case OVNACT_CT_COMMIT_TO_ZONE: >> case OVNACT_CT_COMMIT_V2: >> /* Nothing to do. */ >> break; >> -- >> 2.40.1 >> >> > Thanks, > Ales > Regards, Dumitru
On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 4/17/24 09:04, Ales Musil wrote: > > On Mon, Apr 15, 2024 at 2:15 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, > > > > Hi Martin, Ales, > > > thank you for the followup. I have a couple of comments down below. > > > > I have a few of my own. :) > > > > >> controller/chassis.c | 8 ++++++ > >> include/ovn/actions.h | 1 + > >> include/ovn/features.h | 1 + > >> lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++ > >> northd/en-global-config.c | 11 +++++++ > >> northd/en-global-config.h | 1 + > >> ovn-sb.xml | 24 ++++++++++++++++ > >> tests/ovn.at | 16 +++++++++++ > >> utilities/ovn-trace.c | 1 + > >> 9 files changed, 123 insertions(+) > >> > >> diff --git a/controller/chassis.c b/controller/chassis.c > >> index ad75df288..9bb2eba95 100644 > >> --- a/controller/chassis.c > >> +++ b/controller/chassis.c > >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct > >> ovs_chassis_cfg *ovs_cfg, > >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); > >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); > >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); > >> + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); > >> } > >> > >> /* > >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct > >> ovs_chassis_cfg *ovs_cfg, > >> return true; > >> } > >> > >> + if (!smap_get_bool(&chassis_rec->other_config, > >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, > >> + false)) { > >> + return true; > >> + } > >> + > >> return false; > >> } > >> > >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) > >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); > >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); > >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); > >> + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); > >> } > >> > >> static void > >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h > >> index 8e794450c..4bcd1f58c 100644 > >> --- a/include/ovn/actions.h > >> +++ b/include/ovn/actions.h > >> @@ -68,6 +68,7 @@ struct collector_set_ids; > >> OVNACT(CT_NEXT, ovnact_ct_next) \ > >> /* CT_COMMIT_V1 is not supported anymore. */ \ > >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ > >> + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ > >> OVNACT(CT_DNAT, ovnact_ct_nat) \ > >> OVNACT(CT_SNAT, ovnact_ct_nat) \ > >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ > >> diff --git a/include/ovn/features.h b/include/ovn/features.h > >> index 08f1d8288..35a5d8ba0 100644 > >> --- a/include/ovn/features.h > >> +++ b/include/ovn/features.h > >> @@ -28,6 +28,7 @@ > >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" > >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" > >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" > >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" > >> > >> /* OVS datapath supported features. Based on availability OVN might > >> generate > >> * different types of openflows. > >> diff --git a/lib/actions.c b/lib/actions.c > >> index 39bb5bc8a..f26817018 100644 > >> --- a/lib/actions.c > >> +++ b/lib/actions.c > >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > >> ct = ofpacts->header; > >> ofpact_finish(ofpacts, &ct->ofpact); > >> +} > >> + > >> +static void > >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) > >> +{ > >> + add_prerequisite(ctx, "ip"); > >> + > >> + struct ovnact_ct_commit_nat *ct_commit = > >> + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); > >> > > > > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed > > if anything with a flag indicating whether there should be nat or not. By > > doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone > can > > be the same differentiated by the flag. > > > > Just to be clear, your suggestion is to change the ct_commit_to_zone() > action so that it accepts an optional "nat" argument, right? That would > allow actions like: > > ct_commit_to_zone(snat, nat) > Actually no, it can be done as a follow up as you said however this suggestion was more just to reuse the actions.c functionality we already have for ct_commit_nat. > > Can we do this as a follow up and also update northd to generate flows > with ct_commit_to_zone() instead of ct_commit_nat()? > > > > > >> + > >> + lexer_force_match(ctx->lexer, LEX_T_LPAREN); > >> + > >> + if (lexer_match_id(ctx->lexer, "dnat")) { > >> + ct_commit->dnat_zone = true; > >> + } else if (lexer_match_id(ctx->lexer, "snat")) { > >> + ct_commit->dnat_zone = false; > >> + } else { > >> + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or > >> 'snat'"); > >> + } > >> + > >> + lexer_force_match(ctx->lexer, LEX_T_RPAREN); > >> +} > >> + > >> +static void > >> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, > >> + struct ds *s) > >> +{ > >> + ds_put_format(s, "ct_commit_to_zone(%s);", > >> + ct_commit->dnat_zone ? "dnat" : "snat"); > >> + > >> +} > >> + > >> +static void > >> +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, > >> + const struct ovnact_encode_params *ep, > >> + struct ofpbuf *ofpacts) > >> +{ > >> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > >> + ct->flags = NX_CT_F_COMMIT; > >> + ct->recirc_table = NX_CT_RECIRC_NONE; > >> > > > > I don't think this action is ever used standalone so we can actually > > include the next table to avoid writing "ct_commit_to_zone(snat); next;". > > > > > > We'd have to update ovn-trace too in that case. I'm a bit on the fence > with this one though. We do have "ct_next;" already and that implicitly > advances to the next table. But I think I prefer being explicit in the > logical flow actions (and adding "; next;" in northd). In any case, not > a must, I'm OK with implicit next too. > Only ct_commit is actually explicit about it, any other will implicitly advance to the next table. > > >> + ct->zone_src.ofs = 0; > >> + ct->zone_src.n_bits = 16; > >> + > >> + if (ep->is_switch) { > >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > >> + } else { > >> + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone > >> + ? MFF_LOG_DNAT_ZONE > >> + : MFF_LOG_SNAT_ZONE); > >> + } > >> + > >> + size_t set_field_offset = ofpacts->size; > >> + ofpbuf_pull(ofpacts, set_field_offset); > >> + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > >> + ct = ofpacts->header; > >> + ofpact_finish(ofpacts, &ct->ofpact); > >> > > > > This is the "manual" way of finishing the action encoding. There is a way > > shorter and more explicit way to do it: > > > > ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > > ofpacts->header = ct; > > ofpact_finish_CT(ofpacts, &ct); > > > > > > > >> + > >> + > > Nit: too many empty lines. > > >> } > >> > >> static void > >> @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) > >> parse_CT_NEXT(ctx); > >> } else if (lexer_match_id(ctx->lexer, "ct_commit")) { > >> parse_CT_COMMIT(ctx); > >> + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { > >> + parse_CT_COMMIT_TO_ZONE(ctx); > >> } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { > >> parse_CT_DNAT(ctx); > >> } else if (lexer_match_id(ctx->lexer, "ct_snat")) { > >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c > >> index 34e393b33..842ac5b64 100644 > >> --- a/northd/en-global-config.c > >> +++ b/northd/en-global-config.c > >> @@ -370,6 +370,7 @@ northd_enable_all_features(struct > >> ed_type_global_config *data) > >> .fdb_timestamp = true, > >> .ls_dpg_column = true, > >> .ct_commit_nat_v2 = true, > >> + .ct_commit_to_zone = true, > >> }; > >> } > >> > >> @@ -439,6 +440,16 @@ build_chassis_features(const struct > >> sbrec_chassis_table *sbrec_chassis_table, > >> chassis_features->ct_commit_nat_v2) { > >> chassis_features->ct_commit_nat_v2 = false; > >> } > >> + > >> + bool ct_commit_to_zone = > >> + smap_get_bool(&chassis->other_config, > >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, > >> + false); > >> + if (!ct_commit_to_zone && > >> + chassis_features->ct_commit_to_zone) { > >> + chassis_features->ct_commit_to_zone = false; > >> + } > >> + > >> } > >> } > >> > >> diff --git a/northd/en-global-config.h b/northd/en-global-config.h > >> index 38d732808..842bcee70 100644 > >> --- a/northd/en-global-config.h > >> +++ b/northd/en-global-config.h > >> @@ -20,6 +20,7 @@ struct chassis_features { > >> bool fdb_timestamp; > >> bool ls_dpg_column; > >> bool ct_commit_nat_v2; > >> + bool ct_commit_to_zone; > >> }; > >> > >> struct global_config_tracked_data { > >> diff --git a/ovn-sb.xml b/ovn-sb.xml > >> index 4c26c6714..0d1f640dd 100644 > >> --- a/ovn-sb.xml > >> +++ b/ovn-sb.xml > >> @@ -1432,6 +1432,30 @@ > >> </p> > >> </dd> > >> > >> + <dt><code>ct_commit_to_zone(dnat);</code></dt> > >> + <dt><code>ct_commit_to_zone(snat);</code></dt> > >> + <dd> > >> + <p> > >> + Commit the flow to the specific zone in the connection > >> tracker. > >> + Similar to the <code>ct_commit</code> action, this action > >> requires > >> + previous call to <code>ct_next</code> to initialize > connection > >> + tracking state. > >> + </p> > >> > > > > ct_next implies only the DNAT zone, which is not correct for SNAT. > > > > > >> + > >> + <p> > >> + If you want processing to continue in the next table, you > must > >> + execute the <code>next</code> action after > >> + <code>ct_commit_to_zone</code>. > >> + </p> > >> > > > > As mentioned above, I don't think we will want to commit without > proceeding. > > > > > >> + > >> + <p> > >> + Note that this action is meaningful only in the Logical > Router > >> + Datapath as the Logical Switch Datapath does not use > separate > >> + connection tracking zones. Using this action in Logical > Switch > >> + Datapath falls back to committing the flow into the > switch's > >> + default zone. > > This is not accurate. It's actually the logical port's conntrack zone. > > >> + </p> > >> + </dd> > >> <dt><code>ct_dnat;</code></dt> > >> <dt><code>ct_dnat(<var>IP</var>);</code></dt> > >> <dd> > >> diff --git a/tests/ovn.at b/tests/ovn.at > >> index c8cc1d37f..cbb303459 100644 > >> --- a/tests/ovn.at > >> +++ b/tests/ovn.at > >> @@ -1316,6 +1316,22 @@ ct_commit { > >> ct_label=0x181716151413121110090807060504030201; }; > >> ct_commit { ip4.dst = 192.168.0.1; }; > >> Field ip4.dst is not modifiable. > >> > >> +# ct_commit_to_zone > >> +ct_commit_to_zone(dnat); > >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > >> + has prereqs ip > >> +ct_commit_to_zone(dnat); > >> > > > > s/dnat/snat > > > > > >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > >> + has prereqs ip > >> +ct_commit_to_zone; > >> + Syntax error at `;' expecting `('. > >> +ct_commit_to_zone(); > >> + Syntax error at `)' expecting parameter 'dnat' or 'snat'. > >> +ct_commit_to_zone(foo); > >> + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. > >> +ct_commit_to_zone(dnat; > >> + Syntax error at `;' expecting `)'. > >> + > >> # Legact ct_commit_v1 action. > >> ct_commit(); > >> Syntax error at `(' expecting `;'. > >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > >> index 5e55fbbcc..554682cd0 100644 > >> --- a/utilities/ovn-trace.c > >> +++ b/utilities/ovn-trace.c > >> @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, size_t > >> ovnacts_len, > >> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, > >> super); > >> break; > >> > >> + case OVNACT_CT_COMMIT_TO_ZONE: > >> case OVNACT_CT_COMMIT_V2: > >> /* Nothing to do. */ > >> break; > >> -- > >> 2.40.1 > >> > >> > > Thanks, > > Ales > > > > Regards, > Dumitru > >
Thanks for another round of review. On Wed, Apr 17, 2024 at 1:45 PM Ales Musil <amusil@redhat.com> wrote: > > > On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> On 4/17/24 09:04, Ales Musil wrote: >> > On Mon, Apr 15, 2024 at 2:15 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, >> > >> >> Hi Martin, Ales, >> >> > thank you for the followup. I have a couple of comments down below. >> > >> >> I have a few of my own. :) >> >> > >> >> controller/chassis.c | 8 ++++++ >> >> include/ovn/actions.h | 1 + >> >> include/ovn/features.h | 1 + >> >> lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++ >> >> northd/en-global-config.c | 11 +++++++ >> >> northd/en-global-config.h | 1 + >> >> ovn-sb.xml | 24 ++++++++++++++++ >> >> tests/ovn.at | 16 +++++++++++ >> >> utilities/ovn-trace.c | 1 + >> >> 9 files changed, 123 insertions(+) >> >> >> >> diff --git a/controller/chassis.c b/controller/chassis.c >> >> index ad75df288..9bb2eba95 100644 >> >> --- a/controller/chassis.c >> >> +++ b/controller/chassis.c >> >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct >> >> ovs_chassis_cfg *ovs_cfg, >> >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); >> >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); >> >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); >> >> + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); >> >> } >> >> >> >> /* >> >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct >> >> ovs_chassis_cfg *ovs_cfg, >> >> return true; >> >> } >> >> >> >> + if (!smap_get_bool(&chassis_rec->other_config, >> >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >> >> + false)) { >> >> + return true; >> >> + } >> >> + >> >> return false; >> >> } >> >> >> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) >> >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); >> >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); >> >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); >> >> + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); >> >> } >> >> >> >> static void >> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> >> index 8e794450c..4bcd1f58c 100644 >> >> --- a/include/ovn/actions.h >> >> +++ b/include/ovn/actions.h >> >> @@ -68,6 +68,7 @@ struct collector_set_ids; >> >> OVNACT(CT_NEXT, ovnact_ct_next) \ >> >> /* CT_COMMIT_V1 is not supported anymore. */ \ >> >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >> >> + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ >> >> OVNACT(CT_DNAT, ovnact_ct_nat) \ >> >> OVNACT(CT_SNAT, ovnact_ct_nat) \ >> >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ >> >> diff --git a/include/ovn/features.h b/include/ovn/features.h >> >> index 08f1d8288..35a5d8ba0 100644 >> >> --- a/include/ovn/features.h >> >> +++ b/include/ovn/features.h >> >> @@ -28,6 +28,7 @@ >> >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" >> >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" >> >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" >> >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" >> >> >> >> /* OVS datapath supported features. Based on availability OVN might >> >> generate >> >> * different types of openflows. >> >> diff --git a/lib/actions.c b/lib/actions.c >> >> index 39bb5bc8a..f26817018 100644 >> >> --- a/lib/actions.c >> >> +++ b/lib/actions.c >> >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >> >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >> >> ct = ofpacts->header; >> >> ofpact_finish(ofpacts, &ct->ofpact); >> >> +} >> >> + >> >> +static void >> >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) >> >> +{ >> >> + add_prerequisite(ctx, "ip"); >> >> + >> >> + struct ovnact_ct_commit_nat *ct_commit = >> >> + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); >> >> >> > >> > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be >> renamed >> > if anything with a flag indicating whether there should be nat or not. >> By >> > doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone >> can >> > be the same differentiated by the flag. >> > >> >> Just to be clear, your suggestion is to change the ct_commit_to_zone() >> action so that it accepts an optional "nat" argument, right? That would >> allow actions like: >> >> ct_commit_to_zone(snat, nat) >> > > Actually no, it can be done as a follow up as you said however this > suggestion was more just to reuse the actions.c functionality we already > have for ct_commit_nat. > Just to make sure I understand this correctly Ales, you'd like me to rename the `ovnact_ct_commit_nat` structure and add a field like `bool do_nat` to it. Then adjust current `{parse,encode,format}_CT_COMMIT_NAT` functions to differentiate based on the `do_nat` variable (and presumably rename these functions too, to match the new structure name)? > > >> >> Can we do this as a follow up and also update northd to generate flows >> with ct_commit_to_zone() instead of ct_commit_nat()? >> > > >> >> > >> >> + >> >> + lexer_force_match(ctx->lexer, LEX_T_LPAREN); >> >> + >> >> + if (lexer_match_id(ctx->lexer, "dnat")) { >> >> + ct_commit->dnat_zone = true; >> >> + } else if (lexer_match_id(ctx->lexer, "snat")) { >> >> + ct_commit->dnat_zone = false; >> >> + } else { >> >> + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or >> >> 'snat'"); >> >> + } >> >> + >> >> + lexer_force_match(ctx->lexer, LEX_T_RPAREN); >> >> +} >> >> + >> >> +static void >> >> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, >> >> + struct ds *s) >> >> +{ >> >> + ds_put_format(s, "ct_commit_to_zone(%s);", >> >> + ct_commit->dnat_zone ? "dnat" : "snat"); >> >> + >> >> +} >> >> + >> >> +static void >> >> +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, >> >> + const struct ovnact_encode_params *ep, >> >> + struct ofpbuf *ofpacts) >> >> +{ >> >> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >> >> + ct->flags = NX_CT_F_COMMIT; >> >> + ct->recirc_table = NX_CT_RECIRC_NONE; >> >> >> > >> > I don't think this action is ever used standalone so we can actually >> > include the next table to avoid writing "ct_commit_to_zone(snat); >> next;". >> > >> > >> >> We'd have to update ovn-trace too in that case. I'm a bit on the fence >> with this one though. We do have "ct_next;" already and that implicitly >> advances to the next table. But I think I prefer being explicit in the >> logical flow actions (and adding "; next;" in northd). In any case, not >> a must, I'm OK with implicit next too. >> > > Only ct_commit is actually explicit about it, any other will implicitly > advance to the next table. > > >> >> >> + ct->zone_src.ofs = 0; >> >> + ct->zone_src.n_bits = 16; >> >> + >> >> + if (ep->is_switch) { >> >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >> >> + } else { >> >> + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone >> >> + ? MFF_LOG_DNAT_ZONE >> >> + : MFF_LOG_SNAT_ZONE); >> >> + } >> >> + >> >> + size_t set_field_offset = ofpacts->size; >> >> + ofpbuf_pull(ofpacts, set_field_offset); >> >> + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >> >> + ct = ofpacts->header; >> >> + ofpact_finish(ofpacts, &ct->ofpact); >> >> >> > >> > This is the "manual" way of finishing the action encoding. There is a >> way >> > shorter and more explicit way to do it: >> > >> > ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); >> > ofpacts->header = ct; >> > ofpact_finish_CT(ofpacts, &ct); >> > >> > >> > >> >> + >> >> + >> >> Nit: too many empty lines. >> > ack > >> >> } >> >> >> >> static void >> >> @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) >> >> parse_CT_NEXT(ctx); >> >> } else if (lexer_match_id(ctx->lexer, "ct_commit")) { >> >> parse_CT_COMMIT(ctx); >> >> + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { >> >> + parse_CT_COMMIT_TO_ZONE(ctx); >> >> } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { >> >> parse_CT_DNAT(ctx); >> >> } else if (lexer_match_id(ctx->lexer, "ct_snat")) { >> >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >> >> index 34e393b33..842ac5b64 100644 >> >> --- a/northd/en-global-config.c >> >> +++ b/northd/en-global-config.c >> >> @@ -370,6 +370,7 @@ northd_enable_all_features(struct >> >> ed_type_global_config *data) >> >> .fdb_timestamp = true, >> >> .ls_dpg_column = true, >> >> .ct_commit_nat_v2 = true, >> >> + .ct_commit_to_zone = true, >> >> }; >> >> } >> >> >> >> @@ -439,6 +440,16 @@ build_chassis_features(const struct >> >> sbrec_chassis_table *sbrec_chassis_table, >> >> chassis_features->ct_commit_nat_v2) { >> >> chassis_features->ct_commit_nat_v2 = false; >> >> } >> >> + >> >> + bool ct_commit_to_zone = >> >> + smap_get_bool(&chassis->other_config, >> >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >> >> + false); >> >> + if (!ct_commit_to_zone && >> >> + chassis_features->ct_commit_to_zone) { >> >> + chassis_features->ct_commit_to_zone = false; >> >> + } >> >> + >> >> } >> >> } >> >> >> >> diff --git a/northd/en-global-config.h b/northd/en-global-config.h >> >> index 38d732808..842bcee70 100644 >> >> --- a/northd/en-global-config.h >> >> +++ b/northd/en-global-config.h >> >> @@ -20,6 +20,7 @@ struct chassis_features { >> >> bool fdb_timestamp; >> >> bool ls_dpg_column; >> >> bool ct_commit_nat_v2; >> >> + bool ct_commit_to_zone; >> >> }; >> >> >> >> struct global_config_tracked_data { >> >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> >> index 4c26c6714..0d1f640dd 100644 >> >> --- a/ovn-sb.xml >> >> +++ b/ovn-sb.xml >> >> @@ -1432,6 +1432,30 @@ >> >> </p> >> >> </dd> >> >> >> >> + <dt><code>ct_commit_to_zone(dnat);</code></dt> >> >> + <dt><code>ct_commit_to_zone(snat);</code></dt> >> >> + <dd> >> >> + <p> >> >> + Commit the flow to the specific zone in the connection >> >> tracker. >> >> + Similar to the <code>ct_commit</code> action, this action >> >> requires >> >> + previous call to <code>ct_next</code> to initialize >> connection >> >> + tracking state. >> >> + </p> >> >> >> > >> > ct_next implies only the DNAT zone, which is not correct for SNAT. >> > Oh right, my bad, `ct_snat` or `ct_dnat` are the correct thing to do here (based on which zone you want to use), right? > > >> > >> >> + >> >> + <p> >> >> + If you want processing to continue in the next table, you >> must >> >> + execute the <code>next</code> action after >> >> + <code>ct_commit_to_zone</code>. >> >> + </p> >> >> >> > >> > As mentioned above, I don't think we will want to commit without >> proceeding. >> > >> > >> >> + >> >> + <p> >> >> + Note that this action is meaningful only in the Logical >> Router >> >> + Datapath as the Logical Switch Datapath does not use >> separate >> >> + connection tracking zones. Using this action in Logical >> Switch >> >> + Datapath falls back to committing the flow into the >> switch's >> >> + default zone. >> >> This is not accurate. It's actually the logical port's conntrack zone. >> > thx, ack. > >> >> + </p> >> >> + </dd> >> >> <dt><code>ct_dnat;</code></dt> >> >> <dt><code>ct_dnat(<var>IP</var>);</code></dt> >> >> <dd> >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> >> index c8cc1d37f..cbb303459 100644 >> >> --- a/tests/ovn.at >> >> +++ b/tests/ovn.at >> >> @@ -1316,6 +1316,22 @@ ct_commit { >> >> ct_label=0x181716151413121110090807060504030201; }; >> >> ct_commit { ip4.dst = 192.168.0.1; }; >> >> Field ip4.dst is not modifiable. >> >> >> >> +# ct_commit_to_zone >> >> +ct_commit_to_zone(dnat); >> >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >> >> + has prereqs ip >> >> +ct_commit_to_zone(dnat); >> >> >> > >> > s/dnat/snat >> > thx, ack. > > >> > >> >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >> >> + has prereqs ip >> >> +ct_commit_to_zone; >> >> + Syntax error at `;' expecting `('. >> >> +ct_commit_to_zone(); >> >> + Syntax error at `)' expecting parameter 'dnat' or 'snat'. >> >> +ct_commit_to_zone(foo); >> >> + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. >> >> +ct_commit_to_zone(dnat; >> >> + Syntax error at `;' expecting `)'. >> >> + >> >> # Legact ct_commit_v1 action. >> >> ct_commit(); >> >> Syntax error at `(' expecting `;'. >> >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >> >> index 5e55fbbcc..554682cd0 100644 >> >> --- a/utilities/ovn-trace.c >> >> +++ b/utilities/ovn-trace.c >> >> @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, >> size_t >> >> ovnacts_len, >> >> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, >> pipeline, >> >> super); >> >> break; >> >> >> >> + case OVNACT_CT_COMMIT_TO_ZONE: >> >> case OVNACT_CT_COMMIT_V2: >> >> /* Nothing to do. */ >> >> break; >> >> -- >> >> 2.40.1 >> >> >> >> >> > Thanks, >> > Ales >> > >> >> Regards, >> Dumitru >> >> > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> >
On Thu, Apr 18, 2024 at 11:45 AM Martin Kalcok <martin.kalcok@canonical.com> wrote: > Thanks for another round of review. > > On Wed, Apr 17, 2024 at 1:45 PM Ales Musil <amusil@redhat.com> wrote: > >> >> >> On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >>> On 4/17/24 09:04, Ales Musil wrote: >>> > On Mon, Apr 15, 2024 at 2:15 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, >>> > >>> >>> Hi Martin, Ales, >>> >>> > thank you for the followup. I have a couple of comments down below. >>> > >>> >>> I have a few of my own. :) >>> >>> > >>> >> controller/chassis.c | 8 ++++++ >>> >> include/ovn/actions.h | 1 + >>> >> include/ovn/features.h | 1 + >>> >> lib/actions.c | 60 >>> +++++++++++++++++++++++++++++++++++++++ >>> >> northd/en-global-config.c | 11 +++++++ >>> >> northd/en-global-config.h | 1 + >>> >> ovn-sb.xml | 24 ++++++++++++++++ >>> >> tests/ovn.at | 16 +++++++++++ >>> >> utilities/ovn-trace.c | 1 + >>> >> 9 files changed, 123 insertions(+) >>> >> >>> >> diff --git a/controller/chassis.c b/controller/chassis.c >>> >> index ad75df288..9bb2eba95 100644 >>> >> --- a/controller/chassis.c >>> >> +++ b/controller/chassis.c >>> >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct >>> >> ovs_chassis_cfg *ovs_cfg, >>> >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); >>> >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); >>> >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); >>> >> + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); >>> >> } >>> >> >>> >> /* >>> >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct >>> >> ovs_chassis_cfg *ovs_cfg, >>> >> return true; >>> >> } >>> >> >>> >> + if (!smap_get_bool(&chassis_rec->other_config, >>> >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >>> >> + false)) { >>> >> + return true; >>> >> + } >>> >> + >>> >> return false; >>> >> } >>> >> >>> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) >>> >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); >>> >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); >>> >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); >>> >> + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); >>> >> } >>> >> >>> >> static void >>> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >>> >> index 8e794450c..4bcd1f58c 100644 >>> >> --- a/include/ovn/actions.h >>> >> +++ b/include/ovn/actions.h >>> >> @@ -68,6 +68,7 @@ struct collector_set_ids; >>> >> OVNACT(CT_NEXT, ovnact_ct_next) \ >>> >> /* CT_COMMIT_V1 is not supported anymore. */ \ >>> >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >>> >> + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ >>> >> OVNACT(CT_DNAT, ovnact_ct_nat) \ >>> >> OVNACT(CT_SNAT, ovnact_ct_nat) \ >>> >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ >>> >> diff --git a/include/ovn/features.h b/include/ovn/features.h >>> >> index 08f1d8288..35a5d8ba0 100644 >>> >> --- a/include/ovn/features.h >>> >> +++ b/include/ovn/features.h >>> >> @@ -28,6 +28,7 @@ >>> >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" >>> >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" >>> >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" >>> >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" >>> >> >>> >> /* OVS datapath supported features. Based on availability OVN might >>> >> generate >>> >> * different types of openflows. >>> >> diff --git a/lib/actions.c b/lib/actions.c >>> >> index 39bb5bc8a..f26817018 100644 >>> >> --- a/lib/actions.c >>> >> +++ b/lib/actions.c >>> >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >>> >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >>> >> ct = ofpacts->header; >>> >> ofpact_finish(ofpacts, &ct->ofpact); >>> >> +} >>> >> + >>> >> +static void >>> >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) >>> >> +{ >>> >> + add_prerequisite(ctx, "ip"); >>> >> + >>> >> + struct ovnact_ct_commit_nat *ct_commit = >>> >> + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); >>> >> >>> > >>> > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be >>> renamed >>> > if anything with a flag indicating whether there should be nat or not. >>> By >>> > doing that the parse + encoding for ct_commit_nat and >>> ct_commit_to_zone can >>> > be the same differentiated by the flag. >>> > >>> >>> Just to be clear, your suggestion is to change the ct_commit_to_zone() >>> action so that it accepts an optional "nat" argument, right? That would >>> allow actions like: >>> >>> ct_commit_to_zone(snat, nat) >>> >> >> Actually no, it can be done as a follow up as you said however this >> suggestion was more just to reuse the actions.c functionality we already >> have for ct_commit_nat. >> > > Just to make sure I understand this correctly Ales, you'd like me to > rename the `ovnact_ct_commit_nat` structure and add a field like `bool > do_nat` to it. Then adjust current `{parse,encode,format}_CT_COMMIT_NAT` > functions to differentiate based on the `do_nat` variable (and presumably > rename these functions too, to match the new structure name)? > Yeah, having some common functions for the parse and encode (the format is very simple, it doesn't need anything special). Just be aware that we need to keep the function ending with uppercase as it's part of the macro. > >> >>> >>> Can we do this as a follow up and also update northd to generate flows >>> with ct_commit_to_zone() instead of ct_commit_nat()? >>> >> >> >>> >>> > >>> >> + >>> >> + lexer_force_match(ctx->lexer, LEX_T_LPAREN); >>> >> + >>> >> + if (lexer_match_id(ctx->lexer, "dnat")) { >>> >> + ct_commit->dnat_zone = true; >>> >> + } else if (lexer_match_id(ctx->lexer, "snat")) { >>> >> + ct_commit->dnat_zone = false; >>> >> + } else { >>> >> + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or >>> >> 'snat'"); >>> >> + } >>> >> + >>> >> + lexer_force_match(ctx->lexer, LEX_T_RPAREN); >>> >> +} >>> >> + >>> >> +static void >>> >> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat >>> *ct_commit, >>> >> + struct ds *s) >>> >> +{ >>> >> + ds_put_format(s, "ct_commit_to_zone(%s);", >>> >> + ct_commit->dnat_zone ? "dnat" : "snat"); >>> >> + >>> >> +} >>> >> + >>> >> +static void >>> >> +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat >>> *ct_commit, >>> >> + const struct ovnact_encode_params *ep, >>> >> + struct ofpbuf *ofpacts) >>> >> +{ >>> >> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >>> >> + ct->flags = NX_CT_F_COMMIT; >>> >> + ct->recirc_table = NX_CT_RECIRC_NONE; >>> >> >>> > >>> > I don't think this action is ever used standalone so we can actually >>> > include the next table to avoid writing "ct_commit_to_zone(snat); >>> next;". >>> > >>> > >>> >>> We'd have to update ovn-trace too in that case. I'm a bit on the fence >>> with this one though. We do have "ct_next;" already and that implicitly >>> advances to the next table. But I think I prefer being explicit in the >>> logical flow actions (and adding "; next;" in northd). In any case, not >>> a must, I'm OK with implicit next too. >>> >> >> Only ct_commit is actually explicit about it, any other will implicitly >> advance to the next table. >> >> >>> >>> >> + ct->zone_src.ofs = 0; >>> >> + ct->zone_src.n_bits = 16; >>> >> + >>> >> + if (ep->is_switch) { >>> >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >>> >> + } else { >>> >> + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone >>> >> + ? MFF_LOG_DNAT_ZONE >>> >> + : MFF_LOG_SNAT_ZONE); >>> >> + } >>> >> + >>> >> + size_t set_field_offset = ofpacts->size; >>> >> + ofpbuf_pull(ofpacts, set_field_offset); >>> >> + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >>> >> + ct = ofpacts->header; >>> >> + ofpact_finish(ofpacts, &ct->ofpact); >>> >> >>> > >>> > This is the "manual" way of finishing the action encoding. There is a >>> way >>> > shorter and more explicit way to do it: >>> > >>> > ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); >>> > ofpacts->header = ct; >>> > ofpact_finish_CT(ofpacts, &ct); >>> > >>> > >>> > >>> >> + >>> >> + >>> >>> Nit: too many empty lines. >>> >> ack > >> >>> >> } >>> >> >>> >> static void >>> >> @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) >>> >> parse_CT_NEXT(ctx); >>> >> } else if (lexer_match_id(ctx->lexer, "ct_commit")) { >>> >> parse_CT_COMMIT(ctx); >>> >> + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { >>> >> + parse_CT_COMMIT_TO_ZONE(ctx); >>> >> } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { >>> >> parse_CT_DNAT(ctx); >>> >> } else if (lexer_match_id(ctx->lexer, "ct_snat")) { >>> >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >>> >> index 34e393b33..842ac5b64 100644 >>> >> --- a/northd/en-global-config.c >>> >> +++ b/northd/en-global-config.c >>> >> @@ -370,6 +370,7 @@ northd_enable_all_features(struct >>> >> ed_type_global_config *data) >>> >> .fdb_timestamp = true, >>> >> .ls_dpg_column = true, >>> >> .ct_commit_nat_v2 = true, >>> >> + .ct_commit_to_zone = true, >>> >> }; >>> >> } >>> >> >>> >> @@ -439,6 +440,16 @@ build_chassis_features(const struct >>> >> sbrec_chassis_table *sbrec_chassis_table, >>> >> chassis_features->ct_commit_nat_v2) { >>> >> chassis_features->ct_commit_nat_v2 = false; >>> >> } >>> >> + >>> >> + bool ct_commit_to_zone = >>> >> + smap_get_bool(&chassis->other_config, >>> >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >>> >> + false); >>> >> + if (!ct_commit_to_zone && >>> >> + chassis_features->ct_commit_to_zone) { >>> >> + chassis_features->ct_commit_to_zone = false; >>> >> + } >>> >> + >>> >> } >>> >> } >>> >> >>> >> diff --git a/northd/en-global-config.h b/northd/en-global-config.h >>> >> index 38d732808..842bcee70 100644 >>> >> --- a/northd/en-global-config.h >>> >> +++ b/northd/en-global-config.h >>> >> @@ -20,6 +20,7 @@ struct chassis_features { >>> >> bool fdb_timestamp; >>> >> bool ls_dpg_column; >>> >> bool ct_commit_nat_v2; >>> >> + bool ct_commit_to_zone; >>> >> }; >>> >> >>> >> struct global_config_tracked_data { >>> >> diff --git a/ovn-sb.xml b/ovn-sb.xml >>> >> index 4c26c6714..0d1f640dd 100644 >>> >> --- a/ovn-sb.xml >>> >> +++ b/ovn-sb.xml >>> >> @@ -1432,6 +1432,30 @@ >>> >> </p> >>> >> </dd> >>> >> >>> >> + <dt><code>ct_commit_to_zone(dnat);</code></dt> >>> >> + <dt><code>ct_commit_to_zone(snat);</code></dt> >>> >> + <dd> >>> >> + <p> >>> >> + Commit the flow to the specific zone in the connection >>> >> tracker. >>> >> + Similar to the <code>ct_commit</code> action, this action >>> >> requires >>> >> + previous call to <code>ct_next</code> to initialize >>> connection >>> >> + tracking state. >>> >> + </p> >>> >> >>> > >>> > ct_next implies only the DNAT zone, which is not correct for SNAT. >>> >> > Oh right, my bad, `ct_snat` or `ct_dnat` are the correct thing to do here > (based on which zone you want to use), right? > Yeah, actually you can commit even without going through CT before, so we might leave this out. You need this if you want to match in ct_state, but that's not required for this function. > > >> > >>> > >>> >> + >>> >> + <p> >>> >> + If you want processing to continue in the next table, >>> you must >>> >> + execute the <code>next</code> action after >>> >> + <code>ct_commit_to_zone</code>. >>> >> + </p> >>> >> >>> > >>> > As mentioned above, I don't think we will want to commit without >>> proceeding. >>> > >>> > >>> >> + >>> >> + <p> >>> >> + Note that this action is meaningful only in the Logical >>> Router >>> >> + Datapath as the Logical Switch Datapath does not use >>> separate >>> >> + connection tracking zones. Using this action in Logical >>> Switch >>> >> + Datapath falls back to committing the flow into the >>> switch's >>> >> + default zone. >>> >>> This is not accurate. It's actually the logical port's conntrack zone. >>> >> thx, ack. > >> >>> >> + </p> >>> >> + </dd> >>> >> <dt><code>ct_dnat;</code></dt> >>> >> <dt><code>ct_dnat(<var>IP</var>);</code></dt> >>> >> <dd> >>> >> diff --git a/tests/ovn.at b/tests/ovn.at >>> >> index c8cc1d37f..cbb303459 100644 >>> >> --- a/tests/ovn.at >>> >> +++ b/tests/ovn.at >>> >> @@ -1316,6 +1316,22 @@ ct_commit { >>> >> ct_label=0x181716151413121110090807060504030201; }; >>> >> ct_commit { ip4.dst = 192.168.0.1; }; >>> >> Field ip4.dst is not modifiable. >>> >> >>> >> +# ct_commit_to_zone >>> >> +ct_commit_to_zone(dnat); >>> >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >>> >> + has prereqs ip >>> >> +ct_commit_to_zone(dnat); >>> >> >>> > >>> > s/dnat/snat >>> >> thx, ack. > >> > >>> > >>> >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >>> >> + has prereqs ip >>> >> +ct_commit_to_zone; >>> >> + Syntax error at `;' expecting `('. >>> >> +ct_commit_to_zone(); >>> >> + Syntax error at `)' expecting parameter 'dnat' or 'snat'. >>> >> +ct_commit_to_zone(foo); >>> >> + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. >>> >> +ct_commit_to_zone(dnat; >>> >> + Syntax error at `;' expecting `)'. >>> >> + >>> >> # Legact ct_commit_v1 action. >>> >> ct_commit(); >>> >> Syntax error at `(' expecting `;'. >>> >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >>> >> index 5e55fbbcc..554682cd0 100644 >>> >> --- a/utilities/ovn-trace.c >>> >> +++ b/utilities/ovn-trace.c >>> >> @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, >>> size_t >>> >> ovnacts_len, >>> >> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, >>> pipeline, >>> >> super); >>> >> break; >>> >> >>> >> + case OVNACT_CT_COMMIT_TO_ZONE: >>> >> case OVNACT_CT_COMMIT_V2: >>> >> /* Nothing to do. */ >>> >> break; >>> >> -- >>> >> 2.40.1 >>> >> >>> >> >>> > Thanks, >>> > Ales >>> > >>> >>> Regards, >>> Dumitru >>> >>> >> >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> amusil@redhat.com >> <https://red.ht/sig> >> > > > -- > Best Regards, > Martin Kalcok. > Thanks, Ales
diff --git a/controller/chassis.c b/controller/chassis.c index ad75df288..9bb2eba95 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -371,6 +371,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); } /* @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, return true; } + if (!smap_get_bool(&chassis_rec->other_config, + OVN_FEATURE_CT_COMMIT_TO_ZONE, + false)) { + return true; + } + return false; } @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); } static void diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 8e794450c..4bcd1f58c 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -68,6 +68,7 @@ struct collector_set_ids; OVNACT(CT_NEXT, ovnact_ct_next) \ /* CT_COMMIT_V1 is not supported anymore. */ \ OVNACT(CT_COMMIT_V2, ovnact_nest) \ + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ OVNACT(CT_DNAT, ovnact_ct_nat) \ OVNACT(CT_SNAT, ovnact_ct_nat) \ OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ diff --git a/include/ovn/features.h b/include/ovn/features.h index 08f1d8288..35a5d8ba0 100644 --- a/include/ovn/features.h +++ b/include/ovn/features.h @@ -28,6 +28,7 @@ #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" /* OVS datapath supported features. Based on availability OVN might generate * different types of openflows. diff --git a/lib/actions.c b/lib/actions.c index 39bb5bc8a..f26817018 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); ct = ofpacts->header; ofpact_finish(ofpacts, &ct->ofpact); +} + +static void +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) +{ + add_prerequisite(ctx, "ip"); + + struct ovnact_ct_commit_nat *ct_commit = + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); + + lexer_force_match(ctx->lexer, LEX_T_LPAREN); + + if (lexer_match_id(ctx->lexer, "dnat")) { + ct_commit->dnat_zone = true; + } else if (lexer_match_id(ctx->lexer, "snat")) { + ct_commit->dnat_zone = false; + } else { + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or 'snat'"); + } + + lexer_force_match(ctx->lexer, LEX_T_RPAREN); +} + +static void +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, + struct ds *s) +{ + ds_put_format(s, "ct_commit_to_zone(%s);", + ct_commit->dnat_zone ? "dnat" : "snat"); + +} + +static void +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, + const struct ovnact_encode_params *ep, + struct ofpbuf *ofpacts) +{ + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); + ct->flags = NX_CT_F_COMMIT; + ct->recirc_table = NX_CT_RECIRC_NONE; + ct->zone_src.ofs = 0; + ct->zone_src.n_bits = 16; + + if (ep->is_switch) { + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); + } else { + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone + ? MFF_LOG_DNAT_ZONE + : MFF_LOG_SNAT_ZONE); + } + + size_t set_field_offset = ofpacts->size; + ofpbuf_pull(ofpacts, set_field_offset); + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); + ct = ofpacts->header; + ofpact_finish(ofpacts, &ct->ofpact); + + } static void @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) parse_CT_NEXT(ctx); } else if (lexer_match_id(ctx->lexer, "ct_commit")) { parse_CT_COMMIT(ctx); + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { + parse_CT_COMMIT_TO_ZONE(ctx); } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { parse_CT_DNAT(ctx); } else if (lexer_match_id(ctx->lexer, "ct_snat")) { diff --git a/northd/en-global-config.c b/northd/en-global-config.c index 34e393b33..842ac5b64 100644 --- a/northd/en-global-config.c +++ b/northd/en-global-config.c @@ -370,6 +370,7 @@ northd_enable_all_features(struct ed_type_global_config *data) .fdb_timestamp = true, .ls_dpg_column = true, .ct_commit_nat_v2 = true, + .ct_commit_to_zone = true, }; } @@ -439,6 +440,16 @@ build_chassis_features(const struct sbrec_chassis_table *sbrec_chassis_table, chassis_features->ct_commit_nat_v2) { chassis_features->ct_commit_nat_v2 = false; } + + bool ct_commit_to_zone = + smap_get_bool(&chassis->other_config, + OVN_FEATURE_CT_COMMIT_TO_ZONE, + false); + if (!ct_commit_to_zone && + chassis_features->ct_commit_to_zone) { + chassis_features->ct_commit_to_zone = false; + } + } } diff --git a/northd/en-global-config.h b/northd/en-global-config.h index 38d732808..842bcee70 100644 --- a/northd/en-global-config.h +++ b/northd/en-global-config.h @@ -20,6 +20,7 @@ struct chassis_features { bool fdb_timestamp; bool ls_dpg_column; bool ct_commit_nat_v2; + bool ct_commit_to_zone; }; struct global_config_tracked_data { diff --git a/ovn-sb.xml b/ovn-sb.xml index 4c26c6714..0d1f640dd 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -1432,6 +1432,30 @@ </p> </dd> + <dt><code>ct_commit_to_zone(dnat);</code></dt> + <dt><code>ct_commit_to_zone(snat);</code></dt> + <dd> + <p> + Commit the flow to the specific zone in the connection tracker. + Similar to the <code>ct_commit</code> action, this action requires + previous call to <code>ct_next</code> to initialize connection + tracking state. + </p> + + <p> + If you want processing to continue in the next table, you must + execute the <code>next</code> action after + <code>ct_commit_to_zone</code>. + </p> + + <p> + Note that this action is meaningful only in the Logical Router + Datapath as the Logical Switch Datapath does not use separate + connection tracking zones. Using this action in Logical Switch + Datapath falls back to committing the flow into the switch's + default zone. + </p> + </dd> <dt><code>ct_dnat;</code></dt> <dt><code>ct_dnat(<var>IP</var>);</code></dt> <dd> diff --git a/tests/ovn.at b/tests/ovn.at index c8cc1d37f..cbb303459 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1316,6 +1316,22 @@ ct_commit { ct_label=0x181716151413121110090807060504030201; }; ct_commit { ip4.dst = 192.168.0.1; }; Field ip4.dst is not modifiable. +# ct_commit_to_zone +ct_commit_to_zone(dnat); + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) + has prereqs ip +ct_commit_to_zone(dnat); + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) + has prereqs ip +ct_commit_to_zone; + Syntax error at `;' expecting `('. +ct_commit_to_zone(); + Syntax error at `)' expecting parameter 'dnat' or 'snat'. +ct_commit_to_zone(foo); + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. +ct_commit_to_zone(dnat; + Syntax error at `;' expecting `)'. + # Legact ct_commit_v1 action. ct_commit(); Syntax error at `(' expecting `;'. diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 5e55fbbcc..554682cd0 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, super); break; + case OVNACT_CT_COMMIT_TO_ZONE: case OVNACT_CT_COMMIT_V2: /* Nothing to do. */ break;
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 | 1 + include/ovn/features.h | 1 + lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++ northd/en-global-config.c | 11 +++++++ northd/en-global-config.h | 1 + ovn-sb.xml | 24 ++++++++++++++++ tests/ovn.at | 16 +++++++++++ utilities/ovn-trace.c | 1 + 9 files changed, 123 insertions(+)