On Fri, 2014-11-14 at 09:06 -0500, Simo Sorce wrote: > On Fri, 14 Nov 2014 10:32:23 +0100 > Jakub Hrozek <jhro...@redhat.com> wrote: > > > On Thu, Nov 13, 2014 at 02:40:28PM -0500, Simo Sorce wrote: > > > On Thu, 13 Nov 2014 14:20:14 -0500 > > > Nathaniel McCallum <npmccal...@redhat.com> wrote: > > > > > > > On Thu, 2014-11-13 at 14:02 -0500, Nathaniel McCallum wrote: > > > > > On Fri, 2014-11-07 at 15:39 +0100, Martin Kosek wrote: > > > > > > On 11/07/2014 03:28 PM, Nathaniel McCallum wrote: > > > > > > > > > > > > > >> On Nov 7, 2014, at 9:21 AM, Martin Kosek > > > > > > >> <mko...@redhat.com> wrote: > > > > > > >> > > > > > > >> On 11/07/2014 03:03 PM, Simo Sorce wrote: > > > > > > >>> On Fri, 07 Nov 2014 14:53:17 +0100 > > > > > > >>> Martin Kosek <mko...@redhat.com> wrote: > > > > > > >>> > > > > > > >>>> On 11/07/2014 02:51 PM, Simo Sorce wrote: > > > > > > >>>>> On Fri, 07 Nov 2014 14:26:06 +0100 > > > > > > >>>>> Martin Kosek <mko...@redhat.com> wrote: > > > > > > >>>>> > > > > > > >>>>>> On 11/07/2014 02:20 PM, Simo Sorce wrote: > > > > > > >>>>>>> On Fri, 07 Nov 2014 08:02:02 +0100 > > > > > > >>>>>>> Martin Kosek <mko...@redhat.com> wrote: > > > > > > >>>>>>> > > > > > > >>>>>>>> On 11/07/2014 01:46 AM, Simo Sorce wrote: > > > > > > >>>>>>>>> On Thu, 06 Nov 2014 18:00:21 -0500 > > > > > > >>>>>>>>> Nathaniel McCallum <npmccal...@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 > > > > > > > > > > > > > > > > > > > Or to simply call python and get the value of our state store > > > > > > keeping track what was done on upgrades. This would work: > > > > > > > > > > > > # python2 -c "from ipaserver.install import sysupgrade; import > > > > > > sys; sys.exit(1 if sysupgrade.get_upgrade_state('krb5.conf', > > > > > > 'udp_preference_limit_set') else 0)" # echo $? > > > > > > 0 > > > > > > > > > > > > # python2 -c "from ipaserver.install import sysupgrade; > > > > > > sysupgrade.set_upgrade_state('krb5.conf', > > > > > > 'udp_preference_limit_set', True)" > > > > > > > > > > > > # python2 -c "from ipaserver.install import sysupgrade; import > > > > > > sys; sys.exit(1 if sysupgrade.get_upgrade_state('krb5.conf', > > > > > > 'udp_preference_limit_set') else 0)" # echo $? > > > > > > 1 > > > > > > > > > > > > You would just need to come up with something that's present > > > > > > on the client, this is just on the server sub package. > > > > > > > > > > What about the attached approach? It is untested, but I can > > > > > test it if we like this method. > > > > > > > > > > Basically, create an include directory that we ship defaults in. > > > > > Admins can update /etc/krb5.conf to override the defaults. If we > > > > > need to change the default, we just change the file we ship. > > > > > > > > > > We can enable this on upgrades by ensuring that the appropriate > > > > > includedir statement appears at the top of the file. The nice > > > > > thing about this approach is that is preserves admin > > > > > modifications (unless they removed the configuration directive > > > > > rather than changing it). > > > > > > > > > > Thoughts? > > > > > > > > Simo has just informed me that we will be > > > > getting /etc/krb5.conf.d at some point in the near future. So I > > > > think for now I'd like to have the original patch[1] reviewed > > > > (not the includedir experiment patch). We then document the TCP > > > > issue for upgrades. Then, in the future, when we > > > > get /etc/krb5.conf.d, we can just stick the default in there. > > > > > > > > Is everyone cool with this? > > > > > > +1 > > > > If there's an easy way to set this option in SSSD, I'm fine with > > setting the same default in SSSD as well. > > > > Sorry we dropped the ball on this item from the SSSD side, I think > > it's just b/c there was no ticket filed ... > > We discussed doing that, but we think sssd is not the right place, as > an admin may legitimately want to remove that option.
In my understanding, SSSD gets this for free with this patch. _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel