Agree with Mark that ICMP is still needed for non VIP on gateway. But LB feature on GW is also important. Shall we have a test case to cover this particular scenario?
On Thu, Jun 28, 2018 at 9:41 AM, Mark Michelson <[email protected]> wrote: > Hi Darrell, > > I'm not 100% sure this is correct. I think this change would only be > correct if the router's IP addresses are also load balancer VIPs. In the > case where the VIPs do not overlap with the router IP addresses, I think > we'd want to have the ICMP responses still in place. > > > On 06/28/2018 01:15 AM, Darrell Ball wrote: > >> Non-distributed and distributed gateway load balancing is broken. >> Recent changes for port unreachable handling broke the associated >> unsnat functionality. The fix approach is check for gateway >> contexts and accept packets directed to gateway router IPs. >> >> Fixes: 86558ac2e476 ("OVN: add UDP port unreachable support to OVN >> logical router.") >> Fixes: 159932c9e4ea ("OVN: add TCP port unreachable support to OVN >> logical router.") >> Fixes: 0e858e05f76b ("OVN: add protocol unreachable support to OVN >> router ports.") >> CC: Lorenzo Bianconi <[email protected]> >> Signed-off-by: Darrell Ball <[email protected]> >> --- >> ovn/northd/ovn-northd.8.xml | 17 ++++--- >> ovn/northd/ovn-northd.c | 106 ++++++++++++++++++++++-------- >> -------------- >> 2 files changed, 64 insertions(+), 59 deletions(-) >> >> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml >> index cfd3511..280efd0 100644 >> --- a/ovn/northd/ovn-northd.8.xml >> +++ b/ovn/northd/ovn-northd.8.xml >> @@ -1310,8 +1310,9 @@ nd_na { >> <p> >> UDP port unreachable. Priority-80 flows generate ICMP port >> unreachable messages in reply to UDP datagrams directed to the >> - router's IP address. The logical router doesn't accept any UDP >> - traffic so it always generates such a reply. >> + router's IP address, except in the special case of gateways, >> + which accept traffic directed to a router IP for load balancing >> + purposes. >> </p> >> <p> >> @@ -1321,10 +1322,10 @@ nd_na { >> <li> >> <p> >> - TCP reset. Priority-80 flows generate TCP reset messages in >> reply to >> - TCP datagrams directed to the router's IP address. The logical >> - router doesn't accept any TCP traffic so it always generates >> such a >> - reply. >> + TCP reset. Priority-80 flows generate TCP reset messages in >> reply >> + to TCP datagrams directed to the router's IP address, except in >> + the special case of gateways, which accept traffic directed to >> a >> + router IP for load balancing purposes. >> </p> >> <p> >> @@ -1336,7 +1337,9 @@ nd_na { >> <p> >> Protocol unreachable. Priority-70 flows generate ICMP >> protocol >> unreachable messages in reply to packets directed to the >> router's IP >> - address on IP protocols other than UDP, TCP, and ICMP. >> + address on IP protocols other than UDP, TCP, and ICMP, except >> in the >> + special case of gateways, which accept traffic directed to a >> router >> + IP for load balancing purposes. >> </p> >> <p> >> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> index 72fe4e7..7648bce 100644 >> --- a/ovn/northd/ovn-northd.c >> +++ b/ovn/northd/ovn-northd.c >> @@ -5141,48 +5141,49 @@ build_lrouter_flows(struct hmap *datapaths, >> struct hmap *ports, >> ds_cstr(&match), ds_cstr(&actions)); >> } >> - /* UDP/TCP port unreachable */ >> - for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >> - const char *action; >> - >> - ds_clear(&match); >> - ds_put_format(&match, >> - "ip4 && ip4.dst == %s && !ip.later_frag && >> udp", >> - op->lrp_networks.ipv4_addrs[i].addr_s); >> - action = "icmp4 {" >> - "eth.dst <-> eth.src; " >> - "ip4.dst <-> ip4.src; " >> - "ip.ttl = 255; " >> - "icmp4.type = 3; " >> - "icmp4.code = 3; " >> - "next; };"; >> - ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, >> - ds_cstr(&match), action); >> + if (!smap_get(&op->od->nbr->options, "chassis") >> + && !op->od->l3dgw_port) { >> + /* UDP/TCP port unreachable. */ >> + for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >> + ds_clear(&match); >> + ds_put_format(&match, >> + "ip4 && ip4.dst == %s && !ip.later_frag && >> udp", >> + op->lrp_networks.ipv4_addrs[i].addr_s); >> + const char *action = "icmp4 {" >> + "eth.dst <-> eth.src; " >> + "ip4.dst <-> ip4.src; " >> + "ip.ttl = 255; " >> + "icmp4.type = 3; " >> + "icmp4.code = 3; " >> + "next; };"; >> + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, >> + ds_cstr(&match), action); >> - ds_clear(&match); >> - ds_put_format(&match, >> - "ip4 && ip4.dst == %s && !ip.later_frag && >> tcp", >> - op->lrp_networks.ipv4_addrs[i].addr_s); >> - action = "tcp_reset {" >> - "eth.dst <-> eth.src; " >> - "ip4.dst <-> ip4.src; " >> - "next; };"; >> - ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, >> - ds_cstr(&match), action); >> + ds_clear(&match); >> + ds_put_format(&match, >> + "ip4 && ip4.dst == %s && !ip.later_frag && >> tcp", >> + op->lrp_networks.ipv4_addrs[i].addr_s); >> + action = "tcp_reset {" >> + "eth.dst <-> eth.src; " >> + "ip4.dst <-> ip4.src; " >> + "next; };"; >> + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, >> + ds_cstr(&match), action); >> - ds_clear(&match); >> - ds_put_format(&match, >> - "ip4 && ip4.dst == %s && !ip.later_frag", >> - op->lrp_networks.ipv4_addrs[i].addr_s); >> - action = "icmp4 {" >> - "eth.dst <-> eth.src; " >> - "ip4.dst <-> ip4.src; " >> - "ip.ttl = 255; " >> - "icmp4.type = 3; " >> - "icmp4.code = 2; " >> - "next; };"; >> - ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 70, >> - ds_cstr(&match), action); >> + ds_clear(&match); >> + ds_put_format(&match, >> + "ip4 && ip4.dst == %s && !ip.later_frag", >> + op->lrp_networks.ipv4_addrs[i].addr_s); >> + action = "icmp4 {" >> + "eth.dst <-> eth.src; " >> + "ip4.dst <-> ip4.src; " >> + "ip.ttl = 255; " >> + "icmp4.type = 3; " >> + "icmp4.code = 2; " >> + "next; };"; >> + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 70, >> + ds_cstr(&match), action); >> + } >> } >> ds_clear(&match); >> @@ -5306,19 +5307,20 @@ build_lrouter_flows(struct hmap *datapaths, >> struct hmap *ports, >> } >> /* TCP port unreachable */ >> - for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >> - const char *action; >> - >> - ds_clear(&match); >> - ds_put_format(&match, >> - "ip6 && ip6.dst == %s && !ip.later_frag && >> tcp", >> - op->lrp_networks.ipv6_addrs[i].addr_s); >> - action = "tcp_reset {" >> - "eth.dst <-> eth.src; " >> - "ip6.dst <-> ip6.src; " >> - "next; };"; >> - ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, >> + if (!smap_get(&op->od->nbr->options, "chassis") >> + && !op->od->l3dgw_port) { >> + for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >> + ds_clear(&match); >> + ds_put_format(&match, >> + "ip6 && ip6.dst == %s && !ip.later_frag && >> tcp", >> + op->lrp_networks.ipv6_addrs[i].addr_s); >> + const char *action = "tcp_reset {" >> + "eth.dst <-> eth.src; " >> + "ip6.dst <-> ip6.src; " >> + "next; };"; >> + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, >> ds_cstr(&match), action); >> + } >> } >> } >> >> > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
