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

Tue Mar 1 13:50:31 UTC 2016

On 02/29/2016 05:37 PM, thierry bordaz wrote:
> On 02/26/2016 05:48 PM, Martin Babinsky wrote:
>> 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.
>>
> Thanks Martin for all the reviews.
>
> Here is the patch for ipa-4.3
>
> thnaks
> thierry

Thanks, ACK.

-- 
Martin^3 Babinsky