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

Tue Apr 30 14:54:41 UTC 2013

On 04/29/2013 10:28 PM, Tomas Babej wrote:
> 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
>
[...]
> +        root_logger.info("Could not parse {path}".format(path=target_fname))
> +        root_logger.debug(error_msg.format(path=target_fname, err=str(e)))

To my (limited) knowledge, this would be the first Python 2.6+ feature 
used in the client code. Is it OK?

Normally we use multiple arguments for logging:
root_logger.info("Could not parse %s", target_fname)

Patch works as designed (you get a mix of IPA and non-IPA settings if 
only some of the options are pre-existing).

-- 
Petr³