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> > 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
