Thanks for the review Xavier. On Fri, Oct 10, 2025 at 9:49 AM Xavier Simonart <[email protected]> wrote: > > 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.
Yes this does appear to be correct. I'll address IPv6 in the next version. > > 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? I thought about this as well, since the only time this is a problem is if there is overlap in the proxy ARP IP range and the LSP IP address. My thought on the matter was that the potential extra flows would be less cumbersome than the computation in northd required to determine the necessary flows. I'll explore this some and potentially address this in v2. With regards to the ovn-northd test, that's actually a mistake :) . I had meant to have the addresses overlap. I originally had used 10.0.0.0/8 as the range of addresses for the network, but then ended up changing to 192.168.0.0/24 later. I forgot to update the proxy ARP addresses in the meantime. The test still passes because we don't check for overlap in the proxy ARP and LSP IP addresses. I'll change this in v2 to make the addresses overlap, and I'll make it more realistic, too. >> >> + 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. No, as I mentioned above, this is a mistake, actually. It happens to pass because there is no check for overlap in the LSP IP address and ARP proxy IP range. >> >> +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
