[Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI

[thierry bordaz]

On 01/21/2016 05:04 PM, Martin Babinsky wrote:
> On 01/21/2016 01:37 PM, thierry bordaz wrote:
>>
>>
>>
> Hi Thierry,
>
> I have couple of comments to your patch:
>
> 1.)
> there is a number of PEP8 errors in the patch 
> (http://paste.fedoraproject.org/313246/33893701), please fix them.
>
> See http://www.freeipa.org/page/Python_Coding_Style for our 
> conventions used in Python code.
>
> 2.)
> +        DNA_BIND_METHOD   = "dnaRemoteBindMethod"
> +        DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
> +        DNA_PLUGIN_DN     = 'cn=Distributed Numeric Assignment 
> Plugin,cn=plugins,cn=config'
> +        dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN
>
> Uppercase names are usually reserved for module-level constants. OTOH, 
> local variables should be lowercase. Also you can instantiate 
> dna_config_base directly as DN, using 2-member tuples, i. e:
>
> """
> dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric 
> Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
> """
>
> When passing DN object to the formatting functions/operators, it is 
> automatically converted to string so no need to hold string and DN 
> object separately. This is done in other places (see function/methods 
> in replication.py).
>
> 3.)
>
> +        for i in range(len(entries)) :
> +
> +            mod = []
> +            if entries[i].single_value.get(DNA_BIND_METHOD) != method:
> +                mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD, method))
> +
> +            if entries[i].single_value.get(DNA_CONN_PROTOCOL) != 
> protocol:
> +                mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL, 
> protocol))
>
>
> please use idiomatic Python when handling list of entries, e.g.:
>
> """
> for entry in entries:
>    mod = []
>    if entry.single_value.get(DNA_BIND_METHOD) != method:
>    ...
> """
>
> 4.) I think that this method should in DSInstance class since it deals 
> with directory server configuration. Service is a parent object of all 
> other service installers/configurators and should contain only methods 
> common to more children.
>
> 5.) Since the method is called from every installer, it could be 
> beneficial to call it in DSInstance.__common_post_setup() as a part of 
> Directory server installation. Is there any reason why this is not the 
> case?
>
> 6.)
>
> +        while attempt != MAX_WAIT:
> +            try:
> +                entries = conn.get_entries(sharedcfgdn, 
> scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
> +                break
> +            except errors.NotFound:
> +                root_logger.debug("So far enable not find DNA shared 
> config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn, 
> sharedcfgdn))
> +                attempt = attempt + 1
> +                time.sleep(2)
> +                continue
> +
> +        # safety checking
> +        # there is no return, if there are several entries, as a 
> workaround of #5510
> +        if len(entries) != 1:
>
> I am quite afraid what would happen if the server does not return any 
> entries until 30 s timeout. The code will then continue to the 
> condition which can potentially test an uninitialized variable and 
> blow up with 'NameError'. This should be handled more robustly, e. g. 
> raise an exception when a timeout is reached and no entries were 
> returned.
>
> 7.)
>
> +            if len(mod) > 0:
>
> A Pythonic way to test for non-empty container is
>
> """
> if mods:
>    # do stuff
> """
>
> since an empty list/dict/set evaluates to False and non-empty 
> containers are True.
>
>
> 8.)
>
> +                entry = conn.get_entry(entries[i].dn)
> +                if entry.single_value.get(DNA_BIND_METHOD) != method:
> +                    root_logger.error("Fail to set SASL/GSSAPI bind 
> method to %s" % (entries[i].dn))
> +                if entry.single_value.get(DNA_CONN_PROTOCOL) != 
> protocol:
> +                    root_logger.error("Fail to set LDAP protocol to 
> %s" % (entries[i].dn))
>
> rather than re-fetching the modified entry and testing what happened, 
> you can just catch an exception raised by unsuccessfull mod and log an 
> error like this:
>
> """
> try:
>    conn.modify_s(entry.dn, mod)
> except Exception as e:
>    root_logger.error("Failed to modify entry {}: {}".format(entry, e))
> """
>
> as a matter of fact, if the modify_s operation would fail for some 
> reason, an ldap exception would be raised and you checks would not 
> even be executed.
>
> 9.)
> The debug message on line 219 should read "Unable to find DNA shared 
> config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors 
> at the end of the method should have "Failed" instead of "Fail".
>
Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160224/91accafd/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbordaz-0017-2-configure-DNA-plugin-shared-config-entries-to-allow-.patch
Type: text/x-patch
Size: 8010 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160224/91accafd/attachment.bin>