[ovs-dev] [PATCH] conntrack: document NULL SNAT behavior and add a test case

Eelco Chaudron echaudro at redhat.com
Tue Mar 30 12:22:55 UTC 2021



On 30 Mar 2021, at 13:38, Paolo Valerio wrote:

> "Eelco Chaudron" <echaudro at redhat.com> writes:
>
>> On 22 Mar 2021, at 19:50, Paolo Valerio wrote:
>>
>>> Hi Eelco,
>>>
>>> Thanks for working on this, very useful indeed.
>>> not a full review, but I have a question about a minor thing.
>>>
>>> Eelco Chaudron <echaudro at redhat.com> writes:
>>>
>>>> Currently, conntrack in the kernel has an undocumented feature
>>>> referred
>>>> to as NULL SNAT. Basically, when a source port collision is 
>>>> detected
>>>> during the commit, the source port will be translated to an 
>>>> ephemeral
>>>> port. If there is no collision, no SNAT is performed.
>>>>
>>>> This patchset documents this behavior and adds a self-test to 
>>>> verify
>>>> it's not changing.
>>>>
>>>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>>>> ---
>>>>  lib/ovs-actions.xml              |   10 ++++++++
>>>>  tests/system-kmod-macros.at      |    7 ++++++
>>>>  tests/system-traffic.at          |   45
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>  tests/system-userspace-macros.at |   10 ++++++++
>>>>  4 files changed, 72 insertions(+)
>>>>
>>>> diff --git a/lib/ovs-actions.xml b/lib/ovs-actions.xml
>>>> index a2778de4b..a0070e6c6 100644
>>>> --- a/lib/ovs-actions.xml
>>>> +++ b/lib/ovs-actions.xml
>>>> @@ -1833,6 +1833,16 @@ for <var>i</var> in 
>>>> [1,<var>n_members</var>]:
>>>>              connection, will behave the same as a bare
>>>> <code>nat</code>.
>>>>            </p>
>>>>
>>>> +          <p>
>>>> +            For SNAT, there is a special case when the
>>>> <code>src</code> IP
>>>> +            address is configured as all 0's, i.e.,
>>>> +            <code>nat(src=0.0.0.0)</code>. In this case, when a
>>>> source port
>>>> +            collision is detected during the commit, the source 
>>>> port
>>>> will be
>>>> +            translated to an ephemeral port. If there is no
>>>> collision, no SNAT
>>>> +            is performed. Note that this is currently only
>>>> implemented in the
>>>> +            Linux kernel datapath.
>>>> +          </p>
>>>> +
>>>>            <p>
>>>>              Open vSwitch 2.6 introduced <code>nat</code>.  Linux 
>>>> 4.6
>>>> was the
>>>>              earliest upstream kernel that implemented
>>>> <code>ct</code> support for
>>>> diff --git a/tests/system-kmod-macros.at
>>>> b/tests/system-kmod-macros.at
>>>> index 15628a7c6..38bb1c55c 100644
>>>> --- a/tests/system-kmod-macros.at
>>>> +++ b/tests/system-kmod-macros.at
>>>> @@ -99,6 +99,13 @@ m4_define([CHECK_CONNTRACK_FRAG_OVERLAP],
>>>>  #
>>>>  m4_define([CHECK_CONNTRACK_NAT])
>>>>
>>>> +# CHECK_CONNTRACK_NULL_SNAT()
>>>> +#
>>>> +# Perform requirements checks for running conntrack SNAT NULL 
>>>> tests.
>>>> +# The kernel always supports NULL SNAT, so no check is needed.
>>>> +#
>>>> +m4_define([CHECK_CONNTRACK_NULL_SNAT])
>>>> +
>>>>  # CHECK_CONNTRACK_TIMEOUT()
>>>>  #
>>>>  # Perform requirements checks for running conntrack customized
>>>> timeout tests.
>>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>>> index fb5b9a36d..1be425bb4 100644
>>>> --- a/tests/system-traffic.at
>>>> +++ b/tests/system-traffic.at
>>>> @@ -4433,6 +4433,51 @@
>>>> tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
>>>>  OVS_TRAFFIC_VSWITCHD_STOP
>>>>  AT_CLEANUP
>>>>
>>>> +
>>>> +AT_SETUP([conntrack - NULL SNAT])
>>>> +AT_SKIP_IF([test $HAVE_NC = no])
>>>> +CHECK_CONNTRACK()
>>>> +CHECK_CONNTRACK_NULL_SNAT()
>>>> +OVS_TRAFFIC_VSWITCHD_START()
>>>> +
>>>> +ADD_NAMESPACES(at_ns0, at_ns1)
>>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>>>> +NS_CHECK_EXEC([at_ns0], [ip route add 172.1.1.0/24 via 10.1.1.2])
>>>> +
>>>> +OVS_START_L7([at_ns1], [http])
>>>> +
>>>
>>> I noticed you use nc clients, is there any specific reason you
>>> preferred
>>> httpd over something like:
>>>
>>> NETNS_DAEMONIZE([at_ns1], [nc -l -k 80 > /dev/null], [nc0.pid])
>>
>> No specific reason other than the http server python script was used 
>> in
>> all the other tests above.
>>
>
> ok, thanks.
>
>>>> +AT_DATA([flows.txt], [dnl
>>>> +table=0,priority=30,ct_state=-trk,ip,action=ct(table=0)
>>>> +table=0,priority=20,ip,nw_dst=10.1.1.0/24,actions=ct(commit,nat(src=0.0.0.0),table=10)
>
> While at it, I was noticing the rule above.
> This would be matched in both directions.
> I think it would be better to split the logic here matching -rpl (just 
> like the bare
> nat rule below matches replies only).
>
> WDYT?

Makes sense as it aligns with the openshift SDN solution, will send out 
a v2.

>>>> +table=0,priority=20,ip,nw_dst=172.1.1.2,actions=ct(commit,nat(dst=10.1.1.2),table=10)
>>>> +table=0,priority=10,arp,action=normal
>>>> +table=0,priority=1,action=drop
>>>> +table=10,priority=20,ct_state=+rpl,ip,nw_dst=10.1.1.0/24
>>>> actions=ct(table=20,nat)
>>>> +table=10,priority=10,ip,nw_dst=10.1.1.0/24 actions=resubmit(,20)
>>>> +table=20,priority=10,ip,nw_dst=10.1.1.1,action=1
>>>> +table=20,priority=10,ip,nw_dst=10.1.1.2,action=2
>>>> +])
>>>> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
>>>> +
>>>> +dnl - Test to make sure src nat is NOT done when not needed
>>>> +NS_CHECK_EXEC([at_ns0], [echo "TEST" | nc -p 30000 10.1.1.2 80 >
>>>> nc-1.log])
>>>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep
>>>> "orig=.src=10\.1\.1\.1,"], [0], [dnl
>>>> +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=30000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=30000),protoinfo=(state=TIME_WAIT)
>>>> +])
>>>> +
>>>> +dnl - Test to make sure src nat is done when needed
>>>> +NS_CHECK_EXEC([at_ns0], [echo "TEST2" | nc -p 30001 172.1.1.2 80 >
>>>> nc-2.log])
>>>> +NS_CHECK_EXEC([at_ns0], [echo "TEST3" | nc -p 30001 10.1.1.2 80 >
>>>> nc-3.log])
>>>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 30001 | grep
>>>> "orig=.src=10\.1\.1\.1," | sed -e 
>>>> 's/port=30001/port=<clnt_s_port>/g'
>>>> -e 's/sport=80,dport=[[0-9]]\+/sport=80,dport=<rnd_port>/g' | 
>>>> sort],
>>>> [0], [dnl
>>>> +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<clnt_s_port>,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=<rnd_port>),protoinfo=(state=TIME_WAIT)
>>>> +tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<clnt_s_port>,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=<clnt_s_port>),protoinfo=(state=TIME_WAIT)
>>>> +])
>>>> +
>>>> +OVS_TRAFFIC_VSWITCHD_STOP
>>>> +AT_CLEANUP
>>>> +
>>>> +
>>>>  AT_SETUP([conntrack - simple DNAT])
>>>>  CHECK_CONNTRACK()
>>>>  CHECK_CONNTRACK_NAT()
>>>> diff --git a/tests/system-userspace-macros.at
>>>> b/tests/system-userspace-macros.at
>>>> index 34f82cee3..71acc8618 100644
>>>> --- a/tests/system-userspace-macros.at
>>>> +++ b/tests/system-userspace-macros.at
>>>> @@ -96,6 +96,16 @@ m4_define([CHECK_CONNTRACK_FRAG_OVERLAP])
>>>>  #
>>>>  m4_define([CHECK_CONNTRACK_NAT])
>>>>
>>>> +# CHECK_CONNTRACK_NULL_SNAT()
>>>> +#
>>>> +# Perform requirements checks for running conntrack SNAT NULL 
>>>> tests.
>>>> +# The userspace datapath does not support NULL SNAT.
>>>> +#
>>>> +m4_define([CHECK_CONNTRACK_NULL_SNAT],
>>>> +[
>>>> +    AT_SKIP_IF([:])
>>>> +])
>>>> +
>>>>  # CHECK_CONNTRACK_TIMEOUT()
>>>>  #
>>>>  # Perform requirements checks for running conntrack customized
>>>> timeout tests.



More information about the dev mailing list