Aaron Conole <acon...@redhat.com> writes: > Paolo Valerio <pvale...@redhat.com> writes: > >> Paolo Valerio <pvale...@redhat.com> writes: >> >>> Simon Horman <ho...@ovn.org> writes: >>> >>>> On Wed, Aug 28, 2024 at 07:14:06PM +0200, Paolo Valerio wrote: >>>>> As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT >>>>> result in the unexpected failure of the following tests: >>>>> >>>>> conntrack - multiple zones, local >>>>> conntrack - multi-stage pipeline, local >>>>> conntrack - can match and clear ct_state from outside OVS >>>>> >>>>> this happens because the nf_conncount turns on connection tracking and >>>>> the above tests rely on this side effect. However, this behavior may >>>>> be corrected in the kernel, which could, in turn, cause the tests to >>>>> fail. >>>>> >>>>> The patch removes the assumption by adding explicit iptables rules to >>>>> attach an nf_conn template to the skb resulting tracked once hit the >>>>> OvS pipeline. >>>>> >>>>> While at it, introduce $HAVE_IPTABLES and skip tests if iptables >>>>> binary is not present. >>>>> >>>>> Reported-by: Xin Long <lucien....@gmail.com> >>>>> Reported-at: https://issues.redhat.com/browse/FDP-708 >>>>> Signed-off-by: Paolo Valerio <pvale...@redhat.com> >>>> >>>> Hi Paolo, >>>> >>>> I exercised this using vng with net-next compiled using >>>> tools/testing/selftests/net/config from the upstream kernel tree [1]. >>>> >>>> [1] >>>> https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style >>>> >>>> The resulting config does not have CONFIG_NETFILTER_CONNCOUNT set. >>>> >>> >>> Hi Simon, >>> >>> I used vng with net-next as well, but with a different config. In my >>> case I simply reused a previously used config which is essentially my >>> local one updated with olddefconfig and manually (to remove/add things). >>> >>>> Some observations: >>>> >>>> * CONFIG_NETFILTER_XT_TARGET_CT is required for -j CT >>>> >>>> I don't think this is a problem (other than my own problem >>>> of it taking me a long time to figure that out). But it seems >>>> worth noting (see parentheses in previous sentence:). >>>> >>> >>> It's something I was thinking about right after the patch submission. >>> Although extensions are fairly common in the main distros, maybe we may >>> consider checking they are present. >>> >>> The only ways that comes to mind to reliably check whether the >>> extensions (both kernel and user space) are present is simply by >>> applying a rule. >>> >>> I guess we can (added a diff at the bottom), given the nft discussion >>> and the potential follow-up, change things a bit in order to better >>> accomodate a potential migration/follow-up. >>> >>> WDYT? >>> >>>> * Of the tests that are updated by this patch, >>>> I only observed that the last one, >>>> "conntrack - can match and clear ct_state from outside OVS", >>>> fails without this patch applied. >>>> >>>> I am unsure if that is something that warrants updating this >>>> patch or not. Or if, rather, there is an error in my testing. >>> >>> Thanks for testing it. >>> >>> Interesting. >>> I tried the following config against net-next and it seems I can't >>> reproduce that behaviour: >>> >>> vng --build --config tools/testing/selftests/net/config \ >>> --configitem CONFIG_NF_CONNTRACK_ZONES=y \ >>> --configitem CONFIG_NETFILTER_XT_TARGET_CT=m -v >>> >>> --- >>> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >>> index 5203b1df8..6b5eb32fc 100644 >>> --- a/tests/system-kmod-macros.at >>> +++ b/tests/system-kmod-macros.at >>> @@ -267,3 +267,22 @@ m4_define([OVS_CHECK_BAREUDP], >>> AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635 >>> ethertype mpls_uc 2>&1 >/dev/null]) >>> AT_CHECK([ip link del dev ovs_bareudp0]) >>> ]) >>> + >>> +# CHECK_EXTERNAL_CT() >>> +# >>> +# Checks if packets can be tracked outside OvS. >>> +m4_define([CHECK_EXTERNAL_CT], >>> +[ >>> + dnl Kernel config (CONFIG_NETFILTER_XT_TARGET_CT) >>> + dnl and user space extensions need to be present. >>> + AT_SKIP_IF([! iptables -t raw -I OUTPUT 1 -j CT]) >>> + AT_CHECK([iptables -t raw -D OUTPUT 1]) >>> +]) >>> + >>> +# ADD_EXTERNAL_CT() >>> +# >>> +# Let conntrack start tracking the packets outside OvS. >>> +m4_define([ADD_EXTERNAL_CT], >>> +[ >>> + AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT]) >>> +]) >> >> on_exit here got lost for some reason. >> Below the corrected diff. >> >> --- >> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >> index 5203b1df8..ab89ea24b 100644 >> --- a/tests/system-kmod-macros.at >> +++ b/tests/system-kmod-macros.at >> @@ -267,3 +267,23 @@ m4_define([OVS_CHECK_BAREUDP], >> AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635 >> ethertype mpls_uc 2>&1 >/dev/null]) >> AT_CHECK([ip link del dev ovs_bareudp0]) >> ]) >> + >> +# CHECK_EXTERNAL_CT() >> +# >> +# Checks if packets can be tracked outside OvS. >> +m4_define([CHECK_EXTERNAL_CT], >> +[ >> + dnl Kernel config (CONFIG_NETFILTER_XT_TARGET_CT) >> + dnl and user space extensions need to be present. >> + AT_SKIP_IF([! iptables -t raw -I OUTPUT 1 -j CT]) >> + AT_CHECK([iptables -t raw -D OUTPUT 1]) >> +]) >> + >> +# ADD_EXTERNAL_CT() >> +# >> +# Let conntrack start tracking the packets outside OvS. >> +m4_define([ADD_EXTERNAL_CT], >> +[ >> + AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT]) >> + on_exit 'iptables -t raw -D OUTPUT 1' >> +]) >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 46a9414d4..5435a6241 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -5458,12 +5458,12 @@ OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> AT_SETUP([conntrack - multiple zones, local]) >> -AT_SKIP_IF([test $HAVE_IPTABLES = no]) >> +CHECK_EXTERNAL_CT() >> CHECK_CONNTRACK() >> CHECK_CONNTRACK_LOCAL_STACK() >> OVS_TRAFFIC_VSWITCHD_START() >> >> -IPTABLES_CT([br0]) >> +ADD_EXTERNAL_CT([br0]) >> ADD_NAMESPACES(at_ns0) >> >> AT_CHECK([ip addr add dev br0 "10.1.1.1/24"]) >> @@ -5509,12 +5509,12 @@ OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> AT_SETUP([conntrack - multi-stage pipeline, local]) >> -AT_SKIP_IF([test $HAVE_IPTABLES = no]) >> +CHECK_EXTERNAL_CT() >> CHECK_CONNTRACK() >> CHECK_CONNTRACK_LOCAL_STACK() >> OVS_TRAFFIC_VSWITCHD_START() >> >> -IPTABLES_CT([br0]) >> +ADD_EXTERNAL_CT([br0]) >> ADD_NAMESPACES(at_ns0) >> >> AT_CHECK([ip addr add dev br0 "10.1.1.1/24"]) >> @@ -8392,7 +8392,7 @@ OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> AT_SETUP([conntrack - can match and clear ct_state from outside OVS]) >> -AT_SKIP_IF([test $HAVE_IPTABLES = no]) >> +CHECK_EXTERNAL_CT() >> CHECK_CONNTRACK_LOCAL_STACK() >> OVS_CHECK_GENEVE() >> >> @@ -8403,7 +8403,7 @@ AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> AT_CHECK([ovs-ofctl add-flow br-underlay >> "priority=100,ct_state=+trk,actions=ct_clear,resubmit(,0)"]) >> AT_CHECK([ovs-ofctl add-flow br-underlay "priority=10,actions=normal"]) >> >> -IPTABLES_CT([br0]) >> +ADD_EXTERNAL_CT([br0]) >> ADD_NAMESPACES(at_ns0) >> >> dnl Set up underlay link from host into the namespace using veth pair. >> diff --git a/tests/system-userspace-macros.at >> b/tests/system-userspace-macros.at >> index d9b5b7e4c..c1be97347 100644 >> --- a/tests/system-userspace-macros.at >> +++ b/tests/system-userspace-macros.at >> @@ -357,3 +357,19 @@ m4_define([OVS_CHECK_BAREUDP], >> [ >> AT_SKIP_IF([:]) >> ]) >> + >> +# CHECK_EXTERNAL_CT() >> +# >> +# The userspace datapath does not support external ct. >> +m4_define([CHECK_EXTERNAL_CT], >> +[ >> + AT_SKIP_IF([:]) >> +]) >> + >> +# ADD_EXTERNAL_CT() >> +# >> +# The userspace datapath does not support external ct. >> +m4_define([ADD_EXTERNAL_CT], >> +[ >> + AT_SKIP_IF([:]) >> +]) >> > > This change to a common CT support check makes sense to me - is the plan > to submit it as a patch in this series or will you submit it as > a standalone / follow up work?
The plan is, if no objections, to respin this as a v3. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
