From ccd1ecba1daa0cfc3403b6497f8606921400bc49 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 20 Sep 2024 16:10:52 +0200 Subject: [PATCH] MEDIUM: cfgparse: drop duplicate named defaults sections after use It has never been permitted to explicitly reference named defaults sections for which there are duplicate names. This means that when a duplicate defaults section is found, there's no point in keeping it since it will never be used for lookups, so it can be dropped. However, some such defaults sections might have some rules in them that are implicitly referenced by proxies placed after them. In this case they cannot be removed. What is done here is that upon each new named section creation, if another one is found with the same name, its config location is stored into the new proxy's {prev_file,prev_line} pair, and the old section is either destroyed if its refcount is null, or just unindexed. The dup check when creating a new proxy now consists in checking the prev_line instead of performing a dup lookup on the defaults section. This will guarantee that we can't find duplicate defaults sections in their tree anymore, while still keeping track of what's allocated and releasing everything upon exit. Beyond the consistency gain, there are nice savings for large configs involving many defaults sections: a test with 300k sections saved about 1.9 GB of RAM, and started 25% faster likely thanks to spending less time allocating memory. --- include/haproxy/proxy-t.h | 4 +++- src/cfgparse-listen.c | 26 ++++++++++++++++++++------ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h index a1e01dd4e..eaca98899 100644 --- a/include/haproxy/proxy-t.h +++ b/include/haproxy/proxy-t.h @@ -424,9 +424,11 @@ struct proxy { struct list listeners; /* list of listeners belonging to this frontend */ struct list errors; /* list of all custom error files */ struct arg_list args; /* sample arg list that need to be resolved */ - unsigned int refcount; /* refcount on this proxy (only used for default proxy for now) */ struct ebpt_node by_name; /* proxies are stored sorted by name here */ struct list lf_checks; /* list of logformats found in the proxy section that needs to be checked during postparse */ + const char *file_prev; /* file of the previous instance found with the same name, or NULL */ + int line_prev; /* line of the previous instance found with the same name, or 0 */ + unsigned int refcount; /* refcount on this proxy (only used for default proxy for now) */ } conf; /* config information */ struct http_ext *http_ext; /* http ext options */ struct eb_root used_server_addr; /* list of server addresses in use */ diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c index e190e6e16..bb722b3ee 100644 --- a/src/cfgparse-listen.c +++ b/src/cfgparse-listen.c @@ -229,6 +229,8 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) struct acl_cond *cond = NULL; char *errmsg = NULL; struct bind_conf *bind_conf; + const char *file_prev = NULL; + int line_prev = 0; if (!last_defproxy) { /* we need a default proxy and none was created yet */ @@ -325,6 +327,18 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) err_code |= ERR_ALERT | ERR_ABORT; goto out; } + + /* if the other proxy exists, we don't need to keep it + * since neither will support being explicitly referenced + * so let's drop it from the index but keep a reference to + * its location for error messages. + */ + if (curproxy) { + file_prev = curproxy->conf.file; + line_prev = curproxy->conf.line; + proxy_unref_or_destroy_defaults(curproxy); + curproxy = NULL; + } } curproxy = proxy_find_by_name(args[1], 0, 0); @@ -351,8 +365,6 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) curr_defproxy = last_defproxy; if (strcmp(args[arg], "from") == 0) { - struct ebpt_node *next_by_name; - curr_defproxy = proxy_find_by_name(args[arg+1], PR_CAP_DEF, 0); if (!curr_defproxy) { @@ -361,12 +373,11 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) goto out; } - if ((next_by_name = ebpt_next_dup(&curr_defproxy->conf.by_name))) { - struct proxy *px2 = container_of(next_by_name, struct proxy, conf.by_name); - + if (curr_defproxy->conf.line_prev) { ha_alert("parsing [%s:%d] : ambiguous defaults section name '%s' referenced by %s '%s' exists at least at %s:%d and %s:%d.\n", file, linenum, args[arg+1], proxy_cap_str(rc), name, - curr_defproxy->conf.file, curr_defproxy->conf.line, px2->conf.file, px2->conf.line); + curr_defproxy->conf.file, curr_defproxy->conf.line, + curr_defproxy->conf.file_prev, curr_defproxy->conf.line_prev); err_code |= ERR_ALERT | ERR_FATAL; } @@ -394,6 +405,9 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) goto out; } + curproxy->conf.file_prev = file_prev; + curproxy->conf.line_prev = line_prev; + if (curr_defproxy && (!LIST_ISEMPTY(&curr_defproxy->http_req_rules) || !LIST_ISEMPTY(&curr_defproxy->http_res_rules) || !LIST_ISEMPTY(&curr_defproxy->http_after_res_rules) ||