[Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration

[Dmitri Pal]

On 05/02/2014 02:52 PM, Simo Sorce wrote:
> On Thu, 2014-05-01 at 16:22 -0400, Nathaniel McCallum wrote:
>> On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote:
>>> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote:
>>>> On Tue, 11 Mar 2014, Jan Pazdziora wrote:
>>>>> On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote:
>>>>>> Before this patch, ipa-kdb would load global configuration on startup
>>>>>> and never update it. This means that if global configuration is changed,
>>>>>> the KDC never receives the new configuration until it is restarted.
>>>>>>
>>>>>> This patch enables caching of the global configuration with a timeout of
>>>>>> 60 seconds.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4153
>>>>>> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001
>>>>>> From: Nathaniel McCallum <npmccallum at redhat.com>
>>>>>> Date: Mon, 24 Feb 2014 14:19:13 -0500
>>>>>> Subject: [PATCH] Periodically refresh global ipa-kdb configuration
>>>>>>
>>>>>> Before this patch, ipa-kdb would load global configuration on startup and
>>>>>> never update it. This means that if global configuration is changed, the
>>>>>> KDC never receives the new configuration until it is restarted.
>>>>>>
>>>>>> This patch enables caching of the global configuration with a timeout of
>>>>>> 60 seconds.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4153
>>>>> I have only read the code and it looks sane, so depending on how much
>>>>> you consider my word about code-reading worth, ack.
>>>>>
>>>>> However, my gut feeling is that my preferred way of handling the issue
>>>>> (without knowing much about the background of the ticket) would
>>>>> probably be a HUP signal handler or something similar, rather than
>>>>> polling for current values with the value timeout. This patch
>>>>> introduces small nondeterminism to the behaviour when the usage of the
>>>>> new values cannot really be enfoced by the admin (without the daemon
>>>>> restart).
>>>> Thing is, we need the update to happen when other, non-root process
>>>> makes the changes -- in our case when IPA server running under httpd
>>>> privileges performs series of MS-RPC calls towards smbd which lead to
>>>> creating certain LDAP objects. httpd couldn't send SIGHUP to a process
>>>> not owned by httpd's effective user (non-root).
>>> Yes but this is not really the way to go.
>>>
>>> The proper fix is to use syncrepl/persistent search to get a
>>> notification of when we need to reload the configuration.
>>>
>>> On the patch itself I have a NACK due to this syntax used in various
>>> places: function()->attribute
>>>
>>> don't. do. that. ever.
>>>
>>> assign whatever come from the function to a local variable and *check*
>>> it is not null, *then* use it.
>> Attached patch fixes the NACK issue, but does not address the question
>> of the larger approach. Do we need to take a different approach? If so,
>> what is it?
> LGTM
> I might prefer slightly more explicit names for those temp vars, but
> that's not a big deal.
>
> As for the approach, moving to something like syncrepl would be a good
> idea. As it would allow us to avoid useless polling and changes would be
> applaied as soon as they become available as the syncrepl agreement
> would push them to our client. It does mean we need a way to check that
> there aren't pending changes coming down the syncrepl pipe, but we can
> do that synchronously at every entry point in the KDB. After all we do
> not need to care about a change until the KDC needs something from the
> KDB.
>
> Simo.
>
Do we need a ticket for that then?

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.