On Tue, Sep 30, 2025 at 6:16 PM Martin Kalcok <[email protected]>
wrote:
> Commit 40136a2f2c84 introduced the ability to directly reach networks
> behind
> SNAT and DNAT_AND_SNAT on Distributed Routers, matching the behavior
> of Gateway Routers. This was achieved by committing the incoming traffic
> into
> the SNAT TC zone, which allowed the router to SNAT only traffic that
> originated
> from inside the SNATed network.
> An unintended consequence was that incoming traffic destined for the
> external
> IP of a DNAT_AND_SNAT rule would also be committed to the SNAT CT zone,
> resulting in an unnecessary entry lingering in the table until it expired.
>
> This change attempts to fix that by handling traffic matching DNAT_AND_SNAT
> rule with slightly higher priority than the simple SNAT, and committing it
> to
> DNAT CT zone instead.
>
> Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT network.")
> Signed-off-by: Martin Kalcok <[email protected]>
> ---
> northd/northd.c | 48 +++++++++++++++++++++++++++++++++------------
> tests/ovn-northd.at | 27 ++++++++++++++++---------
> tests/system-ovn.at | 2 --
> 3 files changed, 53 insertions(+), 24 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 8b5413ef3..5ff149fd4 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -16611,27 +16611,49 @@ build_lrouter_out_snat_flow(struct lflow_table
> *lflows,
> * properly tracked so we can decide whether to perform SNAT on
> traffic
> * exiting the network. */
> if (features->ct_commit_to_zone && features->ct_next_zone &&
> - nat_entry->type == SNAT && !od->is_gw_router && !commit_all) {
> - /* For traffic that comes from SNAT network, initiate CT state
> before
> - * entering S_ROUTER_OUT_SNAT to allow matching on various CT
> states.
> - */
> - ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 70,
> - ds_cstr(match), "ct_next(snat);",
> + !od->is_gw_router && !commit_all) {
> + const char *zone;
> + uint16_t prio_offset;
> + if (nat_entry->type == SNAT) {
> + /* Traffic to/from hosts behind SNAT is tracked through the
> + * SNAT CT zone.*/
> + zone = "snat";
> + prio_offset = 0;
> + } else {
> + /* Traffic to/from hosts behind DNAT_AND_SNAT is tracked
> through
> + * the DNAT CT zone with slightly higher priority flows.*/
> + zone = "dnat";
> + prio_offset = 5;
> + }
> +
> + /* For traffic that comes from the SNAT network, initiate CT state
> + * from the correct zone, before entering S_ROUTER_OUT_SNAT to
> allow
> + * matching on various CT states.*/
> + ds_clear(actions);
> + ds_put_format(actions, "ct_next(%s);", zone);
> + ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 70 +
> prio_offset,
> + ds_cstr(match), ds_cstr(actions),
> lflow_ref);
>
> build_lrouter_out_snat_match(lflows, od, nat, match,
> distributed_nat, cidr_bits, is_v6,
> l3dgw_port, lflow_ref, true);
> -
> - /* New traffic that goes into SNAT network is committed to CT to
> avoid
> - * SNAT-ing replies.*/
> - ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, priority,
> - ds_cstr(match), "ct_snat;",
> + size_t match_any_state_len = match->length;
> + ds_put_cstr(match, " && (!ct.trk || !ct.rpl)");
> + ds_clear(actions);
> + ds_put_format(actions, "ct_%s;", zone);
> + ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, priority +
> prio_offset,
> + ds_cstr(match), ds_cstr(actions),
> lflow_ref);
>
> + /* New traffic that goes into the SNAT network is committed to the
> + * correct CT zone to avoid SNAT-ing replies.*/
> + ds_truncate(match, match_any_state_len);
> ds_put_cstr(match, " && ct.new");
> - ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, priority,
> - ds_cstr(match), "ct_commit_to_zone(snat);",
> + ds_clear(actions);
> + ds_put_format(actions, "ct_commit_to_zone(%s);", zone);
> + ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT,
> + priority + prio_offset, ds_cstr(match),
> ds_cstr(actions),
> lflow_ref);
> }
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index c9e998129..d99815dd6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1242,7 +1242,7 @@ AT_CAPTURE_FILE([crflows])
> AT_CHECK([grep -e "lr_out_snat" drflows | ovn_strip_lflows], [0], [dnl
> table=??(lr_out_snat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_snat ), priority=120 , match=(nd_ns),
> action=(next;)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.src == $allowed_range), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.src == $allowed_range && (!ct.trk || !ct.rpl)), action=(ct_snat;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.dst == $allowed_range && (!ct.trk || !ct.rpl)),
> action=(ct_snat(172.16.1.1);)
> ])
>
> @@ -1281,7 +1281,7 @@ AT_CAPTURE_FILE([crflows2])
> AT_CHECK([grep -e "lr_out_snat" drflows2 | ovn_strip_lflows], [0], [dnl
> table=??(lr_out_snat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_snat ), priority=120 , match=(nd_ns),
> action=(next;)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1")),
> action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.1);)
> table=??(lr_out_snat ), priority=163 , match=(ip && ip4.src ==
> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.dst == $disallowed_range), action=(next;)
> ])
> @@ -1321,10 +1321,12 @@ AT_CHECK([grep -e "lr_out_snat" drflows3 |
> ovn_strip_lflows], [0], [dnl
> table=??(lr_out_snat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_snat ), priority=120 , match=(nd_ns),
> action=(next;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.dst == $allowed_range && (!ct.trk || !ct.rpl)),
> action=(ct_snat(172.16.1.2);)
> + table=??(lr_out_snat ), priority=166 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.src == $allowed_range && (!ct.trk || !ct.rpl)), action=(ct_dnat;)
> ])
>
> AT_CHECK([grep -e "lr_out_post_snat" drflows3 | ovn_strip_lflows], [0],
> [dnl
> table=??(lr_out_post_snat ), priority=0 , match=(1), action=(next;)
> + table=??(lr_out_post_snat ), priority=166 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.src == $allowed_range && ct.new), action=(ct_commit_to_zone(dnat);)
> ])
>
> AT_CHECK([grep -e "lr_out_snat" crflows3 | ovn_strip_lflows], [0], [dnl
> @@ -1357,6 +1359,7 @@ AT_CHECK([grep -e "lr_out_snat" drflows4 |
> ovn_strip_lflows], [0], [dnl
> table=??(lr_out_snat ), priority=120 , match=(nd_ns),
> action=(next;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.2);)
> table=??(lr_out_snat ), priority=163 , match=(ip && ip4.src ==
> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> ip4.dst == $disallowed_range), action=(next;)
> + table=??(lr_out_snat ), priority=166 , match=(ip && ip4.dst ==
> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> (!ct.trk || !ct.rpl)), action=(ct_dnat;)
> ])
>
> AT_CHECK([grep -e "lr_out_snat" crflows4 | ovn_strip_lflows], [0], [dnl
> @@ -5999,22 +6002,25 @@ AT_CHECK([grep "lr_out_post_undnat" lr0flows |
> ovn_strip_lflows], [0], [dnl
> table=??(lr_out_post_undnat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src ==
> 10.0.0.0/24 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_next(snat);)
> table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src ==
> 10.0.0.10 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_next(snat);)
> + table=??(lr_out_post_undnat ), priority=75 , match=(ip && ip4.src ==
> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_next(dnat);)
> ])
>
> 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=120 , match=(nd_ns),
> action=(next;)
> - table=??(lr_out_snat ), priority=153 , match=(ip && ip4.dst ==
> 10.0.0.0/24 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=153 , match=(ip && ip4.dst ==
> 10.0.0.0/24 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_snat;)
> table=??(lr_out_snat ), priority=153 , match=(ip && ip4.src ==
> 10.0.0.0/24 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_snat(172.168.0.10);)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 10.0.0.10 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 10.0.0.10 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_snat;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 10.0.0.10 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_snat(172.168.0.30);)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
> + table=??(lr_out_snat ), priority=166 , match=(ip && ip4.dst ==
> 10.0.0.3 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_dnat;)
> ])
>
> AT_CHECK([grep "lr_out_post_snat" lr0flows | ovn_strip_lflows], [0], [dnl
> table=??(lr_out_post_snat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_post_snat ), priority=153 , match=(ip && ip4.dst ==
> 10.0.0.0/24 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && ct.new),
> action=(ct_commit_to_zone(snat);)
> table=??(lr_out_post_snat ), priority=161 , match=(ip && ip4.dst ==
> 10.0.0.10 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && ct.new), action=(ct_commit_to_zone(snat);)
> + table=??(lr_out_post_snat ), priority=166 , match=(ip && ip4.dst ==
> 10.0.0.3 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && ct.new), action=(ct_commit_to_zone(dnat);)
> ])
>
> # Associate load balancer to lr0
> @@ -6157,22 +6163,25 @@ AT_CHECK([grep "lr_out_post_undnat" lr0flows |
> ovn_strip_lflows], [0], [dnl
> table=??(lr_out_post_undnat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src ==
> 10.0.0.0/24 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_next(snat);)
> table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src ==
> 10.0.0.10 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_next(snat);)
> + table=??(lr_out_post_undnat ), priority=75 , match=(ip && ip4.src ==
> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_next(dnat);)
> ])
>
> 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=120 , match=(nd_ns),
> action=(next;)
> - table=??(lr_out_snat ), priority=153 , match=(ip && ip4.dst ==
> 10.0.0.0/24 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=153 , match=(ip && ip4.dst ==
> 10.0.0.0/24 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_snat;)
> table=??(lr_out_snat ), priority=153 , match=(ip && ip4.src ==
> 10.0.0.0/24 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_snat(172.168.0.10);)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 10.0.0.10 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 10.0.0.10 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_snat;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 10.0.0.10 && outport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
> action=(ct_snat(172.168.0.30);)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
> + table=??(lr_out_snat ), priority=166 , match=(ip && ip4.dst ==
> 10.0.0.3 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && (!ct.trk || !ct.rpl)), action=(ct_dnat;)
> ])
>
> AT_CHECK([grep "lr_out_post_snat" lr0flows | ovn_strip_lflows], [0], [dnl
> table=??(lr_out_post_snat ), priority=0 , match=(1), action=(next;)
> table=??(lr_out_post_snat ), priority=153 , match=(ip && ip4.dst ==
> 10.0.0.0/24 && inport == "lr0-public" &&
> is_chassis_resident("cr-lr0-public") && ct.new),
> action=(ct_commit_to_zone(snat);)
> table=??(lr_out_post_snat ), priority=161 , match=(ip && ip4.dst ==
> 10.0.0.10 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && ct.new), action=(ct_commit_to_zone(snat);)
> + table=??(lr_out_post_snat ), priority=166 , match=(ip && ip4.dst ==
> 10.0.0.3 && inport == "lr0-public" && is_chassis_resident("cr-lr0-public")
> && ct.new), action=(ct_commit_to_zone(dnat);)
> ])
>
> # Make the logical router as Gateway router
> @@ -8389,9 +8398,9 @@ AT_CHECK([grep lr_in_unsnat lrflows | grep ct_snat |
> ovn_strip_lflows], [0], [dn
> ])
>
> AT_CHECK([grep lr_out_snat lrflows | grep ct_snat | ovn_strip_lflows],
> [0], [dnl
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 20.0.0.10 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1")),
> action=(ct_snat;)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 20.0.0.10 && inport == "DR-S2" && is_chassis_resident("cr-DR-S2")),
> action=(ct_snat;)
> - table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 20.0.0.10 && inport == "DR-S3" && is_chassis_resident("cr-DR-S3")),
> action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 20.0.0.10 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 20.0.0.10 && inport == "DR-S2" && is_chassis_resident("cr-DR-S2") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat;)
> + table=??(lr_out_snat ), priority=161 , match=(ip && ip4.dst ==
> 20.0.0.10 && inport == "DR-S3" && is_chassis_resident("cr-DR-S3") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat;)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 20.0.0.10 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.10);)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 20.0.0.10 && outport == "DR-S2" && is_chassis_resident("cr-DR-S2") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat(10.0.0.10);)
> table=??(lr_out_snat ), priority=161 , match=(ip && ip4.src ==
> 20.0.0.10 && outport == "DR-S3" && is_chassis_resident("cr-DR-S3") &&
> (!ct.trk || !ct.rpl)), action=(ct_snat(192.168.0.10);)
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index f84290594..59f96e596 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -4176,7 +4176,6 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2
> 172.16.1.4 | FORMAT_PING], \
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep icmp |
> FORMAT_CT(172.16.1.1) | \
> sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
>
> icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>
> -icmp,orig=(src=172.16.1.1,dst=192.168.2.2,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>
>
> icmp,orig=(src=192.168.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
> ])
>
> @@ -4344,7 +4343,6 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2
> fd20::4 | FORMAT_PING], \
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd20::1) | \
> sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
>
> icmpv6,orig=(src=fd11::3,dst=fd20::4,id=<cleared>,type=128,code=0),reply=(src=fd20::4,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
>
> -icmpv6,orig=(src=fd20::1,dst=fd12::2,id=<cleared>,type=128,code=0),reply=(src=fd12::2,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
>
>
> icmpv6,orig=(src=fd20::1,dst=fd20::4,id=<cleared>,type=128,code=0),reply=(src=fd12::2,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
> ])
>
> --
> 2.48.1
>
>
Thank you Martin,
I went ahead and merged this into main and backported down to 24.09.
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
