Hi Ales, thanks for getting to the bottom of this. The ct_snat was indeed used opportunistically as an easiest way to commit connection to the CT, without realising that it has side effects.
The change overall looks good to me. Martin. > On 21 Aug 2024, at 10:55, Ales Musil <[email protected]> wrote: > > In order to get the direct SNAT access working we need to commit new > connections so the reply is not marked as invalid. The CT state to > determine if the connection should be committed was populated by > ct_snat action, however this action also performs the NAT part > if the connection is already known and there is an entry for that. > This would cause issues when the there is combination of FIPs and > SNAT with very broad logical IP. To prevent that use ct_next only > which will populate the state but won't do the NAT part which can > be done later on if needed according to the conditions. > > At the same time add support for ct_next in SNAT zone as ct_next > was assuming that the zone is always DNAT. > > Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT network.") > Reported-at: https://issues.redhat.com/browse/FDP-744 > Signed-off-by: Ales Musil <[email protected]> > --- > controller/chassis.c | 8 ++++++++ > include/ovn/actions.h | 1 + > include/ovn/features.h | 1 + > lib/actions.c | 33 +++++++++++++++++++++++++++++---- > northd/en-global-config.c | 10 ++++++++++ > northd/en-global-config.h | 1 + > northd/northd.c | 6 +++--- > ovn-sb.xml | 2 ++ > tests/ovn-northd.at | 23 ++++++++++++++--------- > tests/ovn.at | 16 ++++++++++++++++ > tests/system-ovn.at | 10 ++++------ > 11 files changed, 89 insertions(+), 22 deletions(-) > > diff --git a/controller/chassis.c b/controller/chassis.c > index 2991a0af3..ee839084a 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -390,6 +390,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg > *ovs_cfg, > smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); > smap_replace(config, OVN_FEATURE_SAMPLE_WITH_REGISTERS, > ovs_cfg->sample_with_regs ? "true" : "false"); > + smap_replace(config, OVN_FEATURE_CT_NEXT_ZONE, "true"); > } > > /* > @@ -549,6 +550,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_NEXT_ZONE, > + false)) { > + return true; > + } > + > return false; > } > > @@ -706,6 +713,7 @@ update_supported_sset(struct sset *supported) > sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); > sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); > sset_add(supported, OVN_FEATURE_SAMPLE_WITH_REGISTERS); > + sset_add(supported, OVN_FEATURE_CT_NEXT_ZONE); > } > > static void > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index c8dd66ed8..a95a0daf7 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -260,6 +260,7 @@ struct ovnact_push_pop { > /* OVNACT_CT_NEXT. */ > struct ovnact_ct_next { > struct ovnact ovnact; > + bool dnat_zone; > uint8_t ltable; /* Logical table ID of next table. */ > }; > > diff --git a/include/ovn/features.h b/include/ovn/features.h > index 4275f7526..3566ab60f 100644 > --- a/include/ovn/features.h > +++ b/include/ovn/features.h > @@ -30,6 +30,7 @@ > #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" > #define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" > #define OVN_FEATURE_SAMPLE_WITH_REGISTERS "ovn-sample-with-registers" > +#define OVN_FEATURE_CT_NEXT_ZONE "ct-next-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 c12d087e7..2e05d4134 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -701,13 +701,32 @@ parse_CT_NEXT(struct action_context *ctx) > } > > add_prerequisite(ctx, "ip"); > - ovnact_put_CT_NEXT(ctx->ovnacts)->ltable = ctx->pp->cur_ltable + 1; > + struct ovnact_ct_next *ct_next = ovnact_put_CT_NEXT(ctx->ovnacts); > + ct_next->dnat_zone = true; > + ct_next->ltable = ctx->pp->cur_ltable + 1; > + > + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) { > + return; > + } > + > + if (lexer_match_id(ctx->lexer, "dnat")) { > + ct_next->dnat_zone = true; > + } else if (lexer_match_id(ctx->lexer, "snat")) { > + ct_next->dnat_zone = false; > + } else { > + lexer_error(ctx->lexer, "\"ct_next\" action accepts only" > + " \"dnat\" or \"snat\" parameter."); > + return; > + } > + > + lexer_force_match(ctx->lexer, LEX_T_RPAREN); > } > > static void > format_CT_NEXT(const struct ovnact_ct_next *ct_next OVS_UNUSED, struct ds *s) > { > - ds_put_cstr(s, "ct_next;"); > + ds_put_cstr(s, "ct_next"); > + ds_put_cstr(s, ct_next->dnat_zone ? "(dnat);" : "(snat);"); > } > > static void > @@ -719,11 +738,17 @@ encode_CT_NEXT(const struct ovnact_ct_next *ct_next, > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next->ltable; > - ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) > - : mf_from_id(MFF_LOG_DNAT_ZONE); > 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_next->dnat_zone > + ? MFF_LOG_DNAT_ZONE > + : MFF_LOG_SNAT_ZONE); > + } > + > ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > ofpacts->header = ct; > ofpact_finish_CT(ofpacts, &ct); > diff --git a/northd/en-global-config.c b/northd/en-global-config.c > index 0ce7f8308..fff2aaa16 100644 > --- a/northd/en-global-config.c > +++ b/northd/en-global-config.c > @@ -382,6 +382,7 @@ northd_enable_all_features(struct ed_type_global_config > *data) > .ct_commit_nat_v2 = true, > .ct_commit_to_zone = true, > .sample_with_reg = true, > + .ct_next_zone = true, > }; > } > > @@ -452,6 +453,15 @@ build_chassis_features(const struct sbrec_chassis_table > *sbrec_chassis_table, > chassis_features->sample_with_reg) { > chassis_features->sample_with_reg = false; > } > + > + bool ct_next_zone = > + smap_get_bool(&chassis->other_config, > + OVN_FEATURE_CT_NEXT_ZONE, > + false); > + if (!ct_next_zone && > + chassis_features->ct_next_zone) { > + chassis_features->ct_next_zone = false; > + } > } > } > > diff --git a/northd/en-global-config.h b/northd/en-global-config.h > index 0cf34482a..767810542 100644 > --- a/northd/en-global-config.h > +++ b/northd/en-global-config.h > @@ -20,6 +20,7 @@ struct chassis_features { > bool ct_commit_nat_v2; > bool ct_commit_to_zone; > bool sample_with_reg; > + bool ct_next_zone; > }; > > struct global_config_tracked_data { > diff --git a/northd/northd.c b/northd/northd.c > index 73367b910..19a484ceb 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -16079,14 +16079,14 @@ build_lrouter_out_snat_flow(struct lflow_table > *lflows, > /* For the SNAT networks, we need to make sure that connections are > * properly tracked so we can decide whether to perform SNAT on traffic > * exiting the network. */ > - if (features->ct_commit_to_zone && !strcmp(nat->type, "snat") && > - !od->is_gw_router) { > + if (features->ct_commit_to_zone && features->ct_next_zone && > + !strcmp(nat->type, "snat") && !od->is_gw_router) { > /* For traffic that comes from SNAT network, initiate CT state before > * entering S_ROUTER_OUT_SNAT to allow matching on various CT states. > */ > ds_truncate(match, original_match_len); > ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 70, > - ds_cstr(match), "ct_snat;", > + ds_cstr(match), "ct_next(snat);", > lflow_ref); > > build_lrouter_out_snat_match(lflows, od, nat, match, > diff --git a/ovn-sb.xml b/ovn-sb.xml > index c11296d7c..95116ac56 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -1369,6 +1369,8 @@ > </dd> > > <dt><code>ct_next;</code></dt> > + <dt><code>ct_next(dnat);</code></dt> > + <dt><code>ct_next(snat);</code></dt> > <dd> > <p> > Apply connection tracking to the flow, initializing > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 93ccbce6b..8816749c4 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1133,7 +1133,8 @@ ovn_start > # DR is connected to S1 and CR is connected to S2 > > check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \ > - -- set chassis gw1 other_config:ct-commit-to-zone="true" > + -- set chassis gw1 other_config:ct-commit-to-zone="true" \ > + -- set chassis gw1 other_config:ct-next-zone="true" > > check ovn-nbctl lr-add DR > check ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24 > @@ -5721,7 +5722,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows | > ovn_strip_lflows], [0], [dnl > ]) > > check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \ > - -- set chassis gw1 other_config:ct-commit-to-zone="true" > + -- set chassis gw1 other_config:ct-commit-to-zone="true" \ > + -- set chassis gw1 other_config:ct-next-zone="true" > > # Create a distributed gw port on lr0 > check ovn-nbctl ls-add public > @@ -5822,8 +5824,8 @@ AT_CHECK([grep "lr_out_undnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_post_undnat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_post_undnat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.0/24 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat;) > - table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.10 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat;) > + table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.0/24 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_next(snat);) > + table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.10 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_next(snat);) > ]) > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > @@ -5980,8 +5982,8 @@ AT_CHECK([grep "lr_out_undnat" lr0flows | > ovn_strip_lflows], [0], [dnl > > AT_CHECK([grep "lr_out_post_undnat" lr0flows | ovn_strip_lflows], [0], [dnl > table=??(lr_out_post_undnat ), priority=0 , match=(1), action=(next;) > - table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.0/24 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat;) > - table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.10 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat;) > + table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.0/24 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_next(snat);) > + table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == > 10.0.0.10 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_next(snat);) > ]) > > AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > @@ -7876,13 +7878,16 @@ ovn_start > # distributed gateway LRPs. > > check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \ > - -- set chassis gw1 other_config:ct-commit-to-zone="true" > + -- set chassis gw1 other_config:ct-commit-to-zone="true" \ > + -- set chassis gw1 other_config:ct-next-zone="true" > > check ovn-sbctl chassis-add gw2 geneve 128.0.0.1 \ > - -- set chassis gw2 other_config:ct-commit-to-zone="true" > + -- set chassis gw2 other_config:ct-commit-to-zone="true" \ > + -- set chassis gw2 other_config:ct-next-zone="true" > > check ovn-sbctl chassis-add gw3 geneve 129.0.0.1 \ > - -- set chassis gw3 other_config:ct-commit-to-zone="true" > + -- set chassis gw3 other_config:ct-commit-to-zone="true" \ > + -- set chassis gw3 other_config:ct-next-zone="true" > > check ovn-nbctl lr-add DR > check ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24 > diff --git a/tests/ovn.at b/tests/ovn.at > index 50c9f04da..632f060cc 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1263,11 +1263,27 @@ ct_lb_mark(backends=192.168.1.2:80,192.168.1.3:80; > hash_fields="eth_src,eth_dst, > > # ct_next > ct_next; > + formats as ct_next(dnat); > + encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > +ct_next(dnat); > + encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > +ct_next(snat); > encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > has prereqs ip > ct_clear; ct_next; > + formats as ct_clear; ct_next(dnat); > encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > has prereqs ip > +ct_next(snat, dnat); > + Syntax error at `,' expecting `)'. > +ct_next(dnat, ignore); > + Syntax error at `,' expecting `)'. > +ct_next(ignore); > + "ct_next" action accepts only "dnat" or "snat" parameter. > +ct_next(); > + "ct_next" action accepts only "dnat" or "snat" parameter. > > # ct_commit > ct_commit; > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 6e4ec4247..01f71161a 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -3518,7 +3518,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat > 172.16.1.3 192.168.1.2 foo1 00:0 > AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.1.3 foo2 > 00:00:02:02:03:05]) > > # Add a SNAT rule > -AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16]) > +AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 0.0.0.0/0]) > > # Add default route to ext-net > AT_CHECK([ovn-nbctl lr-route-add R1 10.0.0.0/24 172.16.1.2]) > @@ -3724,8 +3724,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::3 > fd11::2 foo1 00:00:02:02 > AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::4 fd11::3 foo2 > 00:00:02:02:03:05]) > > # Add a SNAT rule > -AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd11::/64]) > -AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd12::/64]) > +AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 ::/0]) > > ovn-nbctl --wait=hv sync > OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=fd20::1)']) > @@ -3920,7 +3919,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat > 172.16.1.3 192.168.1.2 foo1 00:0 > AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.2.2 bar1 > 00:00:02:02:03:05]) > > # Add a SNAT rule > -AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16]) > +AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 0.0.0.0/0]) > > ovn-nbctl --wait=hv sync > OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=172.16.1.1)']) > @@ -4104,8 +4103,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::3 > fd11::2 foo1 00:00:02:02 > AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::4 fd12::2 bar1 > 00:00:02:02:03:05]) > > # Add a SNAT rule > -AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd11::/64]) > -AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd12::/64]) > +AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 ::/0]) > > ovn-nbctl --wait=hv sync > OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=fd20::1)']) > -- > 2.46.0 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
