Hi Ales, Dumitru, Thanks for catching this and for the revert. I reproduced it and the patch was broken everywhere, not just on Fedora/userspace.
What happens, for a client behind R1 connecting to the VIP on R2: client -> 30.0.0.2:8000 SYN backend <- 172.16.1.1 SYN (force SNAT applied, fine) client <- 172.16.1.2:80 SYN-ACK (unSNATed, but not unDNATed) client -> 172.16.1.2:80 RST For routers with a DGP the unDNAT is done in lr_out_undnat by the flow build_distr_lrouter_nat_flows_for_lb() adds per backend. For the FORCE_SNAT and SKIP_SNAT variants that flow only sets the flag and does "next;" - no ct_dnat. With separate SNAT/DNAT zones nothing else reverses the DNAT, so the reply leaves with the backend address. This is already the case on main without my patch: setting lb_force_snat_ip, or skip_snat on the LB, on a DGP router with this topology breaks the load balancer entirely (same capture, the backend just sees the subnet SNAT address instead). So the bug predates my patch, but I should have caught it and didn't - my bad. The test I added only looked at the SNAT conntrack entry on the backend side, which is correct, and never checked that wget succeeded. It "passed" on the kernel datapath and with GNU wget in the Fedora container while every connection was being reset - I checked both with tcpdump. Fedora's wget is wget2, whose retry timing lets the kernel retransmit the SYN on the same source port after the RST; that retransmit hits the subnet SNAT entry instead (the flag is not set on it), which is what made the conntrack output differ. GNU wget in the same Fedora container passes the old test. skip_snat on DGP routers has the same unDNAT problem and, like force SNAT, its lr_out_snat consumer (priority 120) is outranked by subnet SNAT entries shifted by 128 - so it doesn't actually work there either. I have a fix that unDNATs in the FORCE_SNAT/SKIP_SNAT variants, stops setting flags.force_snat_for_lb on the reply (otherwise the reply gets force SNATed too when it also leaves through the DGP, e.g. a backend connecting to its own VIP), and shifts the skip_snat consumer like the force one. The new system tests check that the client actually gets a response, for a client behind another router and for the hairpin case, and they fail on main and on the reverted version. They pass on the kernel and userspace datapaths, both on Ubuntu and in the Fedora container with wget2. I'll post it as v4 shortly. If you'd rather have the unDNAT fix and the skip_snat priority fix as separate patches, I can split it into a series instead. On Wed, Sep 23, 2026 at 11:20 PM Dumitru Ceara <[email protected]> wrote: > On 9/23/26 2:47 PM, Ales Musil wrote: > > The fix seems to cause a problem with a way how is DNAT and > > especially unDNAT handled on distributed routers when > > lb_force_snat_ip is used. Revert it for now until we find out > > a proper solution. > > > > This reverts commit d294af583a25576a70a8327338a9358b4ae961d5. > > > > CC: Jaygue Lee <[email protected]> > > Fixes: d294af583a25 ("northd: Honor lb_force_snat_ip on distributed > routers.") > > Signed-off-by: Ales Musil <[email protected]> > > --- > > Hi Ales, Jaygue Lee > > Ales, thanks for catching this and for the revert. Applied to main, > 26.09 and 26.03. > > It's a bit of a pitty that upstream CI doesn't exercise the "fedora > container" + userspace tests on each push, we could've caught this earlier. > > In any case, Jaygue Lee, would you happen to have some time to > investigate this? > > An example of CI failure due to the previous change: > https://github.com/dceara/ovn/actions/runs/35845634162/job/107132136817 > > Regards, > Dumitru > > > northd/northd.c | 65 ++++------------------- > > ovn-nb.xml | 12 ----- > > tests/ovn-northd.at | 76 --------------------------- > > tests/system-ovn.at | 123 -------------------------------------------- > > 4 files changed, 10 insertions(+), 266 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 4a93bbda1..077ab31b5 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -14335,11 +14335,6 @@ lrouter_dnat_and_snat_is_stateless(const struct > ovn_nat *nat) > > > > #define NAT_PRIORITY_MATCH_OFFSET 300 > > > > -/* Routers with distributed gateway ports shift their lr_out_snat NAT > > - * priorities up by this offset. Other lr_out_snat flows that have to > keep a > > - * fixed ordering relative to the NAT flows must apply the same offset. > */ > > -#define NAT_PRIORITY_DGP_OFFSET 128 > > - > > static inline uint16_t > > lrouter_nat_get_priority(const struct ovn_datapath *od, > > const struct nbrec_nat *nat, bool is_dnat, > > @@ -14358,7 +14353,7 @@ lrouter_nat_get_priority(const struct > ovn_datapath *od, > > * priority. */ > > uint16_t priority = prefix_len + 1; > > if (!od->is_gw_router && !vector_is_empty(&od->l3dgw_ports)) { > > - priority += NAT_PRIORITY_DGP_OFFSET; > > + priority += 128; > > } > > > > return priority; > > @@ -14707,47 +14702,23 @@ 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); > > > > - /* Higher priority rules to force SNAT with the configured IP > > - * addresses. This only takes effect when the packet has already > been > > - * DNATed or load balanced once. */ > > + /* Higher priority rules to force SNAT with the IP addresses > > + * configured in the Gateway router. This only takes effect > > + * when the packet has already been DNATed or load balanced once. */ > > ds_clear(&match); > > ds_put_format(&match, "flags.force_snat_for_%s == 1 && ip%s", > > context, ip_version); > > - uint16_t snat_prio = 100; > > - 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. > > - * > > - * The NAT flows of such a router are shifted up by > > - * NAT_PRIORITY_DGP_OFFSET, so shift this flow as well to keep > the > > - * same ordering it has on a gateway router. Without the shift > > - * even a plain subnet SNAT entry would outrank it and the > forced > > - * SNAT would never be applied. */ > > - ds_put_format(&match, " && outport == %s && > is_chassis_resident(" > > - "\"%s\")", l3dgw_port->json_key, > > - l3dgw_port->cr_port->key); > > - snat_prio += NAT_PRIORITY_DGP_OFFSET; > > - } > > ds_put_format(&actions, "ct_snat(%s);", ip_addr); > > - ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, snat_prio, > > + ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 100, > > ds_cstr(&match), ds_cstr(&actions), > > lflow_ref); > > > > @@ -19244,46 +19215,30 @@ build_lrouter_nat_defrag_and_lb( > > > > } > > > > - /* Consumers for the force SNAT flags produced by the lr_in_dnat and > > - * lr_out_undnat flows above. > > - */ > > + /* Handle force SNAT options set in the gateway router. */ > > if (od->is_gw_router) { > > if (dnat_force_snat_ip) { > > 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", NULL, lflow_ref); > > + "dnat", 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", NULL, lflow_ref); > > + "dnat", 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", > > - 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", > > - NULL, lflow_ref); > > - } > > - } > > - } else if (lb_force_snat_ip) { > > - 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); > > + 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); > > + lflow_ref); > > } > > } > > } > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index c71066af4..57b81d4b4 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -3373,18 +3373,6 @@ 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. > > - </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 2c2056d56..69932c997 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -5161,82 +5161,6 @@ 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 > > - > > -# A plain subnet SNAT entry. On a router with a distributed gateway > port > > -# the NAT priorities are shifted up by NAT_PRIORITY_DGP_OFFSET, so this > > -# entry outranks an unshifted force SNAT flow. Keep it in the test to > make > > -# sure the force SNAT consumer stays above it. > > -check ovn-nbctl lr-nat-add lr0 snat 172.168.0.100 10.0.0.0/24 > > - > > -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=100 , match=(ip && ip4.dst > == 172.168.0.100 && inport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat;) > > - 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;) > > -]) > > - > > -# The force SNAT flows must sit above the subnet SNAT entry, otherwise > the > > -# latter would SNAT the load balanced traffic first and lb_force_snat_ip > > -# would still be ignored. > > -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=120 , match=(nd_ns), > action=(next;) > > - table=??(lr_out_snat ), priority=153 , match=(ip && ip4.dst > == 10.0.0.0/24 && inport == "lr0-public" && > is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)), > action=(ct_snat;) > > - table=??(lr_out_snat ), priority=153 , match=(ip && ip4.src > == 10.0.0.0/24 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)), > action=(ct_snat(172.168.0.100);) > > - table=??(lr_out_snat ), priority=228 , > 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=228 , > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-public" && > is_chassis_resident("cr-lr0-public")), action=(ct_snat(aef0::4);) > > -]) > > - > > -# The producer and the consumer must both be present: the load balancer > > -# 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 > > -]) > > - > > OVN_FOR_EACH_NORTHD_NO_HV([ > > AT_SETUP([HA chassis group cleanup for external port ]) > > ovn_start > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index d1c35199e..5b6ba3731 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -2559,129 +2559,6 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > > AT_CLEANUP > > ]) > > > > -OVN_FOR_EACH_NORTHD([ > > -AT_SETUP([load balancing with lb_force_snat_ip on a distributed gateway > port]) > > -AT_KEYWORDS([ovnlb]) > > - > > -CHECK_CONNTRACK() > > -CHECK_CONNTRACK_NAT() > > -ovn_start > > -OVS_TRAFFIC_VSWITCHD_START() > > -ADD_BR([br-int]) > > - > > -# Set external-ids in br-int needed for ovn-controller. > > -check ovs-vsctl \ > > - -- set Open_vSwitch . external-ids:system-id=hv1 \ > > - -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > - -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > > - > > -# Start ovn-controller. > > -start_daemon ovn-controller > > - > > -# Logical network: > > -# > > -# foo -- R1 -- join -- R2 == alice > > -# > > -# R2 is not a gateway router; its "alice" port is a distributed gateway > > -# port. The load balancer is attached to R2 and its backend sits on > > -# "alice", i.e. it is reached through the distributed gateway port, so > the > > -# load balanced traffic leaves R2 through that port and is what the > forced > > -# SNAT has to act on. > > - > > -check_uuid ovn-nbctl create Logical_Router name=R1 > > -check_uuid ovn-nbctl create Logical_Router name=R2 > > - > > -check ovn-nbctl ls-add foo > > -check ovn-nbctl ls-add alice > > -check ovn-nbctl ls-add join > > - > > -# Connect foo to R1. > > -check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 > > -check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \ > > - type=router options:router-port=foo addresses=\"00:00:01:01:02:03\" > > - > > -# Connect alice to R2 through a distributed gateway port. > > -check ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24 > > -check ovn-nbctl lrp-set-gateway-chassis alice hv1 20 > > -check ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port > rp-alice \ > > - type=router options:router-port=alice > addresses=\"00:00:02:01:02:03\" > > - > > -# Connect R1 to join. > > -check ovn-nbctl lrp-add R1 R1_join 00:00:04:01:02:03 20.0.0.1/24 > > -check ovn-nbctl lsp-add join r1-join -- set Logical_Switch_Port r1-join > \ > > - type=router options:router-port=R1_join > addresses='"00:00:04:01:02:03"' > > - > > -# Connect R2 to join. > > -check ovn-nbctl lrp-add R2 R2_join 00:00:04:01:02:04 20.0.0.2/24 > > -check ovn-nbctl lsp-add join r2-join -- set Logical_Switch_Port r2-join > \ > > - type=router options:router-port=R2_join > addresses='"00:00:04:01:02:04"' > > - > > -# Static routes. > > -check ovn-nbctl lr-route-add R1 30.0.0.0/24 20.0.0.2 > > -check ovn-nbctl lr-route-add R1 172.16.1.0/24 20.0.0.2 > > -check ovn-nbctl lr-route-add R2 192.168.0.0/16 20.0.0.1 > > - > > -# Logical port 'foo1' in switch 'foo'. This is the client. > > -ADD_NAMESPACES(foo1) > > -ADD_VETH(foo1, foo1, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > - "192.168.1.1") > > -check ovn-nbctl lsp-add foo foo1 \ > > --- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2" > > - > > -# Logical port 'alice1' in switch 'alice'. This is the backend. > > -ADD_NAMESPACES(alice1) > > -ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24", "f0:00:00:01:02:04", > \ > > - "172.16.1.1") > > -check ovn-nbctl lsp-add alice alice1 \ > > --- lsp-set-addresses alice1 "f0:00:00:01:02:04 172.16.1.2" > > - > > -uuid=`ovn-nbctl create load_balancer vips:'"30.0.0.2:8000"'='" > 172.16.1.2:80"'` > > -check ovn-nbctl set logical_router R2 load_balancer=$uuid > > - > > -# A plain subnet SNAT entry that also matches the load balanced traffic > on > > -# its way out of the distributed gateway port. Its lr_out_snat > priority is > > -# shifted up on such a router, so it would take precedence over an > > -# unshifted force SNAT flow and lb_force_snat_ip would be ignored. > > -check ovn-nbctl lr-nat-add R2 snat 172.16.1.100 192.168.0.0/16 > > - > > -check ovn-nbctl set logical_router R2 > options:lb_force_snat_ip="172.16.1.1" > > - > > -check ovn-nbctl --wait=hv sync > > - > > -snat=$(ovn-debug lflow-stage-to-oftable lr_out_snat) > > -OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-flows br-int table=$snat | > \ > > -grep 'nat(src=172.16.1.1)']) > > - > > -# Start a webserver on the backend. > > -OVS_START_L7([alice1], [http]) > > - > > -check ovs-appctl dpctl/flush-conntrack > > - > > -dnl The backend must see the forced SNAT address as the source, not the > > -dnl external IP of the subnet SNAT entry and not the client address. > > -OVS_WAIT_FOR_OUTPUT([ > > -for i in `seq 1 5`; do > > - NS_EXEC([foo1], [wget http://30.0.0.2:8000 -t 5 -T 1 > --retry-connrefused -v -o wget$i.log]) > > -done > > - > > -ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.2) | > > -sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > > > -tcp,orig=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=172.16.1.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>) > > -]) > > - > > -OVN_CLEANUP_CONTROLLER([hv1]) > > - > > -OVN_CLEANUP_NORTHD > > - > > -as > > -OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > > -/Failed to acquire.*/d > > -/connection dropped.*/d"]) > > -AT_CLEANUP > > -]) > > - > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([load balancing in gateway router - IPv6]) > > AT_KEYWORDS([ovnlb]) > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
