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

[Martin Babinsky]

On 02/25/2016 12:17 PM, thierry bordaz wrote:
> 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.
>
> 8-)
>> 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"
>
> I did it and fixed most of them but what remains is long line. I fixed
> some of them but sometime I think it make the code more complex to read.
> Should I split those lines anyway ?
>
There are some lines that are over 120 characters long and that's really 
too much. See my quick patch which I attached to make the long lines 
pep8-conformant. I also fixes some redundant parentheses in the format 
string and replaces raised exception with logger error.
>>
>> 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.
>
> Right. I will do.
>
>>
>> 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.
> ok I understand. I will fix that
>
> Thanks Martin
>
>
+            # a server waits for 30s after its startup to update its 
shared config
+            max_wait = 30
+            for i in range(0, max_wait + 1):

I also realized that we have 30s timeout in the comment here, bu we 
actually wait for 60 seconds (30 * time.sleep(2)). We should fix the 
comment as to not confuse our future selves.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: long_line_format.patch
Type: text/x-patch
Size: 4598 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160225/0ce9b045/attachment.bin>