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