On 6/3/21 2:38 PM, Aaron Conole wrote:
> Eelco Chaudron <[email protected]> writes:
>
>> On 2 Jun 2021, at 18:21, Aaron Conole wrote:
>>
>>> Eelco Chaudron <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
>
>>>> +
>>>> # 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 <[email protected]>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
