On 3 Jun 2021, at 15:27, Ilya Maximets wrote:
> 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. 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 <[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
