[Freeipa-devel] [PATCH 0073] Remove support for IPA deployments with no persistent search

[Petr Spacek]

On 15.7.2013 18:36, Martin Kosek wrote:
> On 07/15/2013 06:28 PM, Simo Sorce wrote:
>> On Mon, 2013-07-15 at 16:41 +0200, Petr Spacek wrote:
>>> On 15.7.2013 16:15, Simo Sorce wrote:
>>>> On Mon, 2013-07-15 at 15:57 +0200, Martin Kosek wrote:
>>>>> On 07/15/2013 03:44 PM, Petr Spacek wrote:
>>>>>> On 15.7.2013 15:31, Martin Kosek wrote:
>>>>>>> On 07/11/2013 05:10 PM, Tomas Babej wrote:
>>>>>>>> On Thursday 11 of July 2013 16:10:33 Ana Krivokapic wrote:
>>>>>>>>
>>>>>>>>> On 07/11/2013 11:20 AM, Tomas Babej wrote:
>>>>>>>>
>>>>>>>>>> boolean_var = {}
>>>>>>>>
>>>>>>>>>> - for var in ('persistent_search', 'serial_autoincrement'):
>>>>>>>>
>>>>>>>>>> + for var in ('serial_autoincrement'):
>>>>>>>>
>>>>>>>>> This won't work - a one element tuple needs a comma at the end:
>>>>>>>>
>>>>>>>>> ('serial_autoincrement', )
>>>>>>>>
>>>>>>>>>> boolean_var[var] = "yes" if getattr(self, var, False) else "no"
>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>>> self.sub_dict = dict(FQDN=self.fqdn,
>>>>>>>>
>>>>>>>>>> @@ -607,9 +604,8 @@ class BindInstance(service.Service):
>>>>>>>>
>>>>>>>>>> SUFFIX=self.suffix,
>>>>>>>>
>>>>>>>>>> OPTIONAL_NTP=optional_ntp,
>>>>>>>>
>>>>>>>>>> ZONEMGR=self.zonemgr,
>>>>>>>>
>>>>>>>>>> - ZONE_REFRESH=self.zone_refresh,
>>>>>>>>
>>>>>>>>>> IPA_CA_RECORD=ipa_ca,
>>>>>>>>
>>>>>>>>>> - PERSISTENT_SEARCH=boolean_var['persistent_search'],
>>>>>>>>
>>>>>>>>>> + PERSISTENT_SEARCH="yes",
>>>>>>>>
>>>>>>>>>> SERIAL_AUTOINCREMENT=boolean_var['serial_autoincrement'],)
>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>> But anyway, I think this piece of code is unnecessarily complicated, I
>>>>>>>>> don't see
>>>>>>>>
>>>>>>>>> a need for the 'boolean_var' dict here. I would suggest replacing it with
>>>>>>>>
>>>>>>>>> something like:
>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>> serial_autoincrement = "yes" if self.serial_autoincrement else "no"
>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>> and then pass serial_autoincrement to self.sub_dict = dict(...)
>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Attached patch refactored the relevant part of the code.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Tomas
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for patches! I am just thinking, should we also hide the respective
>>>>>>> option from ipa global DNS configuration? That's idnszonerefresh attribute.
>>>>>>>
>>>>>>> We may want to mark the attribute as invisible in CLI + remove it from Web UI.
>>>>>>> Petr - what is your take on this? Do you plan to remove idnszonerefresh
>>>>>>> attribute support in the future (Fedora 20) as persistent search will be
>>>>>>> mandatory in that time?
>>>>>>
>>>>>> Yes, you are right. We completely forgot to web UI. And yes - please remove the
>>>>>> option from web UI.
>>>>>
>>>>> Ok, Tomas please do the changes as proposed above.
>>>>>
>>>>>>
>>>>>> The latest development shows that persistent search will be replaced by RFC
>>>>>> 4533 (known as 'syncrepl'), but from user's point of view it doesn't matter.
>>>>>> All options related to persistent search and zone_refresh will simply
>>>>>> disappear. Syncrepl itself doesn't require explicit configuration.
>>>>>
>>>>> Ah, so this means that "psearch" option will be also removed from
>>>>> bind-dyndb-ldap? In Fedora 19 we just plan to hard-code it to "yes", will that
>>>>> cause issues with Fedora 20? Should we already avoid using the "psearch" option
>>>>> and assume that bind-dyndb-ldap in Fedora 19 is using persistent search by default?
>>>>
>>>> Won't the new bind-dyndb-ldap simply ignore the psearch option when it
>>>> moves to syncrepl ?
>>>
>>> I can do it, but I think that cleanest way is to remove the 'psearch' option
>>> in upgrade script.
>>
>> Sure, but if the upgrade, for whatever reason, fails to remove it
>> bind-dyndb-ldap should just ignore.
>>
>>> Another option is to release new bind-dyndb-ldap to Fedora 19 and change
>>> default values to 'psearch yes' right now. Do you agree?
>>
>> Too much churn, I think it is ok to change it when we are done with
>> syncrepl and upgrade config, with the fallback failsafe that even if
>> upgrade doesn't remove the option bind-dyndb-ldap will simply ignore it.
>>
>> This will be safer even for people using stuff like cfengine/puppet to
>> manage configurations and not realizing we changed the conf on upgrade,
>> their confsystems will push again a conf file with psearch yes but
>> bnid-dyndb-ldap won't break.
>>
>> Simo.
>>
>
> Hmm, that's right, it should be safer. Can bind-dyndb-ldap just yell to error
> log that there is an unknown configuration option? (if it does not do that
> already).

Yes, I can do that. Generally, bind-dyndb-ldap refuses to load invalid 
configuration, but I can handle psearch and related options as special-case 
(for some small number of releases).

I will release version 3.5 before end of this week. I have some small fixes 
ready so it is worth to release it now.

To summarize the discussion - please remove following options from 
configuration file and LDAP schema:
cache_ttl
psearch (attribute idnsPersistentSearch in idnsConfigObject)
zone_refresh (attribute idnsZoneRefresh in idnsConfigObject)

-- 
Petr^2 Spacek