[Freeipa-devel] [PATCH 0058] ipa-restore: check whether DS is running before attempting connection

[Martin Babinsky]

On 08/18/2015 02:51 PM, Jan Cholasta wrote:
> On 18.8.2015 14:24, Martin Babinsky wrote:
>> On 08/18/2015 02:05 PM, Jan Cholasta wrote:
>>> On 18.8.2015 13:41, Martin Basti wrote:
>>>>
>>>>
>>>> On 08/18/2015 01:16 PM, Alexander Bokovoy wrote:
>>>>> On Tue, 18 Aug 2015, Martin Babinsky wrote:
>>>>>> https://fedorahosted.org/freeipa/ticket/4838
>>>>>>
>>>>>> --
>>>>>> Martin^3 Babinsky
>>>>>
>>>>>> From d86aae6c3fef4dea1afbbdbacbc978afbbfa5fcf Mon Sep 17 00:00:00
>>>>>> 2001
>>>>>> From: Martin Babinsky <mbabinsk at redhat.com>
>>>>>> Date: Tue, 18 Aug 2015 12:47:46 +0200
>>>>>> Subject: [PATCH] ipa-restore: check whether DS is running before
>>>>>> attempting
>>>>>> connection
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4838
>>>>>> ---
>>>>>> ipaserver/install/ipa_restore.py | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/ipaserver/install/ipa_restore.py
>>>>>> b/ipaserver/install/ipa_restore.py
>>>>>> index
>>>>>> 65cb49355a0567446debe9c367aa3c1bc5a12e1c..b69ea90001a6fd03d0fddde8844870d1aa9c3704
>>>>>>
>>>>>>
>>>>>>
>>>>>> 100644
>>>>>> --- a/ipaserver/install/ipa_restore.py
>>>>>> +++ b/ipaserver/install/ipa_restore.py
>>>>>> @@ -410,6 +410,13 @@ class Restore(admintool.AdminTool):
>>>>>>         '''
>>>>>>         Create an ldapi connection and bind to it using autobind as
>>>>>> root.
>>>>>>         '''
>>>>>> +        instance_name =
>>>>>> installutils.realm_to_serverid(api.env.realm)
>>>>>> +
>>>>>> +        if not
>>>>>> services.knownservices.dirsrv.is_running(instance_name):
>>>>>> +            raise admintool.ScriptError(
>>>>>> +                "directory server instance is not
>>>>>> running/configured"
>>>>>> +            )
>>>>>> +
>>>>>>         if self._conn is not None:
>>>>>>             return self._conn
>>>>>>
>>>>> ACK.
>>>>>
>>>> Pushed to:
>>>> master: 31776755b47f44f894e4f2bb256aca1c5262b8a8
>>>> ipa-4-2: e4b8cffdb4e017874bb7f12a7ca362b927ead67a
>>>>
>>>
>>> I didn't try, but I'm pretty sure this patch breaks ipa-restore on
>>> systems without IPA installed, which was not at all the point of the
>>> ticket - the point was to replace the "Unable to get connection,
>>> skipping disabling agreements: Unable to bind to LDAP server: [Errno 2]
>>> No such file or directory" error message with something meaningful in
>>> such a case.
>>>
>>
>> I have just now tested ipa-restore without installed IPA master and it
>> works just fine.
>>
>> The point of my patch was no not even try to connect to DS if the
>> instance is not running/configured and raise an error telling this fact
>> to the user, instead of timing out on connection and then raising a
>> generic exception.
>>
>> So unless I missed something it should be ok.
>>
>
> I stand corrected.
>
> It was the ScriptError that made me suspicious.
>
> Sorry for the noise.
>

Yeah in retrospect it was not the best exception to raise in this case.

I was probably influenced by the fact that ScriptError gets thrown 
around so much in the ipa_restore code and got a bit lazy :).

-- 
Martin^3 Babinsky