Hi Chris, thanks for the submission. Regarding the actual northd.c changes, I think the patch is correct. I have some notes about the added test, though. See below.
On Wed, Jul 15, 2026 at 11:35 AM Chris Bannister via dev <[email protected]> wrote: > > The switch-side NAT MAC lookup code iterated all DNAT-and-SNAT > entries for every distributed gateway switch port. On routers with > multiple gateway ports this programmed the same NAT MAC on unrelated > logical switches and could steer traffic to the wrong outport. > > Filter NAT MAC lookup flows by the current router peer's > gateway_port before emitting the priority-50 ls_in_l2_lkup flow. > Add an end-to-end test that binds two VMs, configures one NAT per > gateway port, and checks only the priority-50 NAT MAC lookup > entries so the test fails without the fix and passes with it. > > Signed-off-by: Chris Bannister <[email protected]> > --- > northd/northd.c | 4 ++- > tests/ovn.at | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 484c579e9..56660e671 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -11200,7 +11200,9 @@ build_lswitch_ip_unicast_lookup_for_nats( > const struct ovn_nat *nat = > &lr_stateful_rec->lrnat_rec->nat_entries[i]; > > - if (nat->type == DNAT_AND_SNAT && nat->nb->logical_port > + if (nat->type == DNAT_AND_SNAT > + && is_nat_gateway_port(nat->nb, op->peer) > + && nat->nb->logical_port > && nat->nb->external_mac > && eth_addr_from_string(nat->nb->external_mac, &mac)) { > > diff --git a/tests/ovn.at b/tests/ovn.at > index efa53964b..b4bbacefe 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at Since this test is only ensuring that ovn-northd is setting values in the SB DB correctly, I think this test belongs in ovn-northd.at instead of ovn.at. > @@ -24475,6 +24475,87 @@ OVN_CLEANUP([hv1], [hv2], [hv3]) > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD([ Since hypervisors are not needed for this test, you can change this to use OVN_FOR_EACH_NORTHD_NO_HV > +AT_SETUP([distributed NAT gateway port scoping in ls_in_l2_lkup]) > +AT_KEYWORDS([nat-gateway-port-l2-lkup]) > +AT_KEYWORDS([gateway_port]) > +ovn_start > + Everything from here... > +net_add n1 > + > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw1-vm \ > + ofport-request=1 > + > +sim_add hv2 > +as hv2 > +check ovs-vsctl add-br br-phys > +check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys > +ovn_attach n1 br-phys 192.168.0.2 > +check ovs-vsctl -- add-port br-int hv2-vif1 -- \ > + set interface hv2-vif1 external-ids:iface-id=sw2-vm \ > + ofport-request=1 ... to here should be removed from the test. The test only cares about the southbound logical flows, and does not need hypervisors to operate. > + > +check ovn-nbctl ls-add sw1 > +check ovn-nbctl lsp-add sw1 sw1-vm > +check ovn-nbctl lsp-set-addresses sw1-vm "00:00:00:00:01:10 10.0.1.10" > + > +check ovn-nbctl ls-add sw2 > +check ovn-nbctl lsp-add sw2 sw2-vm > +check ovn-nbctl lsp-set-addresses sw2-vm "00:00:00:00:02:20 10.0.2.20" > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:aa:01:01 10.0.1.1/24 > +check ovn-nbctl lsp-add-router-port sw1 sw1-lr0 lr0-sw1 > +check ovn-nbctl lrp-add lr0 lr0-sw2 00:00:00:aa:02:02 10.0.2.1/24 > +check ovn-nbctl lsp-add-router-port sw2 sw2-lr0 lr0-sw2 > + > +check ovn-nbctl lrp-set-gateway-chassis lr0-sw1 hv1 10 > +check ovn-nbctl lrp-set-gateway-chassis lr0-sw2 hv2 10 > + > +check ovn-nbctl --gateway-port=lr0-sw1 \ > + lr-nat-add lr0 dnat_and_snat 192.0.2.11 10.0.1.10 sw1-vm > 00:00:00:01:00:01 > +check ovn-nbctl --gateway-port=lr0-sw2 \ > + lr-nat-add lr0 dnat_and_snat 192.0.2.22 10.0.2.20 sw2-vm > 00:00:00:02:00:02 > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +ovn-sbctl dump-flows sw1 > sw1flows > +ovn-sbctl dump-flows sw2 > sw2flows > +AT_CAPTURE_FILE([sw1flows]) > +AT_CAPTURE_FILE([sw2flows]) > + > +# NAT MAC lookup flows should be present only on the logical switch whose > +# router port corresponds to the NAT's gateway_port. > +AT_CHECK([grep ls_in_l2_lkup sw1flows | \ > + grep 'priority=50' | \ > + grep -F 'eth.dst == 00:00:00:01:00:01 && > is_chassis_resident("sw1-vm")' | \ > + grep -F 'outport = "sw1-lr0"; output;' | wc -l], [0], [1 > +]) > +AT_CHECK([grep ls_in_l2_lkup sw1flows | \ > + grep 'priority=50' | \ > + grep -F '00:00:00:02:00:02' | wc -l], [0], [0 > +]) > +AT_CHECK([grep ls_in_l2_lkup sw2flows | \ > + grep 'priority=50' | \ > + grep -F 'eth.dst == 00:00:00:02:00:02 && > is_chassis_resident("sw2-vm")' | \ > + grep -F 'outport = "sw2-lr0"; output;' | wc -l], [0], [1 > +]) > +AT_CHECK([grep ls_in_l2_lkup sw2flows | \ > + grep 'priority=50' | \ > + grep -F '00:00:00:01:00:01' | wc -l], [0], [0 > +]) > + > +OVN_CLEANUP([hv1], [hv2]) Since you are removing the hypervisors, you can remove this OVN_CLEANUP line. In its place you should add a call to OVN_CLEANUP_NORTHD here instead. > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([IGMP relay - distributed gateway port]) > AT_KEYWORDS([IP-multicast snoop relay]) > -- > 2.54.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
