[Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
Martin Basti
mbasti at redhat.com
Wed Mar 2 15:46:03 UTC 2016
On 01.03.2016 14:50, Martin Babinsky wrote:
> 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.
>
Pushed to master: 6851e560dd1c9f4df98fd6b9d3063cd7dcc3bafc
Pushed to ipa-4-3: 4531eaedfbc45bd8b1d11ebda48b92d1589ad1b3
More information about the Freeipa-devel
mailing list