Hi Jaygue Lee, Thanks for the patch!
On 7/31/26 8:33 AM, enginrect wrote: > From: Jaygue Lee <[email protected]> > > The lr_out_snat flows that consume flags.force_snat_for_lb are only > built for gateway routers (od->is_gw_router). However, the load > balancer DNAT flows in lr_in_dnat and the lr_out_undnat flows set > flags.force_snat_for_lb whenever the option is configured, including on > routers whose gateway is a distributed gateway port. The result is a > flag that is produced but never consumed: the option is silently > ignored and load balanced traffic leaves the router without the forced > SNAT. > > This breaks hairpin scenarios where a load balancer backend reachable > through the gateway port connects to its own VIP: the un-SNATed reply > arrives at the backend with identical source and destination addresses > and is dropped as a martian packet. The logical switch hairpin stages > do not cover this case when the backend resides on a switch with a > localnet port, where attaching the load balancer to that switch is not > viable. > > Build the force SNAT consumer flows for routers with distributed > gateway ports as well, one per gateway port, applying ct_snat on the > chassis where the gateway port is resident with an > is_chassis_resident() match. This is consistent with how dnat_and_snat > and subnet SNAT entries are already centralized on such routers. The > "router_ip" variant and dnat_force_snat_ip remain gateway router only. Hmm, why though? Shouldn't we make these work too? I think skip_snat_for_lb already works. > Just double checking: was AI assistance used to create this patch? If so, can you please add an "Assisted-by" tag as required by our documentation (please see submitting-patches.rst)? If not, I hope you don't mind me asking, it's just a hunch and I might be wrong. > Signed-off-by: Jaygue Lee <[email protected]> > --- > v2: No code changes. Resubmitted so the CI series branch is recreated > on current main and picks up the runner crun fix (de930bc3a) that > landed after v1; v1 CI failures were the crun container-startup error > and unrelated flaky tests in the "unstable" partition. > > NEWS | 4 +++ > northd/northd.c | 43 +++++++++++++++++++++++++++--- > ovn-nb.xml | 14 ++++++++++ > tests/ovn-northd.at | 64 +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 121 insertions(+), 4 deletions(-) > > diff --git a/NEWS b/NEWS > index 84d7f60..6f5fee7 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,5 +1,9 @@ > Post v26.03.0 > ------------- > + - northd: options:lb_force_snat_ip configured with a set of IP addresses > + is now honored on routers with distributed gateway ports. Previously > + the option was silently ignored on such routers, while the > + corresponding force-SNAT flag was still set on load balanced traffic. This is a bug fix, I don't really think we need the NEWS entry for it. > - OVN Interconnection now supports advertising Address_Sets to remote > availability zones. Set options:ic-adv=true on an NB Address_Set to > have ovn-ic publish it to the IC-SB database and import it into the > diff --git a/northd/northd.c b/northd/northd.c > index 1d14527..ef2e1ad 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -14443,12 +14443,20 @@ build_lrouter_force_snat_flows(struct lflow_table > *lflows, > const struct ovn_datapath *od, > const char *ip_version, const char *ip_addr, > const char *context, > + const struct ovn_port *l3dgw_port, > struct lflow_ref *lflow_ref) > { > struct ds match = DS_EMPTY_INITIALIZER; > struct ds actions = DS_EMPTY_INITIALIZER; > ds_put_format(&match, "ip%s && ip%s.dst == %s", > ip_version, ip_version, ip_addr); > + if (l3dgw_port) { > + /* Distributed router: only unSNAT on the chassis where the > + * gateway port is resident. */ > + ds_put_format(&match, " && inport == %s && is_chassis_resident(" > + "\"%s\")", l3dgw_port->json_key, > + l3dgw_port->cr_port->key); > + } > ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 110, > ds_cstr(&match), "ct_snat;", lflow_ref); > > @@ -14458,6 +14466,14 @@ build_lrouter_force_snat_flows(struct lflow_table > *lflows, > ds_clear(&match); > ds_put_format(&match, "flags.force_snat_for_%s == 1 && ip%s", > context, ip_version); > + if (l3dgw_port) { > + /* Distributed router: force SNAT is applied on the chassis > + * where the gateway port is resident, consistent with how > + * regular SNAT entries are handled for such routers. */ > + ds_put_format(&match, " && outport == %s && is_chassis_resident(" > + "\"%s\")", l3dgw_port->json_key, > + l3dgw_port->cr_port->key); > + } > ds_put_format(&actions, "ct_snat(%s);", ip_addr); > ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 100, > ds_cstr(&match), ds_cstr(&actions), > @@ -18836,24 +18852,43 @@ build_lrouter_nat_defrag_and_lb( > if (lrnat_rec->dnat_force_snat_addrs.n_ipv4_addrs) { > build_lrouter_force_snat_flows(lflows, od, "4", > lrnat_rec->dnat_force_snat_addrs.ipv4_addrs[0].addr_s, > - "dnat", lflow_ref); > + "dnat", NULL, lflow_ref); > } > if (lrnat_rec->dnat_force_snat_addrs.n_ipv6_addrs) { > build_lrouter_force_snat_flows(lflows, od, "6", > lrnat_rec->dnat_force_snat_addrs.ipv6_addrs[0].addr_s, > - "dnat", lflow_ref); > + "dnat", NULL, lflow_ref); > } > } > if (lb_force_snat_ip) { > if (lrnat_rec->lb_force_snat_addrs.n_ipv4_addrs) { > build_lrouter_force_snat_flows(lflows, od, "4", > lrnat_rec->lb_force_snat_addrs.ipv4_addrs[0].addr_s, > "lb", > - lflow_ref); > + NULL, lflow_ref); > } > if (lrnat_rec->lb_force_snat_addrs.n_ipv6_addrs) { > build_lrouter_force_snat_flows(lflows, od, "6", > lrnat_rec->lb_force_snat_addrs.ipv6_addrs[0].addr_s, > "lb", > - lflow_ref); > + NULL, lflow_ref); > + } > + } > + } else if (lb_force_snat_ip) { > + /* Distributed routers with gateway ports: the load balancer DNAT > + * and the corresponding force_snat_for_lb flag are already applied > + * on the chassis where the gateway port is resident, so apply the > + * force SNAT flows there as well. Without these flows the flag is > + * set but never consumed and the option is silently ignored. */ This comment doesn't seem to add much value. We could just skip it in my opinion. I would also rephrase the /* Handle force SNAT options set in the gateway router. */ comment before the other branch of the if statement above. We now handle it here too. > + struct ovn_port *dgp; > + VECTOR_FOR_EACH (&od->l3dgw_ports, dgp) { > + if (lrnat_rec->lb_force_snat_addrs.n_ipv4_addrs) { > + build_lrouter_force_snat_flows(lflows, od, "4", > + lrnat_rec->lb_force_snat_addrs.ipv4_addrs[0].addr_s, > "lb", > + dgp, lflow_ref); > + } > + if (lrnat_rec->lb_force_snat_addrs.n_ipv6_addrs) { > + build_lrouter_force_snat_flows(lflows, od, "6", > + lrnat_rec->lb_force_snat_addrs.ipv6_addrs[0].addr_s, > "lb", > + dgp, lflow_ref); > } > } > } > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 68d4237..0b7b346 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3377,6 +3377,20 @@ or > character. > </p> > > + <p> > + A set of IP addresses is also honored on distributed routers > + with one or more distributed gateway ports. In that case the > + SNAT is applied on the chassis where the gateway port is > + resident, consistent with how regular SNAT entries are handled > + for such routers. This is useful, for example, when a load > + balancer backend reachable through the gateway port connects to > + its own VIP: without the forced SNAT the un-SNATed reply would > + arrive at the backend with identical source and destination > + addresses and be discarded as a martian packet. The > + <code>router_ip</code> value is only supported on gateway > + routers. > + </p> > + > <p> > If it is configured with the value <code>router_ip</code>, then > the load balanced packet is SNATed with the IP of router port > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index c58f731..b5531c6 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -4971,6 +4971,70 @@ OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([ > +AT_SETUP([Load Balancers and lb_force_snat_ip for routers with distributed > gateway ports]) > +ovn_start > + > +check ovn-nbctl ls-add sw0 > + > +# Create a logical router with a distributed gateway port. > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > +check ovn-nbctl lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > + > +check ovn-nbctl ls-add public > +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > +check ovn-nbctl lsp-add-router-port public public-lr0 lr0-public > +check ovn-nbctl lrp-set-gateway-chassis lr0-public ch1 > + > +check ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.4:8080 > +check ovn-nbctl lr-lb-add lr0 lb1 > + > +check ovn-nbctl --wait=sb sync > + > +ovn-sbctl dump-flows lr0 > lr0flows > +AT_CAPTURE_FILE([lr0flows]) > + > +# Without lb_force_snat_ip there should be no force SNAT flows. > +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | > ovn_strip_lflows], [0], [dnl > +]) > + > +check ovn-nbctl --wait=sb set logical_router lr0 > options:lb_force_snat_ip="172.168.0.4 aef0::4" > + > +ovn-sbctl dump-flows lr0 > lr0flows > +AT_CAPTURE_FILE([lr0flows]) > + > +AT_CHECK([grep "lr_in_unsnat" lr0flows | ovn_strip_lflows], [0], [dnl > + table=??(lr_in_unsnat ), priority=0 , match=(1), action=(next;) > + table=??(lr_in_unsnat ), priority=110 , match=(ip4 && ip4.dst == > 172.168.0.4 && inport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat;) > + table=??(lr_in_unsnat ), priority=110 , match=(ip6 && ip6.dst == > aef0::4 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")), > action=(ct_snat;) > +]) > + > +AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl > + table=??(lr_out_snat ), priority=0 , match=(1), action=(next;) > + table=??(lr_out_snat ), priority=100 , > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat(172.168.0.4);) > + table=??(lr_out_snat ), priority=100 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat(aef0::4);) > + table=??(lr_out_snat ), priority=120 , match=(nd_ns), > action=(next;) > +]) > + > +# The producer and the consumer must both be present: the load balancer "producer" and "consumer"? > +# DNAT flows on the gateway chassis set flags.force_snat_for_lb, and the > +# flows above consume it. > +AT_CHECK([grep "lr_in_dnat" lr0flows | grep -q "force_snat"], [0], []) > + > +# Removing the option removes the flows. > +check ovn-nbctl --wait=sb remove logical_router lr0 options lb_force_snat_ip > + > +ovn-sbctl dump-flows lr0 > lr0flows > +AT_CAPTURE_FILE([lr0flows]) > + > +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | > ovn_strip_lflows], [0], [dnl > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) Thanks for adding unit tests. But I think it would be safer if we also add a system-ovn.at test to validate that the functionality actually works. Could you please add one? > + > OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([HA chassis group cleanup for external port ]) > ovn_start Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
