On Fri, Aug 8, 2025 at 10:50 PM Tiago Pires <[email protected]> wrote:
> On Fri, Aug 8, 2025 at 6:55 AM Ales Musil <[email protected]> wrote: > > > > > > > > On Fri, Aug 8, 2025 at 11:50 AM <[email protected]> wrote: > >> > >> On Fri, 2025-08-08 at 11:07 +0200, [email protected] 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 <[email protected]> > >> > > wrote: > >> > > > >> > > > > >> > > > > >> > > > On Tue, Aug 5, 2025 at 4:56 PM Tiago Pires via dev < > >> > > > [email protected]> 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 <[email protected]> > >> > > > > --- > >> > > > > > >> > > > > >> > > > 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, > Hi Tiago, > > 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. > This also about access to IPs that do not have dnat_and_snat defined for them. It should work for both, with the revert we will have SNAT on the reply which is wrong. > I can work on the test but could you confirm if the > flags.unsnat_not_tracked == 1 is fisable for the fix? > As mentioned previously it isn't, we need to find a different way that doesn't break the direct access use case. > > Regards, > > Tiago Pires > Thanks, Ales > > > > >> > >> > >> > >> 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 > >> > > > > [email protected] > >> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > > > >> > > > > > >> > > > Thanks, > >> > > > Ales > >> > > > > >> > > _______________________________________________ > >> > > dev mailing list > >> > > [email protected] > >> > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
