| Message ID | 20241002160142.397234-1-pvalerio@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3,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 2 Oct 2024, at 18:01, 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> Thanks for the new revision. The changes look good to me. Cheers, Eelco
On 4 Oct 2024, at 15:56, Eelco Chaudron wrote: > On 2 Oct 2024, at 18:01, 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> > > Thanks for the new revision. The changes look good to me. > > Cheers, > > Eelco For got to add my ACK :( Acked-by: Eelco Chaudron <echaudro@redhat.com>
On Wed, Oct 02, 2024 at 06:01:41PM +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> > --- > 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) ... > 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/ovs-macros.at b/tests/ovs-macros.at > index 06c978555..df2835747 100644 > --- a/tests/ovs-macros.at > +++ b/tests/ovs-macros.at > @@ -366,3 +366,8 @@ dnl Add a rule to always accept the traffic. > m4_define([IPTABLES_ACCEPT], > [AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT]) > on_exit 'iptables -D INPUT 1 -i $1']) > + > +dnl Required to let conntrack start tracking the packets outside ovs > +m4_define([IPTABLES_CT], > + [AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT]) > + on_exit 'iptables -t raw -D OUTPUT 1']) Hi Paolo, I don't think IPTABLES_CT is needed now that we have ADD_EXTERNAL_CT. Otherwise this looks good to me. > 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' > +]) ...
Simon Horman <horms@ovn.org> writes: > On Wed, Oct 02, 2024 at 06:01:41PM +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> >> --- >> 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) > > ... > >> 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/ovs-macros.at b/tests/ovs-macros.at >> index 06c978555..df2835747 100644 >> --- a/tests/ovs-macros.at >> +++ b/tests/ovs-macros.at >> @@ -366,3 +366,8 @@ dnl Add a rule to always accept the traffic. >> m4_define([IPTABLES_ACCEPT], >> [AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT]) >> on_exit 'iptables -D INPUT 1 -i $1']) >> + >> +dnl Required to let conntrack start tracking the packets outside ovs >> +m4_define([IPTABLES_CT], >> + [AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT]) >> + on_exit 'iptables -t raw -D OUTPUT 1']) > > Hi Paolo, > > I don't think IPTABLES_CT is needed now that we have ADD_EXTERNAL_CT. > it's not, indeed. It's a leftover of the old revision. I sent a new revision. Thanks. > Otherwise this looks good to me. > >> 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/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/ovs-macros.at b/tests/ovs-macros.at index 06c978555..df2835747 100644 --- a/tests/ovs-macros.at +++ b/tests/ovs-macros.at @@ -366,3 +366,8 @@ dnl Add a rule to always accept the traffic. m4_define([IPTABLES_ACCEPT], [AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT]) on_exit 'iptables -D INPUT 1 -i $1']) + +dnl Required to let conntrack start tracking the packets outside ovs +m4_define([IPTABLES_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-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([:]) +])
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> --- 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) --- tests/atlocal.in | 3 +++ tests/ovs-macros.at | 5 +++++ tests/system-kmod-macros.at | 21 +++++++++++++++++++++ tests/system-traffic.at | 8 ++++++++ tests/system-userspace-macros.at | 16 ++++++++++++++++ 5 files changed, 53 insertions(+)