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

[Rob Crittenden]

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