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. */ 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. */ + 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 +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]) -- 2.49.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
