Willy,
Am 27.02.20 um 13:32 schrieb Willy Tarreau:
> On Thu, Feb 27, 2020 at 12:50:44PM +0100, Tim Düsterhus wrote:
>>>> /* add unique-id if "header-unique-id" is specified */
>>>> + if (sess->fe->header_unique_id &&
>>>> !LIST_ISEMPTY(&sess->fe->format_unique_id)) {
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> All the unique-id generation seems to be enclosed in this. Am I missing
>>> something ?
>>
>> Yes. The `header_unique_id` check is only in `http_process_request`.
>> I've merged the previous two ifs into a single one. A unique ID in
>> `http_process_request` is only generated when it is actually sent to the
>> upstream server. If it is not sent we don't need to generate one,
>> because no one is seeing it. This might technically be a slight behavior
>> change (the unique ID is generated later), but the user won't see a
>> difference.
>
> Thank for the explanation (this should be part of the commit message
> so that someone bisecting to that commit later doesn't ask the question
> again).
It looks like I need to change this anyway. I guess I can disregard this
then?
>> By replacing all the ad-hoc generation of the unique IDs with a call to
>> `stream_generate_unique_id` the unique ID will be generated when it is
>> first used. This is happening in this order:
>>
>> 1. unique-id-header
>> 2. unique-id sample fetch
>> 3. Logging
>
> I'm just having a doubt now. What if the unique-id-format refernces
> some part of the request ? Let's say I have this :
>
> unique-id-format %ci_%cp_%[req.hdr(host),crc32,hex]
>
> And in my logs I have "%ID".
>
> Does this mean that the unique-id will now be generated only when
> logging ? Because if that's the case it won't find any element from
> the request anymore.
If neither the unique-id-header is set, nor the unique-id sample fetch
is used then this is correct. I'm not sure how useful an ID that is
never referenced elsewhere is, but this indeed would be a breaking
change. I'll have a look.
>> I was thinking about this, but didn't make the change to keep the diff
>> small and because it increases the size of a stream by an additional
>> `size_t` which I'm not sure is desired. I can create a follow up patch
>> if you want.
>
> It would be nice. We're trying to progressively clean such things from
> the code. There are still a lot of them, cookie names and whatever, over
> which we run strlen for each and every use case, or we have a separate
> length and do manual operations to keep them in sync while we already
> have everything needed to manipulate them directly.
Okay, sure.
Best regards
Tim Düsterhus
