Hi Dumitru,
> Just double checking: was AI assistance used to create this patch?
Yes - Claude Opus 5 via Claude Code. My bad. v3 will carry:
Assisted-by: Claude Opus 5, Claude Code
> Hmm, why though? Shouldn't we make these work too? I think
> skip_snat_for_lb already works.
I missed this, and my commit message was wrong about router_ip.
- skip_snat_for_lb: confirmed. build_lrouter_flows_for_lb() adds the
lr_out_snat priority 120 "flags.skip_snat_for_lb == 1" flow for every
LR in lb_dps->nb_lr_map, no is_gw_router guard.
- lb_force_snat_ip=router_ip: not gateway router only.
build_lrouter_force_snat_flows_op() has no is_gw_router guard and is
called per-LRP from build_lrp_lflows_for_lbnats(), so those flows are
already emitted on distributed routers. What's missing there is
is_chassis_resident() scoping - they land on every chassis. I'll drop
the incorrect sentence from the commit message and ovn-nb.xml. Should
the scoping fix go in as a separate patch in the series?
- dnat_force_snat_ip: this one can't be fixed the same way.
flags.force_snat_for_dnat has a single producer,
build_lrouter_in_dnat_flow(), which sets it under od->is_gw_router
only. On a distributed router the flag is never produced, so adding
the consumer alone would be dead code - making the option work there
needs a producer too, which is a new feature rather than the missing
half of an existing one. Left out of v3 and noted in the commit
message. Separate patch?
Re-checking this also turned up a problem with the patch as posted: it
doesn't work on a distributed router that has any SNAT rule.
lrouter_nat_get_priority() adds 128 to SNAT priorities when the router
has distributed gateway ports, so a subnet SNAT on a /24 lands at 153
and a /32 at 161, while build_lrouter_force_snat_flows() hardcodes
priority 100 for the lr_out_snat consumer. The regular SNAT flow wins
and lb_force_snat_ip is still ignored - the packet gets SNATed, just
with the wrong address, so the martian drop is masked but the option is
not honored. Gateway routers have no +128 and IPv4 SNAT tops out at 33,
which is why 100 worked there. The unSNAT side is unaffected (110 vs
the distributed 100).
v3 shifts the consumer priority by the same 128 on distributed routers
so the ordering matches what a gateway router sees.
To keep the two sites in sync I pulled the literal 128 out of
lrouter_nat_get_priority() into a NAT_PRIORITY_DGP_OFFSET define and
used it in both. That makes the fix touch an existing function it
otherwise wouldn't - too much for a bug fix? If you'd rather keep the
two separate I'll just use the literal in the new code and leave
lrouter_nat_get_priority() alone.
One more on the shift: force SNAT now also ends up above the priority
160 stateless-LB lr_out_snat flow from
build_lrouter_flows_for_lb_stateless(), which applies to routers with
more than one distributed gateway port - on a gateway router force SNAT
loses to it. Preserve that ordering, or is winning correct here?
This is also what my ovn-northd.at test missed: no NAT rules configured,
so it passes while a realistic setup fails. Which leads to:
> But I think it would be safer if we also add a system-ovn.at test to
> validate that the functionality actually works.
Agreed. v3 adds one with a distributed gateway port, a load balancer
whose backend is reached through that port, and a competing subnet SNAT
entry, so the priority interaction is exercised end to end. It fails on
current main and passes with the fix.
> This is a bug fix, I don't really think we need the NEWS entry for it.
Dropped.
> I would also rephrase the /* Handle force SNAT options set in the
> gateway router. */ comment [...] "producer" and "consumer"?
Done, in those terms, including a note on the dnat asymmetry above.
v3 is rebased on current main (20b9f0b9a7).
Jaygue
On Tue, Aug 11, 2026 at 7:50 PM Dumitru Ceara <[email protected]> wrote:
> Hi Jaygue Lee,
>
> Thanks for the patch!
>
> On 7/31/26 8:33 AM, enginrect wrote:
> > From: Jaygue Lee <[email protected]>
> >
> > The lr_out_snat flows that consume flags.force_snat_for_lb are only
> > built for gateway routers (od->is_gw_router). However, the load
> > balancer DNAT flows in lr_in_dnat and the lr_out_undnat flows set
> > flags.force_snat_for_lb whenever the option is configured, including on
> > routers whose gateway is a distributed gateway port. The result is a
> > flag that is produced but never consumed: the option is silently
> > ignored and load balanced traffic leaves the router without the forced
> > SNAT.
> >
> > This breaks hairpin scenarios where a load balancer backend reachable
> > through the gateway port connects to its own VIP: the un-SNATed reply
> > arrives at the backend with identical source and destination addresses
> > and is dropped as a martian packet. The logical switch hairpin stages
> > do not cover this case when the backend resides on a switch with a
> > localnet port, where attaching the load balancer to that switch is not
> > viable.
> >
> > Build the force SNAT consumer flows for routers with distributed
> > gateway ports as well, one per gateway port, applying ct_snat on the
> > chassis where the gateway port is resident with an
> > is_chassis_resident() match. This is consistent with how dnat_and_snat
> > and subnet SNAT entries are already centralized on such routers. The
> > "router_ip" variant and dnat_force_snat_ip remain gateway router only.
>
> Hmm, why though? Shouldn't we make these work too? I think
> skip_snat_for_lb already works.
>
> >
>
> Just double checking: was AI assistance used to create this patch? If
> so, can you please add an "Assisted-by" tag as required by our
> documentation (please see submitting-patches.rst)? If not, I hope you
> don't mind me asking, it's just a hunch and I might be wrong.
>
> > Signed-off-by: Jaygue Lee <[email protected]>
> > ---
> > v2: No code changes. Resubmitted so the CI series branch is recreated
> > on current main and picks up the runner crun fix (de930bc3a) that
> > landed after v1; v1 CI failures were the crun container-startup error
> > and unrelated flaky tests in the "unstable" partition.
> >
> > NEWS | 4 +++
> > northd/northd.c | 43 +++++++++++++++++++++++++++---
> > ovn-nb.xml | 14 ++++++++++
> > tests/ovn-northd.at | 64 +++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 121 insertions(+), 4 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 84d7f60..6f5fee7 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,5 +1,9 @@
> > Post v26.03.0
> > -------------
> > + - northd: options:lb_force_snat_ip configured with a set of IP
> addresses
> > + is now honored on routers with distributed gateway ports.
> Previously
> > + the option was silently ignored on such routers, while the
> > + corresponding force-SNAT flag was still set on load balanced
> traffic.
>
> This is a bug fix, I don't really think we need the NEWS entry for it.
>
> > - OVN Interconnection now supports advertising Address_Sets to remote
> > availability zones. Set options:ic-adv=true on an NB Address_Set
> to
> > have ovn-ic publish it to the IC-SB database and import it into the
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 1d14527..ef2e1ad 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -14443,12 +14443,20 @@ build_lrouter_force_snat_flows(struct
> lflow_table *lflows,
> > const struct ovn_datapath *od,
> > const char *ip_version, const char
> *ip_addr,
> > const char *context,
> > + const struct ovn_port *l3dgw_port,
> > struct lflow_ref *lflow_ref)
> > {
> > struct ds match = DS_EMPTY_INITIALIZER;
> > struct ds actions = DS_EMPTY_INITIALIZER;
> > ds_put_format(&match, "ip%s && ip%s.dst == %s",
> > ip_version, ip_version, ip_addr);
> > + if (l3dgw_port) {
> > + /* Distributed router: only unSNAT on the chassis where the
> > + * gateway port is resident. */
> > + ds_put_format(&match, " && inport == %s && is_chassis_resident("
> > + "\"%s\")", l3dgw_port->json_key,
> > + l3dgw_port->cr_port->key);
> > + }
> > ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 110,
> > ds_cstr(&match), "ct_snat;", lflow_ref);
> >
> > @@ -14458,6 +14466,14 @@ build_lrouter_force_snat_flows(struct
> lflow_table *lflows,
> > ds_clear(&match);
> > ds_put_format(&match, "flags.force_snat_for_%s == 1 && ip%s",
> > context, ip_version);
> > + if (l3dgw_port) {
> > + /* Distributed router: force SNAT is applied on the chassis
> > + * where the gateway port is resident, consistent with how
> > + * regular SNAT entries are handled for such routers. */
> > + ds_put_format(&match, " && outport == %s &&
> is_chassis_resident("
> > + "\"%s\")", l3dgw_port->json_key,
> > + l3dgw_port->cr_port->key);
> > + }
> > ds_put_format(&actions, "ct_snat(%s);", ip_addr);
> > ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 100,
> > ds_cstr(&match), ds_cstr(&actions),
> > @@ -18836,24 +18852,43 @@ build_lrouter_nat_defrag_and_lb(
> > if (lrnat_rec->dnat_force_snat_addrs.n_ipv4_addrs) {
> > build_lrouter_force_snat_flows(lflows, od, "4",
> >
> lrnat_rec->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> > - "dnat", lflow_ref);
> > + "dnat", NULL, lflow_ref);
> > }
> > if (lrnat_rec->dnat_force_snat_addrs.n_ipv6_addrs) {
> > build_lrouter_force_snat_flows(lflows, od, "6",
> >
> lrnat_rec->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> > - "dnat", lflow_ref);
> > + "dnat", NULL, lflow_ref);
> > }
> > }
> > if (lb_force_snat_ip) {
> > if (lrnat_rec->lb_force_snat_addrs.n_ipv4_addrs) {
> > build_lrouter_force_snat_flows(lflows, od, "4",
> >
> lrnat_rec->lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb",
> > - lflow_ref);
> > + NULL, lflow_ref);
> > }
> > if (lrnat_rec->lb_force_snat_addrs.n_ipv6_addrs) {
> > build_lrouter_force_snat_flows(lflows, od, "6",
> >
> lrnat_rec->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb",
> > - lflow_ref);
> > + NULL, lflow_ref);
> > + }
> > + }
> > + } else if (lb_force_snat_ip) {
> > + /* Distributed routers with gateway ports: the load balancer
> DNAT
> > + * and the corresponding force_snat_for_lb flag are already
> applied
> > + * on the chassis where the gateway port is resident, so apply
> the
> > + * force SNAT flows there as well. Without these flows the
> flag is
> > + * set but never consumed and the option is silently ignored. */
>
> This comment doesn't seem to add much value. We could just skip it in
> my opinion.
>
> I would also rephrase the /* Handle force SNAT options set in the
> gateway router. */ comment before the other branch of the if statement
> above. We now handle it here too.
>
> > + struct ovn_port *dgp;
> > + VECTOR_FOR_EACH (&od->l3dgw_ports, dgp) {
> > + if (lrnat_rec->lb_force_snat_addrs.n_ipv4_addrs) {
> > + build_lrouter_force_snat_flows(lflows, od, "4",
> > +
> lrnat_rec->lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb",
> > + dgp, lflow_ref);
> > + }
> > + if (lrnat_rec->lb_force_snat_addrs.n_ipv6_addrs) {
> > + build_lrouter_force_snat_flows(lflows, od, "6",
> > +
> lrnat_rec->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb",
> > + dgp, lflow_ref);
> > }
> > }
> > }
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 68d4237..0b7b346 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -3377,6 +3377,20 @@ or
> > character.
> > </p>
> >
> > + <p>
> > + A set of IP addresses is also honored on distributed routers
> > + with one or more distributed gateway ports. In that case the
> > + SNAT is applied on the chassis where the gateway port is
> > + resident, consistent with how regular SNAT entries are handled
> > + for such routers. This is useful, for example, when a load
> > + balancer backend reachable through the gateway port connects
> to
> > + its own VIP: without the forced SNAT the un-SNATed reply would
> > + arrive at the backend with identical source and destination
> > + addresses and be discarded as a martian packet. The
> > + <code>router_ip</code> value is only supported on gateway
> > + routers.
> > + </p>
> > +
> > <p>
> > If it is configured with the value <code>router_ip</code>,
> then
> > the load balanced packet is SNATed with the IP of router port
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index c58f731..b5531c6 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -4971,6 +4971,70 @@ OVN_CLEANUP_NORTHD
> > AT_CLEANUP
> > ])
> >
> > +OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([
> > +AT_SETUP([Load Balancers and lb_force_snat_ip for routers with
> distributed gateway ports])
> > +ovn_start
> > +
> > +check ovn-nbctl ls-add sw0
> > +
> > +# Create a logical router with a distributed gateway port.
> > +check ovn-nbctl lr-add lr0
> > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> > +check ovn-nbctl lsp-add-router-port sw0 sw0-lr0 lr0-sw0
> > +
> > +check ovn-nbctl ls-add public
> > +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13
> 172.168.0.100/24
> > +check ovn-nbctl lsp-add-router-port public public-lr0 lr0-public
> > +check ovn-nbctl lrp-set-gateway-chassis lr0-public ch1
> > +
> > +check ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.4:8080
> > +check ovn-nbctl lr-lb-add lr0 lb1
> > +
> > +check ovn-nbctl --wait=sb sync
> > +
> > +ovn-sbctl dump-flows lr0 > lr0flows
> > +AT_CAPTURE_FILE([lr0flows])
> > +
> > +# Without lb_force_snat_ip there should be no force SNAT flows.
> > +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb |
> ovn_strip_lflows], [0], [dnl
> > +])
> > +
> > +check ovn-nbctl --wait=sb set logical_router lr0
> options:lb_force_snat_ip="172.168.0.4 aef0::4"
> > +
> > +ovn-sbctl dump-flows lr0 > lr0flows
> > +AT_CAPTURE_FILE([lr0flows])
> > +
> > +AT_CHECK([grep "lr_in_unsnat" lr0flows | ovn_strip_lflows], [0], [dnl
> > + table=??(lr_in_unsnat ), priority=0 , match=(1),
> action=(next;)
> > + table=??(lr_in_unsnat ), priority=110 , match=(ip4 && ip4.dst
> == 172.168.0.4 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> > + table=??(lr_in_unsnat ), priority=110 , match=(ip6 && ip6.dst
> == aef0::4 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> > +])
> > +
> > +AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl
> > + table=??(lr_out_snat ), priority=0 , match=(1),
> action=(next;)
> > + table=??(lr_out_snat ), priority=100 ,
> match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat(172.168.0.4);)
> > + table=??(lr_out_snat ), priority=100 ,
> match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat(aef0::4);)
> > + table=??(lr_out_snat ), priority=120 , match=(nd_ns),
> action=(next;)
> > +])
> > +
> > +# The producer and the consumer must both be present: the load balancer
>
> "producer" and "consumer"?
>
> > +# DNAT flows on the gateway chassis set flags.force_snat_for_lb, and the
> > +# flows above consume it.
> > +AT_CHECK([grep "lr_in_dnat" lr0flows | grep -q "force_snat"], [0], [])
> > +
> > +# Removing the option removes the flows.
> > +check ovn-nbctl --wait=sb remove logical_router lr0 options
> lb_force_snat_ip
> > +
> > +ovn-sbctl dump-flows lr0 > lr0flows
> > +AT_CAPTURE_FILE([lr0flows])
> > +
> > +AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb |
> ovn_strip_lflows], [0], [dnl
> > +])
> > +
> > +OVN_CLEANUP_NORTHD
> > +AT_CLEANUP
> > +])
>
> Thanks for adding unit tests. But I think it would be safer if we also
> add a system-ovn.at test to validate that the functionality actually
> works.
>
> Could you please add one?
>
> > +
> > OVN_FOR_EACH_NORTHD_NO_HV([
> > AT_SETUP([HA chassis group cleanup for external port ])
> > ovn_start
>
> Thanks,
> Dumitru
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
