On 10/15/25 6:44 PM, Mark Michelson via dev wrote: > Recheck-request: github-robot >
Unfortunately, this way of trigerring the recheck still doesn't seem to work. The robot just replied with the old job link for "Build and Test" and re-ran only the ovn-kubernetes job: https://mail.openvswitch.org/pipermail/ovs-build/2025-October/049958.html Known pw-ci issue: https://github.com/ovsrobot/pw-ci/issues/16 In order to rerun the Build_and_Test job we need: Recheck-request: github-robot-_Build_and_Test > On Tue, Oct 14, 2025 at 4:48 PM Mark Michelson via dev > <[email protected]> wrote: >> >> By default, northd does not install any responder flows for unicast ARP. >> These are intended to be forwarded to the destination so that the >> destination can respond appropriately. For VIF ports, this tends to work >> as expected in most scenarios. >> >> When proxy ARP is configured on a logical switch port connected to a >> logical router, then we install low priority flows to ensure that >> ARPs for the configured proxy addresses are responded to by the logical >> switch. These proxy ARP flows are hit when unicast ARP requests are sent >> for the VIF ports on the logical switch. We therefore end up responding >> incorrectly to unicast ARP requests with the proxy ARP MAC instead of >> forwarding the ARP request to the proper VIF port. >> >> This commit fixes the issue by installing explicit "next;" actions for >> unicast ARP requests directed towards VIFs so that the proxy ARP flows >> will not be hit. These flows are only installed if proxy ARP is >> configured, since they are unnecessary otherwise. >> >> Reported-at: https://issues.redhat.com/browse/FDP-1646 >> Signed-off-by: Mark Michelson <[email protected]> >> --- >> v3: >> * Split the new build_lswitch_arp_nd_unicast_flows() function into >> smaller pieces. This has several benefits: >> * v2 had several checkpatch warnings about the lines exceeding 80 >> characters. This made it easier to stay under the line length >> limit. >> * v2 had a subtle indexing error that caused a crash in the >> proxy_arp test. Splitting this up helps make the indexing easier >> to follow. >> * This makes the logic much easier to understand, visually. >> * Fixed some typos in the commit message. >> >> v2: >> * This version also fixes the problem with IPv6 ND_NS packets. The >> tests have been updated to test for IPv6. >> * This version only installs unicast ARP/ND_NS flows if the LSP >> address overlaps with the configured proxy_arp addresses. The >> ovn-northd test has been updated to ensure that this is correct. The >> code that does this is pretty gnarly, since it's four levels of >> for loops. But it works! >> * Fixed nits from Xavier (removed ofport request, removed unnecessary >> lflow-list, etc.). >> --- >> >> northd/northd.c | 132 +++++++++++++++++++++++++++++++++++++++----- >> northd/northd.h | 7 +++ >> tests/ovn-northd.at | 93 +++++++++++++++++++++++++++++++ >> tests/ovn.at | 76 +++++++++++++++++++++++++ >> 4 files changed, 294 insertions(+), 14 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 3b1d3eba7..9eaa88d95 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -557,6 +557,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct >> uuid *key, >> od->tunnel_key = sdp->sb_dp->tunnel_key; >> init_mcast_info_for_datapath(od); >> od->datapath_lflows = lflow_ref_create(); >> + od->proxy_arp_addrs = VECTOR_EMPTY_INITIALIZER(struct lport_addresses); >> return od; >> } >> >> @@ -586,6 +587,11 @@ ovn_datapath_destroy(struct ovn_datapath *od) >> destroy_ports_for_datapath(od); >> sset_destroy(&od->router_ips); >> lflow_ref_destroy(od->datapath_lflows); >> + /* The ovn_ports own the proxy_arp_addresses, so we do not >> + * need to call destroy_lport_addresses() on the components >> + * of the vector >> + */ >> + vector_destroy(&od->proxy_arp_addrs); >> free(od); >> } >> } >> @@ -1876,6 +1882,8 @@ join_logical_ports(const struct >> sbrec_port_binding_table *sbrec_pb_table, >> if (extract_addresses(arp_proxy, &op->proxy_arp_addrs, >> &ofs) || >> extract_ip_addresses(arp_proxy, &op->proxy_arp_addrs)) { >> op->od->has_arp_proxy_port = true; >> + vector_push(&op->od->proxy_arp_addrs, >> + &op->proxy_arp_addrs); >> } else { >> static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(1, 5); >> @@ -9836,6 +9844,112 @@ build_lswitch_arp_nd_responder_skip_local(struct >> ovn_port *op, >> &op->nbsp->header_, op->lflow_ref); >> } >> >> +static void >> +build_arp_match(struct ds *match, const char *ipv4_addr, const char *mac) >> +{ >> + ds_put_format(match, "arp.tpa == %s && arp.op == 1 && eth.dst == %s", >> + ipv4_addr, mac); >> +} >> + >> +static void >> +build_nd_match(struct ds *match, const struct ipv6_netaddr *ipv6_addr, >> + bool is_multicast) >> +{ >> + if (is_multicast) { >> + ds_put_format(match, "nd_ns_mcast && ip6.dst == %s && nd.target == >> %s", >> + ipv6_addr->sn_addr_s, ipv6_addr->addr_s); >> + } else { >> + ds_put_format(match, "nd_ns && ip6.dst == %s && nd.target == %s", >> + ipv6_addr->addr_s, ipv6_addr->addr_s); >> + } >> +} >> + >> +static void >> +build_arp_unicast_flows_for_addrs( >> + const struct lport_addresses *proxy_arp_addrs, >> + const struct lport_addresses *lsp_addrs, >> + struct ovn_port *op, >> + struct lflow_table *lflows, >> + struct ds *match) >> +{ >> + for (size_t i = 0; i < proxy_arp_addrs->n_ipv4_addrs; i++) { >> + struct ipv4_netaddr *proxy_arp_ipv4 = >> + &proxy_arp_addrs->ipv4_addrs[i]; >> + for (size_t j = 0; j < lsp_addrs->n_ipv4_addrs; j++) { >> + struct ipv4_netaddr *lsp_ipv4 = &lsp_addrs->ipv4_addrs[j]; >> + ovs_be32 lsp_network = lsp_ipv4->addr & proxy_arp_ipv4->mask; >> + if (lsp_network != proxy_arp_ipv4->network) { >> + continue; >> + } >> + ds_clear(match); >> + build_arp_match(match, lsp_ipv4->addr_s, lsp_addrs->ea_s); >> + ovn_lflow_add_with_hint(lflows, op->od, S_SWITCH_IN_ARP_ND_RSP, >> 50, >> + ds_cstr(match), "next;", >> + &op->nbsp->header_, op->lflow_ref); >> + } >> + } >> +} >> + >> +static void >> +build_nd_ns_unicast_flows_for_addrs( >> + const struct lport_addresses *proxy_arp_addrs, >> + const struct lport_addresses *lsp_addrs, >> + struct ovn_port *op, >> + struct lflow_table *lflows, >> + struct ds *match) >> +{ >> + for (size_t i = 0; i < proxy_arp_addrs->n_ipv6_addrs; i++) { >> + struct ipv6_netaddr *proxy_arp_ipv6 = >> + &proxy_arp_addrs->ipv6_addrs[i]; >> + for (size_t j = 0; j < lsp_addrs->n_ipv6_addrs; j++) { >> + struct ipv6_netaddr *lsp_ipv6 = &lsp_addrs->ipv6_addrs[j]; >> + struct in6_addr *proxy_arp_mask = &proxy_arp_ipv6->mask; >> + struct in6_addr *proxy_arp_network = &proxy_arp_ipv6->network; >> + struct in6_addr network = ipv6_addr_bitand(&lsp_ipv6->addr, >> + proxy_arp_mask); >> + if (!ipv6_addr_equals(proxy_arp_network, &network)) { >> + continue; >> + } >> + ds_clear(match); >> + build_nd_match(match, lsp_ipv6, false); >> + ovn_lflow_add_with_hint(lflows, op->od, >> + S_SWITCH_IN_ARP_ND_RSP, 50, >> + ds_cstr(match), >> + "next;", &op->nbsp->header_, >> + op->lflow_ref); >> + } >> + } >> +} >> + >> +static void >> +build_lswitch_arp_nd_unicast_flows(struct ovn_port *op, >> + struct lflow_table *lflows, >> + struct ds *match) >> +{ >> + /* Typically, we don't need to build any unicast flows for ARP or ND >> + * since the natural switching behavior of the logical switch will >> + * get the packet to its intended destination. However, if proxy ARP >> + * is configured, then we may need to install flows to ensure that >> + * we do not incorrectly respond to unicast ARPs destined to a known >> + * IP with the proxy ARP instead. >> + */ >> + if (!op->od->nbs || !op->od->has_arp_proxy_port) { >> + return; >> + } >> + >> + struct lport_addresses *proxy_arp_addrs; >> + VECTOR_FOR_EACH_PTR (&op->od->proxy_arp_addrs, proxy_arp_addrs) { >> + for (size_t i = 0; i < op->n_lsp_addrs; i++) { >> + build_arp_unicast_flows_for_addrs(proxy_arp_addrs, >> + &op->lsp_addrs[i], >> + op, lflows, match); >> + build_nd_ns_unicast_flows_for_addrs(proxy_arp_addrs, >> + &op->lsp_addrs[i], >> + op, lflows, match); >> + } >> + } >> +} >> + >> /* Ingress table 24: ARP/ND responder, reply for known IPs. >> * (priority 50). */ >> static void >> @@ -9956,15 +10070,8 @@ build_lswitch_arp_nd_responder_known_ips(struct >> ovn_port *op, >> for (size_t i = 0; i < op->n_lsp_addrs; i++) { >> for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) { >> ds_clear(match); >> - /* Do not reply on unicast ARPs, forward them to the target >> - * to have ability to monitor target liveness via unicast >> - * ARP requests. >> - */ >> - ds_put_format(match, >> - "arp.tpa == %s && " >> - "arp.op == 1 && " >> - "eth.dst == ff:ff:ff:ff:ff:ff", >> - op->lsp_addrs[i].ipv4_addrs[j].addr_s); >> + build_arp_match(match, >> op->lsp_addrs[i].ipv4_addrs[j].addr_s, >> + "ff:ff:ff:ff:ff:ff"); >> ds_clear(actions); >> ds_put_format(actions, >> "eth.dst = eth.src; " >> @@ -10017,11 +10124,7 @@ build_lswitch_arp_nd_responder_known_ips(struct >> ovn_port *op, >> */ >> for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { >> ds_clear(match); >> - ds_put_format( >> - match, >> - "nd_ns_mcast && ip6.dst == %s && nd.target == %s", >> - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, >> - op->lsp_addrs[i].ipv6_addrs[j].addr_s); >> + build_nd_match(match, &op->lsp_addrs[i].ipv6_addrs[j], >> true); >> >> ds_clear(actions); >> ds_put_format(actions, >> @@ -10061,6 +10164,7 @@ build_lswitch_arp_nd_responder_known_ips(struct >> ovn_port *op, >> op->lflow_ref); >> } >> } >> + build_lswitch_arp_nd_unicast_flows(op, lflows, match); >> } >> if (op->proxy_arp_addrs.n_ipv4_addrs || >> op->proxy_arp_addrs.n_ipv6_addrs) { >> diff --git a/northd/northd.h b/northd/northd.h >> index cdc532954..dc863ec48 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -471,6 +471,13 @@ struct ovn_datapath { >> /* Reference to the lflows belonging to this datapath currently router >> * only lflows. */ >> struct lflow_ref *datapath_lflows; >> + >> + /* This vector contains pointers to struct lport_addresses. These are >> + * the configured "arp_proxy" addresses of all logical switch ports on >> + * this datapath. The ovn_ports own these addresses, so we should not >> + * free them when destroying the ovn_datapath. >> + */ >> + struct vector proxy_arp_addrs; >> }; >> >> const struct ovn_datapath *ovn_datapath_find(const struct hmap *datapaths, >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 07fb57bd6..6b978bd6a 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -18716,3 +18716,96 @@ AT_CHECK( >> >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([Unicast ARP flows]) >> +ovn_start >> + >> +# Typically on logical switches, the ARP responder stage installs nothing >> +# for unicast ARPs towards VIF MACs. Instead, we rely on the default >> priority >> +# 1 "next;" action to move these ARPs through the pipeline, eventually >> resulting >> +# in the ARP reaching the destination. When proxy ARP is configured, we need >> +# to install explicit flows for the unicast ARPs at a higher priority. >> Otherwise >> +# the proxy ARP responder may respond with incorrect information. >> +# >> +# In this test, we are ensuring that the unicast flows in the ARP responder >> stage >> +# are only installed when proxy ARP is enabled. >> + >> +check ovn-nbctl ls-add ls1 >> +check ovn-nbctl lsp-add ls1 vm1 -- \ >> + lsp-set-addresses vm1 "00:00:00:00:00:02 192.168.0.2 fd01::2" >> + >> +check ovn-nbctl lr-add lr1 >> +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 192.168.0.1 fd01::1 >> +check ovn-nbctl lsp-add ls1 ls1-lr1 -- \ >> + lsp-set-addresses ls1-lr1 router -- \ >> + lsp-set-type ls1-lr1 router -- \ >> + lsp-set-options ls1-lr1 router-port=lr1-ls1 >> + >> +check ovn-nbctl --wait=sb sync >> + >> +# If we check the ARP responder flows in ls1, we should not see any unicast >> +# flows for vm1 (00:00:00:00:00:02). We also should not see any unicast >> +# IPv6 ND flows for vm1 (fd01::2) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::2" | ovn_strip_lflows], [0], [dnl >> +]) >> + >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::1" | ovn_strip_lflows], [0], [dnl >> +]) >> + >> +# Add ARP proxy configuration on the router port. >> +check ovn-nbctl set logical_switch_port ls1-lr1 >> options:arp_proxy="192.168.0.0/24 fd01::/64" >> +check ovn-nbctl --wait=sb sync >> + >> +# Now that we have ARP proxy configured, we should see flows for >> +# vm1's MAC. >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == >> 192.168.0.2 && arp.op == 1 && eth.dst == 00:00:00:00:00:02), action=(next;) >> +]) >> +# And we should see unicast ND flows for vm1's IP address >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::2" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst == >> fd01::2 && nd.target == fd01::2), action=(next;) >> +]) >> + >> +# We should also see an ARP flow for ls1-lr1's MAC. >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == >> 192.168.0.1 && arp.op == 1 && eth.dst == 00:00:00:00:00:01), action=(next;) >> +]) >> +# And we should see an ND flow for ls1-lr1's IPv6 address. >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::1" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst == >> fd01::1 && nd.target == fd01::1), action=(next;) >> +]) >> + >> +check ovn-nbctl remove logical_switch_port ls1-lr1 options arp_proxy >> +check ovn-nbctl --wait=sb sync >> + >> +# We have removed ARP proxy, so we should no longer see a unicast ARP or ND >> flow. >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::2" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::1" | ovn_strip_lflows], [0], [dnl >> +]) >> + >> +# Now we'll set up ARP proxy flows for networks that are different from our >> +# LSPs. This should result in no unicast ARP/ND flows being installed. >> +check ovn-nbctl set logical_switch_port ls1-lr1 >> options:arp_proxy="193.168.0.0/24 fd11::/64" >> +check ovn-nbctl --wait=sb sync >> + >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::2" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst == >> 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl >> +]) >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst == >> fd01::1" | ovn_strip_lflows], [0], [dnl >> +]) >> + >> +AT_CLEANUP >> +]) >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 3f340e7b3..0783376bc 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -44371,3 +44371,79 @@ check ovn-nbctl --wait=hv sync >> OVN_CLEANUP([hv1],[hv2],[hv3]) >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD([ >> +AT_SETUP([Unicast ARP when proxy ARP is configured]) >> +ovn_start >> + >> +check ovn-nbctl ls-add ls1 >> +check ovn-nbctl lsp-add ls1 vm1 -- \ >> + lsp-set-addresses vm1 "00:00:00:00:00:02 10.0.0.2 fd01::2" >> +check ovn-nbctl lsp-add ls1 vm2 -- \ >> + lsp-set-addresses vm2 "00:00:00:00:00:03 10.0.0.3 fd01::3" >> + >> +check ovn-nbctl lr-add lr1 >> +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 10.0.0.1 fd01::1 >> +check ovn-nbctl lsp-add ls1 ls1-lr1 -- \ >> + lsp-set-addresses ls1-lr1 router -- \ >> + lsp-set-type ls1-lr1 router -- \ >> + lsp-set-options ls1-lr1 router-port=lr1-ls1 >> + >> +check ovn-nbctl --wait=hv sync >> + >> +net_add n1 >> +sim_add hv1 >> +as hv1 >> +ovs-vsctl add-br br-phys >> +ovn_attach n1 br-phys 192.168.0.1 >> +ovs-vsctl add-port br-int vif1 -- set Interface vif1 >> external-ids:iface-id=vm1 \ >> + options:tx_pcap=hv1/vif1-tx.pcap \ >> + options:rxq_pcap=hv1/vif1-rx.pcap >> +ovs-vsctl add-port br-int vif2 -- set Interface vif2 >> external-ids:iface-id=vm2 \ >> + options:tx_pcap=hv1/vif2-tx.pcap \ >> + options:rxq_pcap=hv1/vif2-rx.pcap >> + >> +OVN_POPULATE_ARP >> + >> +wait_for_ports_up >> + >> +arp=$(fmt_pkt "Ether(dst='00:00:00:00:00:02', src='00:00:00:00:00:03')/ \ >> + ARP(hwsrc='00:00:00:00:00:03', hwdst='00:00:00:00:00:02', >> + psrc='10.0.0.3', pdst='10.0.0.2')") >> +nd_ns=$(fmt_pkt "Ether(dst='00:00:00:00:00:02', src='00:00:00:00:00:03')/ \ >> + IPv6(src='fd01::3', dst='fd01::2')/ \ >> + ICMPv6ND_NS(tgt='fd01::2')") >> + >> +# If we send a unicast ARP from vm2 towards vm1, the ARP should be forwarded >> +# to vm1 by ls1. >> +as hv1 ovs-appctl netdev-dummy/receive vif2 $arp >> +echo $arp > expected >> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) >> + >> +# And if we send a unicast ND_NS from vm2 towards vm1, the ND_NS should be >> +# forwarded to vm1 by ls1. >> +as hv1 ovs-appctl netdev-dummy/receive vif2 $nd_ns >> +echo $nd_ns >> expected >> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) >> + >> + >> +# Add ARP proxy configuration on the router port. The subnet for the ARP >> proxy >> +# addresses overlaps with the VIF addresses for vm1 and vm2. >> +check ovn-nbctl set logical_switch_port ls1-lr1 >> options:arp_proxy="10.0.0.0/8 fd01::/64" >> +check ovn-nbctl --wait=hv sync >> + >> +# Sending a unicast ARP from vm2 towards vm1 should still result in the ARP >> being >> +# forwarded to vm1 by ls1. >> +as hv1 ovs-appctl netdev-dummy/receive vif2 $arp >> +echo $arp >> expected >> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) >> + >> +# Sending a unicast ND_NS from vm2 towards vm1 should still result in the >> ND_NS being >> +# forwarded to vm1 by ls1. >> +as hv1 ovs-appctl netdev-dummy/receive vif2 $nd_ns >> +echo $nd_ns >> expected >> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) >> + >> +OVN_CLEANUP([hv1]) >> +AT_CLEANUP >> +]) >> -- >> 2.50.1 >> >> _______________________________________________ >> 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
