Hi Mark Thanks for the patch and the nice description! Don't we have the same issue with ipv6 - despite its name (proxy_*arp)* the option seems to be supported by ipv6 as well? Also a few nit and one question below.
Thanks Xavier On Wed, Oct 8, 2025 at 4:49 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 conntected nit: s/conntected/connected > to a > logical router, then we install low priority flows to ensure that > ARPs for the configured proxy addresses is 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]> > --- > northd/northd.c | 30 ++++++++++++++++----- > tests/ovn-northd.at | 65 +++++++++++++++++++++++++++++++++++++++++++++ > tests/ovn.at | 64 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 153 insertions(+), 6 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 4a3463a8e..993073e68 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -9940,15 +9940,33 @@ 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", > + "arp.op == 1", > op->lsp_addrs[i].ipv4_addrs[j].addr_s); > + > + /* Do not reply on unicast ARPs, forward them to the > target > + * to have ability to monitor target liveness via unicast > + * ARP requests. If proxy arp is configured, then we need > + * to set up flows to forward the packets. Otherwise, we > + * could end up replying with the proxy ARP erroneously. > + * Without proxy arp configured, these flows are > + * unnecessary since the packets will hit the default > + * "next" flow at priority 0. > + */ > + if (op->od->has_arp_proxy_port) { > + size_t match_len = match->length; > + ds_put_format(match, " && eth.dst == %s", > + op->lsp_addrs[i].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); > + ds_truncate(match, match_len); > + } > I am wondering: in theory, I think that we could check if the port ip address is part of the arp_proxy_port addresses, and only add flows in that case. e.g. if arp_proxy="192.168.0.3" and vm1="192.168.0.1", then we do not need the unicast additional flow for vm1, as we will not have the priority 30 flows erroneously answering. This would reduce the number of additional flows in some cases, but would make northd more complex. I am not sure however that the scenario I listed is realistic and that the usual configuration is not more something like arp_proxy="192.168.0.0/24" and vm1="192.168.0.1", in which case we would make northd more complex and not reduce number of flows. But your northd test uses arp_proxy="10.0.0.1 10.0.0.2" and vm1 in 192.168.1.0/24. WDYT? > + ds_put_cstr(match, " && eth.dst == ff:ff:ff:ff:ff:ff"); > + > ds_clear(actions); > ds_put_format(actions, > "eth.dst = eth.src; " > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index b1aee0008..6c5a4203a 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -18716,3 +18716,68 @@ 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" > + > +check ovn-nbctl lr-add lr1 > +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 192.168.0.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). > + > +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 "eth.dst > == 00:00:00:00:00:01" | 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="10.0.0.1 10.0.0.2" > nit: Is it on purpose that you use a completely different ip range i.e. 10.0.0.0/24 while ls is on 192.168.0.0/24 ? It does not impact the test. > +check ovn-nbctl --wait=sb sync > + > +ovn-sbctl lflow-list lr1 > nit: not needed > + > +# Now that we have ARP proxy configured, we should see a flow 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;) > +]) > + > +# We should also see a 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;) > +]) > + > +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 > 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 "eth.dst > == 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl > +]) > + > +AT_CLEANUP > +]) > diff --git a/tests/ovn.at b/tests/ovn.at > index 16270151f..18b745033 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -44436,3 +44436,67 @@ 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" > +check ovn-nbctl lsp-add ls1 vm2 -- \ > + lsp-set-addresses vm2 "00:00:00:00:00:03 10.0.0.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 > +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 \ > + ofport-request=1 > nit: you can skip the ofport-request. > +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 \ > + ofport-request=2 > nit: you can skip the ofport-request. > + > +OVN_POPULATE_ARP > + > +wait_for_ports_up > + > +# If we send a unicast ARP from vm2 towards vm1, the ARP should be > forwarded > +# to vm1 by ls1. > +packet=$(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')") > + > +as hv1 ovs-appctl netdev-dummy/receive vif2 $packet > + > +echo $packet > 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" > +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 $packet > +echo $packet >> 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
