[ovs-dev] [PATCH v4] conntrack: document all-zero IP SNAT behavior and add a test case

Eelco Chaudron echaudro at redhat.com
Thu Jun 10 09:11:34 UTC 2021



On 3 Jun 2021, at 15:27, Ilya Maximets wrote:

> On 6/3/21 2:38 PM, Aaron Conole wrote:
>> Eelco Chaudron <echaudro at redhat.com> writes:
>>
>>> On 2 Jun 2021, at 18:21, Aaron Conole wrote:
>>>
>>>> Eelco Chaudron <echaudro at redhat.com> writes:
>>>>
>>>>> Currently, conntrack in the kernel has an undocumented feature referred
>>>>> to as all-zero IP address 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. In addition, a datapath feature flag is added for
>>>>> the all-zero IP SNAT case. This will help applications on top of OVS,
>>>>> like OVN, to determine this feature can be used.
>>>>>
>>>>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>>>>> ---
>>>>> v4: Added datapath support flag for all-zero SNAT.
>>>>> v3: Renamed NULL SNAT to all-zero IP SNAT.
>>>>> v2: Fixed NULL SNAT to only work in the -rpl state to be inline with
>>>>>     OpenShift-SDN's behavior.
>>>>>
>>>>>  lib/ct-dpif.c                    |    8 +++++++
>>>>>  lib/ct-dpif.h                    |    6 +++++
>>>>>  lib/dpif-netdev.c                |    1 +
>>>>>  lib/dpif-netlink.c               |   11 +++++++++
>>>>>  lib/dpif-provider.h              |    5 ++++
>>>>>  lib/ovs-actions.xml              |   10 ++++++++
>>>>>  ofproto/ofproto-dpif.c           |   22 +++++++++++++++++-
>>>>>  ofproto/ofproto-dpif.h           |    5 +++-
>>>>>  tests/system-kmod-macros.at      |    7 ++++++
>>>>>  tests/system-traffic.at          |   46 ++++++++++++++++++++++++++++++++++++++
>>>>>  tests/system-userspace-macros.at |   10 ++++++++
>>>>>  vswitchd/vswitch.xml             |    9 +++++++
>>>>>  12 files changed, 138 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
>>>>> index 6a5ba052d..cfc2315e3 100644
>>>>> --- a/lib/ct-dpif.c
>>>>> +++ b/lib/ct-dpif.c
>>>>> @@ -889,3 +889,11 @@ ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
>>>>>                  dpif, tp_id, dl_type, nw_proto, tp_name, is_generic)
>>>>>              : EOPNOTSUPP);
>>>>>  }
>>>>> +
>>>>> +int
>>>>> +ct_dpif_get_features(struct dpif *dpif, enum ct_features *features)
>>>>> +{
>>>>
>>>> NIT-mode:
>>>>
>>>> Are these features or capabilities?  I ask because we may want to add
>>>> support for things like tcp loose mode, etc, and not sure if there would
>>>> need to be a corresponding set function (to enable / disable), and how
>>>> that should look.  I'm okay with keeping it as-is for here and adding
>>>> the corresponding set function later, but it would seem strange to try
>>>> and "set" support for all-zero snat, etc.
>>>
>>> Guess the line between feature and capabilities is rather thin... The
>>> code for checking the datapath support, check_support(), is calling
>>> all of this features, rather than capabilities.
>>>
>>> I guess ct_zero_snat is a none configurable feature ;) But more
>>> seriously, I could go ahead and change the naming from feature to
>>> capability. As most of the configurable "features" have their own
>>> callback.
>>
>> I guess it doesn't matter too much, but I worry about whether we start
>> trying to enable.  Maybe we just make it unsupportable?  I'm just
>> concerned that when we add things like tcp_loose or try to make
>> tcp_liberal as a configurable in the kernel DP, there could be some
>> confusion.  Maybe it isn't too important.  Okay, we can cross those
>> bridges when we get there.
>>
>>>>> +    return (dpif->dpif_class->ct_get_features
>>>>> +            ? dpif->dpif_class->ct_get_features(dpif, features)
>>>>> +            : EOPNOTSUPP);
>>>>> +}
>>>
>>> <SNIP>
>>>
>>>>> diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
>>>>> index 34f82cee3..9f0d38dfb 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_ZEROIP_SNAT()
>>>>> +#
>>>>> +# Perform requirements checks for running conntrack all-zero IP SNAT tests.
>>>>> +# The userspace datapath does not support all-zero IP SNAT.
>>>>> +#
>>>>> +m4_define([CHECK_CONNTRACK_ZEROIP_SNAT],
>>>>> +[
>>>>> +    AT_SKIP_IF([:])
>>>>> +])
>>>>
>>>> I didn't check too close, but maybe it's possible to make this check the
>>>> capabilities bits and then it could be extended to everywhere.
>>>
>>> I was thinking about using "ovs-appctl dpif/show-dp-features" after I
>>> added the features check. However none of the other test cases do
>>> this, so I thought there might be a reason? It might be that I need to
>>> configure/setup OVS to run the test command, and not sure if there is
>>> a nice clean way to shut down if the feature is not supported.
>>
>> Okay.  Maybe it's not possible in the test framework.
>
> Hmm.  AFAICT, daemons will be stopped by on_exit hooks, so it
> should not be a problem.  Just call the check after starting the daemon.
>
> BTW, have you checked the windows datapath?  I don't see the "ifdef _WIN32"
> in this patch, so we're reporting the feature as supported on windows.

Alin confirmed that windows does not support zero-SNAT, and you are right I once again forgot the stg refresh :(

Will sent a v5 soon…

>>>>> +
>>>>>  # CHECK_CONNTRACK_TIMEOUT()
>>>>>  #
>>>>>  # Perform requirements checks for running conntrack customized timeout tests.
>>>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>>>>> index 4597a215d..d8ea287d5 100644
>>>>> --- a/vswitchd/vswitch.xml
>>>>> +++ b/vswitchd/vswitch.xml
>>>>> @@ -6126,6 +6126,15 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>>>>>          True if the datapath supports OVS_ACTION_ATTR_DROP.  If false,
>>>>>          explicit drop action will not be sent to the datapath.
>>>>>        </column>
>>>>> +      <column name="capabilities" key="ct_zero_snat"
>>>>> +              type='{"type": "boolean"}'>
>>>>> +        True if the datapath supports all-zero SNAT. This is a special case
>>>>> +        if 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.
>>>>> +      </column>
>>>>>      </group>
>>>>>
>>>>>      <group title="Common Columns">
>>
>>
>> Acked-by: Aaron Conole <aconole at redhat.com>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>



More information about the dev mailing list