[Freeipa-devel] [PATCH] ipa-client-install with --no-sssd option should check for nss_ldap

[Rob Crittenden]

Ondrej Hamada wrote:
> On 12/02/2011 04:16 PM, Rob Crittenden wrote:
>> Ondrej Hamada wrote:
>>> On 11/29/2011 10:33 PM, Rob Crittenden wrote:
>>>> Ondrej Hamada wrote:
>>>>> On 11/11/2011 02:55 PM, Ondrej Hamada wrote:
>>>>>> https://fedorahosted.org/freeipa/ticket/2063
>>>>>>
>>>>>> In order to check presence of nss_ldap when installing client with
>>>>>> '--no-sssd' option there was added code into ipa-client-install.
>>>>>> Check
>>>>>> is base on existence of nss_ldap configuration files. This
>>>>>> configuration could be in 'etc/ldap.conf', '/etc/nss_ldap.conf' or
>>>>>> '/etc/libnss_ldap.conf'. Presence of any of these files is considered
>>>>>> as success otherwise failure.
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Freeipa-devel mailing list
>>>>>> Freeipa-devel at redhat.com
>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>> I've rewritten it. Additionally it checks for existence of
>>>>> nss-pam-ldapd
>>>>> and makes the results reusable by configure_{ldap|nslcd}_conf()
>>>>> functions.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/2063
>>>>>
>>>>> In order to check presence of nss_ldap or nss-pam-ldapd when
>>>>> installing
>>>>> client
>>>>> with '--no-sssd' option there was added code into ipa-client-install.
>>>>> Checking is based on existence of nss_ldap configuration files. This
>>>>> configuration could be in 'etc/ldap.conf', '/etc/nss_ldap.conf' or
>>>>> '/etc/libnss_ldap.conf'. Optionaly the nss_ldap could cooperate with
>>>>> pam_ldap
>>>>> module and hence the presence of it is checked by looking for
>>>>> 'pam_ldap.conf' file.
>>>>> Existence of nss-pam-ldapd is checked against existence of
>>>>> 'nslcd.conf'
>>>>> file.
>>>>> All this checking is done by function nssldap_exists().
>>>>> Because both main modules are maintained by two different
>>>>> functions, the
>>>>> function
>>>>> returns tuple containing return code and dictionary structure - its
>>>>> key
>>>>> is name
>>>>> of target function and value is list of existing configuration files.
>>>>> Files to check are specified inside the nssldap_exists() function.
>>>>>
>>>>> In order to fit the returned values, the functions
>>>>> configure_{ldap|nslcd}_conf()
>>>>> were slightly modified. They accept one more parameter which is
>>>>> list of
>>>>> existing files.
>>>>> They are not checking existence of above mentioned files anymore.
>>>>
>>>> The patch looks good, just a couple of issues.
>>>>
>>>> 1. In the nslcd configurator you add ''.join(files). Did you mean
>>>> ','.join(files)?
>>>>
>>>> 2. The commit message lines wrap making it difficult to read. Can you
>>>> limit the lines to ~70 chars per line?
>>>>
>>>> 3. I think the message printed when neither package is available can
>>>> be simplified to:
>>>>
>>>> One of these packages must be installed: nss_ldap or nss-pam-ldapd
>>>>
>>>> It needs a rebase too.
>>>>
>>>> rob
>>> corrected, corrected, changed, rebased
>>>
>>>
>>>
>>> In order to check presence of nss_ldap or nss-pam-ldapd when
>>> installing client with '--no-sssd' option there was added
>>> code intoipa-client-install. Checking is based on existence
>>> of one of nss_ldap configuration files. This configuration
>>> could be in 'etc/ldap.conf', '/etc/nss_ldap.conf' or
>>> '/etc/libnss_ldap.conf'. Optionaly the nss_ldap could
>>> cooperate with pam_ldap module and hence the presence of it
>>> is checked by looking for 'pam_ldap.conf' file. Existence
>>> of nss-pam-ldapd is checked against existence of
>>> 'nslcd.conf' file. All this checking is done by function
>>> nssldap_exists(). Because both modules are maintained by
>>> two different functions, the function returns tuple
>>> containing return code and dictionary structure - its
>>> key is name of target function and value is list of
>>> existing configuration files. Files to check are specified
>>> inside the nssldap_exists() function.
>>>
>>> In order to fit the returned values, the functions
>>> configure_{ldap|nslcd}_conf() were slightly modified. They
>>> accept one more parameter which is list of existing files.
>>> They are not checking existence of above mentioned
>>> files anymore.
>>>
>>> https://fedorahosted.org/freeipa/ticket/2063
>>>
>>
>> Can you add a block header to nssldap_exists()? I think in particular
>> you need explain that it returns 1 and 0 because that value can
>> eventually be the return value of the installer itself (normally an
>> exists would return True/False).
> I've changed it to return True/False and added comment
>>
>> Seeing a traceback:
>>
>> # ipa-client-install --no-sssd
>>
>> [ snip ]
>>
>> Enrolled in IPA realm EXAMPLE.COM
>> Created /etc/ipa/default.conf
>> Configured /etc/krb5.conf for IPA realm EXAMPLE.COM
>> LDAP enabled
>> Kerberos 5 enabled
>> Traceback (most recent call last):
>> File "/usr/sbin/ipa-client-install", line 1294, in <module>
>> sys.exit(main())
>> File "/usr/sbin/ipa-client-install", line 1281, in main
>> rval = install(options, env, fstore, statestore)
>> File "/usr/sbin/ipa-client-install", line 1211, in install
>> (retcode, conf, filename) = configurer(fstore, cli_basedn, cli_realm,
>> cli_domain, cli_server, dnsok, options)
>> TypeError: configure_ldap_conf() takes exactly 8 arguments (7 given)
>>
>> rob
> corrected
>

ack, pushed to master