[Freeipa-devel] [PATCHES 0018-0020] ipa-dns-install: Use LDAPI for all DS connections

[Tomas Babej]

On 03/16/2015 05:01 PM, Martin Basti wrote:
> On 16/03/15 14:26, Martin Babinsky wrote:
>> On 03/16/2015 01:44 PM, Martin Basti wrote:
>>> On 12/03/15 17:15, Martin Babinsky wrote:
>>>> On 03/12/2015 03:59 PM, Martin Babinsky wrote:
>>>>> On 03/11/2015 03:13 PM, Martin Basti wrote:
>>>>>> On 11/03/15 13:00, Martin Babinsky wrote:
>>>>>>> These patches solve https://fedorahosted.org/freeipa/ticket/4933.
>>>>>>>
>>>>>>> They are to be applied to master branch. I will rebase them for
>>>>>>> ipa-4-1 after the review.
>>>>>>>
>>>>>> Thank you for the patches.
>>>>>>
>>>>>> I have a few comments:
>>>>>>
>>>>>> IPA-4-1
>>>>>> Replace simple bind with LDAPI is too big change for 4-1, we should
>>>>>> start TLS if possible to avoid MINSSF>0 error. The LDAPI patches 
>>>>>> should
>>>>>> go only into IPA master branch.
>>>>>>
>>>>>> You can do something like this:
>>>>>> --- a/ipaserver/install/service.py
>>>>>> +++ b/ipaserver/install/service.py
>>>>>> @@ -107,6 +107,10 @@ class Service(object):
>>>>>>                   if not self.realm:
>>>>>>                       raise errors.NotFound(reason="realm is missing
>>>>>> for
>>>>>> %s" % (self))
>>>>>>                   conn = ipaldap.IPAdmin(ldapi=self.ldapi,
>>>>>> realm=self.realm)
>>>>>> +            elif self.dm_password is not None:
>>>>>> +                conn = ipaldap.IPAdmin(self.fqdn, port=389,
>>>>>> + cacert=paths.IPA_CA_CRT,
>>>>>> +                                       start_tls=True)
>>>>>>               else:
>>>>>>                   conn = ipaldap.IPAdmin(self.fqdn, port=389)
>>>>>>
>>>>>>
>>>>>> PATCH 0018:
>>>>>> 1)
>>>>>> please add there more chatty commit message about using LDAPI
>>>>>>
>>>>>> 2)
>>>>>> I do not like much idea of adding 'realm' kwarg into __init__ 
>>>>>> method of
>>>>>> OpenDNSSECInstance
>>>>>> IIUC, it is because get_masters() method, which requires realm to 
>>>>>> use
>>>>>> LDAPI.
>>>>>>
>>>>>> You can just add ods.realm=<realm>, before call get_master() in
>>>>>> ipa-dns-install
>>>>>>      if options.dnssec_master:
>>>>>> +        ods.realm=api.env.realm
>>>>>>          dnssec_masters = ods.get_masters()
>>>>>> (Honza will change it anyway during refactoring)
>>>>>>
>>>>>> PATCH 0019:
>>>>>> 1)
>>>>>> commit message deserves to be more chatty, can you explain there why
>>>>>> you
>>>>>> removed kerberos cache?
>>>>>>
>>>>>> Martin^2
>>>>>>
>>>>>
>>>>> Attaching updated patches.
>>>>>
>>>>> Patch 0018 should go to both 4.1 and master branches.
>>>>>
>>>>> Patch 0019 should go only to master.
>>>>>
>>>>>
>>>>>
>>>>
>>>> One more update.
>>>>
>>>> Patch 0018 is for both 4.1 and master.
>>>> Patch 0019 is for master only.
>>>>
>>>>
>>>>
>>> Thank for patches
>>> Patch 0018:
>>> 1)
>>> Works for me but needs rebase on master
>>>
>>> Patch 0019:
>>> 1)
>>> Please rename the patch/commit message, the patch changes only
>>> ipa-dns-install connections not all DS operations
>>>
>>> 2)
>>> I have some troubles with applying patch, it needs rebase due 0018
>>>
>>>
>>> -- 
>>> Martin Basti
>>>
>>
>> Attaching updated patches:
>>
>> Patch 0018 is for ipa-4-1 branch.
>> Patches 0019 and 0020 are for master branch.
>>
>> I hope they will apply cleanly this time (they did for me).
>>
> ACK
>
Pushed to ipa-4-1 and master.

Please do not bump the base patch number for different (rebased) 
versions of the same patch.