]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
20 months agoMEDIUM: stconn/muxes: Loop on data fast-forwarding to forward at least a buffer
Christopher Faulet [Tue, 7 Nov 2023 09:44:06 +0000 (10:44 +0100)] 
MEDIUM: stconn/muxes: Loop on data fast-forwarding to forward at least a buffer

In the mux-to-mux data forwarding, we now try, as far as possible to send at
least a buffer. Of course, if the consumer side is congested or if nothing
more can be received, we leave. But the idea is to retry to fast-forward
data if less than a buffer was forwarded. It is only performed for buffer
fast-forwarding, not splicing.

The idea behind this patch is to optimise the forwarding, when a first
forward was performed to complete a buffer with some existing data. In this
case, the amount of data forwarded is artificially limited because we are
using a non-empty buffer. But without this limitation, it is highly probable
that a full buffer could have been sent. And indeed, with H2 client, a
significant improvement was observed during our test.

To do so, .done_fastfwd() callback function must be able to deal with
interim forwards. Especially for the H2 mux, to remove H2_SF_NOTIFIED flags
on the H2S on the last call only. Otherwise, the H2 stream can be blocked by
itself because it is in the send_list. IOBUF_FL_INTERIM_FF iobuf flag is
used to notify the consumer it is not the last call. This flag is then
removed on the last call.

20 months agoBUG/MEDIUM: pool: try once to allocate from another bucket if empty
Willy Tarreau [Wed, 8 Nov 2023 15:44:20 +0000 (16:44 +0100)] 
BUG/MEDIUM: pool: try once to allocate from another bucket if empty

In order to limit inter-thread contention on the global pool, in 2.9-dev3
with commit 7bf829ace ("MAJOR: pools: move the shared pool's free_list
over multiple buckets"), it was decided that if the selected bucket had
an empty free list, we would simply give up and fall back to the OS
allocator.

But this causes allocations to be made from the OS for certain threads,
to be released to overloaded pools that are sent back to the OS. One
visible effect is that sending a lot of traffic using h2load with 100
parallel streams over 100 connections causes 5-10k buffers to be
allocated, then reducing the load to only 10 connections doesn't make
these allocations go down, just because some buckets are no longer
visited.

Tests show that giving a second chance to pick another bucket in this
case is sufficient to visit all other buckets and recycle their pending
objects. Now "show pools" that starts at 10k buffers at 100 connections
goes down to about 150 with 1 connection and 100 streams in a fraction
of a second.

No backport is needed, as the issue is only in 2.9.

20 months agoBUG/MINOR: pool: check one other random bucket on alloc conflict
Willy Tarreau [Wed, 8 Nov 2023 15:44:20 +0000 (16:44 +0100)] 
BUG/MINOR: pool: check one other random bucket on alloc conflict

Since 2.9-dev3 with commit 7bf829ace ("MAJOR: pools: move the shared
pool's free_list over multiple buckets"), the global pool supports
multiple heads to reduce inter-thread contention. However, when
grabbing a freelist head fails because another thread is already
picking from it, we just skip to the next one and try again.

Unfortunately, it still maintains a bit of contention between thread
pairs when for some reasons only a few threads are used. This may
happen for example when running on a 4- or 8- thread system and
the two most active ones end up on adjacent buckets.

A better and much simpler solution consists in visiting a random bucket
instead of the current one. Tests show that the CPU usage spent in
pool_refill_local_from_shared() reduces at low number of connections
(hence threads).

No backport is needed, as the issue is only in 2.9.

20 months agoBUG/MEDIUM: pool: fix releasable pool calculation when overloaded
Willy Tarreau [Wed, 8 Nov 2023 15:35:49 +0000 (16:35 +0100)] 
BUG/MEDIUM: pool: fix releasable pool calculation when overloaded

In 2.6-dev1, the method used to decide how many pool entries could be
released at once was revisited to support releases in batches. This was
done with commits 91a8e28f9 ("MINOR: pool: add a function to estimate
how many may be released at once") and 361e31e3f ("MEDIUM: pool: compute
the number of evictable entries once per pool").

The first commit takes care of the possible inconsistency between the
moment the allocated count and the used count are read, but unfortunately
fixed it the wrong way, by adjusting "used" to match "alloc" whenever it
was lower (i.e. almost always). This results in a nasty case which is that
as soon as the allocated value becomes higher than the estimated count of
needed entries, we end up returning pool->minavail, which causes very
small batches to be released, starting from commit 1513c5479 ("MEDIUM:
pools: release cached objects in batches").

The problem was further amplified in 2.9-dev3 with commit 7bf829ace
("MAJOR: pools: move the shared pool's free_list over multiple buckets")
because it now becomes possible for a thread to allocate from one bucket
and release into a few other different ones, causing an accumulation of
entries in that bucket.

The fix is trivial, simply adjust the alloc counter if the used one is
higher, before performing operations.

This must be backported to 2.6.

20 months agoBUG/MEDIUM: freq-ctr: Don't report overshoot for long inactivity period
Christopher Faulet [Tue, 7 Nov 2023 18:16:51 +0000 (19:16 +0100)] 
BUG/MEDIUM: freq-ctr: Don't report overshoot for long inactivity period

The function returning the excess of events over the current period for a
target frequency (the overshoot) has a flaw if the inactivity period is too
long. In this case, the result may overflow. Instead to be negative, a very
high positive value is returned.

This function is used by the bandwidth limitation filter. It means after a
long inactivity period, a huge burst may be detected while it should not.

In fact, the problem arise from the moment we're past the current period. In
this case, we should not report any overshoot and just get the number of
remaining events as usual.

This patch should be backported as far as 2.7.

20 months agoBUG/MINOR: mux-h1: Properly handle http-request and http-keep-alive timeouts
Christopher Faulet [Tue, 7 Nov 2023 17:36:02 +0000 (18:36 +0100)] 
BUG/MINOR: mux-h1: Properly handle http-request and http-keep-alive timeouts

It is now the turn for the H1 mux to be fix to properly handle http-request
and http-keep-alive timeouts. It is quite surprising but it is broken since
the 2.2. For idle connections on client side, the smallest value between the
client timeout and the http-request/http-keep-alive timeout is used while
the client timeout should only be used if other ones are not defined. So, if
the client timeout is the smallest value, the keep-alive timeout is not
respected.

It is only an issue for idle client connections. The http-request timeout is
respected from the moment part of the next request was received.

This patch should fix the issue #2334. It must be backported as far as 2.2. But
be careful during the backports. The H1 mux had evolved a lot since the 2.2.

20 months agoMINOR: stktable/cli: support v6tov4 and v4tov6 conversions
Aurelien DARRAGON [Wed, 8 Nov 2023 10:35:05 +0000 (11:35 +0100)] 
MINOR: stktable/cli: support v6tov4 and v4tov6 conversions

Add a special treatment for the IPV4 and IPV6 cases in
table_process_entry_per_key() function so that input string is parsed
in best effort (STR to pseudo type ADDR): input format is first considered
over table type and then let smp_to_stkey() do the type conversion for us
when needed.

This patch heavily depends on:
- "MEDIUM: stktable/cli: simplify entry key handling"

And optionally depends on:
72514a44 ("MEDIUM: tools/ip: v4tov6() and v6tov4() rework")

20 months agoMEDIUM: stktable/cli: simplify entry key handling
Aurelien DARRAGON [Mon, 6 Nov 2023 16:53:00 +0000 (17:53 +0100)] 
MEDIUM: stktable/cli: simplify entry key handling

Make use of smp_to_stkey() in table_process_entry_per_key() to simplify
key handling and leverage auto type conversions from sample API.

One noticeable side effect is that integer input checks will be relaxed
given that c_str2int() sample conv is more permissible than the integrated
table_process_entry_per_key() integer parser.

20 months agoBUG/MINOR: stick-table/cli: Check for invalid ipv4 key
Aurelien DARRAGON [Mon, 28 Aug 2023 11:57:19 +0000 (13:57 +0200)] 
BUG/MINOR: stick-table/cli: Check for invalid ipv4 key

When an ipv4 key is used to filter a CLI command on a stick table
clear/set/show table ...), inetaddr_host+htonl combination was used
with no error checking.

Instead, we now use inet_pton(), which is what we use for ipv6 addresses
since b7c962b0c0 ("BUG/MINOR: stick-table/cli: Check for invalid ipv6 key")

Doing this allows us to easily check for parsing errors: we're trading off
some parsing efficience to better catch input errors and ensure we get
similar behavior between ipv4 and ipv6 addresses handling.

This patch may be backported to all supported versions.

20 months agoBUG/MINOR: mux-h1: Release empty ibuf during data fast-forwarding
Christopher Faulet [Wed, 8 Nov 2023 15:34:32 +0000 (16:34 +0100)] 
BUG/MINOR: mux-h1: Release empty ibuf during data fast-forwarding

We must take care to release H1 input buffer when it is emptied during the
fast-forwarding nego. Otherwise, it may be kept allocated for a while,
waiting for the next "normal" receive or the H1C release.

No backport needed.

20 months agoMINOR: proto_reverse_connect: use connect timeout
Amaury Denoyelle [Tue, 7 Nov 2023 16:10:38 +0000 (17:10 +0100)] 
MINOR: proto_reverse_connect: use connect timeout

Use backend connect timeout when a new connection is instantiated for
rhttp. This ensures that if connect operation fails after a certain
delay, reverse_connect listener task is woken up. This allows to free
the current connection and retry a new connect.

As a consequence of this change, rev_process() may be woken up even if
connection is not reported with CO_FL_ERROR. This happens if timeout
fired before any network reported issue. Connection freeing is adjusted
as in this case MUX instance is already allocated. Use destroy callback
to release MUX context prior to the connection itself.

This patch is really useful as a side measure for a haproxy bug
impacting connect with SSL for both backend connections and active
reverse connect. This is caused by the delayed allocation of MUX
allocation. Asynchronous connect error detected at the socket layer is
not notified to upper layers. Currently, only connect timeout allows to
release this failed connection.

20 months agoBUG/MEDIUM: mux-h1: Be sure xprt support splicing to use it during fast-forward
Christopher Faulet [Tue, 7 Nov 2023 17:16:33 +0000 (18:16 +0100)] 
BUG/MEDIUM: mux-h1: Be sure xprt support splicing to use it during fast-forward

The commit d6d4abdc3 ("BUILD: mux-h1: Fix build without kernel splicing
support") introduced a regression. The kernel support for the underlying
XPRT is no longer checked. So it is possible to enable the splicing for SSL
connection. This of course leads to a segfault.

This patch restore the test on the xprt rcv_pipe/snd_pipe functions.

This patch should fix a crash reported by Tristan in #2095
(#issuecomment-1788949014). No backport needed.

20 months agoBUG/MEDIUM: quic: fix sslconns on quic_conn alloc failure
Amaury Denoyelle [Mon, 6 Nov 2023 16:47:17 +0000 (17:47 +0100)] 
BUG/MEDIUM: quic: fix sslconns on quic_conn alloc failure

QUIC connections are accounted inside global sslconns. As with QUIC
actconn, it suffered from a similar issue if an intermediary allocation
failed inside qc_new_conn().

Fix this similarly by moving increment operation inside qc_new_conn().
Increment and error path are now centralized and much easier to
validate.

The consequences are similar to the actconn fix : on memory allocation
global sslconns may wrap, this time blocking any future QUIC or SSL
connections on the process.

This must be backported up to 2.6.

20 months agoBUG/MEDIUM: quic: fix actconn on quic_conn alloc failure
Amaury Denoyelle [Mon, 6 Nov 2023 16:45:14 +0000 (17:45 +0100)] 
BUG/MEDIUM: quic: fix actconn on quic_conn alloc failure

Since the following commit, quic_conn instances are accounted into
global actconn and compared against maxconn.

  commit 7735cf3854eb155a50a5ea747406f2a25657e25c
  MEDIUM: quic: count quic_conn instance for maxconn

Increment is always done prior to real allocation to guarantee minimal
resource consumption. Special care is taken to ensure there will always
be one decrement operation for each increment. To help this, decrement
is centralized in quic_conn_release().

This behaves incorrectly in case of an intermediary allocation failure
inside qc_new_conn(). In this case, quic_conn_release() will decrement
actconn. Then, a NULL qc is returned in quic_rx_pkt_retrieve_conn()
which will also decrement the counter on its own error code path.

To properly fix this, actconn incrementation has been moved directly
inside qc_new_conn(). It is thus easier to cover every cases :
* if alloc failure before or on pool_head_quic_conn, actconn is
  decremented manually at the end of qc_new_conn()
* after this step, actconn will be decremented by quic_conn_release()
  either on intermediary alloc failure or on proper connection release

This bug happens on memory allocation failure so it should be rare.
However, its impact is not negligeable as if actconn counter is wrapped
it will block any future connection allocation for both QUIC and TCP.

One small downside of this change is that a CID is now always allocated
before quic_conn even if maxconn will be reached. However, this is
considered as of minor importance compared to a more robust code.

This must be backported up to 2.6.

20 months agoDOC: stconn: Improve comments about lra and fsb usage
Christopher Faulet [Tue, 7 Nov 2023 06:55:51 +0000 (07:55 +0100)] 
DOC: stconn: Improve comments about lra and fsb usage

Recent fixes have shown <lra> and <fsb> uses were not prettu clear. So let's
try to improve documentation about these value. Especially when <lra> is
updated and how to used it.

20 months agoCLEANUP: htx: Properly indent htx_reserve_max_data() function
Christopher Faulet [Tue, 7 Nov 2023 06:50:22 +0000 (07:50 +0100)] 
CLEANUP: htx: Properly indent htx_reserve_max_data() function

Spaces were used instead of tabs to indent htx_reserve_max_data()
function. Let's reindent the whole function.

20 months agoBUG/MINOR: stconn: Sanitize report for read activity
Christopher Faulet [Tue, 7 Nov 2023 06:45:43 +0000 (07:45 +0100)] 
BUG/MINOR: stconn: Sanitize report for read activity

When a EOS or EOI is detected on the endpoint and when the event is reported
at the SC level, a read activity must be reported. It is not really a big
deal because these flags already inhibit any read timeout. But it is
consistent with the <lra> comment. In addition, no read activity is reported
on abort. It is up-down event and it is not an event unblocking the
reads. So there is no reason to report a read activity.

This patch must be backported to 2.8.

20 months agoBUG/MEDIUM: Don't apply a max value on room_needed in sc_need_room()
Christopher Faulet [Mon, 6 Nov 2023 08:13:00 +0000 (09:13 +0100)] 
BUG/MEDIUM: Don't apply a max value on room_needed in sc_need_room()

In sc_need_room(), we compute the maximum room that can be requested to
restarted reading to be sure to be able to unblock the SC. At worst when the
buffer is emptied. Here, the buffer reserve is considered but it is an issue.

Counting the reserve can lead to a wicked bug with the H1 multiplexer, when
small amount of data are found at the end of the HTX buffer. In this case,
to not wrap, the H1 mux requests more room. It is an optim to be able to
resync the buffer with the consumer side and to be able to perform zero-copy
transfers. However, if this amount of data is smaller than the reserve and
if the consumer is congested, we fall in a loop because the wrong value is
used to request more room. The H1 mux continues to pretend there is not
enough space in the buffer, while the effective requested value is lower
than the free space in the buffer. While the consumer is congested and does
not consume these data, the is no way to stop the loop.

We can fix the function by removing the buffer reserve from the
computation. But it remains a dangerous decision to apply a max value on
room_needed. It is safer to require the caller must set a correct value. For
now, it is true. But at the end, it is totally unexepected to wait for more
room than an empty buffer can contain.

This patch must be backported to 2.8.

20 months agoMINOR: stconn: Don't queue stream task in past in sc_notify()
Christopher Faulet [Mon, 6 Nov 2023 07:57:06 +0000 (08:57 +0100)] 
MINOR: stconn: Don't queue stream task in past in sc_notify()

A task must never be queued in past. However, in sc_notify(), the stream
task, if not woken up, is queued. Thanks to previous fixes, the stream task
expiration date should be correct. But to prevent any issue, a BUG_ON() is
added to be sure it never happens. I guess a good idea could be to remove it
or change it to BUG_ON_HOT() for the final release.

20 months agoBUG/MEDIUM: stconn: Don't report rcv/snd expiration date if SC cannot epxire
Christopher Faulet [Mon, 6 Nov 2023 07:45:22 +0000 (08:45 +0100)] 
BUG/MEDIUM: stconn: Don't report rcv/snd expiration date if SC cannot epxire

When receive or send expiration date of a stream-connector is retrieved, we
now automatically check if it may expire. If not, TICK_ETERNITY is returned.

The expiration dates of the frontend and backend stream-connectors are used
to compute the stream expiration date. This operation is performed at 2
places: at the end of process_stream() and in sc_notify() if the stream is
not woken up.

With this patch, there is no special changes for process_stream() because it
was already handled. It make thing a little simpler. However, it fixes
sc_notify() by avoiding to erroneously compute an expiration date in
past. This highly reduce the stream wakeups when there is contention on the
consumer side.

The bug was introduced with the commit 8073094bf ("NUG/MEDIUM: stconn:
Always update stream's expiration date after I/O"). It was an error to
unconditionnaly set the stream expiration data, without testing blocking
conditions on both SC.

This patch must be backported to 2.8.

20 months agoBUG/MEDIUM: stconn: Report send activity during mux-to-mux fast-forward
Christopher Faulet [Tue, 31 Oct 2023 12:43:21 +0000 (13:43 +0100)] 
BUG/MEDIUM: stconn: Report send activity during mux-to-mux fast-forward

When data are directly forwarded from a mux to the opposite one, we must not
forget to report send activity when data are successfully sent or report a
blocked send with data are blocked. It is important because otherwise, if
the transfer is quite long, longer than the client or server timeout, an
error may be triggered because the write timeout is reached.

H1, H2 and PT muxes are concerned. To fix the issue, The done_fastword()
callback now returns the amount of data consummed. This way it is possible
to update/reset the FSB data accordingly.

No backport needed.

20 months agoCLEANUP: Re-apply xalloc_size.cocci (3)
Tim Duesterhus [Sun, 5 Nov 2023 19:02:37 +0000 (20:02 +0100)] 
CLEANUP: Re-apply xalloc_size.cocci (3)

This reapplies the xalloc_size.cocci patch across the whole `src/` tree.

see 16cc16dd8235e7eb6c38b7abd210bd1e1d96b1d9
see 63ee0e4c01b94aee5fc6c6dd98cfc4480ae5ea46
see 9fb57e8c175a0b852b06a0780f48eb8eaf321a47

20 months ago[RELEASE] Released version 2.9-dev9 v2.9-dev9
Willy Tarreau [Sat, 4 Nov 2023 08:38:16 +0000 (09:38 +0100)] 
[RELEASE] Released version 2.9-dev9

Released version 2.9-dev9 with the following main changes :
    - DOC: internal: filters: fix reference to entities.pdf
    - BUG/MINOR: ssl: load correctly @system-ca when ca-base is define
    - MINOR: lua: Add flags to configure logging behaviour
    - MINOR: lua: change tune.lua.log.stderr default from 'on' to 'auto'
    - BUG/MINOR: backend: fix wrong BUG_ON for avail conn
    - BUG/MAJOR: backend: fix idle conn crash under low FD
    - MINOR: backend: refactor insertion in avail conns tree
    - DEBUG: mux-h2/flags: fix list of h2c flags used by the flags decoder
    - BUG/MEDIUM: server/log: "mode log" after server keyword causes crash
    - MINOR: connection: add conn_pr_mode_to_proto_mode() helper func
    - BUG/MEDIUM: server: "proto" not working for dynamic servers
    - MINOR: server: add helper function to detach server from proxy list
    - DEBUG: add a tainted flag when ha_panic() is called
    - DEBUG: lua: add tainted flags for stuck Lua contexts
    - DEBUG: pools: detect that malloc_trim() is in progress
    - BUG/MINOR: quic: do not consider idle timeout on CLOSING state
    - MINOR: frontend: implement a dedicated actconn increment function
    - BUG/MINOR: ssl: use a thread-safe sslconns increment
    - MEDIUM: quic: count quic_conn instance for maxconn
    - MEDIUM: quic: count quic_conn for global sslconns
    - BUG/MINOR: ssl: suboptimal certificate selection with TLSv1.3 and dual ECDSA/RSA
    - REGTESTS: ssl: update the filters test for TLSv1.3 and sigalgs
    - BUG/MINOR: mux-quic: fix early close if unset client timeout
    - BUG/MEDIUM: ssl: segfault when cipher is NULL
    - BUG/MINOR: tcpcheck: Report hexstring instead of binary one on check failure
    - MEDIUM: systemd: be more verbose about the reload
    - MINOR: sample: Add fetcher for getting all cookie names
    - BUG/MINOR: proto_reverse_connect: support SNI on active connect
    - MINOR: proxy/stktable: add resolve_stick_rule helper function
    - BUG/MINOR: stktable: missing free in parse_stick_table()
    - BUG/MINOR: cfgparse/stktable: fix error message on stktable_init() failure
    - MINOR: stktable: stktable_init() sets err_msg on error
    - MINOR: stktable: check if a type should be used as-is
    - MEDIUM: stktable/peers: "write-to" local table on peer updates
    - CI: github: update wolfSSL to 5.6.4
    - DOC: install: update the wolfSSL required version
    - MINOR: server: Add parser support for set-proxy-v2-tlv-fmt
    - MINOR: connection: Send out generic, user-defined server TLVs
    - BUG/MEDIUM: pattern: don't trim pools under lock in pat_ref_purge_range()
    - MINOR: mux-h2: always use h2_send() in h2_done_ff(), not h2_process()
    - OPTIM: mux-h2: call h2_send() directly from h2_snd_buf()
    - BUG/MINOR: server: remove some incorrect free() calls on null elements

20 months agoBUG/MINOR: server: remove some incorrect free() calls on null elements
Willy Tarreau [Sat, 4 Nov 2023 07:56:01 +0000 (08:56 +0100)] 
BUG/MINOR: server: remove some incorrect free() calls on null elements

In commit 6f4bfed3a ("MINOR: server: Add parser support for
set-proxy-v2-tlv-fmt") a few free() calls were made to an element on
error path when it was detected it was NULL. It doesn't have any
effect, however there was one case of use-after-free at the end of
srv_settings_cpy() that was caught by gcc due to attempting to free
the element after freeing its holder.

No backport is needed.

20 months agoOPTIM: mux-h2: call h2_send() directly from h2_snd_buf()
Willy Tarreau [Sat, 4 Nov 2023 07:34:23 +0000 (08:34 +0100)] 
OPTIM: mux-h2: call h2_send() directly from h2_snd_buf()

This allows to eliminate full buffers very quickly and to recycle them
much faster, resulting in higher transfer rates and lower memory usage
at the same time. We just wake the tasklet up if it succeeded so that
h2_process() and friends are called to finalize what needs to.

For regular buffer sizes, the performance level becomes quite close to
the one obtained with the zero-copy mechanism (zero-copy remains much
faster with non-default buffer sizes). The memory savings are huge with
default buffer size: at 64c * 100 streams on a single thread, we used
to forward 4.4 Gbps of traffic using 10400 buffers. After the change,
the performance reaches 5.9 Gbps with only 22-24 buffers, since they
are quickly recycled. That's asaving of 160 MB of RAM.

A concern was an increase in the number of syscalls but this is not
the case, the numbers remained exactly the same before and after.

Some experimentations were made to try to cork data and not send
incomplete buffers, and that always voided these changes. One
explanation might be that keeping a first buffer with only headers
frames is sufficient to prevent a zero-copy of the data coming in
a next snd_buf() call. This still needs to be studied anyway.

20 months agoMINOR: mux-h2: always use h2_send() in h2_done_ff(), not h2_process()
Willy Tarreau [Sat, 4 Nov 2023 07:12:17 +0000 (08:12 +0100)] 
MINOR: mux-h2: always use h2_send() in h2_done_ff(), not h2_process()

By calling h2_process(), the code would theoretically make it possible
for a synchronous ->wake() call to provoke an indirect call to h2_snd_buf()
while we're in h2_done_ff(), which could be quite bad. The current
conditions do not permit it right now but this could easily break by
accident. Better use h2_send() and wake the task up if needed. Precise
performance tests showed no change.

20 months agoBUG/MEDIUM: pattern: don't trim pools under lock in pat_ref_purge_range()
Willy Tarreau [Sat, 4 Nov 2023 06:55:37 +0000 (07:55 +0100)] 
BUG/MEDIUM: pattern: don't trim pools under lock in pat_ref_purge_range()

There's a subtle issue that results from pat_ref_purge_range() trying
to release memory. Since commit 0d93a8186 ("MINOR: pools: work around
possibly slow malloc_trim() during gc") that was backported to 2.3,
trim_all_pools() now protects itself against concurrent malloc() and
free() by isolating itself. The problem is that pat_ref_purge_range()
must be called under a lock, which is precisely what's done in
cli_io_handler_clear_map(). Thus during a clearing of a map, if
another thread tries to access or update an entry in the same map, it
will wait for the ref->lock to be released, and trim_all_pools() will
wait for all threads to be harmless, thus causing a deadlock. Note
that disabling memory trimming cannot work around the problem here
because it's tested only under isolation.

The solution here consists in moving the call to trim_all_pools() to
the caller, out of the lock.

This must be backported as far as 2.4.

20 months agoMINOR: connection: Send out generic, user-defined server TLVs
Alexander Stephan [Sat, 28 Oct 2023 18:57:07 +0000 (20:57 +0200)] 
MINOR: connection: Send out generic, user-defined server TLVs

To follow-up the implementation of the new set-proxy-v2-tlv-fmt
keyword in the server, the connection is updated to use the previously
allocated TLVs. If no value was specified, we send out an empty TLV.
As the feature is fully working with this commit, documentation and a
test for the server and default-server are added as well.

20 months agoMINOR: server: Add parser support for set-proxy-v2-tlv-fmt
Alexander Stephan [Sat, 28 Oct 2023 18:51:26 +0000 (20:51 +0200)] 
MINOR: server: Add parser support for set-proxy-v2-tlv-fmt

This commit introduces a generic server-side parsing of type-value pair
arguments and allocation of a TLV list via a new keyword called
set-proxy-v2-tlv-fmt.

This allows to 1) forward any TLV type with the help of fc_pp_tlv,
2) generally, send out any TLV type and value via a log format expression.
To have this fully working the connection will need to be updated in
a follow-up commit to actually respect the new server TLV list.

default-server support has also been implemented.

20 months agoDOC: install: update the wolfSSL required version
William Lallemand [Fri, 3 Nov 2023 18:02:23 +0000 (19:02 +0100)] 
DOC: install: update the wolfSSL required version

WolfSSL 5.6.4 was released with a lot of fixes for HAProxy, update the
required version so all supported reg-tests are working.

20 months agoCI: github: update wolfSSL to 5.6.4
William Lallemand [Fri, 3 Nov 2023 17:01:18 +0000 (18:01 +0100)] 
CI: github: update wolfSSL to 5.6.4

Update wolfSSL to the 5.6.4 released version.

20 months agoMEDIUM: stktable/peers: "write-to" local table on peer updates
Aurelien DARRAGON [Mon, 2 Oct 2023 14:40:27 +0000 (16:40 +0200)] 
MEDIUM: stktable/peers: "write-to" local table on peer updates

In this patch, we add the possibility to declare on a table definition
("table" in peer section, or "stick-table" in proxy section) that we
want the remote/peer updates on that table to be pushed on a local
haproxy table in addition to the source table.

Consider this example:

  |peers mypeers
  |        peer local 127.0.0.1:3334
  |        peer clust 127.0.0.1:3333
  |        table t1.local type string size 10m store server_id,server_key expire 30s
  |        table t1.clust type string size 10m store server_id,server_key write-to mypeers/t1.local expire 30s

With this setup, we consider haproxy uses t1.local as cache/local table
for read and write operations, and that t1.clust is a remote table
containing datas processed from t1.local and similar tables from other
haproxy peers in a cluster setup. The t1.clust table will be used to
refresh the local/cache one via the "write-to" statement.

What will happen, is that every time haproxy will see entry updates for
the t1.clust table: it will overwrite t1.local table with fresh data and
will update the entry expiration timer. If t1.local entry doesn't exist
yet (key doesn't exist), it will automatically create it. Note that only
types that cannot be used for arithmetic ops will be handled, and this
to prevent processed values from the remote table from interfering with
computations based on values from the local table. (ie: prevent
cumulative counters from growing indefinitely).

"write-to" will only push supported types if they both exist in the source
and the target table. Be careful with server_id and server_key storage
because they are often declared implicitly when referencing a table in
sticking rules but it is required to declare them explicitly for them to
be pushed between a remote and a local table through "write-to" option.

Also note that the "write-to" target table should have the same type as
the source one, and that the key length should be strictly equal,
otherwise haproxy will raise an error due to the tables being
incompatibles. A table that is already being written to cannot be used
as a source table for a "write-to" target.

Thanks to this patch, it will now be possible to use sticking rules in
peer cluster context by using a local table as a local cache which
will be automatically refreshed by one or multiple remote table(s).

This commit depends on:
 - "MINOR: stktable: stktable_init() sets err_msg on error"
 - "MINOR: stktable: check if a type should be used as-is"

20 months agoMINOR: stktable: check if a type should be used as-is
Aurelien DARRAGON [Thu, 26 Oct 2023 15:46:02 +0000 (17:46 +0200)] 
MINOR: stktable: check if a type should be used as-is

stick table types now have an extra bit named 'as_is' that allows us to
check if such type should be used as-is or if it may be involved in
arithmetic operations such as counters. This can be useful since those
types are not common and may require specific handling.

e.g.: stktable_data_types[data_type].as_is will be set to 1 if the type
cannot be used in arithmetic operations.

20 months agoMINOR: stktable: stktable_init() sets err_msg on error
Aurelien DARRAGON [Thu, 2 Nov 2023 17:34:51 +0000 (18:34 +0100)] 
MINOR: stktable: stktable_init() sets err_msg on error

stktable_init() now sets err_msg when error occurs so that caller is able
to precisely report the cause of the failure.

20 months agoBUG/MINOR: cfgparse/stktable: fix error message on stktable_init() failure
Aurelien DARRAGON [Fri, 3 Nov 2023 11:10:56 +0000 (12:10 +0100)] 
BUG/MINOR: cfgparse/stktable: fix error message on stktable_init() failure

As a result of copy paste error in 1b8e68e ("MEDIUM: stick-table: Stop
handling stick-tables as proxies."), postparsing stktable_init() failures
were reported as such for named peer tables:

   "Proxy 'table_name': failed to initialize stick table."

Now they are correctly reported like this:

   "Parsing [file:line]: failed to initialize 'table_name' stick-table."

This should be backported to every stable versions.

20 months agoBUG/MINOR: stktable: missing free in parse_stick_table()
Aurelien DARRAGON [Thu, 2 Nov 2023 08:18:55 +0000 (09:18 +0100)] 
BUG/MINOR: stktable: missing free in parse_stick_table()

When "peers" keyword is encountered within a stick table definition,
peers.name hint gets replaced with a new copy of the provided name using
strdup(). However, there is no detection on whether the name was
previously set or not, so it is currently allowed to reuse the keyword
multiple time to overwrite previous value, but here we forgot to free
previous value for peers.name before assigning it to a new one.

This should be backported to every stable versions.

20 months agoMINOR: proxy/stktable: add resolve_stick_rule helper function
Aurelien DARRAGON [Tue, 8 Aug 2023 09:37:59 +0000 (11:37 +0200)] 
MINOR: proxy/stktable: add resolve_stick_rule helper function

Simplify stick and store sticktable proxy rules postparsing by adding
a sticking rule entry resolve (postparsing) function.

This will ease code maintenance.

20 months agoBUG/MINOR: proto_reverse_connect: support SNI on active connect
Amaury Denoyelle [Fri, 3 Nov 2023 10:03:49 +0000 (11:03 +0100)] 
BUG/MINOR: proto_reverse_connect: support SNI on active connect

SNI may be specify on a server line for connecting to the remote host.
This requires to manually set it on the connection via
ssl_sock_set_servername().

This step was missing when a server line was used for active reverse
HTTP. Fix this by adding the missing ssl_sock_set_servername()
invocation inside new_reverse_conn().

Note that for the moment, no session is instantiated to carry active
reverse connection. A direct consequence of this is that SNI sample
retrieval may crash depending if it depends on session parameters. This
should be fixed by a later commit. In the meantime, this patch is
sufficient to support simple SNI value such as constant expressions.

No need to backport.

20 months agoMINOR: sample: Add fetcher for getting all cookie names
Ruei-Bang Chen [Fri, 27 Oct 2023 20:59:21 +0000 (13:59 -0700)] 
MINOR: sample: Add fetcher for getting all cookie names

This new fetcher can be used to extract the list of cookie names from
Cookie request header or from Set-Cookie response header depending on
the stream direction. There is an optional argument that can be used
as the delimiter (which is assumed to be the first character of the
argument) between cookie names. The default delimiter is comma (,).

Note that we will treat the Cookie request header as a semi-colon
separated list of cookies and each Set-Cookie response header as
a single cookie and extract the cookie names accordingly.

20 months agoMEDIUM: systemd: be more verbose about the reload
William Lallemand [Tue, 31 Oct 2023 17:59:29 +0000 (18:59 +0100)] 
MEDIUM: systemd: be more verbose about the reload

When the `haproxy -c` check during the reload fails, no error is output
in the logs, this can be quite bothersome to understand what's going on.

This patch removes the -q option on the check so we can see the error
with `journalctl -u haproxy` or `systemctl status haproxy`

This will change the behavior when the check works, and will display
"Configuration file is valid"

Note that in some case this test could be completely removed, because
the master process loads the configuration itself and is able to keep
the previous workers running when the reload failed. This is interesting
to disable the test when there are a lot of certificates of files to
load, to divide the reload time by 2.

No need to backport.

20 months agoBUG/MINOR: tcpcheck: Report hexstring instead of binary one on check failure
Christopher Faulet [Mon, 30 Oct 2023 08:00:37 +0000 (09:00 +0100)] 
BUG/MINOR: tcpcheck: Report hexstring instead of binary one on check failure

When an expect rule failed for a tcp-check, information about the expect
rule is dumped in the report. For a check on a binary string, a hexstring is
used in the configuration but the decoded string is dumped. It is an problem
because it can contain special characters. And it is not really handy
because there is no correspondance with the config.

So, now, the hexstring is dumped in the report. This way, we are sure there
is no special characters and it is easy to find it in the configuration.

This patch shoudl solve the issue #2326. It must be backported as far as
2.2.

20 months agoBUG/MEDIUM: ssl: segfault when cipher is NULL
William Lallemand [Mon, 30 Oct 2023 17:08:16 +0000 (18:08 +0100)] 
BUG/MEDIUM: ssl: segfault when cipher is NULL

The patch which fixes the certificate selection uses
SSL_CIPHER_get_id() to skip the SCSV ciphers without checking if cipher
is NULL. This patch fixes the issue by skipping any NULL cipher in the
iteration.

Problem was reported in #2329.

Need to be backported where 23093c72f139eddfce68ea5580193ee131901591 was
backported. No release was made with this patch so the severity is
MEDIUM.

20 months agoBUG/MINOR: mux-quic: fix early close if unset client timeout
Amaury Denoyelle [Thu, 26 Oct 2023 16:17:29 +0000 (18:17 +0200)] 
BUG/MINOR: mux-quic: fix early close if unset client timeout

When no client timeout is defined in the configuration, QCC timeout task
is never allocated. However, a NULL timeout task is also used as a
criteria in qcc_is_dead() to consider that the MUX instance should be
released as timeout stroke earlier.

This bug causes every connection to be closed by haproxy side with a
CONNECTION_CLOSE. This is notable when using several streams per
connection with only the first stream completed and the others failed.

To fix this, change timeout task allocation policy. It is now always
allocated. This means that if no timeout is defined, it will never be
run. This is not considered a waste of resource as no timeout in the
configuration is considered as an exception case. However, this has the
advantage to simplify the rest of the code which can now check for the
task instance without having an extra check on the timeout value.

This bug is labelled as minor as it only occurs if no timeout client is
defined which reports warning on startup as it may caused unexpected
behavior.

This bug should be backported up to 2.6.

20 months agoREGTESTS: ssl: update the filters test for TLSv1.3 and sigalgs
William Lallemand [Thu, 26 Oct 2023 17:13:43 +0000 (19:13 +0200)] 
REGTESTS: ssl: update the filters test for TLSv1.3 and sigalgs

Signature algorithms allows us to select the right certificates when
using TLSv1.3. This patch update the ssl_crt-list_filters.vtc regtest to
do more precise testing with TLSv1.3 in addition to TLSv1.2.

This allow us to test correctly bug #2300.

It could be backported to 2.8 with the previous fix for certificate
selection.

20 months agoBUG/MINOR: ssl: suboptimal certificate selection with TLSv1.3 and dual ECDSA/RSA
William Lallemand [Tue, 24 Oct 2023 21:58:02 +0000 (23:58 +0200)] 
BUG/MINOR: ssl: suboptimal certificate selection with TLSv1.3 and dual ECDSA/RSA

When using TLSv1.3, the signature algorithms extension is used to chose
the right ECDSA or RSA certificate.

However there was an old test for previous version of TLS (< 1.3) which
was testing if the cipher is compatible with ECDSA when an ECDSA
signature algorithm is used. This test was relying on
SSL_CIPHER_get_auth_nid(cipher) == NID_auth_ecdsa to verify if the
cipher is still good.

Problem is, with TLSv1.3, all ciphersuites are compatible with any
authentication algorithm, but SSL_CIPHER_get_auth_nid(cipher) does not
return NID_auth_ecdsa, but NID_auth_any.

Because of this, with TLSv1.3 when both ECDSA and RSA certificates are
available for a domain, the ECDSA one is not chosen in priority.

This patch also introduces a test on the cipher IDs for the signaling
ciphersuites, because they would always return NID_auth_any, and are not
relevent for this selection.

This patch fixes issue #2300.

Must be backported in all stable versions.

20 months agoMEDIUM: quic: count quic_conn for global sslconns
Amaury Denoyelle [Wed, 25 Oct 2023 13:38:50 +0000 (15:38 +0200)] 
MEDIUM: quic: count quic_conn for global sslconns

Similar to the previous commit which check for maxconn before allocating
a QUIC connection, this patch checks for maxsslconn at the same step.
This is necessary as a QUIC connection cannot run without a SSL context.

This should be backported up to 2.6. It relies on the following patch :
  "BUG/MINOR: ssl: use a thread-safe sslconns increment"

20 months agoMEDIUM: quic: count quic_conn instance for maxconn
Amaury Denoyelle [Wed, 25 Oct 2023 08:52:23 +0000 (10:52 +0200)] 
MEDIUM: quic: count quic_conn instance for maxconn

Increment actconn and check maxconn limit when a quic_conn is
instantiated. This is necessary because prior to this patch, quic_conn
instances where not counted. Global actconn was only incremented after
the handshake has been completed and the connection structure is
allocated.

The increment is done using increment_actconn() on INITIAL packet
parsing if a new connection is about to be created. If the limit is
reached, the allocation is cancelled and the INITIAL packet is dropped.

The decrement is done under quic_conn_release(). This means that
quic_cc_conn instances are not taken into account. This seems safe
enough because quic_cc_conn are only used for minimal usage.

The counterpart of this change is that maxconn must not be checked a
second time when listener_accept() is done over a QUIC connection. For
this, a new bind_conf flag BC_O_XPRT_MAXCONN is set for listeners when
maxconn is already counted by the lower layer. For the moment, it is
positionned only for QUIC listeners.

Without this patch, haproxy process could suffer from heavy memory/CPU
load if the number of concurrent handshake is high.

This patch is not considered a bug fix per-se. However, it has a major
benefit to protect against too many QUIC handshakes. As such, it should
be backported up to 2.6. For this, it relies on the following patch :
  "MINOR: frontend: implement a dedicated actconn increment function"

20 months agoBUG/MINOR: ssl: use a thread-safe sslconns increment
Amaury Denoyelle [Wed, 25 Oct 2023 13:38:04 +0000 (15:38 +0200)] 
BUG/MINOR: ssl: use a thread-safe sslconns increment

Each time a new SSL context is allocated, global.sslconns is
incremented. If global.maxsslconn is reached, the allocation is
cancelled.

This procedure was not entirely thread-safe due to the check and
increment operations conducted at different stage. This could lead to
global.maxsslconn slightly exceeded when several threads allocate SSL
context while sslconns is near the limit.

To fix this, use a CAS operation in a do/while loop. This code is
similar to the actconn/maxconn increment for connection.

A new function increment_sslconn() is defined for this operation. For
the moment, only SSL code is using it. However, it is expected that QUIC
will also use it to count QUIC connections as SSL ones.

This should be backported to all stable releases. Note that prior to the
2.6, sslconns was outside of global struct, so this commit should be
slightly adjusted.

20 months agoMINOR: frontend: implement a dedicated actconn increment function
Amaury Denoyelle [Wed, 25 Oct 2023 13:32:28 +0000 (15:32 +0200)] 
MINOR: frontend: implement a dedicated actconn increment function

When a new frontend connection is instantiated, actconn global counter
is incremented. If global maxconn value is reached, the connection is
cancelled. This ensures that system limit are under control.

Prior to this patch, the atomic check/increment operations were done
directly into listener_accept(). Move them in a dedicated function
increment_actconn() in frontend module. This will be useful when QUIC
connections will be counted in actconn counter.

20 months agoBUG/MINOR: quic: do not consider idle timeout on CLOSING state
Amaury Denoyelle [Wed, 25 Oct 2023 12:45:53 +0000 (14:45 +0200)] 
BUG/MINOR: quic: do not consider idle timeout on CLOSING state

When entering closing state, a QUIC connection is maintained during a
certain delay. The principle is to ensure the other peer has received
the CONNECTION_CLOSE frame. In case of packet duplication/reordering,
CONNECTION_CLOSE is reemitted.

QUIC RFC recommends to use at least 3 times the PTO value. However,
prior to this patch, haproxy used instead the max value between 3 times
the PTO and the connection idle timeout. In the default case, idle
timeout is set to 30s which is in most of the times largely superior to
the PTO. This has the downside of keeping the connection in memory for
too long whereas all resources could be released much earlier.

Fix this behavior by using 3 times the PTO on closing or draining state.
This value is limited up to 1s. This ensures that most of connections
are covered by this. If a connection runs with a very high RTT, it must
not impact the whole process and should be released in a reasonable
delay.

This should be backported up to 2.6.

20 months agoDEBUG: pools: detect that malloc_trim() is in progress
Willy Tarreau [Wed, 25 Oct 2023 13:42:27 +0000 (15:42 +0200)] 
DEBUG: pools: detect that malloc_trim() is in progress

Now when calling ha_panic() with a thread still under malloc_trim(),
we'll set a new tainted flag to easily report it, and the output
trace will report that this condition happened and will suggest to
use no-memory-trimming to avoid it in the future.

20 months agoDEBUG: lua: add tainted flags for stuck Lua contexts
Willy Tarreau [Wed, 25 Oct 2023 13:02:59 +0000 (15:02 +0200)] 
DEBUG: lua: add tainted flags for stuck Lua contexts

William suggested that since we can detect the presence of Lua in the
stack, let's combine it with stuck detection to set a new pair of flags
indicating a stuck Lua context and a stuck Lua shared context.

Now, executing an infinite loop in a Lua sample fetch function with
yield disabled crashes with tainted=0xe40 if loaded from a lua-load
statement, or tainted=0x640 from a lua-load-per-thread statement.

In addition, at the end of the panic dump, we can check if Lua was
seen stuck and emit recommendations about lua-load-per-thread and
the choice of dependencies depending on the presence of threads
and/or shared context.

20 months agoDEBUG: add a tainted flag when ha_panic() is called
Willy Tarreau [Wed, 25 Oct 2023 12:34:08 +0000 (14:34 +0200)] 
DEBUG: add a tainted flag when ha_panic() is called

This will make it easier to know that the panic function was called,
for the occasional case where the dump crashes and/or the stack is
corrupted and not much exploitable. Now at least it will be sufficient
to check the tainted value to know that someone called ha_panic(), and
it will also be usable to condition extra analysis.

20 months agoMINOR: server: add helper function to detach server from proxy list
Aurelien DARRAGON [Wed, 18 Oct 2023 15:09:12 +0000 (17:09 +0200)] 
MINOR: server: add helper function to detach server from proxy list

Remove some code duplication by introducing a basic helper function
to detach a server from its parent proxy. It is supported to call
the function even if the server is not yet listed in the proxy list.

If the server is not yet listed in the proxy, the function will do
nothing. In delete_server(), we previously performed some BUG_ON()
to ensure that the detach always succeeded given that we were certain
that the server was in the proxy list because it was retrieved through
get_backend_server().

However this test is superfluous, we can safely assume that the operation
will always succeed if get_backend_server() returned != NULL (we're under
full thread isolation), and if it's not the case, then we have a bigger
API issue anyway..

20 months agoBUG/MEDIUM: server: "proto" not working for dynamic servers
Aurelien DARRAGON [Thu, 19 Oct 2023 14:15:50 +0000 (16:15 +0200)] 
BUG/MEDIUM: server: "proto" not working for dynamic servers

In 304672320e ("MINOR: server: support keyword proto in 'add server' cli")
improper use of conn_get_best_mux_entry() function was made:

First, server's proxy mode was directly passed as "proto_mode" argument
to conn_get_best_mux_entry(), but this is strictly invalid because while
there is some relationship between proto modes and proxy modes, they
don't use the same storage mechanism and cannot be used interchangeably.

Because of this bug, conn_get_best_mux_entry() would not work at all for
TCP because PR_MODE_TCP equals 0, where PROTO_MODE_TCP normally equals 1.

Then another, less sensitive bug, remains:
as its name and description implies, conn_get_best_mux_entry() will try
its best to return something to the user, only using keyword (mux_proto)
input as an hint to return the most relevant mux within the list of
mux that are compatibles with proto_side and proto_mode values.

This means that even if mux_proto cannot be found or is not available
with current proto_side and proto_mode values, conn_get_best_mux_entry()
will most probably fallback to a more generic mux.

However in cli_parse_add_server(), we directly check the result of
conn_get_best_mux_entry() and consider that it will return NULL if the
provided keyword hint for mux_proto cannot be found. This will result in
the function not raising errors as expected, because most of the times if
the expected proto cannot be found, then we'll silently switch to the
fallback one, despite the user providing an explicit proto.

To fix that, we store the result of conn_get_best_mux_entry() to compare
the returned mux proto name with the one we're expecting to get, as it
is originally performed in cfgparse during initial server keyword parsing.

This patch depends on
 - "MINOR: connection: add conn_pr_mode_to_proto_mode() helper func")

It must be backported up to 2.6.

20 months agoMINOR: connection: add conn_pr_mode_to_proto_mode() helper func
Aurelien DARRAGON [Thu, 19 Oct 2023 14:06:03 +0000 (16:06 +0200)] 
MINOR: connection: add conn_pr_mode_to_proto_mode() helper func

This function allows to safely map proxy mode to corresponding proto_mode

This will allow for easier code maintenance and prevent mixups between
proxy mode and proto mode.

20 months agoBUG/MEDIUM: server/log: "mode log" after server keyword causes crash
Aurelien DARRAGON [Thu, 19 Oct 2023 08:59:26 +0000 (10:59 +0200)] 
BUG/MEDIUM: server/log: "mode log" after server keyword causes crash

In 9a74a6c ("MAJOR: log: introduce log backends"), a mistake was made:
it was assumed that the proxy mode was already known during server
keyword parsing in parse_server() function, but this is wrong.

Indeed, "mode log" can be declared late in the proxy section. Due to this,
a simple config like this will cause the process to crash:

   |backend test
   |
   |  server name 127.0.0.1:8080
   |  mode log

In order to fix this, we relax some checks in _srv_parse_init() and store
the address protocol from str2sa_range() in server struct, then we set-up
a postparsing function that is to be called after config parsing to
finish the server checks/initialization that depend on the proxy mode
to be known. We achieve this by checking the PR_CAP_LB capability from
the parent proxy to know if we're in such case where the effective proxy
mode is not yet known (it is assumed that other proxies which are implicit
ones don't provide this possibility and thus don't suffer from this
constraint).

Only then, if the capability is not found, we immediately perform the
server checks that depend on the proxy mode, else the check is postponed
and it will automatically be performed during postparsing thanks to the
REGISTER_POST_SERVER_CHECK() hook.

Note that we remove the SRV_PARSE_IN_LOG_BE flag because it was introduced
in the above commit and it is no longer relevant.

No backport needed unless 9a74a6c gets backported.

20 months agoDEBUG: mux-h2/flags: fix list of h2c flags used by the flags decoder
Willy Tarreau [Wed, 25 Oct 2023 09:43:01 +0000 (11:43 +0200)] 
DEBUG: mux-h2/flags: fix list of h2c flags used by the flags decoder

The two recent commits below each added one flag to h2c but omitted to
update the __APPEND_FLAG macro used by dev/flags so they are not
properly decoded:

  3dd963b35 ("BUG/MINOR: mux-h2: fix http-request and http-keep-alive timeouts again")
  68d02e5fa ("BUG/MINOR: mux-h2: make up other blocked streams upon removal from list")

This can be backported along with these commits.

20 months agoMINOR: backend: refactor insertion in avail conns tree
Amaury Denoyelle [Wed, 25 Oct 2023 08:10:14 +0000 (10:10 +0200)] 
MINOR: backend: refactor insertion in avail conns tree

Define a new function srv_add_to_avail_list(). This function is used to
centralize connection insertion in available tree. It reuses a BUG_ON()
statement to ensure the connection is not present in the idle list.

20 months agoBUG/MAJOR: backend: fix idle conn crash under low FD
Amaury Denoyelle [Tue, 24 Oct 2023 16:31:55 +0000 (18:31 +0200)] 
BUG/MAJOR: backend: fix idle conn crash under low FD

Since the following commit, idle conns are stored in a list as secondary
storage to retrieve them in usage order :
  5afcb686b93c3811bd859a331efd6a8341a61218
  MAJOR: connection: purge idle conn by last usage

The list usage has been extended wherever connections lookup are done
both on idle and safe trees. This reduced the code size by replacing a
two tree loops by a single list loop.

LIST_ELEM() is used in this context to retrieve the first idle list
element from the server list head. However, macro usage was wrong due to
an extra '&' operator which returns an invalid connection reference.
This will most of the time caused a crash on conn_delete_from_tree() or
affiliated functions.

This bug only occurs if the FD pool is exhausted and some idle
connections are selected to be killed.

It can be reproduced using the following config and h2load command :
$ h2load -t 8 -c 800 -m 10 -n 800 "http://127.0.0.1:21080/?s=10k"

global
maxconn 100

defaults
mode http
timeout connect 20s
timeout client  20s
timeout server  20s

listen li
bind :21080 proto h2
server nginx 127.99.0.1:30080 proto h1

This bug has been introduced by the above commit. Thus no need to
backport this fix.

Note that LIST_ELEM() macro usage was slightly adjusted also in
srv_migrate_conns_to_remove(). The function used toremove_list instead
of idle_list connection list element. This is not a bug as they are
stored in the same union. However, the new code is clearer as it intends
to move connection from the idle_list only into the toremove_list
mt-list.

20 months agoBUG/MINOR: backend: fix wrong BUG_ON for avail conn
Amaury Denoyelle [Tue, 24 Oct 2023 16:31:28 +0000 (18:31 +0200)] 
BUG/MINOR: backend: fix wrong BUG_ON for avail conn

Idle connections are both stored in an idle/safe tree and in an idle
list. The list is used as a secondary storage to be able to retrieve
them by usage order.

If a connection is moved into the available tree, it must not be present
in the idle list. A BUG_ON() was written to check this but was placed at
the wrong code section. Fix this by removing the misplaced one and write
new ones for avail_conns tree insertion and lookup.

The impact of this bug is minor as the misplaced BUG_ON() did not seem
to be triggered.

No need to backport.

20 months agoMINOR: lua: change tune.lua.log.stderr default from 'on' to 'auto'
Tristan [Mon, 23 Oct 2023 12:35:40 +0000 (13:35 +0100)] 
MINOR: lua: change tune.lua.log.stderr default from 'on' to 'auto'

After making it configurable in previous commit "MINOR: lua: Add flags
to configure logging behaviour", this patch changes the default value
of tune.lua.log.stderr from 'on' (unconditionally forward LUA logs to
stderr) to 'auto' (only forward LUA logs to stderr if logging via a
standard logger is disabled, or none is configured for the current context)

Since this is a change in behaviour, it shouldn't be backported

20 months agoMINOR: lua: Add flags to configure logging behaviour
Tristan [Mon, 23 Oct 2023 12:07:39 +0000 (13:07 +0100)] 
MINOR: lua: Add flags to configure logging behaviour

Until now, messages printed from LUA log functions were sent both to
the any logger configured for the current proxy, and additionally to
stderr (in most cases)

This introduces two flags to configure LUA log handling:
- tune.lua.log.loggers to use standard loggers or not
- tune.lua.log.stderr to use stderr, or not, or only conditionally

This addresses github feature request #2316

This can be backported to 2.8 as it doesn't change previous behaviour.

20 months agoBUG/MINOR: ssl: load correctly @system-ca when ca-base is define
William Lallemand [Mon, 23 Oct 2023 19:54:23 +0000 (21:54 +0200)] 
BUG/MINOR: ssl: load correctly @system-ca when ca-base is define

The configuration parser still adds the 'ca-base' directory when loading
the @system-ca, preventing it to be loaded correctly.

This patch fixes the problem by not adding the ca-base when a file
starts by '@'.

Fix issue #2313.

Must be backported as far as 2.6.

20 months agoDOC: internal: filters: fix reference to entities.pdf
Aleksandar Lazic [Sun, 22 Oct 2023 16:36:54 +0000 (18:36 +0200)] 
DOC: internal: filters: fix reference to entities.pdf

In doc/internals/api/filters.txt was the referece to
doc/internals/entities.pdf which was delteted in the
past.

20 months ago[RELEASE] Released version 2.9-dev8 v2.9-dev8
Willy Tarreau [Fri, 20 Oct 2023 19:36:47 +0000 (21:36 +0200)] 
[RELEASE] Released version 2.9-dev8

Released version 2.9-dev8 with the following main changes :
    - MINOR: ssl: add an explicit error when 'ciphersuites' are not supported
    - BUILD: ssl: enable 'ciphersuites' for WolfSSL
    - BUILD: ssl: add 'ssl_c_r_dn' fetch for WolfSSL
    - BUILD: ssl: add 'secure_memcmp' converter for WolfSSL and awslc
    - BUILD: ssl: enable keylog for awslc
    - CLEANUP: ssl: remove compat functions for openssl < 1.0.0
    - BUILD: ssl: enable keylog for WolfSSL
    - REGTESTS: pki: add a pki for SSL tests
    - REGTESTS: ssl: update common.pem with the new pki
    - REGTESTS: ssl: disable ssl_dh.vtc for WolfSSL
    - REGTESTS: wolfssl: temporarly disable some failing reg-tests
    - CI: ssl: add wolfssl to build-ssl.sh
    - CI: ssl: add git id support for wolfssl download
    - CI: github: add a wolfssl entry to the CI
    - CI: github: update wolfssl to git revision d83f2fa
    - CI: github: add awslc 1.16.0 to the push CI
    - BUG/MINOR: quic: Avoid crashing with unsupported cryptographic algos
    - REORG: quic: cleanup traces definition
    - BUG/MINOR: quic: reject packet with no frame
    - BUG/MEDIUM: mux-quic: fix RESET_STREAM on send-only stream
    - BUG/MINOR: mux-quic: support initial 0 max-stream-data
    - BUG/MINOR: h3: strengthen host/authority header parsing
    - CLEANUP: connection: drop an uneeded leftover cast
    - BUG/MAJOR: connection: make sure to always remove a connection from the tree
    - BUG/MINOR: quic: fix qc.cids access on quic-conn fail alloc
    - BUG/MINOR: quic: fix free on quic-conn fail alloc
    - BUG/MINOR: mux-quic: fix free on qcs-new fail alloc
    - BUG/MEDIUM: quic-conn: free unsent frames on retransmit to prevent crash
    - MEDIUM: tree-wide: logsrv struct becomes logger
    - MEDIUM: log: introduce log target
    - DOC: config: log <address> becomes log <target> in "log" related doc
    - MEDIUM: sink/log: stop relying on AF_UNSPEC for rings
    - MINOR: log: support explicit log target as argument in __do_send_log()
    - MINOR: log: remove the logger dependency in do_send_log()
    - MEDIUM: log/sink: simplify log header handling
    - MEDIUM: sink: inherit from caller fmt in ring_write() when rings didn't set one
    - MINOR: sink: add sink_new_from_srv() function
    - MAJOR: log: introduce log backends
    - MINOR: log/balance: support for the "sticky" lb algorithm
    - MINOR: log/balance: support for the "random" lb algorithm
    - MINOR: lbprm: support for the "none" hash-type function
    - MINOR: lbprm: compute the hash avalanche in gen_hash()
    - MINOR: sample: add sample_process_cnv() function
    - MEDIUM: log/balance: support for the "hash" lb algorithm
    - REGTEST: add a test for log-backend used as a log target
    - MINOR: server: introduce "log-bufsize" kw
    - BUG/MEDIUM: stconn: Report a send activity everytime data were sent
    - BUG/MEDIUM: applet: Report a send activity everytime data were sent
    - BUG/MINOR: mux-h1: Send a 400-bad-request on shutdown before the first request
    - MINOR: support for http-response set-timeout
    - BUG/MINOR: mux-h2: make up other blocked streams upon removal from list
    - DEBUG: pool: store the memprof bin on alloc() and update it on free()
    - BUG/MEDIUM: quic_conn: let the scheduler kill the task when needed
    - CLEANUP: hlua: Remove dead-code on error path in hlua_socket_new()
    - BUG/MEDIUM: mux-h1: do not forget TLR/EOT even when no data is sent
    - BUG/MINOR: htpp-ana/stats: Specify that HTX redirect messages have a C-L header
    - BUG/MEDIUM: mux-h2: Don't report an error on shutr if a shutw is pending
    - MEDIUM: stconn/channel: Move pipes used for the splicing in the SE descriptors
    - MINOR: stconn: Start to introduce mux-to-mux fast-forwarding notion
    - MINOR: stconn: Extend iobuf to handle a buffer in addition to a pipe
    - MINOR: connection: Add new mux callbacks to perform data fast-forwarding
    - MINOR: stconn: Temporarily remove kernel splicing support
    - MINOR: mux-pt: Temporarily remove splicing support
    - MINOR: mux-h1: Temporarily remove splicing support
    - MINOR: connection: Remove mux callbacks about splicing
    - MEDIUM: stconn: Add mux-to-mux fast-forward support
    - MINOR: mux-h1: Use HTX extra field only for responses with known length
    - MEDIUM: mux-h1: Properly handle state transitions of chunked outgoing messages
    - MEDIUM: raw-sock: Specifiy amount of data to send via snd_pipe callback
    - MINOR: mux-h1: Add function to add size of a chunk to an outgoind message
    - MEDIUM: mux-h1: Simplify zero-copy on sending path
    - MEDIUM: mux-h1: Simplify payload formatting based on HTX blocks on sending path
    - MEDIUM: mux-h1: Add fast-forwarding support
    - MINOR: h2: Set the BODYLESS_RESP flag on the HTX start-line if necessary
    - MEDIUM: mux-h2: Add consumer-side fast-forwarding support
    - MEDIUM: channel: don't look at iobuf to report an empty channel
    - MINOR: tree-wide: Only rely on co_data() to check channel emptyness
    - REGTESTS: Reenable HTTP tests about splicing
    - CLEAN: mux-h1: Remove useless __maybe_unused attribute on h1_make_chunk()
    - MEDIUM: mux-pt: Add fast-forwarding support
    - MINOR: global: Add an option to disable the zero-copy forwarding
    - BUILD: mux-h1: Fix build without kernel splicing support
    - REORG: stconn/muxes: Rename init step in fast-forwarding
    - MINOR: dgram: allow to set rcv/sndbuf for dgram sockets as well
    - BUG/MINOR: mux-h2: fix http-request and http-keep-alive timeouts again
    - BUG/MINOR: trace: fix trace parser error reporting
    - BUG/MEDIUM: peers: Be sure to always refresh recconnect timer in sync task
    - BUG/MEDIUM: peers: Fix synchro for huge number of tables
    - MINOR: cfgparse: forbid mixing reverse and standard listeners
    - MINOR: listener: add nbconn kw for reverse connect
    - MINOR: server: convert @reverse to rev@ standard format
    - MINOR: cfgparse: rename "rev@" prefix to "rhttp@"
    - REGTESTS: remove maxconn from rhttp bind line
    - MINOR: listener: forbid most keywords for reverse HTTP bind
    - MINOR: sample: Added support for Arrays in sample_conv_json_query in sample.c
    - MINOR: mux-h2/traces: explicitly show the error/refused stream states
    - MINOR: mux-h2/traces: clarify the "rejected H2 request" event
    - BUG/MINOR: mux-h2: commit the current stream ID even on reject
    - BUG/MINOR: mux-h2: update tracked counters with req cnt/req err

20 months agoBUG/MINOR: mux-h2: update tracked counters with req cnt/req err
Willy Tarreau [Fri, 20 Oct 2023 16:38:34 +0000 (18:38 +0200)] 
BUG/MINOR: mux-h2: update tracked counters with req cnt/req err

Originally H2 would transfer everything to H1 and parsing errors were
handled there, so that if there was a track-sc rule in effect, the
counters would be updated as well. As we started to add more and more
HTTP-compliance checks at the H2 layer, then switched to HTX, we
progressively lost this ability. It's a bit annoying because it means
we will not maintain accurate error counters for a given source, for
example.

This patch adds the calls to session_inc_http_req_ctr() and
session_inc_http_err_ctr() when needed (i.e. when failing to parse
an HTTP request since all other cases are handled by the stream),
just like mux-h1 does. The same should be done for mux-h3 by the
way.

This can be backported to recent stable versions. It's not exactly a
bug, rather a missing feature in that we had never updated this counter
for H2 till now, but it does make sense to do it especially based on
what the doc says about its usage.

20 months agoBUG/MINOR: mux-h2: commit the current stream ID even on reject
Willy Tarreau [Fri, 20 Oct 2023 15:51:12 +0000 (17:51 +0200)] 
BUG/MINOR: mux-h2: commit the current stream ID even on reject

The H2 spec says that a HEADERS frame turns an idle stream to the open
state, and it may then turn to half-closed(remote) on ES, then to close,
all at once, if we respond with RST (e.g. on error). Due to the fact that
we process a complete frame at once since h2_dec_hdrs() may reassemble
CONTINUATION frames until everything is complete, the state was only
committed after the frame was completley valid (otherwise multiple passes
could result in subsequent frames being rejected as the stream ID would
be equal to the highest one).

However this is not correct because it means that a client may retry on
the same ID as a previously failed one, which technically is forbidden
(for example the client couldn't know which of them a WINDOW_UPDATE or
RST_STREAM frame is for).

In practice, due to the error paths, this would only be possible when
failing to decode HPACK while leaving the HPACK stream intact, thus
when the valid decoded HPACK stream cannot be turned into a valid HTTP
representation, e.g. when the resulting headers are too large for example.
The solution to avoid this consists in committing the stream ID on this
error path as well. h2spec continues to be happy.

Thanks to Annika Wickert and Tim Windelschmidt for reporting this issue.

This fix must be backported to all stable versions.

20 months agoMINOR: mux-h2/traces: clarify the "rejected H2 request" event
Willy Tarreau [Fri, 20 Oct 2023 15:47:33 +0000 (17:47 +0200)] 
MINOR: mux-h2/traces: clarify the "rejected H2 request" event

In h2_frt_handle_headers() all failures lead to a generic message saying
"rejected H2 request". It's quite inexpressive while there are a few
distinct tests that are made before jumping there:

  - trailers on closed stream
  - unparsable request
  - refused stream

Let's emit the traces from these call points instead so that we get more
info about what happened. Since these are user-level messages, we take
care of keeping them aligned as much as possible.

For example before it would say:

  [04|h2|1|mux_h2.c:2859] rejected H2 request : h2c=0x7f5d58036fd0(F,FRE)
  [04|h2|5|mux_h2.c:2860] h2c_frt_handle_headers(): leaving on error : h2c=0x7f5d58036fd0(F,FRE) dsi=1 h2s=0x9fdb60(0,CLO)

And now it says:

  [04|h2|1|mux_h2.c:2817] rcvd unparsable H2 request : h2c=0x7f55f8037160(F,FRH) dsi=1 h2s=CLO
  [04|h2|5|mux_h2.c:2875] h2c_frt_handle_headers(): leaving on error : h2c=0x7f55f8037160(F,FRE) dsi=1 h2s=CLO

20 months agoMINOR: mux-h2/traces: explicitly show the error/refused stream states
Willy Tarreau [Fri, 20 Oct 2023 15:32:13 +0000 (17:32 +0200)] 
MINOR: mux-h2/traces: explicitly show the error/refused stream states

Sometimes it's unclear whether a stream is still open or closed when
certain traces are emitted, for example when the stream was refused,
because the reported pointer and ID in fact correspond to the refused
stream. And for closed streams, no pointer/name is printed, leaving
some confusion about the state. This patch makes the situation easier
to analyse by explicitly reporting "h2s=CLO" on closed/error/refused
streams so that we don't waste time comparing pointers and we instantly
know the stream is closed. Now instead of emitting:

   [03|h2|5|mux_h2.c:2874] h2c_frt_handle_headers(): leaving on error : h2c=0x7fdfa8026820(F,FRE) dsi=201 h2s=0x9fdb60(0,CLO)

It will emit:

   [03|h2|5|mux_h2.c:2874] h2c_frt_handle_headers(): leaving on error : h2c=0x7fdfa8026820(F,FRE) dsi=201 h2s=CLO

20 months agoMINOR: sample: Added support for Arrays in sample_conv_json_query in sample.c
Jens Popp [Mon, 25 Sep 2023 13:30:53 +0000 (13:30 +0000)] 
MINOR: sample: Added support for Arrays in sample_conv_json_query in sample.c

Method now returns the content of Json Arrays, if it is specified in
Json Path as String. The start and end character is a square bracket. Any
complex object in the array is returned as Json, so that you might get Arrays
of Array or objects. Only recommended for Arrays of simple types (e.g.,
String or int) which will be returned as CSV String. Also updated
documentation and fixed issue with parenthesis and other changes from
comments.

This patch was discussed in issue #2281.

Signed-off-by: William Lallemand <wlallemand@haproxy.com>
20 months agoMINOR: listener: forbid most keywords for reverse HTTP bind
Amaury Denoyelle [Fri, 20 Oct 2023 14:49:03 +0000 (16:49 +0200)] 
MINOR: listener: forbid most keywords for reverse HTTP bind

Reverse HTTP bind is very specific in that in rely on a server to
initiate connection. All connection settings are defined on the server
line and ignored from the bind line.

Before this patch, most of keywords were silently ignored. This could
result in a configuration from doing unexpected things from the user
point of view. To improve this situation, add a new 'rhttp_ok' field in
bind_kw structure. If not set, the keyword is forbidden on a reverse
bind line and will cause a fatal config error.

For the moment, only the following keywords are usable with reverse bind
'id', 'name' and 'nbconn'.

This change is safe as it's already forbidden to mix reverse and
standard addresses on the same bind line.

20 months agoREGTESTS: remove maxconn from rhttp bind line
Amaury Denoyelle [Fri, 20 Oct 2023 15:26:24 +0000 (17:26 +0200)] 
REGTESTS: remove maxconn from rhttp bind line

The maxconn keyword is not used anymore for reverse HTTP bind. It has
been replaced recently by the new keyword nbconn. As it's default value
is 1, it can be safely removed from the regtest without affecting its
behavior.

20 months agoMINOR: cfgparse: rename "rev@" prefix to "rhttp@"
Amaury Denoyelle [Fri, 20 Oct 2023 09:34:46 +0000 (11:34 +0200)] 
MINOR: cfgparse: rename "rev@" prefix to "rhttp@"

'rev@' was used to specify a bind/server used with reverse HTTP
transport. This notation was deemed not explicit enough. Rename it
'rhttp@' instead.

20 months agoMINOR: server: convert @reverse to rev@ standard format
Amaury Denoyelle [Thu, 19 Oct 2023 09:07:15 +0000 (11:07 +0200)] 
MINOR: server: convert @reverse to rev@ standard format

Remove the recently introduced '@reverse' notation for HTTP reverse
servers. Instead, reuse the 'rev@' prefix already defined for bind
lines.

20 months agoMINOR: listener: add nbconn kw for reverse connect
Amaury Denoyelle [Thu, 19 Oct 2023 07:25:20 +0000 (09:25 +0200)] 
MINOR: listener: add nbconn kw for reverse connect

Previously, maxconn keyword was reused for a specific usage on reverse
HTTP binds to specify the number of active connect to proceed. To avoid
confusion, introduce a new dedicated keyword 'nbconn' which is specific
to reverse HTTP bind.

This new keyword is forbidden for non-reverse listener. A fatal error is
emitted during config parsing if this rule is not respected. It's safe
because it's also forbidden to mix standard and reverse addresses on the
same bind line.

Internally, nbconn value will be reassigned to 'maxconn' member of
bind_conf structure. This ensures that listener layer will automatically
reenable the preconnect task each time a connection is closed.

20 months agoMINOR: cfgparse: forbid mixing reverse and standard listeners
Amaury Denoyelle [Thu, 19 Oct 2023 10:05:31 +0000 (12:05 +0200)] 
MINOR: cfgparse: forbid mixing reverse and standard listeners

Reverse HTTP listeners are very specific and share only a very limited
subset of keywords with other listeners. As such, it is probable
meaningless to mix standard and reverse addresses on the same bind line.
This patch emits a fatal error during configuration parsing if this is
the case.

20 months agoBUG/MEDIUM: peers: Fix synchro for huge number of tables
Christopher Faulet [Mon, 16 Oct 2023 06:14:50 +0000 (08:14 +0200)] 
BUG/MEDIUM: peers: Fix synchro for huge number of tables

The number of updates sent at once was limited to not loop too long to emit
updates when the buffer size is huge or when the number of sync tables is
huge. The limit can be configured and is set to 200 by default. However,
this fix introduced a bug. It is impossible to syncrhonize two peers if the
number of tables is higher than this limit. Thus by default, it is not
possible to sync two peers if there are more than 200 tables to sync.

Technically speacking, a teaching process is finished if we loop on all tables
with no new update messages sent. Because we are limited at each call, the loop
is splitted on several calls. However the restart point for the next loop is
always the last table for which we emitted an update message. Thus with more
tables than the limit, the loop never reachs the end point.

Worse, in conjunction with the bug fixed by "BUG/MEDIUM: peers: Be sure to
always refresh recconnect timer in sync task", it is possible to trigger the
watchdog because the applets may be woken up in loop and leave requesting
more room while its buffer is empty.

To fix the issue, restart conditions for a teaching loop were changed. If
the teach process is interrupted, we now save the restart point, called
stop_local_table. It is the last evaluated table on the previous loop. This
restart point is reset when the teach process is finished.

In additionn, the updates_sent variable in peer_send_msgs() was renamed to
updates to avoid ambiguities. Indeed, the variable is incremented, whether
messages were sent or not.

This patch must be backported as far as 2.6.

20 months agoBUG/MEDIUM: peers: Be sure to always refresh recconnect timer in sync task
Christopher Faulet [Mon, 16 Oct 2023 05:48:26 +0000 (07:48 +0200)] 
BUG/MEDIUM: peers: Be sure to always refresh recconnect timer in sync task

A sync task used to manage reconnect, sessions creation or shutdown and data
synchronization is responsible to refresh reconnect and heartbeat timers for
each remote peers and trigger applets wakeup. These timers are used to
refresh the sync task timeer itself. Thus it is important to take care to
always properly refresh them.

However, when there are some data to push, the reconnect timer is not
checked. It may be expired and not refreshed. In this case, an expired timer
may be used to the sync task, leading to a storm of wakeups. The sync task
is woken up in loop because its timer is in the past, waking up Peer applets
at each time.

To fix the issue, the peer's reconnect timer is now refresh to the default
reconnect timeout, if necessary, when there are some data to push.

This patch must be backported to all stable versions.

20 months agoBUG/MINOR: trace: fix trace parser error reporting
Willy Tarreau [Thu, 19 Oct 2023 12:45:07 +0000 (14:45 +0200)] 
BUG/MINOR: trace: fix trace parser error reporting

Since traces were adapted to support being declared in the global section
in 2.7 with commit c11f1cdf4 ("MINOR: trace: split the CLI "trace" parser
in CLI vs statement"), the method used to return the error message was
unreliable. For example an invalid sink name in the global section would
produce:

  [ALERT]    (26685) : config : parsing [test-trace.cfg:51] : 'trace': No such sink
  [ALERT]    (26685) : config : parsing [test-trace.cfg:51] : (null)
  [ALERT]    (26685) : config : Error(s) found in configuration file : test-trace.cfg
  [ALERT]    (26685) : config : Fatal errors found in configuration.

The reason is that the trace is emitted manually using ha_error() in
cfg_parse_trace() and -1 is returned without setting the message, and
the caller also prints the empty message. That's quite awkward given
that the API originally comes from the CLI which does support dynamic
strings and that config keywords do as well.

This commit modifies both cli_parse_trace() and cfg_parse_trace() to
return a dynamically allocated message instead, and adapts the central
function trace_parse_statement() to do the same, replacing a few direct
assignments with strdup() or memprintf(). This way the alert is no
longer emitted by the parser function, it just passes the message to
the caller.

A few of the static messages switching to memprintf() also took this
opportunity to report the faulty word:

  [ALERT]    (26772) : config : parsing [test-trace.cfg:51] : No such trace sink 'stduot'
  [ALERT]    (26772) : config : Error(s) found in configuration file : test-trace.cfg
  [ALERT]    (26772) : config : Fatal errors found in configuration.

This may be backported to 2.8 and 2.7.

20 months agoBUG/MINOR: mux-h2: fix http-request and http-keep-alive timeouts again
Willy Tarreau [Wed, 18 Oct 2023 09:39:43 +0000 (11:39 +0200)] 
BUG/MINOR: mux-h2: fix http-request and http-keep-alive timeouts again

Stefan Behte reported that since commit f279a2f14 ("BUG/MINOR: mux-h2:
refresh the idle_timer when the mux is empty"), the http-request and
http-keep-alive timeouts don't work anymore on H2. Before this patch,
and since 3e448b9b64 ("BUG/MEDIUM: mux-h2: make sure control frames do
not refresh the idle timeout"), they would only be refreshed after stream
frames were sent (HEADERS or DATA) but the patch above that adds more
refresh points broke these so they don't expire anymore as long as
there's some activity.

We cannot just revert the fix since it also addressed an isse by which
sometimes the timeout would trigger too early and provoque truncated
responses. The right approach here is in fact to only use refresh the
idle timer when the mux buffer was flushed from any such stream frames.

In order to achieve this, we're now setting a flag on the connection
whenever we write a stream frame, and we consider that flag when deciding
to refresh the buffer after it's emptied. This way we'll only clear that
flag once the buffer is empty and there were stream data in it, not if
there were no such stream data. In theory it remains possible to leave
the flag on if some control data is appended after the buffer and it's
never cleared, but in practice it's not a problem as a buffer will always
get sent in large blocks when the window opens. Even a large buffer should
be emptied once in a while as control frames will not fill it as much as
data frames could.

Given the patch above was backported as far as 2.6, this patch should
also be backported as far as 2.6.

20 months agoMINOR: dgram: allow to set rcv/sndbuf for dgram sockets as well
Willy Tarreau [Tue, 3 Oct 2023 13:33:46 +0000 (15:33 +0200)] 
MINOR: dgram: allow to set rcv/sndbuf for dgram sockets as well

tune.rcvbuf.client and tune.rcvbuf.server are not suitable for shared
dgram sockets because they're per connection so their units are not the
same. However, QUIC's listener and log servers are not connected and
take per-thread or per-process traffic where a socket log buffer might
be too small, causing undesirable packet losses and retransmits in the
case of QUIC. This essentially manifests in listener mode with new
connections taking a lot of time to set up under heavy traffic due to
the small queues causing delays. Let's add a few new settings allowing
to set these shared socket sizes on the frontend and backend side (which
reminds that these are per-front/back and not per client/server hence
not per connection).

20 months agoREORG: stconn/muxes: Rename init step in fast-forwarding
Christopher Faulet [Wed, 18 Oct 2023 10:18:10 +0000 (12:18 +0200)] 
REORG: stconn/muxes: Rename init step in fast-forwarding

Instead of speaking of an initialisation stage for each data
fast-forwarding, we now use the negociate term. Thus init_ff/init_fastfwd
functions were renamed nego_ff/nego_fastfwd.

20 months agoBUILD: mux-h1: Fix build without kernel splicing support
Christopher Faulet [Wed, 18 Oct 2023 09:57:40 +0000 (11:57 +0200)] 
BUILD: mux-h1: Fix build without kernel splicing support

Data fast-forwarding does not build without the kernel splicing support
because counters about splicing don't exist. To make the code more readable,
all code about splicing is disabled if kernel splicing is not supported.

20 months agoMINOR: global: Add an option to disable the zero-copy forwarding
Christopher Faulet [Mon, 16 Oct 2023 16:28:59 +0000 (18:28 +0200)] 
MINOR: global: Add an option to disable the zero-copy forwarding

The zero-copy forwarding or the mux-to-mux forwarding is a way to
fast-forward data without using the channels buffers. Data are transferred
from a mux to the other one. The kernel splicing is an optimization of the
zero-copy forwarding. But it can also use normal buffers (but not channels
ones). This way, it could be possible to fast-forward data with muxes not
supporting the kernel splicing (H2 and H3 muxes) but also with applets.

However, this mode can introduce regressions or bugs in future (just like
the kernel splicing). Thus, It could be usefull to disable this optim. To do
so, in configuration, the global tune settting
'tune.disable-zero-copy-forwarding' may be set in a global section or the
'-dZ' command line parameter may be used to start HAProxy. Of course, this
also disables the kernel splicing.

20 months agoMEDIUM: mux-pt: Add fast-forwarding support
Christopher Faulet [Fri, 6 Oct 2023 13:32:47 +0000 (15:32 +0200)] 
MEDIUM: mux-pt: Add fast-forwarding support

The PT multiplexer now implements callbacks function to produce and consume
fast-forwarded data. Only splicing is support because the mux-pt does not
use its own buffers.

20 months agoCLEAN: mux-h1: Remove useless __maybe_unused attribute on h1_make_chunk()
Christopher Faulet [Thu, 5 Oct 2023 09:24:54 +0000 (11:24 +0200)] 
CLEAN: mux-h1: Remove useless __maybe_unused attribute on h1_make_chunk()

This attribute was added during the dev stage. But it is useless now the
function is used. So, just remove it.

20 months agoREGTESTS: Reenable HTTP tests about splicing
Christopher Faulet [Fri, 4 Aug 2023 12:24:47 +0000 (14:24 +0200)] 
REGTESTS: Reenable HTTP tests about splicing

20 months agoMINOR: tree-wide: Only rely on co_data() to check channel emptyness
Christopher Faulet [Tue, 10 Oct 2023 16:00:38 +0000 (18:00 +0200)] 
MINOR: tree-wide: Only rely on co_data() to check channel emptyness

Because channel_is_empty() function does now only check the channel's
buffer, we can remove it and rely on co_data() instead. Of course, all tests
must be inverted.

channel_is_empty() is thus removed.

20 months agoMEDIUM: channel: don't look at iobuf to report an empty channel
Christopher Faulet [Tue, 25 Jul 2023 06:30:01 +0000 (08:30 +0200)] 
MEDIUM: channel: don't look at iobuf to report an empty channel

It is important to split channels and I/O buffers. When data are pushed in
an I/O buffer, we consider them as forwarded. The channel never sees
them. Fast-forwarded data are now handled in the SE only.

20 months agoMEDIUM: mux-h2: Add consumer-side fast-forwarding support
Christopher Faulet [Thu, 3 Aug 2023 16:18:45 +0000 (18:18 +0200)] 
MEDIUM: mux-h2: Add consumer-side fast-forwarding support

The H2 multiplexer now implements callbacks to consume fast-forwarded
data. It is the most usful case: A H2 client getting data from a H1
server. It is also the easiest case to implement. The producer side is
trickier because of multiplexing. It is not obvious this case would be
improved with data fast-forwarding.

20 months agoMINOR: h2: Set the BODYLESS_RESP flag on the HTX start-line if necessary
Christopher Faulet [Tue, 10 Oct 2023 14:30:52 +0000 (16:30 +0200)] 
MINOR: h2: Set the BODYLESS_RESP flag on the HTX start-line if necessary

When message headers are parsed and an HTX start-line is created, if we
detect the response must not have any payload, a specific flag must be set
on the HTX start-line. It happens for instance for response to HEAD
requests. This flag is useb by the multiplexers to know response payload, if
any, must be silently skipped.

This was not performed when h2 HEADERS frames were decoded. This HTX flag
was specifically added to fix a bug when the splicing is inuse. Thus the H2
multiplexer was not concerned. Because the mux-to-mux fast-forwarding will
be introduced, it is important handle this flag in the H2 multiplexer too.

20 months agoMEDIUM: mux-h1: Add fast-forwarding support
Christopher Faulet [Fri, 29 Sep 2023 12:25:40 +0000 (14:25 +0200)] 
MEDIUM: mux-h1: Add fast-forwarding support

The H1 multiplexer now implements callbacks function to produce and consume
fast-forwarded data.

20 months agoMEDIUM: mux-h1: Simplify payload formatting based on HTX blocks on sending path
Christopher Faulet [Wed, 27 Sep 2023 14:33:29 +0000 (16:33 +0200)] 
MEDIUM: mux-h1: Simplify payload formatting based on HTX blocks on sending path

Just like for the zero-copy, this patch tries to simplify the code
responsible to format the message payload before sending it. But here, we
take care to simplify the loop on the HTX blocks. The result should be
less errorrpone.

20 months agoMEDIUM: mux-h1: Simplify zero-copy on sending path
Christopher Faulet [Wed, 27 Sep 2023 14:28:32 +0000 (16:28 +0200)] 
MEDIUM: mux-h1: Simplify zero-copy on sending path

In h1_make_data(), the function responsible to format the message payload
before sending it, the code dealing with zero-copy was slighly simplified
(at least for me :).

There is no real change but there is a better split between messages with a
content-length and cunked messages.

20 months agoMINOR: mux-h1: Add function to add size of a chunk to an outgoind message
Christopher Faulet [Mon, 25 Sep 2023 09:05:14 +0000 (11:05 +0200)] 
MINOR: mux-h1: Add function to add size of a chunk to an outgoind message

This function should be used to send the chunk size, before appending the
chunk payload. It also takes care to add a CRLF to finish a previous chunk,
if necessary. This function will be used to fix the splicing for re-chunk
responses with an unknown length.

20 months agoMEDIUM: raw-sock: Specifiy amount of data to send via snd_pipe callback
Christopher Faulet [Tue, 26 Sep 2023 16:05:29 +0000 (18:05 +0200)] 
MEDIUM: raw-sock: Specifiy amount of data to send via snd_pipe callback

When data were sent using the kernel splicing, we tried to send all data
with no restriction. Most of time it is valid. However, because the payload
representation may differ between the producer and the consumer, it is
important to be able to specify how must data to send via the splicing.

Of course, for performance reason, it is important to maximize amount of
data send via splicing at each call. However, on edge-cases, this now can be
limited.

20 months agoMEDIUM: mux-h1: Properly handle state transitions of chunked outgoing messages
Christopher Faulet [Tue, 26 Sep 2023 16:00:49 +0000 (18:00 +0200)] 
MEDIUM: mux-h1: Properly handle state transitions of chunked outgoing messages

On the sending path, there are 3 states for chunked payload in H1:

  * H1_MSG_CHUNK_SIZE: the chunk size must be emitted
  * H1_MSH_CHUNK_CRLF: The end of the chunk must be emitted
  * H1_MSG_DATA: Chunked data must be emitted

However, some shortcuts were used on the sending path to avoid some
transitions. Especially, outgoing messages were never switched in
H1_MSG_CHUNK_SIZE state.

However, it will be necessary to properly handle all transitions on the payload
to implement mux-to-mux forwarding, to be sure to always known when the chunk
size or the end of the chunk must be emitted.

20 months agoMINOR: mux-h1: Use HTX extra field only for responses with known length
Christopher Faulet [Mon, 25 Sep 2023 13:59:07 +0000 (15:59 +0200)] 
MINOR: mux-h1: Use HTX extra field only for responses with known length

For now, it is not an issue, but it is safer to explicitly ignore HTX extra
field for responses with unknown length. This will be mandatory to future
fixes, to be able to re-chunk responses with an unknown length..

20 months agoMEDIUM: stconn: Add mux-to-mux fast-forward support
Christopher Faulet [Thu, 3 Aug 2023 15:51:58 +0000 (17:51 +0200)] 
MEDIUM: stconn: Add mux-to-mux fast-forward support

Now the kernel splicing support was removed, we can add mux-to-mux
fast-forward support. Of course, the splicing support will be reintroduced
in the muxes themselves but this will be transparent.

Changes are mainly located into sc_conn_recv() and sc_conn_send().