On Fri, Aug 8, 2025 at 6:55 AM Ales Musil <amu...@redhat.com> wrote:
>
>
>
> On Fri, Aug 8, 2025 at 11:50 AM <martin.kal...@canonical.com> wrote:
>>
>> On Fri, 2025-08-08 at 11:07 +0200, martin.kal...@canonical.com wrote:
>> > On Fri, 2025-08-08 at 10:46 +0200, Ales Musil via dev wrote:
>> > > On Fri, Aug 8, 2025 at 9:03 AM Ales Musil <amu...@redhat.com>
>> > > wrote:
>> > >
>> > > >
>> > > >
>> > > > On Tue, Aug 5, 2025 at 4:56 PM Tiago Pires via dev <
>> > > > ovs-dev@openvswitch.org> wrote:
>> > > >
>> > > > > This patch fix the behavior introduced by the commit
>> > > > > 40136a2f2c84,
>> >
>> > Hi Tiago and Ales,
>> > as a perpetrator of 40136a2f2c84, I'll try to help out with getting
>> > this sorted.
>> >
>> > > > > where a regular communication between an external IP to a
>> > > > > dnat_and_snat rule IP would always keep an unreplied
>> > > > > conntrack entry:
>> > > > >
>> > > > > nat d8b589d8-7948-4470-a3ee-a8ae7edb6425
>> > > > > external ip: "172.16.1.101"
>> > > > > logical ip: "192.168.10.10"
>> > > > > type: "dnat_and_snat"
>> > > > >
>> > > > > The conntrack entry would be like below:
>> > > > >
>> > > > > tcp 6 118 SYN_SENT src=172.16.1.50 dst=192.168.10.10 \
>> > > > > sport=44742 dport=80 [UNREPLIED] src=192.168.10.10 \
>> > > > > dst=172.16.1.50 sport=80 dport=44742 mark=0 zone=13 use=1
>> > > > >
>> > > > > When we have many connections on the chassis gateways, more
>> > > > > than 50% of the conntrack entries stay in this UNREPLIED state
>> > > > > until the entry is expired according with the
>> > > > > nf_conntrack_tcp_timeout_syn_sent setting.
>> > > > >
>> > > > > Running the ab(apache benchmarking tool) test with 3000
>> > > > > requests
>> > > > > to a dnat_and_snat IP, without this patch we would have 2997
>> > > > > SYN_SENT unreplied entries and with this patch applied
>> > > > > we would have 0 entries.
>> > > > >
>> > > > > Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT
>> > > > > network.")
>> > > > > Signed-off-by: Tiago Pires <tiago.pi...@luizalabs.com>
>> > > > > ---
>> > > > >
>> > > >
>> > > > Hi Tiago,
>> > > >
>> > > > I'm afraid this acts like a revert, the "unsnat_not_tracked"
>> > > > cannot
>> > > > be 1 if ct_commit_all is disabled. But at the same time this flow
>> > > > won't be created if ct_commit_all is enabled. Which basically
>> > > > boils
>> > > > down to the fact that the flow can never be matched.
>> > > >
>> > > > This made me wonder what would happen if we would remove the
>> > > > whole
>> > > > if statement, which would be a full revert of the original
>> > > > commit.
>> > > > The system tests are still passing and there isn't any issue
>> > > > with the unreplied CT. I didn't dig deeper but I have a suspicion
>> > > > that some additional change in between allowed this behavior
>> > > > without
>> > > > the need for the extra SNAT commit. It would be nice if we can
>> > > > track
>> > > > down when the direct access breaks if we revert 40136a2f.
>> >
>> > I can try bisecting the history to find what "fixed" the issue as a
>> > side-effect.
>> >
>> > > >
>> > > > With that said I'm fine with reverting 40136a2f as long as we
>> > > > don't
>> > > > break any scenario that might not be tested currently and we
>> > > > figure
>> > > > out what allows the direct access even after the revert.
>> > > >
>> > >
>> > >
>> > > Unfortunately the revert doesn't work, tests are fine, but
>> > > the reply is SNATed which shouldn't be the case. So we
>> > > probably need a solution that involves excluding dnat_and_snat
>> > > ips from being commited during the SNAT stage.
>> >
>> > Wouldn't this break the "DNAT and SNAT on distributed router" test
>> > that
>> > tries accessing the internal IP from external network?
>>
>> Ah, I see. It was one of the tricky things about the original issue
>> (before 40136a2f2c84). The first packet in reply direction would go
>> unchanged, so the SYN-ACK would pass and session would be established.
>> However any other traffic in reply direction would get SNATed as you
>> say Ales.
>>
>> Looking at the "DNAT and SNAT on distributed router", we should
>> probably improve it as well. We are testing:
>> * UDP with "nc -u -z" which tests traffic only in direction from client
>> to server and not the replies.
>> * TCP with "nc --send-only" which apparently does not wait for ACK
>> after PSH and closes connection immediately.
>
>
> Right, that's why the test doesn't fail with revert or partial revert.
> So whatever the fix will be we need to make sure to adjust the
> test to see if the reply isn't SNATed.
Hi Ales and Martin,
I was only able to make the scenario of revert working when using
flags.unsnat_not_tracked == 1 .
If the remote external network also wants to communicate with the
dnat_and_snat private IP, it is covered and works.
I can work on the test but could you confirm if the
flags.unsnat_not_tracked == 1 is fisable for the fix?
Regards,
Tiago Pires
>
>>
>>
>>
>> Martin.
>
>
> Thanks,
> Ales
>
>>
>>
>> >
>> > Anyway, I'll try to take a closer look as well.
>> > Martin.
>> >
>> > >
>> > >
>> > > >
>> > > >
>> > > > northd/northd.c | 2 +-
>> > > > > tests/ovn-northd.at | 18 +++++++++---------
>> > > > > tests/system-ovn.at | 2 --
>> > > > > 3 files changed, 10 insertions(+), 12 deletions(-)
>> > > > >
>> > > > > diff --git a/northd/northd.c b/northd/northd.c
>> > > > > index d027d5c66..ad9fabc8e 100644
>> > > > > --- a/northd/northd.c
>> > > > > +++ b/northd/northd.c
>> > > > > @@ -16521,7 +16521,7 @@ build_lrouter_out_snat_flow(struct
>> > > > > lflow_table
>> > > > > *lflows,
>> > > > > ds_cstr(match), "ct_snat;",
>> > > > > lflow_ref);
>> > > > >
>> > > > > - ds_put_cstr(match, " && ct.new");
>> > > > > + ds_put_cstr(match, " && ct.new &&
>> > > > > flags.unsnat_not_tracked ==
>> > > > > 1");
>> > > > > ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT,
>> > > > > priority,
>> > > > > ds_cstr(match),
>> > > > > "ct_commit_to_zone(snat);",
>> > > > > lflow_ref);
>> > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> > > > > index 5ddb15587..a0d6cfc66 100644
>> > > > > --- a/tests/ovn-northd.at
>> > > > > +++ b/tests/ovn-northd.at
>> > > > > @@ -1248,7 +1248,7 @@ AT_CHECK([grep -e "lr_out_snat" drflows |
>> > > > > ovn_strip_lflows], [0], [dnl
>> > > > >
>> > > > > AT_CHECK([grep -e "lr_out_post_snat" drflows |
>> > > > > ovn_strip_lflows], [0],
>> > > > > [dnl
>> > > > > table=??(lr_out_post_snat ), priority=0 , match=(1),
>> > > > > action=(next;)
>> > > > > - table=??(lr_out_post_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.new),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > + table=??(lr_out_post_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.new && flags.unsnat_not_tracked
>> > > > > == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > ])
>> > > > >
>> > > > > AT_CHECK([grep -e "lr_out_snat" crflows | ovn_strip_lflows],
>> > > > > [0], [dnl
>> > > > > @@ -1288,7 +1288,7 @@ AT_CHECK([grep -e "lr_out_snat" drflows2
>> > > > > |
>> > > > > ovn_strip_lflows], [0], [dnl
>> > > > >
>> > > > > AT_CHECK([grep -e "lr_out_post_snat" drflows2 |
>> > > > > ovn_strip_lflows], [0],
>> > > > > [dnl
>> > > > > table=??(lr_out_post_snat ), priority=0 , match=(1),
>> > > > > action=(next;)
>> > > > > - table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-
>> > > > > S1")
>> > > > > &&
>> > > > > ct.new), action=(ct_commit_to_zone(snat);)
>> > > > > + table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-
>> > > > > S1")
>> > > > > &&
>> > > > > ct.new && flags.unsnat_not_tracked == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > ])
>> > > > >
>> > > > > AT_CHECK([grep -e "lr_out_snat" crflows2 | ovn_strip_lflows],
>> > > > > [0], [dnl
>> > > > > @@ -6013,8 +6013,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows |
>> > > > > ovn_strip_lflows], [0], [dnl
>> > > > >
>> > > > > 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=153 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 10.0.0.0/24 && inport == "lr0-public" &&
>> > > > > is_chassis_resident("cr-lr0-public") && ct.new &&
>> > > > > flags.unsnat_not_tracked == 1),
>> > > > > 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 && flags.unsnat_not_tracked == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > ])
>> > > > >
>> > > > > # Associate load balancer to lr0
>> > > > > @@ -6171,8 +6171,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows |
>> > > > > ovn_strip_lflows], [0], [dnl
>> > > > >
>> > > > > 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=153 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 10.0.0.0/24 && inport == "lr0-public" &&
>> > > > > is_chassis_resident("cr-lr0-public") && ct.new &&
>> > > > > flags.unsnat_not_tracked == 1),
>> > > > > 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 && flags.unsnat_not_tracked == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > ])
>> > > > >
>> > > > > # Make the logical router as Gateway router
>> > > > > @@ -8399,9 +8399,9 @@ AT_CHECK([grep lr_out_snat lrflows | grep
>> > > > > ct_snat |
>> > > > > ovn_strip_lflows], [0], [dnl
>> > > > >
>> > > > > AT_CHECK([grep lr_out_post_snat lrflows | ovn_strip_lflows],
>> > > > > [0], [dnl
>> > > > > table=??(lr_out_post_snat ), priority=0 , match=(1),
>> > > > > action=(next;)
>> > > > > - table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 20.0.0.10 && inport == "DR-S1" && is_chassis_resident("cr-DR-
>> > > > > S1")
>> > > > > &&
>> > > > > ct.new), action=(ct_commit_to_zone(snat);)
>> > > > > - table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 20.0.0.10 && inport == "DR-S2" && is_chassis_resident("cr-DR-
>> > > > > S2")
>> > > > > &&
>> > > > > ct.new), action=(ct_commit_to_zone(snat);)
>> > > > > - table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 20.0.0.10 && inport == "DR-S3" && is_chassis_resident("cr-DR-
>> > > > > S3")
>> > > > > &&
>> > > > > ct.new), action=(ct_commit_to_zone(snat);)
>> > > > > + table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 20.0.0.10 && inport == "DR-S1" && is_chassis_resident("cr-DR-
>> > > > > S1")
>> > > > > &&
>> > > > > ct.new && flags.unsnat_not_tracked == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > + table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 20.0.0.10 && inport == "DR-S2" && is_chassis_resident("cr-DR-
>> > > > > S2")
>> > > > > &&
>> > > > > ct.new && flags.unsnat_not_tracked == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > + table=??(lr_out_post_snat ), priority=161 , match=(ip &&
>> > > > > ip4.dst ==
>> > > > > 20.0.0.10 && inport == "DR-S3" && is_chassis_resident("cr-DR-
>> > > > > S3")
>> > > > > &&
>> > > > > ct.new && flags.unsnat_not_tracked == 1),
>> > > > > action=(ct_commit_to_zone(snat);)
>> > > > > ])
>> > > > >
>> > > > > check ovn-nbctl --wait=sb lr-nat-del DR snat 20.0.0.10
>> > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> > > > > index e0407383a..a000637d2 100644
>> > > > > --- a/tests/system-ovn.at
>> > > > > +++ b/tests/system-ovn.at
>> > > > > @@ -4244,7 +4244,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,c
>> > > > > od
>> > > > > e=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,
>> > > > > co
>> > > > > de=0),zone=<cleared>
>> > > > >
>> > > > > -
>> > > > > icmp,orig=(src=172.16.1.1,dst=192.168.2.2,id=<cleared>,type=8,c
>> > > > > ode
>> > > > > =0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,c
>> > > > > od
>> > > > > e=0),zone=<cleared>
>> > > > >
>> > > > > icmp,orig=(src=192.168.1.3,dst=172.16.1.4,id=<cleared>,type=8,
>> > > > > co
>> > > > > de=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=<cleared>,type=0,
>> > > > > co
>> > > > > de=0),zone=<cleared>
>> > > > > ])
>> > > > >
>> > > > > @@ -4412,7 +4411,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,cod
>> > > > > e=
>> > > > > 0),reply=(src=fd20::4,dst=fd20::1,id=<cleared>,type=129,code=0)
>> > > > > ,z
>> > > > > one=<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),z
>> > > > > on
>> > > > > e=<cleared>
>> > > > >
>> > > > > icmpv6,orig=(src=fd20::1,dst=fd20::4,id=<cleared>,type=128,cod
>> > > > > e=
>> > > > > 0),reply=(src=fd12::2,dst=fd20::1,id=<cleared>,type=129,code=0)
>> > > > > ,z
>> > > > > one=<cleared>
>> > > > > ])
>> > > > >
>> > > > > --
>> > > > > 2.43.0
>> > > > >
>> > > > >
>> > > > > --
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > _'Esta mensagem é direcionada apenas para os endereços
>> > > > > constantes
>> > > > > no
>> > > > > cabeçalho inicial. Se você não está listado nos endereços
>> > > > > constantes no
>> > > > > cabeçalho, pedimos-lhe que desconsidere completamente o
>> > > > > conteúdo
>> > > > > dessa
>> > > > > mensagem e cuja cópia, encaminhamento e/ou execução das ações
>> > > > > citadas
>> > > > > estão
>> > > > > imediatamente anuladas e proibidas'._
>> > > > >
>> > > > >
>> > > > > * **'Apesar do Magazine Luiza tomar
>> > > > > todas as precauções razoáveis para assegurar que nenhum vírus
>> > > > > esteja
>> > > > > presente nesse e-mail, a empresa não poderá aceitar a
>> > > > > responsabilidade
>> > > > > por
>> > > > > quaisquer perdas ou danos causados por esse e-mail ou por seus
>> > > > > anexos'.*
>> > > > >
>> > > > >
>> > > > >
>> > > > > _______________________________________________
>> > > > > dev mailing list
>> > > > > d...@openvswitch.org
>> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> > > > >
>> > > > >
>> > > > Thanks,
>> > > > Ales
>> > > >
>> > > _______________________________________________
>> > > dev mailing list
>> > > d...@openvswitch.org
>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >
>>
--
_‘Esta mensagem é direcionada apenas para os endereços constantes no
cabeçalho inicial. Se você não está listado nos endereços constantes no
cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa
mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão
imediatamente anuladas e proibidas’._
* **‘Apesar do Magazine Luiza tomar
todas as precauções razoáveis para assegurar que nenhum vírus esteja
presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por
quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.*
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
