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

[Nathaniel McCallum]

On Thu, 2014-06-12 at 13:29 +0200, Tomas Babej wrote:
> 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.

I think I would prefer something like inotify watching for
IN_CLOSE_WRITE rather than this polling.

Nathaniel