diff mbox series

[ovs-dev,v4,1/2] system-traffic: Do not rely on conncount for already tracked packets.

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

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Paolo Valerio Oct. 7, 2024, 3:54 p.m. UTC
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)
---
 tests/atlocal.in                 |  3 +++
 tests/system-kmod-macros.at      | 21 +++++++++++++++++++++
 tests/system-traffic.at          |  8 ++++++++
 tests/system-userspace-macros.at | 16 ++++++++++++++++
 4 files changed, 48 insertions(+)

Comments

Simon Horman Oct. 8, 2024, 1:24 p.m. UTC | #1
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>
Aaron Conole Oct. 9, 2024, 8:28 p.m. UTC | #2
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.
Ilya Maximets Oct. 10, 2024, 10:26 a.m. UTC | #3
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.
Aaron Conole Oct. 10, 2024, 4:25 p.m. UTC | #4
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 mbox series

Patch

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([:])
+])