Re: [ovs-dev] [PATCH ovn v3] northd: Set REGBIT_CONNTRACK_COMMIT earlier.

Mark Michelson via dev Tue, 06 May 2025 11:02:28 -0700

On 5/1/25 17:23, Dumitru Ceara wrote:

On 4/30/25 4:04 PM, Mark Michelson via dev wrote:

REGBIT_CONNTRACK_COMMIT determines if a packet will be committed to
conntrack when it reaches the STATEFUL stage of a logical switch. When
stateful ACLs are present, the goal is to have this bit set for all
traffic. However, if the packet hit only "pass" ACLs, then the packet
was being allowed but not being committed to conntrack.


This patch addresses the error by setting REGBIT_CONNTRACK_COMMIT during
the ACL_HINT stage. Any time we set REGBIT_ACL_HINT_ALLOW_NEW, we also
set REGBIT_CONNTRACK_COMMIT. If the packet gets denied by ACLs, then the
packet will get dropped or rejected before REGBIT_CONNTRACK_COMMIT is
used. If the packet is allowed (statelessly, statefully, or by default),
then the packet will be committed to conntrack.

Reported-at: https://issues.redhat.com/browse/FDP-1321

Signed-off-by: Mark Michelson <mmich...@redhat.com>
---



Hi Mark,

Thanks for the fix!  I have a few comments on this patch (two nits and a
bug in the test).  They can easily be fixed up when applying the patch.
If you do so, please add my ack:

Acked-by: Dumitru Ceara <dce...@redhat.com>

Thanks Dumitru. I fixed the issues you described below and pushed thechange to main and all branches back to 24.03.

  northd/northd.c     |  20 +++---
  tests/ovn-northd.at | 172 ++++++++++++++++++++++----------------------
  tests/system-ovn.at | 123 +++++++++++++++++++++++++++++++
  3 files changed, 220 insertions(+), 95 deletions(-)



[...]

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 6e71286ad..54274f8cb 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -17617,3 +17617,126 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port 
patch-.*/d
  /connection dropped.*/d"])
  AT_CLEANUP
  ])
+
+



Nit: no need for the second empty line.

+OVN_FOR_EACH_NORTHD([
+AT_SETUP([conntrack on pass ACLs])
+
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+#
+# Set external-ids in br-int needed for ovn-controller
+check ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . 
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+# Ensure that when stateful ACLs are present, a "pass"
+# action results in the packet being allowed (since we
+# do not have whatever that thing is called that
+# drops packets by default when using ACLs enabled). If


:) However I'd rephrase this to:

... since we do not have NB.NB_Global.default_acl_drop=true set ...

+# this is the final verdict of all ACL tiers, then the
+# packet should also be committed to conntrack, the same
+# as if an "allow" of "allow-related" verdict were final.
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls lsp1 \
+-- lsp-set-addresses lsp1 "f0:00:00:00:00:01 192.168.1.1"
+check ovn-nbctl lsp-add ls lsp2 \
+-- lsp-set-addresses lsp2 "f0:00:00:00:00:02 192.168.1.2"
+check ovn --wait=hv sync



I'm guessing you meant "ovn-nbctl --wait=hv sync" here.  That's also why
ovsrobot reported the system test failing in CI.

On the other hand we don't really need this sync call.  We call sync
again just below, before sending traffic.

With this line removed the CI passes in my fork:
https://github.com/dceara/ovn/actions/runs/14782845190

+
+ADD_NAMESPACES(lsp1)
+ADD_VETH(lsp1, lsp1, br-int, "192.168.1.1/24", "f0:00:00:00:00:01", \
+         "192.168.1.100")
+
+ADD_NAMESPACES(lsp2)
+ADD_VETH(lsp2, lsp2, br-int, "192.168.1.2/24", "f0:00:00:00:00:02", \
+         "192.168.1.100")
+
+# First, set up a "pass" ACL by itself.
+check ovn-nbctl acl-add ls from-lport 1000 "ip4.src == 192.168.1.1" pass
+check ovn-nbctl acl-add ls to-lport 1000 "ip4.src == 192.168.1.2" pass
+check ovn-nbctl --wait=hv sync
+
+# Ping should succeed since from-lport "pass" ACL is the only one matched.
+NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 192.168.1.2 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+# Ping the other way should also succeed since to-lport "pass" ACL is matched.
+NS_CHECK_EXEC([lsp2], [ping -q -c 3 -i 0.3 -w 2 192.168.1.1 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+# There should be no conntrack entries created since there are no stateful 
ACLs.
+# Check conntrack here
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.1.2) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [1], [dnl
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.1.1) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [1], [dnl
+])
+
+# Now add an arbitrary stateful ACL to the mix. We'll never match on this
+# ACL, but its presence should change things.
+check ovn-nbctl acl-add ls from-lport 200 "ip4.src == 192.168.1.50" 
allow-related
+check ovn-nbctl --wait=hv sync
+
+# Pings should still succeed.
+NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 192.168.1.2 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([lsp2], [ping -q -c 3 -i 0.3 -w 2 192.168.1.1 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+# Now there should be conntrack entries from the pings
+# We should have an entry for each direction of traffic in
+# each port's zone: a total of four.
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.1.2) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [dnl
+icmp,orig=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=8,code=0),reply=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=8,code=0),reply=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=0,code=0),zone=<cleared>
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.1.1) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [dnl
+icmp,orig=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=8,code=0),reply=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.2,dst=192.168.1.1,id=<cleared>,type=8,code=0),reply=(src=192.168.1.1,dst=192.168.1.2,id=<cleared>,type=0,code=0),zone=<cleared>
+])
+
+OVN_CLEANUP_CONTROLLER([hv1])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([ovn-northd])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+
+AT_CLEANUP
+])



Regards,
Dumitru


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v3] northd: Set REGBIT_CONNTRACK_COMMIT earlier.

Reply via email to