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