[Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop

[Tomas Babej]

On 06/12/2014 10:45 AM, Martin Kosek wrote:
> On 06/11/2014 06:49 PM, Nathaniel McCallum wrote:
>> On Wed, 2014-06-11 at 11:08 +0200, Tomas Babej wrote:
>>> Hi,
>>>
>>> As due to possible race conditions, the preop.pin might not be
>>> written in the CS.cfg at the time installer tries to read it.
>>>
>>> In case no value for preop.pin was found, retry until timeout
>>> was reached.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3382
>>>
>>> (applies on ipa-3-0 branch)
>> There is inconsistent spacing around '='. For instance:
>> +            f=open(filename, 'r')
>> +            data = f.read()
>>
>> Also, I'm fairly certain this is incorrect:
>> +                total_timeout =+ 1
>>
>> Nathaniel
> +1, this is certainly is not a deterministic, constant measuring of a timeout.

Fixed.

>
> I would rather see something like
>
>     op_timeout = time.time() + api.env.startup_timeout
>
>     while preop_pin is None and time.time() < op_timeout:
>
>
> Also, I do not think this will work, IOError is risen when a file is not found,
> so this code would not solve the problem if waiting on file to appear.

Yes, but the problem was not in being unable to open the file, but that
certain file content
is not in the file yet. It seems that pkicreate creates the file in
time, but the content written
later and that causes the race condition.

If you inspect the original code, and bugzilla, you can see that
installation fails with:

2013-01-25T02:58:44Z INFO The ipa-server-install command failed, exception: 
RuntimeError: Unable to find preop.pin in /var/lib/pki-ca/conf/CS.cfg. Is your CA already configured?

While the original code

    # read the config file and get the preop pin
   
try:                                                                                                   

       
f=open(filename)                                                                                   

    except IOError,
e:                                                                                     

        root_logger.error("Cannot open configuration file." +
str(e))                                      
        raise
e                                                                                            

    data =
f.read()                                                                                        

    data =
data.split('\n')                                                                                

    pattern = re.compile("preop.pin=(.*)"
)                                                                
    for line in
data:                                                                                      

        match = re.search(pattern,
line)                                                                   
        if
(match):                                                                                        

           
preop_pin=match.group(1)                                                                       

            break 
    if preop_pin is None:
        raise RuntimeError("Unable to find preop.pin in %s. Is your CA
already configured?" % filename)
 
does raise the IOError in case the file was not able to be opened. Since
we get the Runtime error,
file must exist, only the desired content is missing.

That said, I have no objections to amending the patch so that it
properly handles this race condition
we did not encounter. Better safe than sorry, it's a simple change and
already included in the
current iteration of the patch.

Updated patch attached.

>
> Martin

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0224-2-cainstance-Read-CS.cfg-for-preop.pin-in-a-loop.patch
Type: text/x-patch
Size: 2856 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140612/2e08a546/attachment.bin>