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

Thu Feb 25 18:17:18 UTC 2016

On 02/25/2016 12:03 PM, Martin Babinsky wrote:
> On 02/24/2016 04:30 PM, thierry bordaz wrote:
>> 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
>
> Hi Thierry,
>
> the patch works as expected. I have some more nitpicks though:
>
> 1.) Please fix the following pep8 errors:
>
> http://paste.fedoraproject.org/329086/56397841/
>
> you can check whether you recent commit conforms to PEP8 by running
>
> "git show -U0 | pep8 --diff"
>
> 2.)
> +        self.step("update DNA shared config entry", 
> self.update_dna_shared_config)
>
> I would rather change the message to "Updating DNA shared config 
> entry" since all other messages use continuous tense.
>
> 3.)
> +            else:
> +                raise RuntimeError("Could not get dnaHostname entries 
> in {} seconds".format(max_wait * 2))
>
> Please use root_logger.error() and then return as is used elsewhere in 
> the method. We do not want a runaway exception crashing upgrade.
>
Hi Martin,

Updated/tested the patch with your help/recommendations. pep8 is clear 
now :-)

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