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

[Rob Crittenden]

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).

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