Re: [PATCH] mod_unique_id: use ap_random_insecure_bytes() to get unique ID

[Stefan Fritsch <sf...@sfritsch.de>]

On Wednesday 26 June 2013, Jan Kaluža wrote:
> currently mod_unique_id uses apr_gethostname(...) and PID pair as a
> base to generate unique ID. The way how it's implemented brings
> some problems:
> 
> 1. For IPv6-only hosts it uses low-order bits of IPv6 address as if
> they were unique, which is wrong.
> 
> 2. It relies on working DNS. It can happen that hostname does not
> have the IP assigned during httpd start (for example during the
> boot) and I think it is still valid use-case (without
> mod_unique_id module loaded, httpd works well in this case).
> 
> 3. It calls 1 second sleep to overcome possible usage of the same
> PID after restart as the one used before the restart.
> 
> If I'm right, we could fix the problems above by using
> ap_random_insecure_bytes instead of "in_addr"/"pid" pair as a base
> for unique ID generation. It would also make the code simpler. I
> think the randomness generated by ap_random_insecure_bytes() is at
> least the same as the one introduced by apr_gethostname() + pid
> pair.
> 
> The attached patch implements it by removing in_addr/pid fields
> unique_id_rec struct and introduces new "root" field which is
> initialized using ap_random_insecure_bytes() function and is used
> as a base for unique IDs.

I agree in principle, but I would prefer the ID length to not increase 
that much. You replace 8 bytes ip+pid with 20 bytes "root", which 
means 16 bytes increase in base64 format. The unique id is also used 
as a request log id and having an additional 16 bytes of prefix in the 
error log (if one uses that feature) does not really appeal to me.

But 20 bytes may be excessive, anyway: If one assumes 10000 servers 
over which the IDs needs to be unique, and uses a 8 byte "root" field, 
the probability for a collision is 3*10^(-12) if I did the math 
correctly. This still has to be multiplied with the number of restarts 
of the servers, so to be save we could use a 10 byte "root" field, 
giving a collision probability (per server restart) of 4*10^(-17), 
which should be ok.

Re: [PATCH] mod_unique_id: use ap_random_insecure_bytes() to get unique ID

[Jan Kaluža <jk...@redhat.com>]

On 07/10/2013 06:24 PM, Joe Orton wrote:
> On Tue, Jul 09, 2013 at 11:02:18PM +0200, Stefan Fritsch wrote:
>> On Tuesday 09 July 2013, Joe Orton wrote:
>>> On Tue, Jul 09, 2013 at 10:00:19AM +0200, Jan Kaluza wrote:
>>>> I agree 20 bytes could be too much. I have changed my patch to
>>>> have only 10 bytes long root. I will check the Daniel's ideas
>>>> mentioned in another mail in this thread and try to implement
>>>> it, but if we are going to do it my way, I think this patch
>>>> should be OK.
>>>
>>> +1 here, this looks good to me.
>>
>> +1
>
> Thanks for the review Stefan & thanks for the patch Jan!
>
> => http://svn.apache.org/viewvc?view=revision&revision=1501827
>
> This patch actually removes the noticeable delay when running the test
> suite, while A::T waits for httpd to start serving requests!  I had no
> idea that was caused by mod_unique_id.  Very cool :)
>
> Regards, Joe
>

Thank you all for reviewing and accepting the patch.

Regards,
Jan Kaluza

Re: [PATCH] mod_unique_id: use ap_random_insecure_bytes() to get unique ID

[Joe Orton <jo...@redhat.com>]

On Tue, Jul 09, 2013 at 11:02:18PM +0200, Stefan Fritsch wrote:
> On Tuesday 09 July 2013, Joe Orton wrote:
> > On Tue, Jul 09, 2013 at 10:00:19AM +0200, Jan Kaluza wrote:
> > > I agree 20 bytes could be too much. I have changed my patch to
> > > have only 10 bytes long root. I will check the Daniel's ideas
> > > mentioned in another mail in this thread and try to implement
> > > it, but if we are going to do it my way, I think this patch
> > > should be OK.
> > 
> > +1 here, this looks good to me.
> 
> +1

Thanks for the review Stefan & thanks for the patch Jan!

=> http://svn.apache.org/viewvc?view=revision&revision=1501827

This patch actually removes the noticeable delay when running the test 
suite, while A::T waits for httpd to start serving requests!  I had no 
idea that was caused by mod_unique_id.  Very cool :)

Regards, Joe

Re: [PATCH] mod_unique_id: use ap_random_insecure_bytes() to get unique ID

[Stefan Fritsch <sf...@sfritsch.de>]

On Tuesday 09 July 2013, Joe Orton wrote:
> On Tue, Jul 09, 2013 at 10:00:19AM +0200, Jan Kaluza wrote:
> > I agree 20 bytes could be too much. I have changed my patch to
> > have only 10 bytes long root. I will check the Daniel's ideas
> > mentioned in another mail in this thread and try to implement
> > it, but if we are going to do it my way, I think this patch
> > should be OK.
> 
> +1 here, this looks good to me.

+1

> Any objections to this approach?

Re: [PATCH] mod_unique_id: use ap_random_insecure_bytes() to get unique ID

[Joe Orton <jo...@redhat.com>]

On Tue, Jul 09, 2013 at 10:00:19AM +0200, Jan Kaluza wrote:
> I agree 20 bytes could be too much. I have changed my patch to have
> only 10 bytes long root. I will check the Daniel's ideas mentioned
> in another mail in this thread and try to implement it, but if we
> are going to do it my way, I think this patch should be OK.

+1 here, this looks good to me.  Any objections to this approach?

Regards, Joe

Re: [PATCH] mod_unique_id: use ap_random_insecure_bytes() to get unique ID

[Jan Kaluža <jk...@redhat.com>]

On 07/06/2013 01:58 AM, Stefan Fritsch wrote:
> On Wednesday 26 June 2013, Jan Kaluža wrote:
>> currently mod_unique_id uses apr_gethostname(...) and PID pair as a
>> base to generate unique ID. The way how it's implemented brings
>> some problems:
>>
>> 1. For IPv6-only hosts it uses low-order bits of IPv6 address as if
>> they were unique, which is wrong.
>>
>> 2. It relies on working DNS. It can happen that hostname does not
>> have the IP assigned during httpd start (for example during the
>> boot) and I think it is still valid use-case (without
>> mod_unique_id module loaded, httpd works well in this case).
>>
>> 3. It calls 1 second sleep to overcome possible usage of the same
>> PID after restart as the one used before the restart.
>>
>> If I'm right, we could fix the problems above by using
>> ap_random_insecure_bytes instead of "in_addr"/"pid" pair as a base
>> for unique ID generation. It would also make the code simpler. I
>> think the randomness generated by ap_random_insecure_bytes() is at
>> least the same as the one introduced by apr_gethostname() + pid
>> pair.
>>
>> The attached patch implements it by removing in_addr/pid fields
>> unique_id_rec struct and introduces new "root" field which is
>> initialized using ap_random_insecure_bytes() function and is used
>> as a base for unique IDs.
>
> I agree in principle, but I would prefer the ID length to not increase
> that much. You replace 8 bytes ip+pid with 20 bytes "root", which
> means 16 bytes increase in base64 format. The unique id is also used
> as a request log id and having an additional 16 bytes of prefix in the
> error log (if one uses that feature) does not really appeal to me.

Thanks for reviewing the patch.

I agree 20 bytes could be too much. I have changed my patch to have only 
10 bytes long root. I will check the Daniel's ideas mentioned in another 
mail in this thread and try to implement it, but if we are going to do 
it my way, I think this patch should be OK.

> But 20 bytes may be excessive, anyway: If one assumes 10000 servers
> over which the IDs needs to be unique, and uses a 8 byte "root" field,
> the probability for a collision is 3*10^(-12) if I did the math
> correctly. This still has to be multiplied with the number of restarts
> of the servers, so to be save we could use a 10 byte "root" field,
> giving a collision probability (per server restart) of 4*10^(-17),
> which should be ok.
>

Regards,
Jan Kaluza