| Message ID | 1530162943-58952-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] ovn: Fix gateway load balancing. | expand |
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 <lorenzo.bianconi@redhat.com> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > 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); > + } > } > } > >
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 <mmichels@redhat.com> 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 <lorenzo.bianconi@redhat.com> >> Signed-off-by: Darrell Ball <dlu998@gmail.com> >> --- >> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Jun 28, 2018 at 9:41 AM, Mark Michelson <mmichels@redhat.com> 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. Pls double check that we are in same page first. You can run the ovn system tests without the patch and take a look at one of the 5 failures, for example AT_SETUP([ovn -- multiple gateway routers, load-balancing]) The load balance VIP is "vips:30.0.0.1" One the the router port's of interest has IP 20.0.0.2 > > > 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 <lorenzo.bianconi@redhat.com> >> Signed-off-by: Darrell Ball <dlu998@gmail.com> >> --- >> 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); >> + } >> } >> } >> >> > >
Lorenzo actually brought up in today's OVN IRC meeting that writing ICMP tests for IPv6 is problematic right now. IPv4 tests have an M4 macro called OVN_POPULATE_ARP that they call. This manually populates tables in OVS so that there are no ARP requests being sent during the test. It does this by calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. IPv6 does not have an equivalent. The result is that tests will sometimes succeed, but sometimes will fail because IPv6 ND requests will be sent during the test. I haven't looked into this myself, but Lorenzo mentioned that there would need to be changes made to OVS itself in order to have an IPv6 version of a working OVN_POPULATE_ARP. On 06/28/2018 02:16 PM, Han Zhou wrote: > 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 <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com > <mailto:lorenzo.bianconi@redhat.com>> > Signed-off-by: Darrell Ball <dlu998@gmail.com > <mailto:dlu998@gmail.com>> > --- >  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 > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > >
Hi Mark, what I meant is the test for the feature of LB in Gateway. If we had a test case, the problem would have been noticed when Lorenzo is working on ICMP feature. On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> wrote: > Lorenzo actually brought up in today's OVN IRC meeting that writing ICMP > tests for IPv6 is problematic right now. IPv4 tests have an M4 macro called > OVN_POPULATE_ARP that they call. This manually populates tables in OVS so > that there are no ARP requests being sent during the test. It does this by > calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. > > IPv6 does not have an equivalent. The result is that tests will sometimes > succeed, but sometimes will fail because IPv6 ND requests will be sent > during the test. I haven't looked into this myself, but Lorenzo mentioned > that there would need to be changes made to OVS itself in order to have an > IPv6 version of a working OVN_POPULATE_ARP. > > On 06/28/2018 02:16 PM, Han Zhou wrote: > >> 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 <mmichels@redhat.com >> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com >> <mailto:lorenzo.bianconi@redhat.com>> >> Signed-off-by: Darrell Ball <dlu998@gmail.com >> <mailto:dlu998@gmail.com>> >> >> --- >> 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 >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> >> >> >
On 28 June 2018 at 11:44, Han Zhou <zhouhan@gmail.com> wrote: > Hi Mark, what I meant is the test for the feature of LB in Gateway. If we > had a test case, the problem would have been noticed when Lorenzo is > working on ICMP feature. > We have a few test cases. The tests weren't run likely as they are system tests. > > On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> > wrote: > > > Lorenzo actually brought up in today's OVN IRC meeting that writing ICMP > > tests for IPv6 is problematic right now. IPv4 tests have an M4 macro > called > > OVN_POPULATE_ARP that they call. This manually populates tables in OVS so > > that there are no ARP requests being sent during the test. It does this > by > > calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. > > > > IPv6 does not have an equivalent. The result is that tests will sometimes > > succeed, but sometimes will fail because IPv6 ND requests will be sent > > during the test. I haven't looked into this myself, but Lorenzo mentioned > > that there would need to be changes made to OVS itself in order to have > an > > IPv6 version of a working OVN_POPULATE_ARP. > > > > On 06/28/2018 02:16 PM, Han Zhou wrote: > > > >> 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 <mmichels@redhat.com > >> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com > >> <mailto:lorenzo.bianconi@redhat.com>> > >> Signed-off-by: Darrell Ball <dlu998@gmail.com > >> <mailto:dlu998@gmail.com>> > >> > >> --- > >> 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 > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > >> > >> > >> > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Jun 28, 2018 at 11:44 AM, Han Zhou <zhouhan@gmail.com> wrote: > Hi Mark, what I meant is the test for the feature of LB in Gateway. If we > had a test case, the problem would have been noticed when Lorenzo is > working on ICMP feature. > Here are tests: system-ovn 113: ovn -- 2 LRs connected via LS, gateway router, SNAT and DNAT ok 114: ovn -- 2 LRs connected via LS, gateway router, easy SNAT FAILED ( system-ovn.at:262) 115: ovn -- multiple gateway routers, SNAT and DNAT FAILED ( system-ovn.at:432) 116: ovn -- load-balancing ok 117: ovn -- load-balancing - same subnet. ok 118: ovn -- load balancing in gateway router ok 119: ovn -- multiple gateway routers, load-balancing FAILED ( system-ovn.at:1051) 120: ovn -- load balancing in router with gateway router port ok 121: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( system-ovn.at:1344) 122: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( system-ovn.at:1517) > > On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> > wrote: > >> Lorenzo actually brought up in today's OVN IRC meeting that writing ICMP >> tests for IPv6 is problematic right now. IPv4 tests have an M4 macro called >> OVN_POPULATE_ARP that they call. This manually populates tables in OVS so >> that there are no ARP requests being sent during the test. It does this by >> calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. >> >> IPv6 does not have an equivalent. The result is that tests will sometimes >> succeed, but sometimes will fail because IPv6 ND requests will be sent >> during the test. I haven't looked into this myself, but Lorenzo mentioned >> that there would need to be changes made to OVS itself in order to have an >> IPv6 version of a working OVN_POPULATE_ARP. >> >> On 06/28/2018 02:16 PM, Han Zhou wrote: >> >>> 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 <mmichels@redhat.com >>> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com >>> <mailto:lorenzo.bianconi@redhat.com>> >>> Signed-off-by: Darrell Ball <dlu998@gmail.com >>> <mailto:dlu998@gmail.com>> >>> >>> --- >>> 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 >>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>> >>> >>> >> >
Thanks Darrell! These tests weren't even listed in make check TESTSUITEFLAGS="--list". What's the best way to run them? I'd like to run them for each of my patch to make sure there is no regression. On Thu, Jun 28, 2018 at 11:46 AM, Darrell Ball <dlu998@gmail.com> wrote: > > > On Thu, Jun 28, 2018 at 11:44 AM, Han Zhou <zhouhan@gmail.com> wrote: > >> Hi Mark, what I meant is the test for the feature of LB in Gateway. If we >> had a test case, the problem would have been noticed when Lorenzo is >> working on ICMP feature. >> > > Here are tests: > > system-ovn > > 113: ovn -- 2 LRs connected via LS, gateway router, SNAT and DNAT ok > 114: ovn -- 2 LRs connected via LS, gateway router, easy SNAT FAILED ( > system-ovn.at:262) > 115: ovn -- multiple gateway routers, SNAT and DNAT FAILED ( > system-ovn.at:432) > 116: ovn -- load-balancing ok > 117: ovn -- load-balancing - same subnet. ok > 118: ovn -- load balancing in gateway router ok > 119: ovn -- multiple gateway routers, load-balancing FAILED ( > system-ovn.at:1051) > 120: ovn -- load balancing in router with gateway router port ok > 121: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( > system-ovn.at:1344) > 122: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( > system-ovn.at:1517) > > > > >> >> On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> >> wrote: >> >>> Lorenzo actually brought up in today's OVN IRC meeting that writing ICMP >>> tests for IPv6 is problematic right now. IPv4 tests have an M4 macro called >>> OVN_POPULATE_ARP that they call. This manually populates tables in OVS so >>> that there are no ARP requests being sent during the test. It does this by >>> calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. >>> >>> IPv6 does not have an equivalent. The result is that tests will >>> sometimes succeed, but sometimes will fail because IPv6 ND requests will be >>> sent during the test. I haven't looked into this myself, but Lorenzo >>> mentioned that there would need to be changes made to OVS itself in order >>> to have an IPv6 version of a working OVN_POPULATE_ARP. >>> >>> On 06/28/2018 02:16 PM, Han Zhou wrote: >>> >>>> 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 <mmichels@redhat.com >>>> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com >>>> <mailto:lorenzo.bianconi@redhat.com>> >>>> Signed-off-by: Darrell Ball <dlu998@gmail.com >>>> <mailto:dlu998@gmail.com>> >>>> >>>> --- >>>> 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 >>>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>>> >>>> >>>> >>> >> >
On Thu, Jun 28, 2018 at 11:55 AM, Han Zhou <zhouhan@gmail.com> wrote: > Thanks Darrell! These tests weren't even listed in make check > TESTSUITEFLAGS="--list". What's the best way to run them? I'd like to run > them for each of my patch to make sure there is no regression. > sudo make check-kmod -C _gcc //kernel sudo make check-system-userspace -C _gcc //Userspace DP > > On Thu, Jun 28, 2018 at 11:46 AM, Darrell Ball <dlu998@gmail.com> wrote: > >> >> >> On Thu, Jun 28, 2018 at 11:44 AM, Han Zhou <zhouhan@gmail.com> wrote: >> >>> Hi Mark, what I meant is the test for the feature of LB in Gateway. If >>> we had a test case, the problem would have been noticed when Lorenzo is >>> working on ICMP feature. >>> >> >> Here are tests: >> >> system-ovn >> >> 113: ovn -- 2 LRs connected via LS, gateway router, SNAT and DNAT ok >> 114: ovn -- 2 LRs connected via LS, gateway router, easy SNAT FAILED ( >> system-ovn.at:262) >> 115: ovn -- multiple gateway routers, SNAT and DNAT FAILED ( >> system-ovn.at:432) >> 116: ovn -- load-balancing ok >> 117: ovn -- load-balancing - same subnet. ok >> 118: ovn -- load balancing in gateway router ok >> 119: ovn -- multiple gateway routers, load-balancing FAILED ( >> system-ovn.at:1051) >> 120: ovn -- load balancing in router with gateway router port ok >> 121: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( >> system-ovn.at:1344) >> 122: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( >> system-ovn.at:1517) >> >> >> >> >>> >>> On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> >>> wrote: >>> >>>> Lorenzo actually brought up in today's OVN IRC meeting that writing >>>> ICMP tests for IPv6 is problematic right now. IPv4 tests have an M4 macro >>>> called OVN_POPULATE_ARP that they call. This manually populates tables in >>>> OVS so that there are no ARP requests being sent during the test. It does >>>> this by calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. >>>> >>>> IPv6 does not have an equivalent. The result is that tests will >>>> sometimes succeed, but sometimes will fail because IPv6 ND requests will be >>>> sent during the test. I haven't looked into this myself, but Lorenzo >>>> mentioned that there would need to be changes made to OVS itself in order >>>> to have an IPv6 version of a working OVN_POPULATE_ARP. >>>> >>>> On 06/28/2018 02:16 PM, Han Zhou wrote: >>>> >>>>> 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 <mmichels@redhat.com >>>>> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com >>>>> <mailto:lorenzo.bianconi@redhat.com>> >>>>> Signed-off-by: Darrell Ball <dlu998@gmail.com >>>>> <mailto:dlu998@gmail.com>> >>>>> >>>>> --- >>>>> 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 >>>>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>>>> >>>>> >>>>> >>>> >>> >> >
On Thu, Jun 28, 2018 at 11:26 AM, Darrell Ball <dlu998@gmail.com> wrote: > On Thu, Jun 28, 2018 at 9:41 AM, Mark Michelson <mmichels@redhat.com> > 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. > > > > Pls double check that we are in same page first. > > You can run the ovn system tests without the patch and take a look at one > of the 5 failures, for example > > AT_SETUP([ovn -- multiple gateway routers, load-balancing]) > > The load balance VIP is "vips:30.0.0.1" > > One the the router port's of interest has IP 20.0.0.2 > > Once you confirm that there is no overlap b/w VIP and router port IP, I think one possibility is to only exclude "lb_force_snat_ip" router port IPs; I did not give it too much thought yet, however, but seems doable. There would be a cost to this finer grained filtering, of course. > > >> >> >> 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 <lorenzo.bianconi@redhat.com> >>> Signed-off-by: Darrell Ball <dlu998@gmail.com> >>> --- >>> 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); >>> + } >>> } >>> } >>> >>> >> >> >
Great. It works: sudo make check-system-userspace TESTSUITEFLAGS="-k ovn" Thanks again! On Thu, Jun 28, 2018 at 11:57 AM, Darrell Ball <dlu998@gmail.com> wrote: > > > On Thu, Jun 28, 2018 at 11:55 AM, Han Zhou <zhouhan@gmail.com> wrote: > >> Thanks Darrell! These tests weren't even listed in make check >> TESTSUITEFLAGS="--list". What's the best way to run them? I'd like to run >> them for each of my patch to make sure there is no regression. >> > > > sudo make check-kmod -C _gcc //kernel > sudo make check-system-userspace -C _gcc //Userspace DP > > >> >> On Thu, Jun 28, 2018 at 11:46 AM, Darrell Ball <dlu998@gmail.com> wrote: >> >>> >>> >>> On Thu, Jun 28, 2018 at 11:44 AM, Han Zhou <zhouhan@gmail.com> wrote: >>> >>>> Hi Mark, what I meant is the test for the feature of LB in Gateway. If >>>> we had a test case, the problem would have been noticed when Lorenzo is >>>> working on ICMP feature. >>>> >>> >>> Here are tests: >>> >>> system-ovn >>> >>> 113: ovn -- 2 LRs connected via LS, gateway router, SNAT and DNAT ok >>> 114: ovn -- 2 LRs connected via LS, gateway router, easy SNAT FAILED ( >>> system-ovn.at:262) >>> 115: ovn -- multiple gateway routers, SNAT and DNAT FAILED ( >>> system-ovn.at:432) >>> 116: ovn -- load-balancing ok >>> 117: ovn -- load-balancing - same subnet. ok >>> 118: ovn -- load balancing in gateway router ok >>> 119: ovn -- multiple gateway routers, load-balancing FAILED ( >>> system-ovn.at:1051) >>> 120: ovn -- load balancing in router with gateway router port ok >>> 121: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( >>> system-ovn.at:1344) >>> 122: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( >>> system-ovn.at:1517) >>> >>> >>> >>> >>>> >>>> On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> >>>> wrote: >>>> >>>>> Lorenzo actually brought up in today's OVN IRC meeting that writing >>>>> ICMP tests for IPv6 is problematic right now. IPv4 tests have an M4 macro >>>>> called OVN_POPULATE_ARP that they call. This manually populates tables in >>>>> OVS so that there are no ARP requests being sent during the test. It does >>>>> this by calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. >>>>> >>>>> IPv6 does not have an equivalent. The result is that tests will >>>>> sometimes succeed, but sometimes will fail because IPv6 ND requests will be >>>>> sent during the test. I haven't looked into this myself, but Lorenzo >>>>> mentioned that there would need to be changes made to OVS itself in order >>>>> to have an IPv6 version of a working OVN_POPULATE_ARP. >>>>> >>>>> On 06/28/2018 02:16 PM, Han Zhou wrote: >>>>> >>>>>> 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 <mmichels@redhat.com >>>>>> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com >>>>>> <mailto:lorenzo.bianconi@redhat.com>> >>>>>> Signed-off-by: Darrell Ball <dlu998@gmail.com >>>>>> <mailto:dlu998@gmail.com>> >>>>>> >>>>>> --- >>>>>> 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 >>>>>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
On Thu, Jun 28, 2018 at 11:59 AM, Darrell Ball <dlu998@gmail.com> wrote: > > > On Thu, Jun 28, 2018 at 11:26 AM, Darrell Ball <dlu998@gmail.com> wrote: > >> On Thu, Jun 28, 2018 at 9:41 AM, Mark Michelson <mmichels@redhat.com> >> 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. >> >> >> >> Pls double check that we are in same page first. >> >> You can run the ovn system tests without the patch and take a look at one >> of the 5 failures, for example >> >> AT_SETUP([ovn -- multiple gateway routers, load-balancing]) >> >> The load balance VIP is "vips:30.0.0.1" >> >> One the the router port's of interest has IP 20.0.0.2 >> >> > Once you confirm that there is no overlap b/w VIP and router port IP, > > I think one possibility is to only exclude "lb_force_snat_ip" router port > IPs; I did not give it too much thought yet, however, but seems doable. > There would be a cost to this finer grained filtering, of course. > After a bit of chewing on the finer grained filtering question, I take back the above suggestion regarding "lb_force_snat_ip", as it will not work in all cases. Implementing finer grained filtering would involve checking several pieces of information; I am not sure it is worth the added cost and complexity. > > > >> >> >>> >>> >>> 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 <lorenzo.bianconi@redhat.com> >>>> Signed-off-by: Darrell Ball <dlu998@gmail.com> >>>> --- >>>> 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); >>>> + } >>>> } >>>> } >>>> >>>> >>> >>> >> >
On 06/28/2018 02:26 PM, Darrell Ball wrote: > On Thu, Jun 28, 2018 at 9:41 AM, Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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. > > > > Pls double check that we are in same page first. > > You can run the ovn system tests without the patch and take a look at > one of the 5 failures, for example > > AT_SETUP([ovn -- multiple gateway routers, load-balancing]) > > The load balance VIP is "vips:30.0.0.1" > > One the the router port's of interest has IP 20.0.0.2 Ah, I may have been mistaken then. Based on skimming the code, I thought that the ICMP rules were only added for the configured router IP addresses, and not for load balancer VIPs. But if the ICMP rules are also installed for load balancer VIPs, then I was mistaken. > > > > 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 <lorenzo.bianconi@redhat.com > <mailto:lorenzo.bianconi@redhat.com>> > Signed-off-by: Darrell Ball <dlu998@gmail.com > <mailto:dlu998@gmail.com>> > --- >  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); > +      } >      } >    } > > >
> > > > After a bit of chewing on the finer grained filtering question, I take back > the above suggestion regarding "lb_force_snat_ip", as it will not work > in all cases. Implementing finer grained filtering would involve checking > several pieces of information; I am not sure it is worth the added cost > and complexity. > It looks to me that we need to skip the protocol port unreachable message for the following 2 cases (unless I am missing some other case) 1. If any SNAT, DNAT or LB rules exist for the router in question where the router IP is used for NATting. 2. If there is a dnat_force_snat_ip or lb_force_snat_ip set for the router. But my personal opinion is that it need not be part of this patch as this one fixes a regression and should come as a patch that enhances ICMP port unreachability feature. > > > > > > > > > > >> > >> > >>> > >>> > >>> 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 <lorenzo.bianconi@redhat.com> > >>>> Signed-off-by: Darrell Ball <dlu998@gmail.com> > >>>> --- > >>>> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Thu, Jun 28, 2018 at 11:55 AM, Han Zhou <zhouhan@gmail.com> wrote: > > > Thanks Darrell! These tests weren't even listed in make check > > TESTSUITEFLAGS="--list". What's the best way to run them? I'd like to run > > them for each of my patch to make sure there is no regression. > > > > > sudo make check-kmod -C _gcc //kernel > sudo make check-system-userspace -C _gcc //Userspace DP Hi Darrell, thx for the info, I will always use it in subsequent patches. I implemented what is indicated in ovn-northd doc and just run ovn tests (not system ones). Thx for fixing it Regards, Lorenzo > > > > > > On Thu, Jun 28, 2018 at 11:46 AM, Darrell Ball <dlu998@gmail.com> wrote: > > > >> > >> > >> On Thu, Jun 28, 2018 at 11:44 AM, Han Zhou <zhouhan@gmail.com> wrote: > >> > >>> Hi Mark, what I meant is the test for the feature of LB in Gateway. If > >>> we had a test case, the problem would have been noticed when Lorenzo is > >>> working on ICMP feature. > >>> > >> > >> Here are tests: > >> > >> system-ovn > >> > >> 113: ovn -- 2 LRs connected via LS, gateway router, SNAT and DNAT ok > >> 114: ovn -- 2 LRs connected via LS, gateway router, easy SNAT FAILED ( > >> system-ovn.at:262) > >> 115: ovn -- multiple gateway routers, SNAT and DNAT FAILED ( > >> system-ovn.at:432) > >> 116: ovn -- load-balancing ok > >> 117: ovn -- load-balancing - same subnet. ok > >> 118: ovn -- load balancing in gateway router ok > >> 119: ovn -- multiple gateway routers, load-balancing FAILED ( > >> system-ovn.at:1051) > >> 120: ovn -- load balancing in router with gateway router port ok > >> 121: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( > >> system-ovn.at:1344) > >> 122: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( > >> system-ovn.at:1517) > >> > >> > >> > >> > >>> > >>> On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson <mmichels@redhat.com> > >>> wrote: > >>> > >>>> Lorenzo actually brought up in today's OVN IRC meeting that writing > >>>> ICMP tests for IPv6 is problematic right now. IPv4 tests have an M4 macro > >>>> called OVN_POPULATE_ARP that they call. This manually populates tables in > >>>> OVS so that there are no ARP requests being sent during the test. It does > >>>> this by calling `ovs-appct tnl/neigh/set` for each IP address-MAC pair. > >>>> > >>>> IPv6 does not have an equivalent. The result is that tests will > >>>> sometimes succeed, but sometimes will fail because IPv6 ND requests will be > >>>> sent during the test. I haven't looked into this myself, but Lorenzo > >>>> mentioned that there would need to be changes made to OVS itself in order > >>>> to have an IPv6 version of a working OVN_POPULATE_ARP. > >>>> > >>>> On 06/28/2018 02:16 PM, Han Zhou wrote: > >>>> > >>>>> 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 <mmichels@redhat.com > >>>>> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com > >>>>> <mailto:lorenzo.bianconi@redhat.com>> > >>>>> Signed-off-by: Darrell Ball <dlu998@gmail.com > >>>>> <mailto:dlu998@gmail.com>> > >>>>> > >>>>> --- > >>>>> 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 > >>>>> dev@openvswitch.org <mailto:dev@openvswitch.org> > >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > >>>>> > >>>>> > >>>>> > >>>> > >>> > >> > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Jun 29, 2018 at 3:31 AM, Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > On Thu, Jun 28, 2018 at 11:55 AM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > Thanks Darrell! These tests weren't even listed in make check > > > TESTSUITEFLAGS="--list". What's the best way to run them? I'd like to > run > > > them for each of my patch to make sure there is no regression. > > > > > > > > > sudo make check-kmod -C _gcc //kernel > > sudo make check-system-userspace -C _gcc //Userspace DP > > Hi Darrell, > > thx for the info, I will always use it in subsequent patches. > I implemented what is indicated in ovn-northd doc and just run > ovn tests (not system ones). Thx for fixing it > > Regards, > Lorenzo > No worries Lorenzo. This is a pretty unexpected overlap in functionality. Darrell > > > > > > > > > > > On Thu, Jun 28, 2018 at 11:46 AM, Darrell Ball <dlu998@gmail.com> > wrote: > > > > > >> > > >> > > >> On Thu, Jun 28, 2018 at 11:44 AM, Han Zhou <zhouhan@gmail.com> wrote: > > >> > > >>> Hi Mark, what I meant is the test for the feature of LB in Gateway. > If > > >>> we had a test case, the problem would have been noticed when Lorenzo > is > > >>> working on ICMP feature. > > >>> > > >> > > >> Here are tests: > > >> > > >> system-ovn > > >> > > >> 113: ovn -- 2 LRs connected via LS, gateway router, SNAT and DNAT ok > > >> 114: ovn -- 2 LRs connected via LS, gateway router, easy SNAT FAILED ( > > >> system-ovn.at:262) > > >> 115: ovn -- multiple gateway routers, SNAT and DNAT FAILED ( > > >> system-ovn.at:432) > > >> 116: ovn -- load-balancing ok > > >> 117: ovn -- load-balancing - same subnet. ok > > >> 118: ovn -- load balancing in gateway router ok > > >> 119: ovn -- multiple gateway routers, load-balancing FAILED ( > > >> system-ovn.at:1051) > > >> 120: ovn -- load balancing in router with gateway router port ok > > >> 121: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( > > >> system-ovn.at:1344) > > >> 122: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( > > >> system-ovn.at:1517) > > >> > > >> > > >> > > >> > > >>> > > >>> On Thu, Jun 28, 2018 at 11:29 AM, Mark Michelson < > mmichels@redhat.com> > > >>> wrote: > > >>> > > >>>> Lorenzo actually brought up in today's OVN IRC meeting that writing > > >>>> ICMP tests for IPv6 is problematic right now. IPv4 tests have an M4 > macro > > >>>> called OVN_POPULATE_ARP that they call. This manually populates > tables in > > >>>> OVS so that there are no ARP requests being sent during the test. > It does > > >>>> this by calling `ovs-appct tnl/neigh/set` for each IP address-MAC > pair. > > >>>> > > >>>> IPv6 does not have an equivalent. The result is that tests will > > >>>> sometimes succeed, but sometimes will fail because IPv6 ND requests > will be > > >>>> sent during the test. I haven't looked into this myself, but Lorenzo > > >>>> mentioned that there would need to be changes made to OVS itself in > order > > >>>> to have an IPv6 version of a working OVN_POPULATE_ARP. > > >>>> > > >>>> On 06/28/2018 02:16 PM, Han Zhou wrote: > > >>>> > > >>>>> 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 < > mmichels@redhat.com > > >>>>> <mailto:mmichels@redhat.com>> 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 <lorenzo.bianconi@redhat.com > > >>>>> <mailto:lorenzo.bianconi@redhat.com>> > > >>>>> Signed-off-by: Darrell Ball <dlu998@gmail.com > > >>>>> <mailto:dlu998@gmail.com>> > > >>>>> > > >>>>> --- > > >>>>> 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 > > >>>>> dev@openvswitch.org <mailto:dev@openvswitch.org> > > >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >>>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > >>>>> > > >>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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); + } } }
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 <lorenzo.bianconi@redhat.com> Signed-off-by: Darrell Ball <dlu998@gmail.com> --- ovn/northd/ovn-northd.8.xml | 17 ++++--- ovn/northd/ovn-northd.c | 106 ++++++++++++++++++++++---------------------- 2 files changed, 64 insertions(+), 59 deletions(-)