Export _srv_parse_kw() and srv_postinit() so they can be called from
haload (to come), which needs to configure servers using HAProxy's configuration
parser keywords.
port_range was never freed. That used not to be a problem, but now that
we can dynamically add and remove servers, it becomes one, as that leads
to a memory leak each time a server with a "source" directive is destroyed.
However, just adding a free() is not enough. We have to add a refcount,
because the server is not the only one with a reference to it. We may
also have one in fdinfo, so that we know which port to release when we
finally close the fd.
So add a refcount, and make sure to call port_range_release() when a
server is destroyed.
This should be backported up to 3.0.
Add the ability to rename a HAProxy server at runtime via the CLI:
set server <backend>/<server> name <newname>
This is useful in slot-based dynamic scaling setups where servers are
pre-allocated with generic names (e.g. srv001, srv002) but the operator
wants the names to reflect the current workload (e.g. pod name or
IP:port) for observability and server-state-file consistency.
The implementation:
- validates the new name: non-empty, passes invalid_char() check
(allows [A-Za-z0-9_:.-]), and fits in the event data name field
- requires the server to be administratively in maintenance mode
(same precondition as 'del server')
- rejects the rename if the server has SRV_F_NAME_REFD set (use-server
target, track target, sample-fetch ARGT_SRV referent) - keeps the
running state consistent with the configuration text
- re-indexes the server in the name tree under thread_isolate(),
mirroring the locking pattern used by 'add server' / 'del server'
- publishes a new EVENT_HDL_SUB_SERVER_NAME event with the old and
new names so downstream consumers (logs, observability backends)
can track the rename
- frees the old name immediately under thread isolation: srv_name
sample consumers (ACLs, log formats, ...) act on the fetched pointer
within the current task and do not retain it across wake-ups, so
no extra deferred-free machinery is needed
There is no opt-in directive: like 'add server' and 'del server', the
operation is gated by the server's properties rather than by a
per-backend toggle. This avoids the runtime-surprise failure mode
where an operator discovers at the CLI that renaming is forbidden by
a missing 'option server-rename' rather than by an actual structural
reference.
This feature was discussed in:
https://github.com/haproxy/haproxy/issues/952
Until now, every form of "this server is referenced by something in the
running config" was collapsed onto a single flag, SRV_F_NON_PURGEABLE,
which prevents the server from being removed via 'del server'. This
catches everything but conflates two distinct properties:
- the server object itself is pinned by another runtime structure
(e.g. DNS resolution attached to it), versus
- the server's *name* is referenced statically (use-server rules,
track chains, sample-fetch arguments of type ARGT_SRV)
These differ for any operation that touches the name but not the
object identity, e.g. the runtime rename feature added next. Removing
a name-referenced server is still forbidden (the rule text would
dangle), but renaming such a server should also be forbidden for the
same reason - while renaming a resolver-pinned server is fine, since
the resolver holds the object pointer and doesn't care about the name.
Introduce SRV_F_NAME_REFD for the name-reference case and move the
three name-based setters (sample.c ARGT_SRV resolution, proxy.c
use-server resolution, server.c track chain setup) from
SRV_F_NON_PURGEABLE to SRV_F_NAME_REFD. The resolvers.c call site
keeps SRV_F_NON_PURGEABLE since it is the object-pinned case.
Adjust 'del server' to check both flags so the set of servers it
refuses to remove is unchanged: same observable behavior, just a
richer internal taxonomy.
A subsequent patch introducing 'set server name' will gate on
SRV_F_NAME_REFD only.
srv_settings_init() sets agent.rise but forgets agent.health, while
srv_settings_cpy() sets both. check.health is fixed up later when the
server's admin state is updated at startup, but nothing does the same
for agent.health.
This used to be harmless because servers were always set up through
srv_settings_cpy(). But since 49a619aca ("MEDIUM: proxy: no longer
allocate the default-server entry by default") the defsrv pointer is
NULL when a proxy has no "default-server" line, and srv_settings_cpy()
then falls back to srv_settings_init(). So a server whose agent-check is
declared entirely on its "server" line ends up with agent.health == 0,
which is below agent.rise.
The wrong value only bites when the server has to come back up. While it
stays up nobody notices agent.health is 0, but as soon as the regular
health check fails and recovers, agent.health is still 0 (below rise) and
check_notify_success() won't bring the server back up. The agent never
sends an explicit "up", which is the only thing that raises agent.health,
so the server stays down for good. Moving the agent settings to a
"default-server" line works around it.
Just initialise agent.health in srv_settings_init() like
srv_settings_cpy() already does.
This should be backported to 3.3 and 3.4.
When a dynamic server is added with consistent hash balancing on the
backend, its lb_nodes elements are allocated and associated with a
calculated server key. This operation is performed in add server handler
via srv_alloc_lb(). By default, the server key is based on its ID.
However, automatic server ID is calculated later in add server handler,
which means the initial lb_nodes are not valid.
This could cause load balacing issue but in fact this is not directly
visible as the server key is recalculated when the dynamic server is
enabled via chash_queue_dequeue_srv() : all server lb_nodes are dequeued
and requeued with the now proper key.
Thus, "add server" handler must be corrected as it is buggy when
considering it alone. The simplest solution of the current patch is to
initialize server ID before srv_alloc_lb() is invoked. There is no issue
as handler runs under thread isolation so there is no risk of multiple
servers manipulating the same ID. Server insertion in proxy ID tree is
still performed at the end of the handler when all fallible operation
are completed.
The fact that server key is recalculated when the server is set to ready
state is a side effect of the following patch which was introduced in
3.0. What this means though is that users of older releases are facing a
bigger issue, with load-balancing not working as expected. Thus,
this patch is even more crucial for 2.8 and older releases.
faa8c3e024
MEDIUM: lb-chash: Deterministic node hashes based on server address
This should fix github issue #3413. Thanks to Joao Morais for is
analysis on the problem.
This must be backported to all stable releases.
Having a single task to take care of idle connection cleanup across all
servers leads to high contention. It uses a lock to maintain its tree of
servers to track, and then can acquire the idle_conns lock for each thread.
Instead, have one task per thread. Each thread will maintain its own
tree, so there will be no need for any lock, and it will just acquire
its own idle_conns lock, so it will lead to less contention.
This is a performance improvement, so backporting is optional, but may be
considered if it is worth it. That would require backporting commit
6f8dab2583 too.
The healthcheck keyword could be parsed on default-server lines but not
copied during server initialization, making it ineffective. But there is
also a true issue by setting it on a default-server. The pseudo server used
to parse the default-server line is not initialized via the new_server()
function, as regular servers. So there is no tcpcheck information inherited
from the proxy. We must take care of that when the "healthcheck" keyword is
parsed to avoid crashes.
This patch must be backported to 3.4.
Instead of relying on malloc(n*size), we now pass array_size_or_fail(n,m)
so that it becomes possible to detect overflow. This is particularly
interesting for global settings that might be set large enough to cause
overflows on 32-bit systems for example, resulting in small values that
then cause trouble. Now the overflow will be detected at allocation time.
Around 25 locations were updated.
When we store the negociated server parameters, such as the ALPN, also
store the calculated hash with the connection. If it is different, as
can happen because the IP address is different because set-dst was used,
we certainly do not want to reuse the information in the cache,
otherwise we could end up using the wrong ALPN and mux.
That means we already have to calculate the hash in connect_server()
now, while before we would not do it for Websockets, if we could not do
connection reuse, as that's all the hash was used for.
This should fix Github issue #3386
This should be backported as far as 3.2.
When server fleets are constantly updated, using a stable distribution
across a bunch of load balancers can be convenient. The addr and port
already provide a bit of this but for situations were addresses might
differ between sites or change dynamically this does not work. The guid
is perfect for this because by definition it's supposed to designate a
single server and be unique. So when two servers anywhere have the same,
the tool that provisionned them promises that they are the same server.
So here we introduce "hash-key guid" which performs a 32-bit hash on
the GUID value. When no guid is provided, a fallback is performed on
ID, as is done for other keys.
The "id" hash-key scales the ID by a factor of 16 that tries to leave
room between the nodes on the 32-bit space to permit smooth weight
variations (e.g. during slowstart). However this does not deal well
with overlaps between server IDs. For example, assigning IDs that are
only multiples of 256 million to 16 servers yields traffic only on
one since in practice they all have the same 28 lower bits.
The new "id32" hash key bridges this gap by using the full 32-bit ID
of the server as the key. On the other hand, the user must be careful
not to switch the hash function to "none" when using incremental IDs
because in this case they might be very poorly distributed. But this
can be convenient for automated provisionning systems which assign
IDs themselves, as the full 32 bits are used now.
Due to the check of the stored value instead of the parsed one, it was not
permitted to use server IDs above 2^31 while they are perfectly possible.
Let's refine the parsing and also update the error message to indicate the
range. The doc was also refined to reflect the relation with hash-key.
This may be backported though it wouldn't have any effect on working
configs.
Add a postparsing check on TCP ALPN bind and server setting. An error is
reported if the token "h3" is present and expose-experimental-directives
is not globally activated. This ensures that QMux protocol won't be
selected if experimental features are not explicitely requested.
The check is not performed though if "proto qmux" is explicitely
defined, as this setting already checks for experimental support.
Currently, it's not possible to activate QMux without any explicit
"proto qmux" config. However, this will be implemented in a next patch,
so this check will become necessary.
In conn_get_best_mux() and conn_get_best_mux_entry(), the mux name was
provided sometimes based on the "proto" directive, sometimes based on
the ALPN, but in any case, it was compared again the mux_proto_list
mux_proto field. This is not correct, as ALPN can be different from the
internal mux_proto. So enhance those functions so that they wll accept
an ALPN as well. If a mux_proto is provided, that will be used, if not,
and if an ALPN is provided, then that will be used, and compared against
the ALPN provided by the mux, if any.
In struct mux_proto_list, rename the "token" field to "mux_proto". That
field should only be used to match the name provided in the "proto"
directive, and it will be soon.
This should be a no-op.
Unlike stated in the configuration manual, the server 'init-state' parameter
was not evaluated during haproxy startup/reload. After a review, it appeared
there were also issues if combined with the 'track' parameter. In addtition,
this parameter was only evaluated when health-checks were enabled for the
server, leading to unexpected behavior if the serve settings are dynamically
changed via the CLI.
To fix those issues, behavior of the 'init-state' parameter was slightly
adapted. It is always evaluated, even when there is no running health-checks
for the server. An error is reported if the 'track' parameter is also
defined. Both cannot work together.
In addition, the "none" state was introduced to be able to restore the
default behavior. It will be especially useful when the parameter is
inherited from a 'default-server' directive.
This patch should fix the issue #3298. It must be backported as far as 3.2.
In cli_parse_set_server()'s 'ssl' branch, the server lock is taken,
and not released in case srv_set_ssl() fails, resulting in a dead lock
and a panic the next time an attempt to touch this server is made. The
lock must be released on all error paths.
This was introduced in 3.3 by commit f8f94ffc9 ("BUG/MEDIUM: server:
Use sni as pool connection name for SSL server only") which was marked
for backporting to 3.0, so this must likely be backported that far.
The condition 'if (srv->pool_conn_name)' was checking the destination
instead of the source 'src->pool_conn_name', meaning the strdup() would
never fire (since newly calloc'd servers start with NULL pool_conn_name),
and the pool_conn_name setting from default-server was silently ignored.
Introduced in 3.2 with commit f0f1816f1 ("MINOR: check: implement
check-pool-conn-name srv keyword") when pool_conn_name support was added
to srv_settings_cpy(). The bug caused any 'pool-conn-name' setting in a
'default-server' line to be lost for all servers inheriting from it.
Note that it's not the first time this function induces such a bug due
to the poor choice of "srv" vs "src" that should be renamed to avoid
keyboard mistakes and visual confusion.
This needs to be backported to 3.2.
This function may face an OOM on strdup() in the middle of the hostname
or hostname_dn replacement, leaving NULLs in either or both of the server's
fields, which is definitely not good for other call places.
Let's perform a safe replacement instead: we first allocate the new
values, and only if they are successful, then we release the previous
ones and replace them.
It is not necessary to backport this unless the issue is reported (it
was found via code review).
In init_srv_requeue(), only attempt to run the tasklet if the server is
actually running, otherwise it will end up being queued a second time,
when the server is actually brought up, and that will lead to a
corrupted mt_list.
This can easily be reproduced by adding a dynamic server, as those start
disabled, and then enabling and disabling it a couple of times.
This should fix github issue #3360.
This should be backported up to 3.2.
In 3.4-dev6, commit de5fc2f515 ("BUG/MINOR: server: set auto SNI for
dynamic servers") allowed to properly set the SNI, and return an error
message. However the error message is leaked after being printed on the
CLI.
This should be backported to 3.3.
Storing the protocol directly into the check was not a good idea,
because the protocol may not be determined until after a DNS resolution
on the server, and may even change at runtime, if the DNS changes.
What we can, however, figure out at start up, is the net_addr_type,
which will contain all that we need to find out which protocol to use
later.
Also revert the changes made by commit 07edaed191
that would not reuse the server xprt if a different alpn is set for
checks. The alpn is just a string, and should not influence the choice
of the xprt.
We'll now make sure to use the server xprt, unless an address is
provided, in which case we'll use whatever xprt matches that address, or
a port, in which case we'll assume we want TCP, and use check_ssl to
know whetver we want the SSL xprt or not.
Now that the check contains all that is needed to know which protocol to
look up, always just use that when creating a new check connection if it
is the default check connection, and for now, always use TCP when a
tcp-check or http-check connect rule is used (which means those can't be
used for QUIC so far).
This should hopefully fix github issue #3324.
The parsing of the "healthcheck" parameter for dynamic servers was not
finished. The post-config was missing, leading to a crash because the
ruleset pointer was NULL.
To fix the issue, check_server_tcpcheck() function is called in
cli_parse_add_server().
No backport needed.
The proxy flag PR_O_TCPCHK_SSL is replaced by a flag on the tcpcheck
itself. When TCPCHK_FL_USE_SSL flag is set, it means the healthcheck will
use an SSL connection and the SSL xprt must be prepared for the server.
disable-on-404 and send-state options, configured on an HTTP healtcheck,
were handled as proxy options. Now, these options are handled in the
tcp-check itself. So the corresponding PR_O and PR_02 flags are removed.
The tcpcheck_rules structure is replaced by the tcpcheck structure. The main
difference is that the ruleset is now referenced in the tcpcheck structure,
instead of the rules list. The flags about the ruleset type are moved into
the ruleset structure and flags to track unused rules remains on the
tcpcheck structure. So it should be easier to track unused rulesets. But it
should be possible to configure a set of tcpcheck rules outside of the proxy
scope.
The main idea of these changes is to prepare the parsing of a new
healthcheck section. So this patch is quite huge, but it is mainly about
renaming some fields.
There are two settings to control idle connection sharing across
threads.
tune.idle-pool.shared, that enables or disables it, and then
tune.takeover-other-tg-connections, which lets you or not get idle
connections from other thread groups.
Add a new keyword for tune.idle-pool.shared, "full", that lets you get
connections from other thread groups (equivalent to "full" keyword for
tune.takeover-other-tg-connections). The "on" keyword now will be
equivalent to the "restrict" one, which allowed getting connection from
other thread groups only when not doing it would result in a connection
failure (when reverse-http or when strict-macxonn are used).
tune.takeover-other-tg-connections will be deprecated.
When picking a mux, pay attention to its MX_FL_FRAMED. If it is set,
then it means we explicitely want QUIC, so don't use that mux for any
protocol that is not QUIC.
When parsing the check address, store the associated proto too.
That way we can use the notation like quic4@address, and the right
protocol will be used. It is possible for checks to use a different
protocol than the server, ie we can have a QUIC server but want to run
TCP checks, so we can't just reuse whatever the server uses.
WIP: store the protocol in checks
Most calls to mux ops were instrumented with a CALL_MUX_WITH_RET() or
CALL_MUX_NO_RET() macro in order to make the current thread's context
point to the called mux and be able to track its allocations. Only
a bunch of harmless mux_ctl() and ->subscribe/unsubscribe calls were
left untouched since useless. But destroy/detach/shut/init/snd_buf
and rcv_buf are now tracked.
It will not show allocations performed in IO callback via tasklet
wakeups however.
In order to ease reading of the output, cmp_memprof_ctx() knows about
muxes and sorts based on the .subscribe function address instead of
the mux_ops address so as to keep various callers grouped.
Currenly, quic_conn on the backend side may access their parent proxy
instance during their lifetime. In particular, this is the case for
counters update, with <prx_counters> field directly referencing a proxy
memory zone.
As such, this prevents safe backend removal. One solution would be to
check if the upper connection instance is still alive, as a proxy cannot
be removed if connection are still active. However, this would
completely prevent proxy counters update via
quic_conn_prx_cntrs_update(), as this is performed on quic_conn release.
Another solution would be to use refcount, or a dedicated counter on the
which account for QUIC connections on a backend instance. However,
refcount is currently only used by short-term references, and it could
also have a negative impact on performance.
Thus, the simplest solution for now is to disable a backend removal if a
QUIC server is/was used in it. This is considered acceptable for now as
QUIC on the backend side is experimental.
When a server is deleted via "del server", increment refcount of its
parent backend. This is necessary as the server is not referenced
anymore in the backend, but can still access it via its own <proxy>
member. Thus, backend removal must not happen until the complete purge
of the server.
The proxy refcount is released in srv_drop() if the flag SRV_F_DELETED
is set, which indicates that "del server" was used. This operation is
performed after the complete release of the server instance to ensure no
access will be performed on the proxy via itself. The refcount must not
be decremented if a server is freed without "del server" invokation.
Another solution could be for servers to always increment the refcount.
However, for now in haproxy refcount usage is limited, so the current
approach is preferred. It should also ensure that if the refcount is
still incremented, it may indicate that some servers are not completely
purged themselves.
Note that this patch may cause issues if "del backend" are used in
parallel with LUA scripts referencing servers. Currently, any servers
referenced by LUA must be released by its garbage collector to ensure it
can be finally freed. However, it appeas that in some case the gc does
not run for several minutes. At least this has been observed with Lua
version 5.4.8. In the end, this will result in indefinitely blocking of
"del backend" commands.
As seen with the last changes to counters allocation, the move of the
counters storage to the thread group as operated in commit 04a9f86a85
("MEDIUM: counters: add a dedicated storage for extra_counters in various
structs") causes some random errors when using ASAN, because the extra
counters are freed in srv_drop() after calling srv_free_params(), which
is responsible for freeing the per-thread group storage.
For the proxies however it's OK because free calls are made before the
call to deinit_proxy() which frees the per_tgrp area.
No backport is needed, this is purely 3.4-dev.
We'll need to permit any user to update its own tgroup's extra counters
instead of the global ones. For this we now store the per-tgroup step
between two consecutive data storages, for when they're stored in a
tgroup array. When shared (e.g. resolvers or listeners), we just store
zero to indicate that it doesn't scale with tgroups. For now only the
registration was handled, it's not used yet.
Servers, proxies, listeners and resolvers all use extra_counters. We'll
need to move the storage to per-tgroup for those where it matters. Now
we're relying on an external storage, and the data member of the struct
was replaced with a pointer to that pointer to data called datap. When
the counters are registered, these datap are set to point to relevant
locations. In the case of proxies and servers, it points to the first
tgrp's storage. For listeners and resolvers, it points to a local
storage. The rationale here is that listeners are limited to a single
group anyway, and that resolvers have a low enough load so that we do
not care about contention there.
Nothing should change for the user at this point.
It appears that in cli_parse_add_server(), we're calling srv_alloc_lb()
and stats_allocate_proxy_counters_internal() before srv_preinit() which
allocates the thread groups. LB algos can make use of the per_tgrp part
which is initialized by srv_preinit(). Fortunately for now no algo uses
both tgrp and ->server_init() so this explains why this remained
unnoticed to date. Also, extra counters will soon require per_tgrp to
already be initialized. So let's move these between srv_preinit() and
srv_postinit(). It's possible that other parts will have to be moved
in between.
This could be backported to recent versions for the sake of safety but
it looks like the current code cannot tell the difference.
Auto SNI configuration is configured during check config validity.
However, nothing was implemented for dynamic servers.
Fix this by implementing auto SNI configuration during "add server" CLI
handler. Auto SNI configuration code is moved in a dedicated function
srv_configure_auto_sni() called both for static and dynamic servers.
Along with this, allows the keyword "no-sni-auto" on dynamic servers, so
that this process can be deactivated if wanted. Note that "sni-auto"
remains unavailable as it only makes sense with default-servers which
are never used for dynamic server creation.
This must be backported up to 3.3.
An issue was introduced in 3.0 with commit faa8c3e024 ("MEDIUM: lb-chash:
Deterministic node hashes based on server address"): the new server_key
field and lb_nodes entries initialization were not updated for servers
added at run time with "add server": server_key remains zero and the key
used in lb_node remains the one depending only on the server's ID.
This will cause trouble when adding new servers with consistent hashing,
because the hash-key will be ignored until the server's weight changes
and the key difference is detected, leading to its recalculation.
This is essentially caused by the poorly placed lb_nodes initialization
that is specific to lb-chash and had to be replicated in the code dealing
with server addition.
This commit solves the problem by adding a new ->server_init() function
in the lbprm proxy struct, that is called by the server addition code.
This also allows to abandon the complex check for LB algos that was
placed there for that purpose. For now only lb-chash provides such a
function, and calls it as well during initial setup. This way newly
added servers always use the correct key now.
While it should also theoretically have had an impact on servers added
with the "random" algorithm, it's unlikely that the difference between
proper server keys and those based on their ID could have had any visible
effect.
This patch should be backported as far as 3.0. The backport may be eased
by a preliminary backport of previous commit "CLEANUP: lb-chash: free
lb_nodes from chash's deinit(), not global", though this is not strictly
necessary if context is manually adjusted.
There's an ambuity on the ownership of lb_nodes in chash, it's allocated
by chash but freed by the server code in srv_free_params() from srv_drop()
upon deinit. Let's move this free() call to a chash-specific function
which will own the responsibility for doing this instead. Note that
the .server_deinit() callback is properly called both on proxy being
taken down and on server deletion.
Move backend compatibility checks performed during 'add server' in a
dedicated function be_supports_dynamic_srv(). This should simplify
addition of future restriction.
This function will be reused when implementing backend creation at
runtime.
The code in srv_add_to_idle_list() has its roots in 2.0 with commit
9ea5d361ae ("MEDIUM: servers: Reorganize the way idle connections are
cleaned."). At this era we didn't yet have the current set of atomic
load/store operations and we used to perform loads using volatile casts
after a barrier. It turns out that this function has kept this schema
over the years, resulting in a big mfence stalling all the pipeline
in the function:
| static __inline void
| __ha_barrier_full(void)
| {
| __asm __volatile("mfence" ::: "memory");
27.08 | mfence
| if ((volatile void *)srv->idle_node.node.leaf_p == NULL) {
0.84 | cmpq $0x0,0x158(%r15)
0.74 | je 35f
| return 1;
Switching these for a pair of atomic loads got rid of this and brought
0.5 to 3% extra performance depending on the tests due to variations
elsewhere, but it has never been below 0.5%. Note that the second load
doesn't need to be atomic since it's protected by the lock, but it's
cleaner from an API and code review perspective. That's also why it's
relaxed.
This was the last user of _ha_barrier_full(), let's try not to
reintroduce it now!
Function proxy_preset_defaults() purpose has evolved over time.
Originally, it was only used to initialize defaults proxies instances.
Until today, it was extended so that all proxies use it. Its objective
is to initialize settings to common default values.
To remove the confusion, this function is now removed. Its content is
integrated directly into init_new_proxy().
There remained some cases (on error paths) were a server could be freed
while still attached on the parent proxy server list. In 3.3 this can be
problematic because new_server() automatically adds the server to the
parent proxy list.
The bug is insignificant because it is on errors paths during init and
often haproxy exits right after. But let's fix that to ensure no UAF or
undefined behavior occurs because of that.
This patch depends on ("MINOR: cli: use srv_drop() when server was created using new_server()")
It must be backported in 3.3 with the above mentioned patch.
