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

Mon Apr 29 20:28:20 UTC 2013

On 04/29/2013 08:13 PM, Rob Crittenden wrote:
> Tomas Babej wrote:
>> 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
>
> Ok, this works as advertised, I just have a general question.
>
> This could enable a mix of IPA and non-IPA settings. In my case I left 
> BASE configured and only URI and TLS_CACERT got set.
>
> This could cause some unexpected results I think, depending on what 
> got changed.
>
> Do we rather want to punt on all of them if any of them can't be 
> updated? This would require a bit more code, and wouldn't be as 
> generic. I just wonder if this would cause subtle failures.
>
> rob

After IRC conversation with Rob, we decided to keep the behaviour, while 
having it explicitly mentioned in the ldap.conf file.

For illustration, the ldap.conf file could look like this:

[/etc/openldap/ldap.conf]
# File modified by ipa-client-install

# We do not want to break your existing configuration, hence:
#   URI, BASE and TLS_CACERT have been added if they were not set.
#   In case any of them were set, a comment with trailing note
#   "# modified by IPA" note has been inserted.
# To use IPA server with openLDAP tools, please comment out your
# existing configuration for these options and uncomment the
# corresponding lines generated by IPA.

#
# LDAP Defaults
#

# See ldap.conf(5) for details
# This file should be world readable but not world writable.

#BASE dc=ipa,dc=example,dc=com # modified by IPA
BASE    dc=example,dc=com
#URI ldaps://ipa.example.com # modified by IPA
URI     ldap://ldap.example.com

#SIZELIMIT      12
#TIMELIMIT      15
#DEREF          never

TLS_CACERTDIR /etc/openldap/cacerts
TLS_CACERT /etc/ipa/ca.crt

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