Amaury Denoyelle [Fri, 18 Jun 2021 09:11:36 +0000 (11:11 +0200)]
BUG/MAJOR: server: fix deadlock when changing maxconn via agent-check
The server_parse_maxconn_change_request locks the server lock. However,
this function can be called via agent-checks or lua code which already
lock it. This bug has been introduced by the following commit :
This commit tried to fix another deadlock with can occur because
previoulsy server_parse_maxconn_change_request requires the server lock
to be held. However, it may call internally process_srv_queue which also
locks the server lock. The locking policy has thus been updated. The fix
is functional for the CLI 'set maxconn' but fails to address the
agent-check / lua counterparts.
This new issue is fixed in two steps :
- changes from the above commit have been reverted. This means that
server_parse_maxconn_change_request must again be called with the
server lock.
- to counter the deadlock fixed by the above commit, process_srv_queue
now takes an argument to render the server locking optional if the
caller already held it. This is only used by
server_parse_maxconn_change_request.
The above commit was subject to backport up to 1.8. Thus this commit
must be backported in every release where it is already present.
Tim Duesterhus [Sat, 19 Jun 2021 14:56:30 +0000 (16:56 +0200)]
CLEANUP: Prevent channel-t.h from being detected as C++ by GitHub
GitHub uses github/linguist to determine the programming language used for each
source file to show statistics and to power the search. In cases of unique file
extensions this is easy, but for `.h` files the situation is less clear as they
are used for C, C++, Objective C and more. In these cases linguist makes use of
heuristics to determine the language.
One of these heuristics for C++ is that the file contains a line beginning with
`try`, only preceded by whitespace indentation. This heuristic matches the long
comment at the bottom of `channel-t.h`, as one sentence includes the word `try`
after a linebreak.
Fix this misdetection by changing the comment to follow the convention that all
lines start with an asterisk.
Willy Tarreau [Fri, 18 Jun 2021 08:33:47 +0000 (10:33 +0200)]
MINOR: queue: update the stream's pend_pos before queuing it
Since commit c7eedf7a5 ("MINOR: queue: reduce the locked area in
pendconn_add()") the stream's pend_pos is set out of the lock, after
the pendconn is queued. While this entry is only manipulated by the
stream itself and there is no bug caused by this right now, it's a
bit dangerous because another thread could decide to look at this
field during dequeuing and could randomly see something else. Also
in case of crashes, memory inspection wouldn't be as trustable.
Let's assign the pendconn before it can be found in the queue.
Amaury Denoyelle [Fri, 18 Jun 2021 14:30:36 +0000 (16:30 +0200)]
REGTESTS: server: test ssl support for dynamic servers
Create a new regtest to test SSL support for dynamic servers.
The first step of the test is to create the ca-file via the CLI. Then a
dynamic server is created with the ssl option using the ca-file. A
client request is made through it to achieve the test.
Amaury Denoyelle [Wed, 19 May 2021 07:49:41 +0000 (09:49 +0200)]
MINOR: ssl: support ssl keyword for dynamic servers
Activate the 'ssl' keyword for dynamic servers. This is the final step
to have ssl dynamic servers feature implemented. If activated,
ssl_sock_prepare_srv_ctx will be called at the end of the 'add server'
CLI handler.
At the same time, update the management doc to list all ssl keywords
implemented for dynamic servers.
Amaury Denoyelle [Mon, 14 Jun 2021 08:10:32 +0000 (10:10 +0200)]
MINOR: ssl: support crl arg for dynamic servers
File-access through ssl_store_load_locations_file is deactivated if
srv_parse_crl is used at runtime for a dynamic server. The crl must
have already been loaded either in the config or through the 'ssl crl'
CLI commands.
Amaury Denoyelle [Fri, 21 May 2021 14:22:53 +0000 (16:22 +0200)]
MINOR: ssl: support crt arg for dynamic servers
File-access through ssl_store_load_locations_file is deactivated if
srv_parse_crt is used at runtime for a dynamic server. The cert must
have already been loaded either in the config or through the 'ssl cert'
CLI commands.
Amaury Denoyelle [Wed, 19 May 2021 07:46:59 +0000 (09:46 +0200)]
MINOR: ssl: support ca-file arg for dynamic servers
File-access through ssl_store_load_locations_file is deactivated if
srv_parse_ca_file is used at runtime for a dynamic server. The ca-file
must have already been loaded either in the config or through the 'ssl
ca-file' CLI commands.
Amaury Denoyelle [Fri, 21 May 2021 14:22:11 +0000 (16:22 +0200)]
MINOR: ssl: render file-access optional on server crt loading
The function ssl_sock_load_srv_cert will be used at runtime for dynamic
servers. If the cert is not loaded on ckch tree, we try to access it
from the file-system.
Now this access operation is rendered optional by a new function
argument. It is only allowed at parsing time, but will be disabled for
dynamic servers at runtime.
Amaury Denoyelle [Wed, 19 May 2021 13:00:54 +0000 (15:00 +0200)]
MINOR: server: disable CLI 'set server ssl' for dynamic servers
'set server ssl' uses ssl parameters from default-server. As dynamic
servers does not reuse any default-server parameters, this command has
no sense for them.
Amaury Denoyelle [Wed, 19 May 2021 13:35:29 +0000 (15:35 +0200)]
MINOR: ssl: always initialize random generator
Explicitly call ssl_initialize_random to initialize the random generator
in init() global function. If the initialization fails, the startup is
interrupted.
This commit is in preparation for support of ssl on dynamic servers. To
be able to activate ssl on dynamic servers, it is necessary to ensure
that the random generator is initialized on startup regardless of the
config. It cannot be called at runtime as access to /dev/urandom is
required.
This also has the effect to fix the previous non-consistent behavior.
Indeed, if bind or server in the config are using ssl, the
initialization function was called, and if it failed, the startup was
interrupted. Otherwise, the ssl initialization code could have been
called through the ssl server for lua, but this times without blocking
the startup on error. Or not called at all if lua was deactivated.
RFC 7231#5.3.4 makes a difference between a completely missing
'accept-encoding' header and an 'accept-encoding' header without any values.
This case was already correctly handled by accident, because an empty accept
encoding does not match any known encoding. However this resulted in the
'other' encoding being added to the bitmap. Usually this also succeeds in
serving cached responses, because the cached response likely has no
'content-encoding', thus matching the identity case instead of not serving the
response, due to the 'other' encoding. But it's technically not 100% correct.
Fix this by special-casing 'accept-encoding' values with a length of zero and
extend the test to check that an empty accept-encoding is correctly handled.
Due to the reasons given above the test also passes without the change in
cache.c.
Vary support was added in HAProxy 2.4. This fix should be backported to 2.4+.
BUG/MINOR: server/cli: Fix locking in function processing "set server" command
The commit c7b391aed ("BUG/MEDIUM: server/cli: Fix ABBA deadlock when fqdn
is set from the CLI") introduced 2 bugs. The first one is a typo on the
server's lock label (s/SERVER_UNLOCK/SERVER_LOCK/). The second one is about
the server's lock itself. It must be acquired to execute the "agent-send"
subcommand.
The patch above is marked to be backported as far as 1.8. Thus, this one
must also backported as far 1.8.
BUG/MINOR: server/cli: Don't forget to lock server on agent-send subcommand
BUG/MINOR: resolvers: Use resolver's lock in resolv_srvrq_expire_task()
The commit dcac41806 ("BUG/MEDIUM: resolvers: Add a task on servers to check
SRV resolution status") introduced a type. In resolv_srvrq_expire_task()
function, the resolver's lock must be used instead of the resolver itself.
This patch must be backported with the patch above (at least as far as 2.2).
BUG/MINOR: backend: do not set sni on connection reuse
When reusing a backend connection, do not reapply the SNI on the
connection. It should already be defined when the connection was
instantiated on a previous connect_server invocation. As the SNI is a
parameter used to select a connection, only connection with same value
can be reused.
The impact of this bug is unknown and may be null. No memory leak has
been reported by valgrind. So this is more a cleaning fix.
This commit relies on the SF_SRV_REUSED flag and thus depends on the
following fix :
BUG/MINOR: backend: restore the SF_SRV_REUSED flag original purpose
Amaury Denoyelle [Thu, 17 Jun 2021 13:14:49 +0000 (15:14 +0200)]
BUG/MINOR: backend: restore the SF_SRV_REUSED flag original purpose
The SF_SRV_REUSED flag was set if a stream reused a backend connection.
One of its purpose is to count the total reuse on the backend in
opposition to newly instantiated connection.
However, the flag was diverted from its original purpose since the
following commit :
With this change, the flag is not set anymore if the mux is not ready
when a connection is picked for reuse. This can happen for multiplexed
connections which are inserted in the available list as soon as created
in http-reuse always mode. The goal of this change is to not retry
immediately this request in case on an error on the same server if the
reused connection is not fully ready.
This change is justified for the retry timeout handling but it breaks
other places which still uses the flag for its original purpose. Mainly,
in this case the wrong 'connect' backend counter is incremented instead
of the 'reuse' one. The flag is also used in http_return_srv_error and
may have an impact if a http server error is replied for this stream.
To fix this problem, the original purpose of the flag is restored by
setting it unconditionaly when a connection is reused. Additionally, a
new flag SF_SRV_REUSED_ANTICIPATED is created. This flag is set when the
connection is reused but the mux is not ready yet. For the timeout
handling on error, the request is retried immediately only if the stream
reused a connection without this newly anticipated flag.
BUG/MEDIUM: resolvers: Add a task on servers to check SRV resolution status
When a server relies on a SRV resolution, a task is created to clean it up
(fqdn/port and address) when the SRV resolution is considered as outdated
(based on the resolvers 'timeout' value). It is only possible if the server
inherits outdated info from a state file and is no longer selected to be
attached to a SRV item. Note that most of time, a server is attached to a
SRV item. Thus when the item becomes obsolete, the server is cleaned
up.
It is important to have such task to be sure the server will be free again
to have a chance to be resolved again with fresh information. Of course,
this patch is a workaround to solve a design issue. But there is no other
obvious way to fix it without rewritting all the resolvers part. And it must
be backportable.
This patch relies on following commits:
* MINOR: resolvers: Clean server in a dedicated function when removing a SRV item
* MINOR: resolvers: Remove server from named_servers tree when removing a SRV item
All the series must be backported as far as 2.2 after some observation
period. Backports to 2.0 and 1.8 must be evaluated.
MINOR: resolvers: Remove server from named_servers tree when removing a SRV item
When a server is cleaned up because the corresponding SRV item is removed,
we always remove the server from the srvrq's name_servers tree. For now, it
is useless because, if a server was attached to a SRV item, it means it was
already removed from the tree. But it will be mandatory to fix a bug.
MINOR: resolvers: Clean server in a dedicated function when removing a SRV item
A dedicated function is now used to clean up servers when a SRV item becomes
obsolete or when a requester is removed from a resolution. This patch is
mandatory to fix a bug.
BUG/MEDIUM: server/cli: Fix ABBA deadlock when fqdn is set from the CLI
To perform servers resolution, the resolver's lock is first acquired then
the server's lock when necessary. However, when the fqdn is set via the CLI,
the opposite is performed. So, it is possible to experience an ABBA
deadlock.
To fix this bug, the server's lock is acquired and released for each
subcommand of "set server" with an exception when the fqdn is set. The
resolver's lock is first acquired. Of course, this means we must be sure to
have a resolver to lock.
BUG/MINOR: server: Forbid to set fqdn on the CLI if SRV resolution is enabled
If a server is configured to rely on a SRV resolution, we must forbid to
change its fqdn on the CLI. Indeed, in this case, the server retrieves its
fqdn from the SRV resolution. If the fqdn is changed via the CLI, this
conflicts with the SRV resolution and leaves the server in an undefined
state. Most of time, the SRV resolution remains enabled with no effect on
the server (no update). Some time the A/AAAA resolution for the new fqdn is
not enabled at all. It depends on the server state and resolver state when
the CLI command is executed.
This patch must be backported as far as 2.0 (maybe to 1.8 too ?) after some
observation period.
Miroslav Zagorac [Tue, 15 Jun 2021 13:33:20 +0000 (15:33 +0200)]
CLEANUP: server: a separate function for initializing the per_thr field
To avoid repeating the same source code, allocating memory and initializing
the per_thr field from the server structure is transferred to a separate
function.
Willy Tarreau [Thu, 17 Jun 2021 13:39:30 +0000 (15:39 +0200)]
CI: ssl: keep the old method for ancient OpenSSL versions
I forgot about OpenSSL 1.0.2, which neither supports the build_sw target
to build only the software, nor reliably supports parallel builds. Given
that we're building 1.0.2 and 3.0.0, let's stay on the safe side and
keep 1.x sequential.
Willy Tarreau [Thu, 10 Jun 2021 05:52:23 +0000 (07:52 +0200)]
CI: ssl: do not needlessly build the OpenSSL docs
1/4 of the OpenSSL build time is spent building the docs, let's just
build the software and not the doc, by replacing the "all" target
with "build_sw". With this my build time drops from 1'28 to 1'09.
Nothing was done for the other libs, as it's unknown whether they
provide specific build targets.
Willy Tarreau [Thu, 10 Jun 2021 05:52:23 +0000 (07:52 +0200)]
CI: ssl: enable parallel builds for OpenSSL on Linux
Running the "make all" phase on my machine with -j$(nproc) shrinks the
build time from 4'52 to 1'28. It will not be that big of a change in
the CI since it looks like two CPUs are exposed, but it should still
remain a net win. Let's enable it. The install phase obviously remains
sequential however.
Tim Duesterhus [Sun, 13 Jun 2021 13:02:24 +0000 (15:02 +0200)]
CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
Using 'sudo' required quite a few workarounds in various places. Setting an
explicit 'ulimit -n' removes the requirement for 'sudo', resulting in a cleaner
workflow configuration.
Willy Tarreau [Thu, 17 Jun 2021 06:40:04 +0000 (08:40 +0200)]
BUG/MINOR: mux-h2/traces: bring back the lost "sent H2 REQ/RES" traces
In 2.4, commit d1ac2b90c ("MAJOR: htx: Remove the EOM block type and
use HTX_FL_EOM instead") changed the HTX processing to destroy the
blocks as they are processed. So the traces that were emitted at the
end of the send headers functions didn't have anything to show.
Let's move these traces earlier in the function, right before the HTX
processing, so that everything is still in place.
Willy Tarreau [Thu, 17 Jun 2021 06:29:14 +0000 (08:29 +0200)]
BUG/MINOR: mux-h2/traces: bring back the lost "rcvd H2 REQ" trace
Since commit 7d013e796 ("BUG/MEDIUM: mux-h2: Xfer rxbuf to the upper
layer when creating a front stream"), the rxbuf is lost during the
call to h2c_frt_stream_new(), so the trace that happens later cannot
find a request there and we've lost the useful part indicating what
the request looked like. Let's move the trace before this call.
Willy Tarreau [Thu, 17 Jun 2021 06:08:48 +0000 (08:08 +0200)]
MINOR: mux-h2: obey http-ignore-probes during the preface
We're seeing some browsers setting up multiple connections and closing
some to just keep one. It looks like they do this in case they'd
negotiate H1. This results in aborted prefaces and log pollution about
bad requests and "PR--" in the status flags.
We already have an option to ignore connections with no data, it's called
http-ignore-probes. But it was not used by the H2 mux. However it totally
makes sense to use it during the preface.
This patch changes this so that connections aborted before sending the
preface can avoid being logged.
This should be backported to 2.4 and 2.3 at least, and probably even
as far as 2.0.
Willy Tarreau [Thu, 17 Jun 2021 05:22:27 +0000 (07:22 +0200)]
BUG/MINOR: stats: make "show stat typed desc" work again
As part of the changes to support per-module stats data in 2.3-dev6
with commit ee63d4bd6 ("MEDIUM: stats: integrate static proxies stats
in new stats"), a small change resulted in the description field to
be replaced by the name field, making it pointless. Let's fix this
back.
This should fix issue #1291. Thanks to Nick Ramirez for reporting this
issue.
Willy Tarreau [Wed, 16 Jun 2021 16:32:42 +0000 (18:32 +0200)]
CLEANUP: mux-h2/traces: better align user messages
"sent H2 request" was already misaligned with the 3 other ones
(sent/rcvd, request/response), and now with "new H2 connection" that's
yet another alignment making the traces even less legible. Let's just
realign all 5 messages, this even eases quick pointer comparisons. This
should probably be backported to 2.4 as it's where it's the most likely
to be used in the mid-term.
Willy Tarreau [Wed, 16 Jun 2021 15:47:24 +0000 (17:47 +0200)]
MINOR: mux-h2/trace: report a few connection-level info during h2_init()
It is currently very difficult to match some H2 trace outputs against
some log extracts because there's no exactly equivalent info.
This patch tries to address this by adding a TRACE_USER() call in h2_init()
that is matched in h2_trace() to report:
- connection pointer and direction
- frontend's name or server's name
- transport layer and control layer (e.g. "SSL/tcpv4")
- source and/or destination depending on what is set
This now permits to get something like this at verbosity level complete:
This function appends to a buffer some information from a connection.
This will be used by traces and possibly some debugging as well. A
frontend/backend/server, transport/control layers, source/destination
ip:port, connection pointer and direction are reported depending on
the available information.
Willy Tarreau [Wed, 16 Jun 2021 13:06:43 +0000 (15:06 +0200)]
BUG/MINOR: mux-h1: do not skip the error response on bad requests
Since 2.4-dev3 with commit c4bfa59f1 ("MAJOR: mux-h1: Create the client
stream as later as possible"), a request error doesn't result in any
error response if "option http-ignore-probes" is set, there's just a
close. This is caused by an unneeded b_reset() in h1_process_demux()'s
error path, which makes h1_handle_bad_req() believe there was an empty
request. There is no reason for this reset to be there, it must have
been a leftover of an earlier attempt at dealing with the error, let's
drop it.
Willy Tarreau [Wed, 9 Jun 2021 13:56:16 +0000 (15:56 +0200)]
MINOR: backend: only skip LB when there are actual connections
In 2.3, a significant improvement was brought against situations where
the queue was heavily used, because some LB algos were still checked
for no reason before deciding to put the request into the queue. This
was commit 82cd5c13a ("OPTIM: backend: skip LB when we know the backend
is full").
As seen in previous commit ("BUG/MAJOR: queue: set SF_ASSIGNED when
setting strm->target on dequeue") the dequeuing code is extremely
tricky, and the optimization above tends to emphasize transient issues
by making them permanent until the next reload, which is not acceptable
as the code must always be robust against any bad situation.
This commit brings a protection against such a situation by slightly
relaxing the test. Instead of checking that there are pending connections
in the backend queue, it also verifies that the backend's connections are
not solely composed of queued connections, which would then indicate we
are in this situation. This is not rocket science, but at least if the
situation happens, we know that it will unlock by itself once the streams
have left, as new requests will be allowed to reach the servers and to
flush the queue again.
Willy Tarreau [Wed, 16 Jun 2021 06:42:23 +0000 (08:42 +0200)]
BUG/MAJOR: queue: set SF_ASSIGNED when setting strm->target on dequeue
Commit 82cd5c13a ("OPTIM: backend: skip LB when we know the backend is
full") has uncovered a long-burried bug in the dequeing code: when a
server releases a connection, it picks a new one from the proxy's or
its queue. Technically speaking it only picks a pendconn which is a
link between a position in the queue and a stream. It then sets this
pendconn's target to itself, and wakes up the stream's task so that
it can try to connect again.
The stream then goes through the regular connection setup phases,
calls back_try_conn_req() which calls pendconn_dequeue(), which
sets the stream's target to the pendconn's and releases the pendconn.
It then reaches assign_server() which sees no SF_ASSIGNED and calls
assign_server_and_queue() to perform load balancing or queuing. This
one first destroys the stream's target and gets ready to perform load
balancing. At this point we're load-balancing for no reason since we
already knew what server was available. And this is where the commit
above comes into play: the check for the backend's queue above may
detect other connections that arrived in between, and will immediately
return FULL, forcing this request back into the queue. If the server
had a very low maxconn (e.g. 1 due to a long slowstart), it's possible
that this evicted connection was the last one on the server and that
no other one will ever be present to process the queue. Usually a
regularly processed request will still have its own srv_conn that will
be used during stream_free() to dequeue other connections. But if the
server had a down-up cycle, then a call to pendconn_grab_from_px()
may start to dequeue entries which had no srv_conn and which will have
no server slot to offer when they expire, thus maintaining the situation
above forever. Worse, as new requests arrive, there are always some
requests in the queue and the situation feeds on itself.
The correct fix here is to properly set SF_ASSIGNED in pendconn_dequeue()
when the stream's target is assigned (as it's what this flag means), so
as to avoid a load-balancing pass when dequeuing.
Many thanks to Pierre Cheynier for the numerous detailed traces he
provided that helped narrow this problem down.
This could be backported to all stable versions, but in practice only
2.3 and above are really affected since the presence of the commit
above. Given how tricky this code is it's better to limit it to those
versions that really need it.
Willy Tarreau [Tue, 15 Jun 2021 14:11:33 +0000 (16:11 +0200)]
CLEANUP: shctx: remove the different inter-process locking techniques
With a single process, we don't need to USE_PRIVATE_CACHE, USE_FUTEX
nor USE_PTHREAD_PSHARED anymore. Let's only keep the basic spinlock
to lock between threads.
Willy Tarreau [Tue, 15 Jun 2021 07:08:18 +0000 (09:08 +0200)]
MEDIUM: global: remove the relative_pid from global and mworker
The relative_pid is always 1. In mworker mode we also have a
child->relative_pid which is always equalt relative_pid, except for a
master (0) or external process (-1), but these types are usually tested
for, except for one place that was amended to carefully check for the
PROC_O_TYPE_WORKER option.
Changes were pretty limited as most usages of relative_pid were for
designating a process in stats output and peers protocol.
Willy Tarreau [Tue, 15 Jun 2021 06:49:05 +0000 (08:49 +0200)]
MEDIUM: config: simplify cpu-map handling
As there's no more nbproc>1, we can remove some loops and tests in cpu-map.
Both the lack of thread number and thread 1 can count as the whole process
now (which is still used for whole process binding when threads are disabled).
Willy Tarreau [Tue, 15 Jun 2021 06:36:30 +0000 (08:36 +0200)]
MEDIUM: global: remove dead code from nbproc/bind_proc removal
Lots of places iterating over nbproc or comparing with nbproc could be
simplified. Further, "bind-process" and "process" parsing that was
already limited to process 1 or "all" or "odd" resulted in a bind_proc
field that was either 0 or 1 during the init phase and later always 1.
All the checks for compatibilities were removed since it's not possible
anymore to run a frontend and a backend on different processes or to
have peers and stick-tables bound on different ones. This is the largest
part of this patch.
The bind_proc field was removed from both the proxy and the receiver
structs.
Since the "process" and "bind-process" directives are still parsed,
configs making use of correct values allowing process 1 will continue
to work.
Willy Tarreau [Tue, 15 Jun 2021 05:58:09 +0000 (07:58 +0200)]
MEDIUM: init: remove the loop over processes during init
There was a loop iterating over all nbproc values during init that
couldn't be immediately removed because the loop's index was used
to distinguish a child from a parent. That's now fixed by replacing
the iterator with an in_parent flag. All bindings that were checking
(1UL << proc) or cpu_map.proc[proc] were adjusted to always use zero
for proc.
Willy Tarreau [Tue, 15 Jun 2021 09:39:57 +0000 (11:39 +0200)]
CLEANUP: global: remove unused definition of stopping_task[]
This is a leftover of a previous attempt that was introduced in 2.4 by
commit d3a88c1c3 ("MEDIUM: connection: close front idling connection on
soft-stop"). It can be backported, as the variable doesn't exist.
Willy Tarreau [Tue, 15 Jun 2021 06:59:19 +0000 (08:59 +0200)]
BUG/MINOR: mworker: fix typo in chroot error message
Since its introduction in 1.8 with commit 095ba4c24 ("MEDIUM: mworker:
replace systemd mode by master worker mode"), it says "cannot chroot1(...)"
which seems to be a leftover of a debug message. It could be backported but
probably nobody will notice.
Willy Tarreau [Tue, 15 Jun 2021 14:39:22 +0000 (16:39 +0200)]
BUG/MINOR: ssl: use atomic ops to update global shctx stats
The global shctx lookups and misses was updated without using atomic
ops, so the stats available in "show info" are very likely off by a few
units over time. This should be backported as far as 1.8. Versions
without _HA_ATOMIC_INC() can use HA_ATOMIC_ADD(,1).
Willy Tarreau [Tue, 15 Jun 2021 13:03:19 +0000 (15:03 +0200)]
BUG/MEDIUM: shctx: use at least thread-based locking on USE_PRIVATE_CACHE
Since threads were introduced in 1.8, the USE_PRIVATE_CACHE mode of the
shctx was not updated to use locks. Originally it was meant to disable
sharing between processes, so it removes the lock/unlock instructions.
But with threads enabled, it's not possible to work like this anymore.
It's easy to see that once built with private cache and threads enabled,
sending violent SSL traffic to the the process instantly makes it die.
The HTTP cache is very likely affected as well.
This patch addresses this by falling back to our native spinlocks when
USE_PRIVATE_CACHE is used. In practice we could use them also for other
modes and remove all older implementations, but this patch aims at keeping
the changes very low and easy to backport. A new SHCTX_LOCK label was
added to help with debugging, but OTHER_LOCK might be usable as well
for backports.
An even lighter approach for backports may consist in always declaring
the lock (or reusing "waiters"), and calling pl_take_s() for the lock()
and pl_drop_s() for the unlock() operation. This could even be used in
all modes (process and threads), even when thread support is disabled.
Subsequent patches will further clean up this area.
This patch must be backported to all supported versions since 1.8.
BUG/MEDIUM: server: do not auto insert a dynamic server in px addr_node
Until then, the servers were automatically attached on their creation
into the proxy addr_node tree via _srv_parse_init. In case of an invalid
dynamic server which is instantly freed, no detach operation was made
leaving a NULL server in the tree.
Change this mode of operation by marking the attach operation as
optional in _srv_parse_init. This operation is not conduct for a dynamic
server. The server is attached only at the end of the CLI handler when
it is marked as valid.
BUG/MINOR: server: do not keep an invalid dynamic server in px ids tree
A bug is present when trying to create a dynamic server with a fixed id.
If the server is detected invalid due to a later parsing arguments
error, the server is not removed from the proxy used ids tree before
being freed.
Change the mode of operation of 'id' keyword parsing handler. The
insertion in the backend tree is removed from the handler and is not
taken in charge by parse_server for configuration parsing. For the
dynamic servers, the insertion is called at the end of the 'add server'
CLI handler when the server has been validated.
BUG/MEDIUM: server: do not forget to generate the dynamic servers ids
If no id is specified by the user for a dynamic server, it is necessary
to generate a new one. This operation is now done at the end of 'add
server' CLI handler. The server is then inserted into the proxy ids
tree.
Without this, several features may be broken for dynamic servers. Among
them, there is the "first" lb algorithm, the persistence using
stick-tables or the uniqueness internal check of srv_parse_id.
BUG/MEDIUM: server: clear dynamic srv on delete from proxy id/name trees
Do not leave deleted server in used_server_id/used_server_addr backend
trees. This might lead to crashes if a deleted server is used through
these trees.
At this moment, dynamic servers are only added in used_server_id if they
have a fixed id. They are never inserted in used_server_addr as this
code is missing. So these new delete instructions are noop. However, a
fix will be provided soon to insert properly all dynamic servers in both
used_server_id and used_server_addr trees so the deletion counterpart
will be mandatory in the CLI server delete handler.
Amaury Denoyelle [Thu, 10 Jun 2021 13:26:44 +0000 (15:26 +0200)]
BUG/MEDIUM: server: extend thread-isolate over much of CLI 'add server'
Some config parsing handlers were designed to be run at startup on a
single-thread. When executing at runtime for dynamic servers,
thread-safety is not guaranteed. This is the case for example in
srv_parse_id which manipulates backend used_ids tree.
One solution could be to add locks but it might be tricky to found all
affected functions and it can be an easy source of deadlock. The other
solution which has been chosen is to use thread-isolation over almost
all of the cli_parse_add_server CLI handler.
For now this solution is sufficient. If some users make heavy use of the
'add server', hurting the overall performance, it will be necessary to
design a much thinner solution.
Amaury Denoyelle [Mon, 14 Jun 2021 15:04:25 +0000 (17:04 +0200)]
BUG/MINOR: stick-table: insert srv in used_name tree even with fixed id
If the server id is fixed in the configuration, it is immediately
inserted in the 'used_server_id' backend tree via srv_parse_id. On
check_config_validity, the dynamic id generation is thus skipped for
fixed-id servers. However, it must nevertheless be inserted in the
'used_server_name' backend tree.
This bug seems to be not noticeable for the user. Indeed, before the
fix, the search in sticking_rule_find_target always returned NULL for
the name, then the fallback search with server id succeeded, so the
persistence is properly applied. However with the fix the fallback
search is not executed anymore, which saves from the locking of
STK_SESS.
DOC: lua: Add a warning about buffers modification in HTTP
Since the 1.9, it is forbidden to alter the channel buffer from an HTTP
stream because there is no way to keep the HTTP parser synchronized if the
buffer content is altered. In addition, since the HTX is the only
reprensentation for HTTP messages, the data in HTTP buffers are structured
and cannot be read or updated in a raw fashion.
A warning is triggered when a user tries to alter an HTTP buffer. However,
it was not documented. This patch adds a warning in the lua documentation.
This patch is related to the issue #1287. It may be backported as far as
2.0.
Willy Tarreau [Fri, 11 Jun 2021 14:50:29 +0000 (16:50 +0200)]
MAJOR: config: remove parsing of the global "nbproc" directive
This one was deprecated in 2.3 and marked for removal in 2.5. It suffers
too many limitations compared to threads, and prevents some improvements
from being engaged. Instead of a bypassable startup error, there is now
a hard error.
The parsing code was removed, and very few obvious cases were as well.
The code is deeply rooted at certain places (e.g. "for" loops iterating
from 0 to nbproc) so it will not be that trivial to remove everywhere.
The "bind" and "bind-process" parsers will have to be adjusted, though
maybe not completely changed if we later want to support thread groups
for large NUMA machines. Some stats socket restrictions were removed,
and the doc was updated according to what was done. A few places in the
doc still refer to nbproc and will have to be revisited. The master-worker
code also refers to the process number to distinguish between master and
workers and will have to be carefully adjusted. The MAX_PROCS macro was
reset to 1, this will at least reduce the size of some remaining arrays.
Two regtests were dependieng on this directive, one with an explicit
"nbproc 1" and another one testing the master's CLI using nbproc 4.
Both were adapted.
Willy Tarreau [Fri, 11 Jun 2021 14:27:10 +0000 (16:27 +0200)]
MEDIUM: proxy: remove the deprecated "grace" keyword
Commit ab0a5192a ("MEDIUM: config: mark "grace" as deprecated") marked
the "grace" keyword as deprecated in 2.3, tentative removal for 2.4
with a hard deadline in 2.5, so let's remove it and return an error now.
This old and outdated feature was incompatible with soft-stop, reload
and socket transfers, and keeping it forced ugly hacks in the lower
layers of the protocol stack.
It was marked as deprecated in 2.1-dev2 and for removal in 2.2, but it
was missed. A warning was already emitted and the doc didn't refer to
it any more, let's now get rid of it.
It's been warning as being deprecated since 2.0-dev4, it's about time
to drop it now. The error message recommends to either remove it or
use "option httpclose" instead. It's still referred to in the old
internal doc about the connection header, which itself seems highly
inaccurate by now.
Willy Tarreau [Fri, 11 Jun 2021 13:46:02 +0000 (15:46 +0200)]
MINOR: http: remove the long deprecated "set-cookie()" sample fetch function
This one was marked as deprecated 9 years ago by commit 28376d62c
("MEDIUM: http: merge ACL and pattern cookie fetches into a single one")
and has disappeared from any documentation, so it never appeared in any
released version. Let's remove it now.
Willy Tarreau [Fri, 11 Jun 2021 13:37:45 +0000 (15:37 +0200)]
MINOR: log: remove the long-deprecated early log-format tags
The following 10 log-format tags were implemented during log-format
development and changed before the release. They were marked as deprecated
in 2012 by commit 2beef5888 ("MEDIUM: log: change a few log tokens to make
them easier to remember") and were not documented. They've been emitting a
warning since then, with a suggestion of the one to use instead. Let's get
rid of them now.
Bi => bi, Bp => bp, Ci => ci, Cp => cp, Fi => fi
Fp => fp, Si => si, Sp => sp, cc => CC, cs => CS
Willy Tarreau [Fri, 11 Jun 2021 13:29:31 +0000 (15:29 +0200)]
MINOR: config: remove support for deprecated option "tune.chksize"
It was marked as deprecated for immediate removal as it was not used,
let's reject it and remove it from the doc. A specific error suggests
to check tune.bufsize instead.
BUG/MINOR: server-state: load SRV resolution only if params match the config
When the state of a server is loaded, if there is no hostname defined for
this server and if a fqdn and a server record are retrieved from the state
file, it means the server should rely on a SRV resolution. But we must be
sure the server is configured this way. A SRV resolution must be configured
with the same SRV record. This part must be skipped if there is no SRV
resolution configured for this server or if the SRV record used is not the
same.
This patch should be backported as far as 1.8 after some observation period.
Emeric Brun [Fri, 11 Jun 2021 08:48:45 +0000 (10:48 +0200)]
MEDIUM: resolvers: add a ref between servers and srv request or used SRV record
This patch add a ref into servers to register them onto the
record answer item used to set their hostnames.
It also adds a head list into 'srvrq' to register servers free
to be affected to a SRV record.
A head of a tree is also added to srvrq to put servers which
present a hotname in server state file. To re-link them fastly
to the matching record as soon an item present the same name.
This results in better performances on SRV record response
parsing.
This is an optimization but it could avoid to trigger the haproxy's
internal wathdog in some circumstances. And for this reason
it should be backported as far we can (2.0 ?)
Emeric Brun [Fri, 11 Jun 2021 08:08:05 +0000 (10:08 +0200)]
MEDIUM: resolvers: add a ref on server to the used A/AAAA answer item
This patch adds a head list into answer items on servers which use
this record to set their IPs. It makes lookup on duplicated ip faster and
allow to check immediatly if an item is still valid renewing the IP.
This results in better performances on A/AAAA resolutions.
This is an optimization but it could avoid to trigger the haproxy's
internal wathdog in some circumstances. And for this reason
it should be backported as far we can (2.0 ?)
Emeric Brun [Thu, 10 Jun 2021 13:25:25 +0000 (15:25 +0200)]
BUG/MINOR: resolvers: answser item list was randomly purged or errors
In case of SRV records, The answer item list was purged by the
error callback of the first requester which considers the error
could not be safely ignored. It makes this item list unavailable
for subsequent requesters even if they consider the error
could be ignored.
On A resolution or do_resolve action error, the answer items were
never trashed.
This patch re-work the error callbacks and the code to check the return code
If a callback return 1, we consider the error was ignored and
the answer item list must be kept. At the opposite, If all error callbacks
of all requesters of the same resolution returns 0 the list will be purged
CLEANUP: l7-retries: do not test the buffer before calling b_alloc()
The return value is enough now to know if the allocation succeeded or
failed.
This cleanup was already pushed by Willy (f499f50) but a revert crushed
it. It may be backported to the 2.4 because the original patch was done on
this version.
BUG/MINOR: h1-htx: Fix a signess bug with char data type when parsing chunk size
On some platform, a char may be unsigned. Of course, we should not rely on
the signess of a char to be portable. Unfortunatly, since the commit a835f3cb ("MINOR: h1-htx: Use a correlation table to speed-up small chunks
parsing") we rely on it to test the value retrieved from the hexadecimal
correlation table when the size of a chunk is parsed.
To fix the bug, we now test the result is in the range [0,15] with a bitwise
AND.
This patch should fix the issue #1272. It is 2.5-specific, no backport is
needed except if the commit above is backported.
BUG/MINOR: mux-fcgi: Expose SERVER_SOFTWARE parameter by default
As specified in the RFC3875 (section 4.1.17), this parameter must be set to
the name and version of the information server software making the CGI
request. Thus, it is now added to the default parameters defined by
HAProxy. It is set to the string "HAProxy $version".
This patch should fix the issue #1285 and must be backported as far as 2.2.
BUG/MAJOR: htx: Fix htx_defrag() when an HTX block is expanded
When an HTX block is expanded, a defragmentation may be performed first to
have enough space to copy the new data. When it happens, the meta data of
the HTX message must take account of the new data length but copied data are
still unchanged at this stage (because we need more space to update the
message content). And here there is a bug because the meta data are updated
by the caller. It means that when the blocks content is copied, the new
length is already set. Thus a block larger than the reality is copied and
data outside the buffer may be accessed, leading to a crash.
To fix this bug, htx_defrag() is updated to use an extra argument with the
new meta data to use for the referenced block. Thus the caller does not need
to update the HTX message by itself. However, it still have to update the
data.
Most of time, the bug will be encountered in the HTTP compression
filter. But, even if it is highly unlikely, in theory it is also possible to
hit it when a HTTP header (or only its value) is replaced or when the
start-line is changed.
REGTESTS: ssl: show_ssl_ocspresponce.vtc is broken with BoringSSL
The `show ssl ocsp-response` feature is not available with BoringSSL,
but we don't have a way to disable this feature only with boringSSL on
the CI. Disable the reg-test until we do.
Willy Tarreau [Fri, 11 Jun 2021 05:31:57 +0000 (07:31 +0200)]
BUG/MEDIUM: errors: include missing obj_type file
A tiny change in commit 6af81f80f ("MEDIUM: errors: implement parsing
context type") triggered an awful bug in gcc 5 and below (4.7.4 to 5.5
confirmed affected, at least on aarch64/mips/x86_64) causing the startup
to loop forever in acl_find_target().
This was tracked down to the acl.c file seeing a different definition
of the struct proxy than other files. The reason for this is that it
sees an unpacked "enum obj_type" (4 bytes) while others see it packed
(1 byte), thus all fields in the struct are having a different
alignment, and the "acl" list is shifted one pointer to the next struct
and seems to loop onto itself.
The commit above did nothing more than adding "enum obj_type *obj" in a
new struct without including obj_type.h, and that was apparently enough
for the compiler to internally declare obj_type as a regular enum and
silently ignore the packed attribute that it discovers later, so depending
on the order of includes, some files would see it as 1 byte and others as
4.
This patch simply adds the missing include but due to the nature of the
bug, probably that creating a special "packed_enum" definition to disable
the packed attribute on such compilers could be a safer option.
Willy Tarreau [Thu, 10 Jun 2021 15:31:48 +0000 (17:31 +0200)]
MEDIUM: pools: remove the locked pools implementation
Now that the modified lockless variant does not need a DWCAS anymore,
there's no reason to keep the much slower locked version, so let's
just get rid of it.
Willy Tarreau [Wed, 9 Jun 2021 16:59:58 +0000 (18:59 +0200)]
BUG/MAJOR: pools: fix possible race with free() in the lockless variant
In GH issue #1275, Fabiano Nunes Parente provided a nicely detailed
report showing reproducible crashes under musl. Musl is one of the libs
coming with a simple allocator for which we prefer to keep the shared
cache. On x86 we have a DWCAS so the lockless implementation is enabled
for such libraries.
And this implementation has had a small race since day one: the allocator
will need to read the first object's <next> pointer to place it into the
free list's head. If another thread picks the same element and immediately
releases it, while both the local and the shared pools are too crowded, it
will be freed to the OS. If the libc's allocator immediately releases it,
the memory area is unmapped and we can have a crash while trying to read
that pointer. However there is no problem as long as the item remains
mapped in memory because whatever value found there will not be placed
into the head since the counter will have changed.
The probability for this to happen is extremely low, but as analyzed by
Fabiano, it increases with the buffer size. On 16 threads it's relatively
easy to reproduce with 2MB buffers above 200k req/s, where it should
happen within the first 20 seconds of traffic usually.
This is a structural issue for which there are two non-trivial solutions:
- place a read lock in the alloc call and a barrier made of lock/unlock
in the free() call to force to serialize operations; this will have
a big performance impact since free() is already one of the contention
points;
- change the allocator to use a self-locked head, similar to what is
done in the MT_LISTS. This requires two memory writes to the head
instead of a single one, thus the overhead is exactly one memory
write during alloc and one during free;
This patch implements the second option. A new POOL_DUMMY pointer was
defined for the locked pointer value, allowing to both read and lock it
with a single xchg call. The code was carefully optimized so that the
locked period remains the shortest possible and that bus writes are
avoided as much as possible whenever the lock is held.
Tests show that while a bit slower than the original lockless
implementation on large buffers (2MB), it's 2.6 times faster than both
the no-cache and the locked implementation on such large buffers, and
remains as fast or faster than the all implementations when buffers are
48k or higher. Tests were also run on arm64 with similar results.
Note that this code is not used on modern libcs featuring a fast allocator.
A nice benefit of this change is that since it removes a dependency on
the DWCAS, it will be possible to remove the locked implementation and
replace it with this one, that is then usable on all systems, thus
significantly increasing their performance with large buffers.
Given that lockless pools were introduced in 1.9 (not supported anymore),
this patch will have to be backported as far as 2.0. The code changed
several times in this area and is subject to many ifdefs which will
complicate the backport. What is important is to remove all the DWCAS
code from the shared cache alloc/free lockless code and replace it with
this one. The pool_flush() code is basically the same code as the
allocator, retrieving the whole list at once. If in doubt regarding what
barriers to use in older versions, it's safe to use the generic ones.
This patch depends on the following previous commits:
- MINOR: pools: do not maintain the lock during pool_flush()
- MINOR: pools: call malloc_trim() under thread isolation
- MEDIUM: pools: use a single pool_gc() function for locked and lockless
The last one also removes one occurrence of an unneeded DWCAS in the
code that was incompatible with this fix. The removal of the now unused
seq field will happen in a future patch.
Many thanks to Fabiano for his detailed report, and to Olivier for
his help on this issue.
Willy Tarreau [Thu, 10 Jun 2021 08:21:35 +0000 (10:21 +0200)]
MEDIUM: pools: use a single pool_gc() function for locked and lockless
Locked and lockless shared pools don't need to use a different pool_gc()
function because this function isolates itself during the operation, so
we do not need to rely on DWCAS nor any atomic operation in fact. Let's
just get rid of the lockless one in favor of the simple one. This should
even result in a faster execution.
The ifdefs were slightly moved so that we can have pool_gc() defined
as soon as there are global pools, this avoids duplicating the function.
projects / thirdparty / haproxy.git / log
