[Freeipa-devel] [PATCH 0019] Prefer TCP connections to UDP in krb5 clients

Fri Nov 7 14:28:57 UTC 2014

> On Nov 7, 2014, at 9:21 AM, Martin Kosek <mkosek at redhat.com> wrote:
> 
> On 11/07/2014 03:03 PM, Simo Sorce wrote:
>> On Fri, 07 Nov 2014 14:53:17 +0100
>> Martin Kosek <mkosek at redhat.com> wrote:
>> 
>>> On 11/07/2014 02:51 PM, Simo Sorce wrote:
>>>> On Fri, 07 Nov 2014 14:26:06 +0100
>>>> Martin Kosek <mkosek at redhat.com> wrote:
>>>> 
>>>>> On 11/07/2014 02:20 PM, Simo Sorce wrote:
>>>>>> On Fri, 07 Nov 2014 08:02:02 +0100
>>>>>> Martin Kosek <mkosek at redhat.com> wrote:
>>>>>> 
>>>>>>> On 11/07/2014 01:46 AM, Simo Sorce wrote:
>>>>>>>> On Thu, 06 Nov 2014 18:00:21 -0500
>>>>>>>> Nathaniel McCallum <npmccallum at redhat.com> wrote:
>>>>>>>> 
>>>>>>>>> On Fri, 2013-10-04 at 06:12 -0400, Simo Sorce wrote:
>>>>>>>>>> 
>>>>>>>>>> ----- Original Message -----
>>>>>>>>>>> On 3.10.2013 23:43, Nathaniel McCallum wrote:
>>>>>>>>>>>> Patch attached.
>>>>>>>>>>> 
>>>>>>>>>>> I'm curious - what is the purpose of this patch? To prevent 1
>>>>>>>>>>> second timeouts and re-transmits when OTP is in place?
>>>>>>>>>>> 
>>>>>>>>>>> What is the expected performance impact? Could it be
>>>>>>>>>>> configured for OTP separately - somehow? (I guess that it is
>>>>>>>>>>> not possible now ...)
>>>>>>>>>> 
>>>>>>>>>> It benefits also communication of large packets (when large
>>>>>>>>>> MS-PAC or CAMMAC AD Data are attached), so it is a better
>>>>>>>>>> choice for IPA in general. Especially given we have multiple
>>>>>>>>>> KDC processes configured we do not want clients wasting KDC
>>>>>>>>>> resources by making multiple processes do the same operation.
>>>>>>>>> 
>>>>>>>>> So apparently this patch never got reviewed over a year ago.
>>>>>>>>> 
>>>>>>>>> It was related to a bug which was opened in SSSD. However, when
>>>>>>>>> it became clear we wanted to solve this in FreeIPA, the SSSD
>>>>>>>>> bug was closed but no corresponding FreeIPA bug was opened. The
>>>>>>>>> patch then fell through the cracks.
>>>>>>> 
>>>>>>> Right - without an associated ticket tracking the patch, it is
>>>>>>> too easy to loose it unless the author prods people to review it.
>>>>>>> 
>>>>>>>>> Without this patch, if OTP validation runs long we get
>>>>>>>>> retransmits and failures.
>>>>>>>>> 
>>>>>>>>> One question I have is how to handle this for upgrades since (I
>>>>>>>>> think) this patch only handles new installs.
>>>>>>>>> 
>>>>>>>>> Anyway, this patch is somewhat urgent now. So help is
>>>>>>>>> appreciated.
>>>>>>>>> 
>>>>>>>>> I have attached a rebased version which has no other changes.
>>>>>>>>> 
>>>>>>>>> Nathaniel
>>>>>>>> 
>>>>>>>> I am not sure we can do much on updates, we do not have a
>>>>>>>> client-update tool, I would just document it I guess.
>>>>>>>> Otherwise we'd have to go back to sssd which can inject
>>>>>>>> additional values in krb5.conf, however I am not sure it would
>>>>>>>> be ok to set something like this in the sssd's pubconf
>>>>>>>> includes ...
>>>>>>> 
>>>>>>> Agreed, pubconf update does not sound right.
>>>>>>> 
>>>>>>> However, we already update krb5.conf on client updates, in %post:
>>>>>>> 
>>>>>>> %post client
>>>>>>> if [ $1 -gt 1 ] ; then
>>>>>>>        # Has the client been configured?
>>>>>>>        restore=0
>>>>>>>        test -f '/var/lib/ipa-client/sysrestore/sysrestore.index'
>>>>>>> && restore=$(wc -l
>>>>>>> '/var/lib/ipa-client/sysrestore/sysrestore.index' | awk '{print
>>>>>>> $1}')
>>>>>>> 
>>>>>>>        if [ -f '/etc/sssd/sssd.conf' -a $restore -ge 2 ]; then
>>>>>>>            if ! grep -E -q
>>>>>>> '/var/lib/sss/pubconf/krb5.include.d/' /etc/krb5.conf
>>>>>>>     2>/dev/null ; then
>>>>>>>                echo
>>>>>>> "includedir /var/lib/sss/pubconf/krb5.include.d/"
>>>>>>>> /etc/krb5.conf.ipanew cat /etc/krb5.conf
>>>>>>>>>> /etc/krb5.conf.ipanew
>>>>>>>                mv /etc/krb5.conf.ipanew /etc/krb5.conf
>>>>>>>                /sbin/restorecon /etc/krb5.conf
>>>>>>>            fi
>>>>>>>        fi
>>>>>>> ...
>>>>>>> 
>>>>>>> 
>>>>>>> This particular update is more difficult as not a first line
>>>>>>> needs to be changed. Without adding ipa client update tool with
>>>>>>> some reasonable krb5.conf parser, we could do something along
>>>>>>> the lines of
>>>>>>> 
>>>>>>> sed -E 0i 's/(forwardable = \w+)/\1\n udp_preference_limit =
>>>>>>> 0/g' /etc/krb5.conf
>>>>>>> 
>>>>>>> (tested), but it is not pretty.
>>>>>> 
>>>>>> What happen the next time you run it again ?
>>>>>> 
>>>>>> Simo.
>>>>>> 
>>>>> 
>>>>> It would of course be added again, you would need to first grep for
>>>>> presence of udp_preference_limit setting. Question is if this
>>>>> approach is safe enough to be in our client %post upgrade. We
>>>>> already upgrade krb5.conf here, just the change is much easier as
>>>>> we just add a line to the beginning of the file.
>>>> 
>>>> Well the concern (aside of duplication) is that  an admin may
>>>> "correct" the krb5.conf file to remove that option (for example
>>>> because his clients also connect to a differen (older) KDC and must
>>>> use UDP in preference. But now we end up messing with its krb5.conf
>>>> every time an update is released. An update tool that keep tracks
>>>> of whether a specific update has already been applied and does not
>>>> retry every time would be needed IMO.
>>>> 
>>>> Simo.
>>> 
>>> In 4.1.x (as there is not much time to develop a separate client
>>> update tool), we could grep just for "udp_preference_limit" presence
>>> so that if admin changes it's value or comment it, it would not be
>>> added again.
>> 
>> Ok then maybe we add this:
>> 
>> # The following value has been added by a freeipa client update
>> # if you want to disable it, please comment it, do not delete it
>> # or it will be re-added on the next update
>> udp_preference_limit = 0
>> 
>> What do you think ?
>> Simo.
>> 
> 
> Sure, this could work (though it is quite lengthy).

I think it would be sufficient to only perform the addition of udp_preference_limit if it does not already exist and if the version number of the package we are upgrading from is less than the one where we introduced the change.

Nathaniel