On 2 Jun 2021, at 18:21, Aaron Conole wrote:
> Eelco Chaudron <echau...@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 <echau...@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.
>> + 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.
>> +
>> # 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">
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev