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

[Martin Babinsky]

On 02/26/2016 04:24 PM, thierry bordaz wrote:
> On 02/25/2016 07: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. 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
>
> Hi Martin,
>
> Following your recommendation it is an updated patch to not check/update
> shared config entry in DSinstance.__post_common_setup().
> In fact at this step, DNA plugin is disabled and such the check would be
> a no-op.
>
> thanks
> thierry

Thanks Thierry,

the patch will need a rebased version which applies cleanly on top of 
ipa-4-3 branch, but otherwise ACK.

-- 
Martin^3 Babinsky