| Message ID | 20241007155425.28710-1-pvalerio@redhat.com |
|---|---|
| State | Accepted |
| Commit | 60917c822de64b516469430c2e1e966ce69b981d |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev,v4,1/2] system-traffic: Do not rely on conncount for already tracked packets. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Mon, Oct 07, 2024 at 05:54:24PM +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 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.xin@gmail.com> > Reported-at: https://issues.redhat.com/browse/FDP-708 > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > v4: > - removed IPTABLES_CT() leftover (Simon) > > v3: > - generalized introducing CHECK_EXTERNAL_CT()/ADD_EXTERNAL_CT() > to ease the transition toward a different front-end > > v2: > - add $HAVE_IPTABLES > - reduced subject length (0-day Robot) Thanks for the updates. Acked-by: Simon Horman <horms@ovn.org>
Paolo Valerio <pvalerio@redhat.com> writes: > 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 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.xin@gmail.com> > Reported-at: https://issues.redhat.com/browse/FDP-708 > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- Thanks to Paolo, Simon, and Eelco - merged.
On 10/9/24 22:28, Aaron Conole wrote: > Paolo Valerio <pvalerio@redhat.com> writes: > >> 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 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.xin@gmail.com> >> Reported-at: https://issues.redhat.com/browse/FDP-708 >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> --- > > Thanks to Paolo, Simon, and Eelco - merged. Thanks, Aaron! AFAIU, we need these changes on all supported branches. Could you, please, check and backport? Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 10/9/24 22:28, Aaron Conole wrote: >> Paolo Valerio <pvalerio@redhat.com> writes: >> >>> 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 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.xin@gmail.com> >>> Reported-at: https://issues.redhat.com/browse/FDP-708 >>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >> >> Thanks to Paolo, Simon, and Eelco - merged. > > Thanks, Aaron! AFAIU, we need these changes on all supported branches. > Could you, please, check and backport? Will do. > Best regards, Ilya Maximets.
diff --git a/tests/atlocal.in b/tests/atlocal.in index 8565a0bae..d6b87f8ec 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -185,6 +185,9 @@ find_command lftp # Set HAVE_ETHTOOL find_command ethtool +# Set HAVE_IPTABLES +find_command iptables + CURL_OPT="-g -v --max-time 1 --retry 2 --retry-delay 1 --connect-timeout 1" # Determine whether "diff" supports "normal" diffs. (busybox diff does not.) diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index 5203b1df8..135892e91 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -267,3 +267,24 @@ 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([test $HAVE_IPTABLES = no]) + 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 202ff0492..5435a6241 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -1094,6 +1094,7 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/Invalid Geneve tunnel metadata on bridge br0 while AT_CLEANUP AT_SETUP([datapath - ping over gre tunnel by simulated packets]) +AT_SKIP_IF([test $HAVE_IPTABLES = no]) OVS_CHECK_MIN_KERNEL(3, 10) OVS_TRAFFIC_VSWITCHD_START() @@ -1140,6 +1141,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([datapath - ping over erspan v1 tunnel by simulated packets]) +AT_SKIP_IF([test $HAVE_IPTABLES = no]) OVS_CHECK_MIN_KERNEL(3, 10) OVS_TRAFFIC_VSWITCHD_START() @@ -5456,10 +5458,12 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([conntrack - multiple zones, local]) +CHECK_EXTERNAL_CT() CHECK_CONNTRACK() CHECK_CONNTRACK_LOCAL_STACK() OVS_TRAFFIC_VSWITCHD_START() +ADD_EXTERNAL_CT([br0]) ADD_NAMESPACES(at_ns0) AT_CHECK([ip addr add dev br0 "10.1.1.1/24"]) @@ -5505,10 +5509,12 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([conntrack - multi-stage pipeline, local]) +CHECK_EXTERNAL_CT() CHECK_CONNTRACK() CHECK_CONNTRACK_LOCAL_STACK() OVS_TRAFFIC_VSWITCHD_START() +ADD_EXTERNAL_CT([br0]) ADD_NAMESPACES(at_ns0) AT_CHECK([ip addr add dev br0 "10.1.1.1/24"]) @@ -8386,6 +8392,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([conntrack - can match and clear ct_state from outside OVS]) +CHECK_EXTERNAL_CT() CHECK_CONNTRACK_LOCAL_STACK() OVS_CHECK_GENEVE() @@ -8396,6 +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"]) +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([:]) +])