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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky