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
