Hi Jaygue Lee, Thanks for the new revision!
On 8/25/26 6:15 AM, JayGue Lee 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. > > Such routers also shift their lr_out_snat NAT priorities up by 128, so > the consumer flow has to be shifted by the same amount. Otherwise even > a plain subnet SNAT entry, which lands at priority 153 for a /24, would > outrank the force SNAT flow at priority 100 and the option would still > not be honored. > > flags.force_snat_for_dnat is only ever produced on gateway routers > (build_lrouter_in_dnat_flow() sets it under od->is_gw_router), so the > dnat_force_snat_ip consumer is left unchanged. > > Assisted-by: Claude Opus 5, Claude Code > Signed-off-by: Jaygue Lee <[email protected]> > --- > v3: - Shift the lr_out_snat consumer priority by NAT_PRIORITY_DGP_OFFSET > on routers with distributed gateway ports. Without it any SNAT > entry, which is shifted by the same amount, outranks the force > SNAT flow and the option is still ignored. Pulled the literal > 128 out of lrouter_nat_get_priority() into the new define so both > sites stay in sync. > - Drop the NEWS entry. > - Rephrase the force SNAT comment in producer/consumer terms and > record why the dnat_force_snat_ip consumer stays gateway router > only: its flag has no producer on distributed routers. > - Drop the incorrect claim that lb_force_snat_ip=router_ip is > gateway router only, from both the commit message and ovn-nb.xml. > - Add a subnet SNAT entry to the ovn-northd.at test so the priority > ordering is actually covered. > - Add a system-ovn.at test with a load balancer whose backend is > reached through the distributed gateway port. > - Add the Assisted-by tag. > 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. > northd/northd.c | 75 +++++++++++++++++++++++---- > ovn-nb.xml | 12 +++++ > tests/ovn-northd.at | 76 +++++++++++++++++++++++++++ > tests/system-ovn.at | 123 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 276 insertions(+), 10 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 88e3ece884..9e1e22bca6 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -14266,6 +14266,11 @@ 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, > @@ -14284,7 +14289,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 += 128; > + priority += NAT_PRIORITY_DGP_OFFSET; > } > > return priority; > @@ -14633,23 +14638,47 @@ 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 IP addresses > - * configured in the Gateway router. This only takes effect > - * when the packet has already been DNATed or load balanced once. */ > + /* 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. */ > 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, 100, > + ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, snat_prio, > ds_cstr(&match), ds_cstr(&actions), > lflow_ref); > > @@ -19135,30 +19164,56 @@ build_lrouter_nat_defrag_and_lb( > > } > > - /* Handle force SNAT options set in the gateway router. */ > + /* Consumers for the force SNAT flags produced by the lr_in_dnat and > + * lr_out_undnat flows above. > + * > + * flags.force_snat_for_lb is produced both on gateway routers and on > + * routers with a distributed gateway port, so the consumer is built for > + * both; on the latter it is applied on the chassis where the gateway > + * port is resident. flags.force_snat_for_dnat is only ever produced on > + * gateway routers, so its consumer stays gateway router only. */ This second part of the comment is superfluous in my opinion, I'd remove it. > 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", 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. */ Same here, this comment is superfluous. > + 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 cce227a828..71deac27fd 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3377,6 +3377,18 @@ 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 6d191c1a0b..6fc673e094 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -4976,6 +4976,82 @@ 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 dc1bb1d749..3eb4b8e6ea 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -2559,6 +2559,129 @@ 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 Nit: comments should be sentences and end with period. This applies to multiple comments in this patch. > +ovs-vsctl \ Nit: missing check. > + -- 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]) I took care of the minor issues above and applied the patch to main, 26.09 and 26.03. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
