On Wed, Aug 28, 2024 at 11:28 PM Brian Haley <[email protected]> wrote:
> Hi Ales, > Hello Brian, > I was able to test this along with a neutron patch [0] to program > '0.0.0.0/0' as the snat logical_ip address and it seemed to work with > various combinations of fixed and floating IP addresses. > > You can add this if you like: > > Tested-by: Brian Haley <[email protected]> > > Thanks for fixing this! > Thank you for testing it out. > Is there any guess on how far back this could be applied? I'm guessing > it would need Martin's other snat fix? > So the original commit is applied to 24.03 onwards, so I will make sure it gets backported there. AFAIK 23.09 doesn't have this issue, not sure if anyone tested 23.06 and 23.03. > > -Brian > > [0] https://review.opendev.org/c/openstack/neutron/+/926495 > > On 8/27/24 4:52 AM, Ales Musil 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]> > > --- > > v2: Make sure we don't SNAT FIP reply traffic > > --- > > 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 | 8 +++----- > > ovn-sb.xml | 2 ++ > > tests/ovn-northd.at | 23 ++++++++++++++--------- > > tests/ovn.at | 16 ++++++++++++++++ > > tests/system-ovn.at | 10 ++++------ > > 11 files changed, 89 insertions(+), 24 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 ed959535b..ed5706f40 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -16051,7 +16051,6 @@ build_lrouter_out_snat_flow(struct lflow_table > *lflows, > > build_lrouter_out_snat_match(lflows, od, nat, match, > distributed_nat, > > cidr_bits, is_v6, l3dgw_port, > lflow_ref, > > false); > > - size_t original_match_len = match->length; > > > > if (!od->is_gw_router && distributed_nat) { > > ds_put_format(actions, "eth.src = "ETH_ADDR_FMT"; ", > > @@ -16073,14 +16072,13 @@ 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..564c9446e 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") && (!ct.trk || !ct.rpl)), > 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") && (!ct.trk || !ct.rpl)), > 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") && (!ct.trk || !ct.rpl)), > 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") && (!ct.trk || !ct.rpl)), > 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)']) > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
