Here are my updates from the feedback. It took me quite a while to figure out how to send this diff properly formatted with gmail.
If you would like a single patch with all my changes, I can send that as well. >From 9c71b643e993dcafca36feae71cdfacc24ffaaa2 Mon Sep 17 00:00:00 2001 From: Thayne McCombs <[email protected]> Date: Sat, 5 Dec 2020 23:09:24 -0700 Subject: [PATCH 1/2] Feedback from initial review --- doc/configuration.txt | 4 ++-- include/haproxy/dict.h | 1 + include/haproxy/server-t.h | 1 - src/cfgparse-listen.c | 1 + src/dict.c | 24 +++++++++++++++++++++- src/peers.c | 7 ++++++- src/server.c | 41 ++++++++++++++++++++------------------ src/stick_table.c | 8 ++++++++ src/stream.c | 2 +- src/tools.c | 3 ++- 10 files changed, 66 insertions(+), 26 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index 95c9abd8c..d1f7374ea 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -10711,8 +10711,8 @@ stick-table type {ip | integer | string [len <length>] | binary [len <length>]} <srvkey> specifies how each server is identified for the purposes of the stick table. The valid values are "name" and "addr". If "name" is given, then <name> argument for the server (may be generated by - a template). If "addr" is given, then the server is idntified - by it's current network address, including the port. "addr" is + a template). If "addr" is given, then the server is identified + by its current network address, including the port. "addr" is especially useful if you are using service discovery to generate the addresses for servers with peered stick-tables and want to consistently use the same host across peers for a stickiness diff --git a/include/haproxy/dict.h b/include/haproxy/dict.h index 59e81352c..c55834ca5 100644 --- a/include/haproxy/dict.h +++ b/include/haproxy/dict.h @@ -31,5 +31,6 @@ struct dict *new_dict(const char *name); struct dict_entry *dict_insert(struct dict *d, char *str); +void dict_entry_unref(struct dict *d, struct dict_entry *de); #endif /* _HAPROXY_DICT_H */ diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index 5a4dadfbd..13f5a5dab 100644 --- a/include/haproxy/server-t.h +++ b/include/haproxy/server-t.h @@ -274,7 +274,6 @@ struct server { const struct netns_entry *netns; /* contains network namespace name or NULL. Network namespace comes from configuration */ /* warning, these structs are huge, keep them at the bottom */ struct sockaddr_storage addr; /* the address to connect to, doesn't include the port */ - char *addr_desc; /* string description of the address and port for the server */ struct xprt_ops *xprt; /* transport-layer operations */ unsigned int svc_port; /* the port to connect to (for relevant families) */ unsigned down_time; /* total time the server was down */ diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c index 97a97e746..a493e741c 100644 --- a/src/cfgparse-listen.c +++ b/src/cfgparse-listen.c @@ -457,6 +457,7 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) curproxy->grace = defproxy.grace; curproxy->conf.used_listener_id = EB_ROOT; curproxy->conf.used_server_id = EB_ROOT; + curproxy->used_server_addr = EB_ROOT_UNIQUE; if (defproxy.check_path) curproxy->check_path = strdup(defproxy.check_path); diff --git a/src/dict.c b/src/dict.c index 903f07323..9b3536d96 100644 --- a/src/dict.c +++ b/src/dict.c @@ -87,8 +87,10 @@ struct dict_entry *dict_insert(struct dict *d, char *s) HA_RWLOCK_RDLOCK(DICT_LOCK, &d->rwlock); de = __dict_lookup(d, s); HA_RWLOCK_RDUNLOCK(DICT_LOCK, &d->rwlock); - if (de) + if (de) { + HA_ATOMIC_ADD(&de->refcount, 1); return de; + } de = new_dict_entry(s); if (!de) @@ -105,3 +107,23 @@ struct dict_entry *dict_insert(struct dict *d, char *s) return de; } + +/* + * Unreference a dict entry previously acquired with <dict_insert>. + * If this is the last live reference to the entry, it is + * removed from the dictionary. + */ +void dict_entry_unref(struct dict *d, struct dict_entry *de) +{ + if (!de) + return; + + if (HA_ATOMIC_SUB(&de->refcount, 1) != 0) + return; + + HA_RWLOCK_WRLOCK(DICT_LOCK, &d->rwlock); + ebpt_delete(&de->value); + HA_RWLOCK_WRUNLOCK(DICT_LOCK, &d->rwlock); + + free_dict_entry(de); +} diff --git a/src/peers.c b/src/peers.c index 3f4e2c575..3fa1a28b4 100644 --- a/src/peers.c +++ b/src/peers.c @@ -1634,12 +1634,15 @@ static int peer_treat_updatemsg(struct appctx *appctx, struct peer *p, int updt, *msg_cur += value_len; de = dict_insert(&server_key_dict, chunk->area); + dict_entry_unref(&server_key_dict, dc->rx[id - 1].de); dc->rx[id - 1].de = de; } if (de) { data_ptr = stktable_data_ptr(st->table, ts, data_type); - if (data_ptr) + if (data_ptr) { + HA_ATOMIC_ADD(&de->refcount, 1); stktable_data_cast(data_ptr, std_t_dict) = de; + } } break; } @@ -3059,6 +3062,8 @@ static inline void flush_dcache(struct peer *peer) for (i = 0; i < dc->max_entries; i++) { ebpt_delete(&dc->tx->entries[i]); dc->tx->entries[i].key = NULL; + dict_entry_unref(&server_key_dict, dc->rx[i].de); + dc->rx[i].de = NULL; } dc->tx->prev_lookup = NULL; dc->tx->lru_key = 0; diff --git a/src/server.c b/src/server.c index dc7a85052..ef536d718 100644 --- a/src/server.c +++ b/src/server.c @@ -193,6 +193,9 @@ void srv_set_dyncookie(struct server *s) HA_RWLOCK_RDUNLOCK(PROXY_LOCK, &p->lock); } +/* + * Must be called with the server lock held, and will write-lock the proxy. + */ static void srv_set_addr_desc(struct server *s) { struct proxy *p = s->proxy; @@ -200,31 +203,26 @@ static void srv_set_addr_desc(struct server *s) key = sa2str(&s->addr, s->svc_port, s->flags & SRV_F_MAPPORTS); - if (strcmp(key, s->addr_desc) == 0) { - free(key); - return; - } + if (s->addr_node.key) { + if (strcmp(key, s->addr_node.key) == 0) { + free(key); + return; + } - HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock); - ebpt_delete(&s->addr_node); - HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); + HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock); + ebpt_delete(&s->addr_node); + HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); + + free(s->addr_node.key); + } - free(s->addr_desc); - s->addr_desc = key; s->addr_node.key = key; - // TODO: should this use a dedicated lock? HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock); ebis_insert(&p->used_server_addr, &s->addr_node); HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); } -static void srv_addr_updated(struct server *srv) -{ - srv_set_dyncookie(srv); - srv_set_addr_desc(srv); -} - /* * Registers the server keyword list <kwl> as a list of valid keywords for next * parsing sessions. @@ -2084,6 +2082,8 @@ int parse_server(const char *file, int linenum, char **args, struct proxy *curpr newsrv->addr = *sk; newsrv->svc_port = port; + // we don't need to lock the server here, because + // we are in the process of initializing srv_set_addr_desc(newsrv); if (!newsrv->srvrq && !newsrv->hostname && !protocol_by_family(newsrv->addr.ss_family)) { @@ -3551,7 +3551,8 @@ int update_server_addr(struct server *s, void *ip, int ip_sin_family, const char memcpy(((struct sockaddr_in6 *)&s->addr)->sin6_addr.s6_addr, ip, 16); break; }; - srv_addr_updated(s); + srv_set_dyncookie(s); + srv_set_addr_desc(s); return 0; } @@ -3723,7 +3724,8 @@ out: if (changed) { /* force connection cleanup on the given server */ srv_cleanup_connections(s); - srv_addr_updated(s); + srv_set_dyncookie(s); + srv_set_addr_desc(s); } if (updater) chunk_appendf(msg, " by '%s'", updater); @@ -4203,7 +4205,8 @@ static int srv_iterate_initaddr(struct server *srv) return_code |= ERR_ALERT | ERR_FATAL; return return_code; out: - srv_addr_updated(srv); + srv_set_dyncookie(srv); + srv_set_addr_desc(srv); return return_code; } diff --git a/src/stick_table.c b/src/stick_table.c index abdf229fb..825005c2e 100644 --- a/src/stick_table.c +++ b/src/stick_table.c @@ -22,6 +22,7 @@ #include <haproxy/arg.h> #include <haproxy/cfgparse.h> #include <haproxy/cli.h> +#include <haproxy/dict.h> #include <haproxy/errors.h> #include <haproxy/global.h> #include <haproxy/http_rules.h> @@ -94,6 +95,12 @@ void __stksess_free(struct stktable *t, struct stksess *ts) */ void stksess_free(struct stktable *t, struct stksess *ts) { + void *data; + data = stktable_data_ptr(t, ts, STKTABLE_DT_SERVER_KEY); + if (data) { + dict_entry_unref(&server_key_dict, stktable_data_cast(data, server_key)); + stktable_data_cast(data, server_key) = NULL; + } HA_SPIN_LOCK(STK_TABLE_LOCK, &t->lock); __stksess_free(t, ts); HA_SPIN_UNLOCK(STK_TABLE_LOCK, &t->lock); @@ -894,6 +901,7 @@ int parse_stick_table(const char *file, int linenum, char **args, goto out; } + idx++; } else { ha_alert("parsing [%s:%d] : %s: unknown argument '%s'.\n", diff --git a/src/stream.c b/src/stream.c index f64390652..ed5a6515f 100644 --- a/src/stream.c +++ b/src/stream.c @@ -1385,7 +1385,7 @@ static int process_store_rules(struct stream *s, struct channel *rep, int an_bit if (t->server_key_type == STKTABLE_SRV_NAME) key = __objt_server(s->target)->id; else if (t->server_key_type == STKTABLE_SRV_ADDR) - key = __objt_server(s->target)->addr_desc; + key = __objt_server(s->target)->addr_node.key; else continue; diff --git a/src/tools.c b/src/tools.c index 21e3f06ef..27043c186 100644 --- a/src/tools.c +++ b/src/tools.c @@ -1228,7 +1228,8 @@ char * sa2str(const struct sockaddr_storage *addr, int port, int map_ports) case AF_UNIX: path = ((struct sockaddr_un *)addr)->sun_path; if (path[0] == '\0') { - return memprintf(&out, "abns@%107s", path+1); + const int max_length = sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path) - 1; + return memprintf(&out, "abns@%.*s", max_length, path+1); } else { return strdup(path); } -- 2.29.2 >From a5cbb749f8a3122f1280163fdaa5d88d530e1537 Mon Sep 17 00:00:00 2001 From: Thayne McCombs <[email protected]> Date: Wed, 9 Dec 2020 00:36:45 -0700 Subject: [PATCH 2/2] Add test for srvkey addr --- reg-tests/peers/srvkey_addr.vtc | 118 ++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 reg-tests/peers/srvkey_addr.vtc diff --git a/reg-tests/peers/srvkey_addr.vtc b/reg-tests/peers/srvkey_addr.vtc new file mode 100644 index 000000000..52e43a6cb --- /dev/null +++ b/reg-tests/peers/srvkey_addr.vtc @@ -0,0 +1,118 @@ +vtest "Test for peering by server address" +feature ignore_unknown_macro + +#REQUIRE_VERSION=2.0 +#REGTEST_TYPE=slow + +server s1 { + rxreq + txresp -body "A" +} -repeat 3 -start + +server s2 { + rxreq + txresp -body "B" +} -repeat 3 -start + +server s3 { + rxreq + txresp -body "C" +} -repeat 3 -start + +haproxy h1 -arg "-L A" -conf { + defaults + mode http + timeout client 1s + timeout connect 1s + timeout server 1s + + peers peers + bind "fd@${A}" + server A + server B ${h2_B_addr}:${h2_B_port} + + listen stkt + bind "fd@${fe}" + balance roundrobin + stick-table type string size 10m store gpc0 srvkey addr peers peers + stick on path + http-request track-sc0 path + http-request sc-inc-gpc0(0) + server s1_A ${s1_addr}:${s1_port} + server s2_A ${s2_addr}:${s2_port} + server s3_A ${s3_addr}:${s3_port} +} + +haproxy h2 -arg "-L B" -conf { + defaults + mode http + timeout client 1s + timeout connect 1s + timeout server 1s + + peers peers + bind "fd@${B}" + server A ${h1_A_addr}:${h1_A_port} + server B + server C ${h2_C_addr}:${h2_C_port} + + listen stkt + bind "fd@${fe}" + balance random + stick-table type string size 10m store gpc0 srvkey addr peers peers + stick on path + http-request track-sc0 path + http-request sc-inc-gpc0(0) + server s2_B ${s2_addr}:${s2_port} + server s3_B ${s3_addr}:${s3_port} + server s1_B ${s1_addr}:${s1_port} +} + +haproxy h3 -arg "-L C" -conf { + defaults + mode http + timeout client 1s + timeout connect 1s + timeout server 1s + + peers peers + bind "fd@${C}" + server A ${h1_A_addr}:${h1_A_port} + server B ${h2_B_addr}:${h2_B_port} + server C + + listen stkt + bind "fd@${fe}" + balance random + stick-table type string size 10m store gpc0 srvkey addr peers peers + stick on path + server s3_C ${s3_addr}:${s3_port} + server s1_C ${s1_addr}:${s1_port} + server s2_C ${s2_addr}:${s2_port} +} + +haproxy h1 -start +delay 0.2 +haproxy h2 -start +delay 0.2 +haproxy h3 -start +delay 0.2 + +shell { + set -e + c1=`curl -sS ${h1_fe_addr}:${h1_fe_port}/c1` + c2=`curl -sS ${h1_fe_addr}:${h1_fe_port}/c2` + c3=`curl -sS ${h1_fe_addr}:${h1_fe_port}/c3` + + echo $c1 $c2 $c3 + + sleep 1 + + [ "`curl -sS ${h2_fe_addr}:${h2_fe_port}/c1`" = "$c1" ] || exit 1 + [ "`curl -sS ${h2_fe_addr}:${h2_fe_port}/c2`" = "$c2" ] || exit 1 + [ "`curl -sS ${h2_fe_addr}:${h2_fe_port}/c3`" = "$c3" ] || exit 1 + + [ "`curl -sS ${h3_fe_addr}:${h3_fe_port}/c1`" = "$c1" ] || exit 1 + [ "`curl -sS ${h3_fe_addr}:${h3_fe_port}/c2`" = "$c2" ] || exit 1 + [ "`curl -sS ${h3_fe_addr}:${h3_fe_port}/c3`" = "$c3" ] || exit 1 +} -- 2.29.2
