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

[thierry bordaz]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160229/a784f332/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-43-tbordaz-0017-4-configure-DNA-plugin-shared-config-entries-to-allow-.patch
Type: text/x-patch
Size: 8263 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160229/a784f332/attachment.bin>