On Fri, Mar 22, 2024 at 09:45:59AM +0000, Damien Claisse wrote: > Hi Amaury! > Sorry for the HTML message, I have to use a *** well-known enterprise MUA :( > Le 22/03/2024 09:09, « Amaury Denoyelle » <adenoye...@haproxy.com> a écrit : >> This patch raises several interrogations. First, dynamic servers are >> currently intended to not support cookies, hence why the keyword is >> disabled for them. It has been done as a convenience but maybe it would >> be a good time to review it carefully and see if whole cookie support >> can be enabled. > Indeed, there could be an opportunity to revisit this. What we observed is > that, even with dynamic servers, calling “set server bkd1/srv1 addr a.b.c.d” > would re-add the dynamic cookie for this server, and calling “enable > dynamic-cookie backend bkd1” would re-compute cookie for all servers in the > backend. It is expected as in these calls code path there is a call to > srv_set_dyncookie(). So there currently is at least a partial support for > dynamic cookies on dynamic servers, even if it’s not expected :) >> Second, I'm unsure srv_set_dyncookie() should be called on >> _srv_parse_init(). This function is also called for configuration file >> servers. In particular, I do not know how we should handled duplicate >> cookie values in this case. > Not sure we really create duplicates here as we basically reset the same > cookie on the server, not on another one in the backend, at least I didn’t > observe such warnings in my logs while testing this patch yet. But I agree > that in the context of haproxy startup there would be 2 calls to > srv_set_dyncookie() per server which is useless and could create issues. > Maybe I could move that at the end of cli_parse_add_server()? Feel free to > suggest any better option.
Okay, I had the time to review dynamic cookie handling. For me it's fine to use srv_set_dyncookie() for dynamic servers. I think however its new invocation should be placed near the end of cli_parse_add_server(). Maybe with an extra check (be->ck_opts & PR_CK_DYNAMIC) to be similar with invocation in check_config_validity(). This location will prevent duplicate invocation of srv_set_dyncookie() for static servers, as for them the function must be called on post parsing to ensure the backend key has been parsed. Could you please adjust your patch ? Also, as it is a bugfix, you can specify that it must be backported up to 2.4. In the meantime, I will continue code inspection to determine if cookie keyword can also be enabled for dynamic servers as well. Regards, -- Amaury Denoyelle
