On 3/19/26 10:21 AM, Xavier Simonart via dev wrote: > Hi Mark, > Hi Xavier,
> Thanks for the patch. > I only have one question below > Thanks for checking it out! I'll try to reply below. > Others than that looks good to me > Thanks > Xavier > > On Wed, Mar 18, 2026 at 6:52 PM Mark Michelson via dev < > [email protected]> wrote: > >> UDP health checks rely on receiving either an ICMP error to indicate >> that the service is down, or no response as a way to indicate that the >> service is live. >> >> If a particularly dumb service (for instance a UDP "echo" service) is >> configured, it will send UDP responses to our health check probes. In >> this case, we don't want to send the response to our health check >> processing, because the health check does not expect to get a UDP >> response. Instead, ignore the response and let it get dropped eventually >> by the logical router. >> >> Fixes: ceea1d7cccad ("northd: Allow LB health checks from router IPs.") >> Reported-at: https://redhat.atlassian.net/browse/FDP-3435 >> Co-authored-by: Dumitru Ceara <[email protected]> >> Signed-off-by: Dumitru Ceara <[email protected]> >> Signed-off-by: Mark Michelson <[email protected]> >> --- >> northd/northd.c | 32 +++++++++-------- >> tests/ovn.at | 26 +++----------- >> tests/system-ovn.at | 87 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 109 insertions(+), 36 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 734efea65..89d58074c 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -8872,25 +8872,29 @@ build_lb_health_check_response_lflows( >> /* icmp6 type 1 and icmp4 type 3 are included in the match, >> because >> * the controller is using them to detect unreachable ports. >> */ >> if (addr_is_ipv6(backend_nb->svc_mon_src_ip)) { >> - ds_put_format(match, >> - "inport == \"%s\" && ip6.dst == %s && " >> - "eth.dst == %s && " >> - "(%s.src == %s || icmp6.type == 1)", >> + ds_put_format(match, "inport == \"%s\" && ip6.dst == %s >> && " >> + "ip6.src == %s && eth.dst == %s && ", >> backend_nb->logical_port, >> backend_nb->svc_mon_src_ip, >> - backend_nb->svc_mon_lrp->lrp_networks.ea_s, >> - protocol, >> - backend->port_str); >> + backend->ip_str, >> + backend_nb->svc_mon_lrp->lrp_networks.ea_s); >> + if (!strcmp(protocol, "tcp")) { >> + ds_put_format(match, "tcp.src == %s", >> backend->port_str); >> + } else { >> + ds_put_cstr(match, "icmp6.type == 1"); >> + } >> } else { >> - ds_put_format(match, >> - "inport == \"%s\" && ip4.dst == %s && " >> - "eth.dst == %s && " >> - "(%s.src == %s || icmp4.type == 3)", >> + ds_put_format(match, "inport == \"%s\" && ip4.dst == %s >> && " >> + "ip4.src == %s && eth.dst == %s && ", >> backend_nb->logical_port, >> backend_nb->svc_mon_src_ip, >> - backend_nb->svc_mon_lrp->lrp_networks.ea_s, >> - protocol, >> - backend->port_str); >> + backend->ip_str, >> + backend_nb->svc_mon_lrp->lrp_networks.ea_s); >> + if (!strcmp(protocol, "tcp")) { >> + ds_put_format(match, "tcp.src == %s", >> backend->port_str); >> + } else { >> + ds_put_cstr(match, "icmp4.type == 3"); >> + } >> > Can't we sometimes receive an ICMPv6 type 1 (or v4 equivalent) as a > negative reply to a TCP health check? > It has always been the way the service monitor has worked in OVN, for TCP we expect replies to be TCP.RST. That was the case in the original implementation: https://github.com/ovn-org/ovn/commit/8be01f4a5329ed1d4c920e0b8488bd4a5410ffb9 And that is still the case today. If we'd receive an ICMP error as a result to a TCP.SYN we'd fail to find the correct svc_monitor in pinctrl anyway. So, while this could be improved potentially, this patch doesn't introduce a behavior change. Do you agree? Regards, Dumitru >> } >> >> /* ovn-controller expects health check responses from the LS >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 6580de6c2..750cff7c2 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -27208,20 +27208,11 @@ AT_CHECK([grep "priority=110" sbflows2 | grep >> "handle_svc_check" | \ >> grep 'inport == "sw1-p1"' | grep "ip4.dst == 20.0.0.1" | \ >> grep -q "tcp.src == 80" ]) >> >> -# Verify flow also matches ICMP unreachable (type 3 for IPv4). >> -AT_CHECK([grep "priority=110" sbflows1 | grep "handle_svc_check" | \ >> - grep 'inport == "sw0-p1"' | grep "ip4.dst == 10.0.0.1" | \ >> - grep -q "icmp4.type == 3"]) >> - >> -AT_CHECK([grep "priority=110" sbflows2 | grep "handle_svc_check" | \ >> - grep 'inport == "sw1-p1"' | grep "ip4.dst == 20.0.0.1" | \ >> - grep -q "icmp4.type == 3"]) >> - >> ## UDP Load balancer health check flows. >> -# UDP backend - verify flow uses UDP instead of TCP. >> +# UDP backend - verify flow uses ICMP type 3 instead of TCP. >> AT_CHECK([grep "priority=110" sbflows1 | grep "handle_svc_check" | \ >> grep 'inport == "sw0-p2"' | grep "ip4.dst == 10.0.0.1" | \ >> - grep -q "udp.src == 53"]) >> + grep -q "icmp4.type == 3"]) >> >> # Verify NO TCP match in the UDP backend flow. >> AT_CHECK([grep "priority=110" sbflows1 | grep "handle_svc_check" | \ >> @@ -27370,20 +27361,11 @@ AT_CHECK([grep "priority=110" sbflows2 | grep >> "handle_svc_check" | \ >> grep 'inport == "sw1-p1"' | grep "ip6.dst == 2002::1" | \ >> grep -q "tcp.src == 80" ]) >> >> -# Verify flow also matches ICMP unreachable (type 1 for IPv6). >> -AT_CHECK([grep "priority=110" sbflows1 | grep "handle_svc_check" | \ >> - grep 'inport == "sw0-p1"' | grep "ip6.dst == 2001::1" | \ >> - grep -q "icmp6.type == 1"]) >> - >> -AT_CHECK([grep "priority=110" sbflows2 | grep "handle_svc_check" | \ >> - grep 'inport == "sw1-p1"' | grep "ip6.dst == 2002::1" | \ >> - grep -q "icmp6.type == 1"]) >> - >> ## UDP Load balancer health check flows. >> -# UDP backend - verify flow uses UDP instead of TCP. >> +# UDP backend - verify flow uses ICMP instead of TCP. >> AT_CHECK([grep "priority=110" sbflows1 | grep "handle_svc_check" | \ >> grep 'inport == "sw0-p2"' | grep "ip6.dst == 2001::1" | \ >> - grep -q "udp.src == 53"]) >> + grep -q "icmp6.type == 1"]) >> >> # Verify NO TCP match in the UDP backend flow. >> AT_CHECK([grep "priority=110" sbflows1 | grep "handle_svc_check" | \ >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index 7edb8a45b..79bad55a9 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -21496,3 +21496,90 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port >> patch-.*/d >> /connection dropped.*/d"]) >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD([ >> +AT_SETUP([Unsupported protocol message]) >> +AT_SKIP_IF([test $HAVE_NC = no]) >> + >> +ovn_start >> +OVS_TRAFFIC_VSWITCHD_START() >> +ADD_BR([br-int]) >> + >> +# Set external-ids in br-int needed for ovn-controller. >> +check ovs-vsctl \ >> + -- set Open_vSwitch . external-ids:system-id=hv1 \ >> + -- set Open_vSwitch . >> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ >> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ >> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ >> + -- set bridge br-int fail-mode=secure >> other-config:disable-in-band=true >> + >> +# Start ovn-controller. >> +start_daemon ovn-controller >> + >> +check ovn-nbctl ls-add ls1 >> +check ovn-nbctl lsp-add ls1 ls1p1 >> +check ovn-nbctl lsp-set-addresses ls1p1 "00:00:00:01:01:01 192.168.1.1" >> +check ovn-nbctl lsp-add ls1 ls1p2 >> +check ovn-nbctl lsp-set-addresses ls1p2 "00:00:00:01:01:02 192.168.1.2" >> + >> +check ovn-nbctl lr-add lr1 >> +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 192.168.1.254/24 >> +check ovn-nbctl lsp-add ls1 ls1-lr1 >> +check ovn-nbctl lsp-set-addresses ls1-lr1 "00:00:00:00:00:01 >> 192.168.1.254" >> +check ovn-nbctl lsp-set-type ls1-lr1 router >> +check ovn-nbctl lsp-set-options ls1-lr1 router-port=lr1-ls1 >> + >> +check ovn-nbctl lrp-add lr1 lr1-ls2 00:00:00:00:00:02 192.168.2.254/24 >> + >> +check ovn-nbctl ls-add ls2 >> +check ovn-nbctl lsp-add ls2 ls2-lr1 >> +check ovn-nbctl lsp-set-addresses ls2-lr1 "00:00:00:00:00:02 >> 192.168.2.254" >> +check ovn-nbctl lsp-set-type ls2-lr1 router >> +check ovn-nbctl lsp-set-options ls2-lr1 router-port=lr1-ls2 >> + >> +check ovn-nbctl lsp-add ls2 ls2p1 >> +check ovn-nbctl lsp-set-addresses ls2p1 "00:00:00:01:02:01 192.168.2.1" >> + >> +ADD_NAMESPACES(ls1p1) >> +ADD_VETH(ls1p1, ls1p1, br-int, "192.168.1.1/24", "00:00:00:01:01:01", >> + "192.168.1.254") >> + >> +ADD_NAMESPACES(ls2p1) >> +ADD_VETH(ls2p1, ls2p1, br-int, "192.168.2.1/24", "00:00:00:01:02:01", >> + "192.168.2.254") >> + >> +ADD_NAMESPACES(ls1p2) >> +ADD_VETH(ls1p2, ls1p2, br-int, "192.168.1.2/24", "00:00:00:01:01:02", >> + "192.168.1.254") >> + >> +check ovn-nbctl lb-add lb0 192.168.5.1:12345 192.168.1.1:12345, >> 192.168.1.2:12345 >> +check ovn-nbctl ls-lb-add ls1 lb0 >> +check ovn-nbctl lr-lb-add lr1 lb0 >> +lb_uuid=$(fetch_column nb:Load_Balancer _uuid name=lb0) >> +check ovn-nbctl set Load_Balancer $lb_uuid protocol=udp >> +check ovn-nbctl --wait=hv set Logical_Router lr1 options:chassis="hv1" >> + >> +wait_for_ports_up ls1p1 ls1p2 ls2p1 >> + >> +NETNS_DAEMONIZE([ls1p1], [nc -l 12345 --udp -k --sh-exec ls], [nc1.pid]) >> +NETNS_DAEMONIZE([ls1p2], [nc -l 12345 --udp -k --sh-exec ls], [nc2.pid]) >> + >> +hc_uuid=$(ovn-nbctl --id=@hc create Load_Balancer_Health_Check >> vip="192.168.5.1\:12345" -- \ >> + add Load_Balancer $lb_uuid health_check @hc) >> +check ovn-nbctl set Load_Balancer_Health_Check $hc_uuid >> options:timeout=20 options:success_count=3 options:failure_count=3 >> +check ovn-nbctl --wait=sb set load_balancer $lb_uuid >> ip_port_mappings:192.168.1.1=ls1p1:192.168.1.254 >> + >> +NS_EXEC([ls2p1], [nc --udp 192.168.5.1 12345 <<< h]) >> + >> +# It may not seem like we're actually testing anything in this test. >> +# If there is a warning or error in the ovn-controller log about >> +# an unsupported health check protocol, it will cause a test failure >> +# when we stop ovn-controller. >> +OVN_CLEANUP_CONTROLLER([hv1]) >> +OVN_CLEANUP_NORTHD >> + >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> +AT_CLEANUP >> +]) >> > Thanks > Xavier > >> -- >> 2.52.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
