Hi Mark, Thanks for the patch. I only have one question 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? > } > > /* 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
