[Freeipa-devel] [PATCH 0051] Preserve already configured options in openldap conf

[Tomas Babej]

On 04/25/2013 12:42 PM, Martin Kosek wrote:
> On 04/25/2013 12:29 PM, Jan Cholasta wrote:
>> On 25.4.2013 08:51, Martin Kosek wrote:
>>> On 04/24/2013 08:02 PM, Rob Crittenden wrote:
>>>> Jan Cholasta wrote:
>>>>> On 24.4.2013 14:54, Martin Kosek wrote:
>>>>>> On 04/24/2013 02:51 PM, Rob Crittenden wrote:
>>>>>>> Jan Cholasta wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 23.4.2013 12:28, Tomas Babej wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> We should respect already configured options present in
>>>>>>>>> /etc/openldap/ldap.conf when generating our own configuration.
>>>>>>>>> With this patch, we only rewrite URI, BASE and TLS_CACERT 
>>>>>>>>> options.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3582
>>>>>>>>>
>>>>>>>>
>>>>>>>> the changeConf call will fail when the file does not exist, we 
>>>>>>>> might
>>>>>>>> want to handle that gracefully.
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>>
>>>>>>> We also need to handle the case where these items are already
>>>>>>> defined. I'm
>>>>>>> honestly not sure what the behavior should be: overwrite, warn and
>>>>>>> overwrite,
>>>>>>> fail.
>>>>>>>
>>>>>>> rob
>>>>>>>
>>>>>>
>>>>>> I am also thinking that we may want to be more cautious before
>>>>>> updating this
>>>>>> file. AFAIK, we do not need the updated file for our function, its
>>>>>> only updated
>>>>>> for user convenience so that he can run ldapsearches more easily.
>>>>>>
>>>>>> I see several options here that could help this goal:
>>>>>> 1) Update ldap.conf if BASE and URI and TLS_CACERT only if these
>>>>>> options are
>>>>>> not set. If the options are already set, we could just print a note
>>>>>> that we
>>>>>> skipped it. When I see my vanilla /etc/openldap/ldap.conf, it has
>>>>>> these options
>>>>>> commented out, so it should be possible to implement this check.
>>>>>>
>>>>>> 2) Do ldap.conf changes only if a new special option is passe (e.g.
>>>>>> --configure-ldap-cong)
>>>>>>
>>>>>> 3) Do not update ldap.conf when a new special option is not 
>>>>>> passed (e.g.
>>>>>> --no-ldap-conf
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>
>>>>> If we don't need the file for our function, we can just not 
>>>>> configure it
>>>>> at all IMO. We can document how to configure it for users who want 
>>>>> it.
>>>>
>>>> It was an RFE that we create this file. It is handy to have 
>>>> pre-configured, I
>>>> like having it actually.
>>>>
>>>> We just need to try to have a gentler touch than my first crack at 
>>>> it, which
>>>> overwrote it completely. I think #1 is probably enough for now. I'm 
>>>> not sure I
>>>> want to add two new options this late in the game, and the client 
>>>> already has a
>>>> lot of knobs.
>>>>
>>>> rob
>>>>
>>>
>>> Yeah, I also agree that 1) is enough. It will not add any more 
>>> options and will
>>> let us be more gentle and respectful to already existent custom user 
>>> settings
>>> in ldap.conf. So Tomas, this seems like the way to go :-)
>>>
>>> Martin
>>>
>>
>> I don't see the point of updating only some of these values. What about
>
> Not some of them - either all of them (BASE, URI, TLS_CACERT) when 
> none of them is already configured or none at all.
>
>>
>> 4) Update BASE and URI and TLS_CACERT, comment any old settings out.
>>
>> ?
>
> This would still break an existing user configuration, we would just 
> tell user what we broke :-)
>
> Martin
>
>>
>> Honza
>>
>
The following version of the patch configures (BASE, URI, TLS_CACERT) 
attributes if they are not set.

However, to preserve user-friendliness, our suggested option is added as 
an comment. See commit message for details.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0051-2-Preserve-already-configured-options-in-openldap-conf.patch
Type: text/x-patch
Size: 5775 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130429/a6fd1270/attachment.bin>