On 5/27/21 10:09 AM, Eelco Chaudron wrote: > > > On 27 May 2021, at 10:01, Dumitru Ceara wrote: > >> On 5/27/21 9:26 AM, Eelco Chaudron wrote: >>> >>> >>> On 25 May 2021, at 15:20, Aaron Conole wrote: >>> >>>> Dumitru Ceara <[email protected]> writes: >>>> >>>>> On 5/21/21 11:27 AM, Paolo Valerio wrote: >>>>>> Aaron Conole <[email protected]> writes: >>>>>> >>>>>>> Dumitru Ceara <[email protected]> writes: >>>>>>> >>>>>>>> On 4/26/21 2:19 PM, Eelco Chaudron wrote: >>>>>>>>> Currently, conntrack in the kernel has an undocumented feature >>>>>>>>> referred >>>>>>>>> to as all-zero IP address 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 <[email protected]> >>>>>>>>> --- >>>>>>>>> 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. >>>>>>>> >>>>>>>> Hi Eelco, >>>>>>>> >>>>>>>> Would it be possible to add this capability to the list of kernel >>>>>>>> Datapath.capabilities ovsdb column? [0] >>>>>>>> >>>>>>>> Given that the patch to add userspace datapath support for all-zero IP >>>>>>>> SNAT is not accepted yet [1], and even if it does it will likely not be >>>>>>>> backported to LTS because it's a feature, this would make it easier for >>>>>>>> OVN (for example ovn-controller) to determine at runtime if it should >>>>>>>> use all-zero IP SNAT or not. >>>>>>> >>>>>>> I think it would be rather difficult to add this to the userspace patch >>>>>>> being proposed. >>>>>>> >>>>>>> Actually, if we want something where datapath can detect whether such >>>>>>> feature is supported, there isn't a good test. For instance, normally >>>>>>> we would attempt to insert a flow that has the characteristics we desire >>>>>>> and look for a failure to insert. In the case of all-zero SNAT, >>>>>>> both datapaths can "accept" it, so it becomes difficult. >>>>>>> >>>>>> >>>>>> I was wondering the same, a successful insertion doesn't ensure the >>>>>> correctness and so the support of this feature. >>>>>> >>>>>>> Also, the netlink datapath under linux has had this support since it was >>>>>>> introduced, so we're really just documenting it here. It can still be >>>>>>> used in older releases. OTOH, userspace doesn't have such support. >>>>>>> Both datapaths will accept flows with SNAT set to 0, as far as I can >>>>>>> tell. This means there's not an easy way to distinguish them. >>>>>>> >>>>>>> Maybe we need a change to this patchset to reject such flows from the >>>>>>> netdev datapath, and then we can modify [1] to remove such limitation. >>>>> >>>>> That would work, I guess. >>>>> >>>>>>> Then it can be detected by the datapath capabilities that already exist. >>>>> >>>>> Sounds good to me but which already existing datapath capability would >>>>> you use? As far as I see the existing ones are: >>>>> >>>>> https://github.com/openvswitch/ovs/blob/13c0eaa7b4fc2694a8c6cc8e6487ec6538c607e4/ofproto/ofproto-dpif.c#L5567 >>>>> >>>>> Shouldn't we add a new one, e.g., "ct_zero_nat"? >>>> >>>> Yes, sorry - I think it was meant to use the mechanism that already >>>> existed, rather than reuse an existing bit. >>>> >>> >>> I’ll ping Paolo to see if he started the effort, if not I can do it today. >>> >>> Dumitru, so from OVN you will check to see if ct_zero_nat support field is >>> in the database, if not you will support SNAT only on the kernel dpath. If >>> it is present, you use it depending not the value set? (Which will be true >>> for both kernel and userspace). Or do I miss something? >>> >> >> OVN cannot differentiate between datapath types unless we hardcode >> datapath-type names which doesn't seem OK to me. > > Can’t it use the datapath_type from the bridge table?
Ideally we should avoid doing something like "if datapath_type == 'system' then generate flows that use zero-ip SNAT". Instead use the value of ct_zero_nat in the Datapath.capabilities to decide whether to use it or not. E.g., from the OVN RFC series I sent: http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ https://github.com/dceara/ovn/commit/9894af7660139d15fc649116ffa22fdc5189484b#diff-220cd89c1bf69b5cf68c6e9ea3771dd48da861c58aaf871f29f13d8cccd6cff1R481 > >> The plan is for OVN to use all-zero IP SNAT only if the ct_zero_nat >> field is in the database. >> >> So, in order to use it with the kernel datapath, the field needs to be >> set to 'true' when the kernel datapath is used. > > Is Paolo’s patch good enough? Meaning if we add the ct_zero_nat to his patch > will that be enough? I thought the agreed approach was: 1. Add a patch that sets ct_zero_nat in capabilities if the datapath accepts all-zero IP SNAT, similar to other action checks: https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif.c#L1572 2. Add a patch that makes the userspace datapath refuse flows that use action ct(nat(src=0.0.0.0)). 3. Add Paolo's series to support all-zero IP SNAT, and revert the change in patch #2 above. Patches 1 & 2 could be backported to stable branches and patch 3 would be available in the next OVS release. That would allow OVN to be generic and work with any OVS release (upstream/downstream). Please correct me if I'm wrong. Thanks! > >> Thanks, >> Dumitru >> >>>>>>> >>>>>>> Maybe I missed something, or misunderstood something. >>>>>>> >>>>>>> Thoughts? >>>>>>> >>>>>> >>>>>> Considering that linux supports all-zero snat since day one, the >>>>>> assumption seems solid enough to me for it. >>>>>> >>>>>> I have no details about it, but what about Windows dp? >>>>>> >>>>>>>> [0] >>>>>>>> https://github.com/openvswitch/ovs/commit/27501802d09f782b8133031c1eae3394ae5ce147 >>>>>>>> >>>>>>>> [1] >>>>>>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=241223 >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Dumitru >>>>>> >>> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
