Historically, pat_ref lookup operations were performed on the whole
pat_ref elements list. As such, set, find and delete operations on a given
key would cause any matching element in pat_ref to be considered.
When prepare/commit operations were added, gen_id was impelemnted in
order to be able to work on a subset from pat_ref without impacting
the current (live) version from pat_ref, until a new subset is committed
to replace the current one.
While the logic was good, there remained a design flaw from the historical
implementation: indeed, legacy functions such as pat_ref_set(),
pat_ref_delete() and pat_ref_find_elt() kept performing the lookups on the
whole set of elements instead of considering only elements from the current
subset. Because of this, mixing new prepare/commit operations with legacy
operations could yield unexpected results.
For instance, before this commit:
echo "add map #0 key oldvalue" | socat /tmp/ha.sock -
echo "prepare map #0" | socat /tmp/ha.sock -
New version created: 1
echo "add map @1 #0 key newvalue" | socat /tmp/ha.sock -
echo "del map #0 key" | socat /tmp/ha.sock -
echo "commit map @1 #0" | socat /tmp/ha.sock -
-> the result would be that "key" entry doesn't exist anymore after the
commit, while we would expect the new value to be there instead.
Thanks to the previous commits, we may finally fix this issue: for set,
find_elt and delete operations, the current generation id is considered.
With the above example, it means that the "del map #0 key" would only
target elements from the current subset, thus elements in "version 1" of
the map would be immune to the delete (as we would expect it to work).
Don't set all duplicates from a given node if they don't have the same
gen_id. Indeed, now we consider the gen_id to only work on the same
pattern ref revision.
pat_ref_gen_delete(ref, gen_id, key) tries to delete all samples belonging
to <gen_id> and matching <key> under <ref>
The goal is to be able to target a single subset from <ref>
pat_ref_gen_find_elt(ref, gen_id, key) tries to find <elt> element
belonging to <gen_id> and matching <key> in <ref> reference.
The goal is to be able to target a single subset from <ref>
pat_ref_gen_set(ref, gen_id, value, err) modifies to <value> the sample
of all patterns matching <key> and belonging to <gen_id> (generation id)
under <ref>
The goal is to be able to target a single subset from <ref>
split pat_ref_set() function in 2 distinct functions. Indeed, since
0844bed7d3 ("MEDIUM: map/acl: Improve pat_ref_set() efficiency (for
"set-map", "add-acl" action perfs)"), pat_ref_set() prototype was updated
to include an extra <elt> argument. But the logic behind is not explicit
because the function will not only try to set <elt>, but also its
duplicate (unlike pat_ref_set_elt() which only tries to update <elt>).
Thus, to make it clearer and better distinguish between the key-based
lookup version and the elt-based one, restotre pat_ref_set() previous
prototype and add a dedicated pat_ref_set_elt_duplicate() that takes
<elt> as argument and tries to update <elt> and all duplicates.
Released version 3.1.0 with the following main changes :
- BUG/MAJOR: mux-h1: Properly handle wrapping on obuf when dumping the first-line
- BUILD: activity/memprofile: fix a build warning in the posix_memalign handler
- BUG/MINOR: quic: Avoid BUG_ON() on ->on_pkt_lost() BBR callback call
- CI: update to the latest AWS-LC version
- CI: update to the latest WolfSSL version
- DOC: ot: mention planned deprecation of the OT filter
- Revert "CI: update to the latest WolfSSL version"
- CI: github: add a WolfSSL job which tries the latest version
- BUILD: systemd: fix usage of reserved name "sun" in the address field
- BUILD: init: use the more portable FD_CLOEXEC for /dev/null
- CI: github: improve the Wolfssl job
- CI: github: improve the AWS-LC job
- BUG/MINOR: mux-quic: fix show quic report of QCS prepared bytes
- BUG/MEDIUM: quic: fix sending performance due to qc_prep_pkts() return
- MINOR: mux-quic: use sched call time for pacing
- CI: github: allow to run the Illumos job manually
- BUILD: tcp_sample: var_fc_counter defined but not used
- CI: github: add 'workflow_dispatch' on remaining build jobs
- DOC: config: refine a little bit the text on QUIC pacing
- MINOR: proto_sockpair: send_fd_uxst: init iobuf, cmsghdr, cmsgbuf to zeros
- MINOR: startup: rename on_new_child_failure to mworker_on_new_child_failure
- REORG: startup: move on_new_child_failure in mworker.c
- MINOR: startup: prefix prepare_master and run_master with mworker_*
- REORG: startup: move mworker_prepare_master in mworker.c
- MINOR: startup: keep updating verbosity modes only in haproxy.c
- REORG: startup: move mworker_run_master and mworker_loop in mworker.c
- REORG: startup: move mworker_reexec and mworker_reload in mworker.c
- MINOR: startup: prefix apply_master_worker_mode with mworker_*
- REORG: startup: move mworker_apply_master_worker_mode in mworker.c
- MINOR: cfgparse-quic: strengthen quic-cc-algo parsing
- BUG/MAJOR: quic: fix wrong packet building due to already acked frames
- DEV: lags/show-sess-to-flags: Properly handle fd state on server side
- BUG/MEDIUM: http-ana: Don't release too early the L7 buffer
- MINOR: quic: make bbr consider the max window size setting
- DOC: quic: Amend the pacing information about BBR.
- BUG/MEDIUM: quic: prevent EMSGSIZE with GSO for larger bufsize
- MINOR: cli: Add a "help" keyword to show sess
- MINOR: cli/quic: Add a "help" keyword to show quic
- DOC: management: mention "show sess help" and "show quic help"
- DOC: install: update the list of supported versions
- MINOR: version: mention that 3.1 is stable now
A UDP datagram cannot be greater than 65535 bytes, as UDP length header
field is encoded on 2 bytes. As such, sendmsg() will reject a bigger
input with error EMSGSIZE. By default, this does not cause any issue as
QUIC datagrams are limited to 1.252 bytes and sent individually.
However, with GSO support, value bigger than 1.252 bytes are specified
on sendmsg(). If using a bufsize equal to or greater than 65535, syscall
could reject the input buffer with EMSGSIZE. As this value is not
expected, the connection is immediately closed by haproxy and the
transfer is interrupted.
This bug can easily reproduced by requesting a large object on loopback
interface and using a bufsize of 65535 bytes. In fact, the limit is
slightly less than 65535, as extra room is also needed for IP + UDP
headers.
Fix this by reducing the count of datagrams encoded in a single GSO
invokation via qc_prep_pkts(). Previously, it was set to 64 as specified
by man 7 udp. However, with 1252 datagrams, this is still too many.
Reduce it to a value of 52. Input to sendmsg will thus be restricted to
at most 65.104 bytes if last datagram is full.
If there is still data available for encoding in qc_prep_pkts(), they
will be written in a separate batch of datagrams. qc_send_ppkts() will
then loop over the whole QUIC Tx buffer and call sendmsg() for each
series of at most 52 datagrams.
This does not need to be backported.
Limit the BBR congestion control window size as this is done for all the others
congestion control algorithms with tune.quic.frontend.default-max-window-size
or as first argument passed to "bbr" option for "quic-cc-algo".
In some cases, the buffer used to store the request to be able to perform a
L7 retry is released released too early, leading to a crash because a retry
is performed with an empty request.
First, there is a test on invalid 101 responses that may be caught by the
"junk-response" retry policy. Then, it is possible to get an error
(empty-response, bad status code...) after an interim response. In both
cases, the L7 buffer is already released while it should not.
To fix the issue, the L7 buffer is now released at the end of the
AN_RES_WAIT_HTTP analyser, but only when a response was successfully
received and processed. In all error cases, the stream is quickly released,
with the L7 buffer. So there is no leak and it is safer this way.
This patch may fix the issue #2793. It must be as far as 2.4.
If a packet build was asked to probe the peer with frames which have just
been acked, the frames build run by qc_build_frms() could be cancelled by
qc_stream_frm_is_acked() whose aim is to check that current frames to
be built have not been already acknowledged. In this case the packet build run
by qc_do_build_pkt() is not interrupted, leading to the build of an empty packet
which should be ack-eliciting.
This is a bug detected by the BUG_ON() statement in qc_do_build_pk():
BUG_ON(qel->pktns->tx.pto_probe &&
!(pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING));
Thank you to @Tristan971 for having reported this issue in GH #2709
This is an old bug which must be backported as far as 2.6.
quic-cc-algo is a bind keyword which is used to specify the congestion
control algorithm. It is parsed via function bind_parse_quic_cc_algo().
The parsing function was too laxed as it used strncmp for algo token
matching. This could cause surprise if specifying an invalid algorithm
but starting identically to another entry. Especially if extra
parameters are specified in parenthesis, as in this case parameters
value will be completely ignored and default value used instead.
To fix this, convert algo argument to ist. Then, use istsplit() to
extract algo token from the optional extra arguments and compare the
whole value with isteq().
Let's move mworker_reexec() and mworker_reload() in mworker.c. mworker_reload()
is called only within the functions, which are already in mworker.c. So, this
reorganization allows to declare mworker_reload() as a static.
mworker_run_master() is called only in master mode. mworker_loop() is static
and called only in mworker_run_master(). So let's move these both functions in
mworker.c.
We also need here to make run_thread_poll_loop() accessible from other units,
as it's used in mworker_loop().
This commit prepares the move of mworker_run_master() in mworker.c.
Let's remove from it's definition the code, which adjusts verbosity in
dependency of other global run time modes (daemon or foreground). This part
should stay in main(), where all verbosity modes are handeled for
different mode combinations.
mworker_prepare_master() performs some preparation routines for the new worker
process, which will be forked during the startup. It's called only in
master-worker mode, so let's move it in mworker.c.
mworker_on_new_child_failure() performs some routines for the worker process,
if it has failed the reload. As it's called only in mworker_catch_sigchld()
from mworker.c, let's move mworker_on_new_child_failure() in mworker.c as well.
Like this it could also be declared as a static.
This patch prepares the moving of on_new_child_failure definition into
mworker.c. So, let's rename it accordingly and let's also update its
description.
In master-worker mode, worker process uses now send_fd_uxst() to send
'_send_status' command to master. Since refactoring, this started to trigger
the following Valgrind reports:
==810584== Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
==810584== at 0x4AAC99D: __libc_sendmsg (sendmsg.c:28)
==810584== by 0x4AAC99D: sendmsg (sendmsg.c:25)
==810584== by 0x56350F: send_fd_uxst (proto_sockpair.c:271)
==810584== by 0x3AA25C: main (haproxy.c:4151)
==810584== Address 0x1ffefffbfe is on thread 1's stack
==810584== in frame #1, created by send_fd_uxst (proto_sockpair.c:241)
==810584==
==810584== Syscall param sendmsg(msg.msg_control) points to uninitialised byte(s)
==810584== at 0x4AAC99D: __libc_sendmsg (sendmsg.c:28)
==810584== by 0x4AAC99D: sendmsg (sendmsg.c:25)
==810584== by 0x56350F: send_fd_uxst (proto_sockpair.c:271)
==810584== by 0x3AA25C: main (haproxy.c:4151)
==810584== Address 0x1ffefffc14 is on thread 1's stack
==810584== in frame #1, created by send_fd_uxst (proto_sockpair.c:241)
==810584==
So, let's initialize with zeros all buffers, which are passed to sendmsg
syscall(), used in send_fd_uxst() to avoid these Valgrind messages. They
increase Valgrind output and could make unnoticeable some other, more important
reports.
The QUIC pacing options changed a few times during their development.
For example the unit is now in datagrams not bytes. Also a few
sentences were slightly ambiguous so let's reword this.
No backport is needed.
Add 'workflow_dispatch' on the remaining scheduled build jobs that does
not have it.
This keyword allows to start manually a job from the "Actions" interface
in github.
var_fc_counter is not used on Illumos and emit a warning
src/tcp_sample.c:291:12: warning: ‘var_fc_counter’ defined but not used [-Wunused-function]
291 | static int var_fc_counter(struct arg *args, char **err)
| ^~~~~~~~~~~~~~
Let's add an ifdef to build it.
QUIC pacing was recently implemented to limit burst and improve overall
bandwidth. This is used only for MUX STREAM emission. Pacing requires
nanosecond resolution. As such, it used now_cpu_time() which relies on
clock_gettime() syscall.
The usage of clock_gettime() has several drawbacks :
* it is a syscall and thus requires a context-switch which may hurt
performance
* it is not be available on all systems
* timestamp is retrieved multiple times during a single task execution,
thus yielding different values which may tamper pacing calculation
Improve this by using task_mono_time() instead. This returns task call
time from the scheduler thread context. It requires the flag
TASK_F_WANTS_TIME on QUIC MUX tasklet to force the scheduler to update
call time with now_mono_time(). This solves every limitations listed
above :
* syscall invokation is only performed once before tasklet execution,
thus reducing context-switch impact
* on non compatible system, a millisecond timer is used as a fallback
which should ensure that pacing works decently for them
* timer value is now guaranteed to be fixed duing task execution
qc_prep_pkts() is a QUIC transport level function which encodes one or
several datagrams in a buffer before sending them. It returns the number
of encoded datagram. This is especially important when pacing is used to
limit packet bursts.
This datagram accounting was not trivial as qc_prep_pkts() used several
code paths depending on the condition of the current encoded packet.
Thus, there were several places were the local variable dgram_cnt could
have been incremented. This was implemented by the following commit :
commit 5cb8f8a622
MINOR: quic: support a max number of built packet per send iteration
However, there is a bug due to a missing increment when all frames from
the current QEL have been encoded. In this case, the encoding continue
in the same datagram to coalesce a futur packet. However, if this is the
last QEL, encoding loop will then break. As first_pkt is not NULL,
qc_txb_store() is called outside but dgram_cnt is yet not incremented.
In particular, this causes qc_prep_pkts() to return 0 when there is only
small STREAM frames to emit for application QEL. In qc_send(), this is
interpreted as a value which prevents further emission for the current
invokation. Thus, it may hurts performance, both without and with
pacing.
To fix this, removing multiple dgram_cnt increment. Now, it is modified
only in a single place which should cover every case, and render the
code easier to validate.
The most notable case where the bug is visible is when using cubic with
pacing without any burst, with quic-cc-algo cubic(,1). First, transfer
bandwidth in average was suboptimal, with significant variation. Worst,
it could sometimes fall dramatically for a particular stream without
recovering before returning to an expected level on the next one.
No need to backport.
On show quic, each MUX streams are listed with their various indicator
for buffering on Rx and Tx. In particular, txoff displays in parenthesis
the current level of data prepared by the upper stream instance not yet
emitted by QUIC transport layer.
This value is only accessible after a substract operation. However,
there was a typo which caused the result to be always 0. Fix this by
reusing the correct offsets in the calculation.
This should be backported up to 3.0.
In 3.1-dev10, commit 8dd4efe42f ("MAJOR: mworker: move master-worker
fork in init()"), the FD associated to /dev/null was made CLOEXEC
using O_CLOEXEC. Unfortunately this is not portable on older OSes,
doesn't build on Solaris for example, and was even reported as breaking
moderately old Linux OSes for other projects. Better not use it unless
absolutely certain it will work (currently we only use it for Linux
namespaces, which are optional), and use the conventional FD_CLOEXEC
instead.
No backport is needed.
systemd.c doesn't build on Solaris / Illumos because it uses "sun" as
the field name in a structure, while "sun" is the name of the macro
used to detect Solaris:
src/systemd.c: In function 'sd_notify':
src/systemd.c:43:22: error: expected identifier or '(' before numeric constant
struct sockaddr_un sun;
^
src/systemd.c:44:2: warning: no semicolon at end of struct or union
} socket_addr = {
^
Admittedly, the OS could have instead defined "sun" to itself to avoid
this. Any other name will work, let's just use "ux" for the short form
of "unix".
The problem appeared in 3.0-dev with commit aa3632962f ("MEDIUM:
mworker: get rid of libsystemd"), though by then this file was only
built when USE_SYSTEMD was set, which was not the case for non-linux
platforms. However since 3.1-dev14 with commit 15845247db ("MEDIUM:
mworker: remove USE_SYSTEMD requirement for -Ws"), all platforms
now build this file.
No backport is needed even though it will not hurt to have it in 3.0
for completeness.
Like the AWS-LC job, add a CI job which looks for the latest WolfSSL
version and tries to build it.
The patch adds a function which determines the latest version of WolfSSL
from the github tag, and the yml which describes the job.
Miroslav mentioned below that he's currently working on an OpenTelemetry
replacement for the OpenTracing filter since OpenTracing itself is no
longer maintained nor supported:
https://github.com/haproxy/haproxy/issues/2782#issuecomment-2493576327
Given that he aims for 3.2, let's already settle on an upcoming deprecation
of the filter for 3.3 with a removal for 3.5. This will leave time to finish
the development and permit users to switch smoothly. At this point no warning
is emitted (since the users have no alternative) but better mention this plan
in the doc to make them aware of future changes.
The per-packet delivery rate sample is applied to ack-eliciting packet only
calling ->drs_on_transmit() BBR callback. So, ->on_pkt_lost() which inspects the
delivery rate sampling information during packet loss detection must not be
called for non ack-eliciting packet. If not, it would be facing with non
initialized variables with big chance to trigger a BUG_ON().
As BBR is implemented in the current developement version, there is
no need to backport this patch.
A "return NULL" statement was placed for error handling in the
posix_memalign() handler instead of an int errno value, by recent
commit 5ddc8b3ad4 ("MINOR: activity/memprofile: monitor non-portable
calls as well"). Surprisingly the warning only triggered on gcc-4.8.
Let's use ENOMEM instead. No backport needed.
The formatting of the first-line, for a request or a response, does not
properly handle the wrapping of the output buffer. This may lead to a data
corruption for the current response or eventually for the previous one.
Utility functions used to format the first-line of the request or the
response rely on the chunk API. So it is not expected to pass a buffer that
wraps. Unfortunatly, because of a change performed during the 2.9 dev cycle,
the output buffer was direclty used instead of a non-wrapping buffer created
from it with b_make() function. It is not an issue for the request because
its start-line is always the first block formatted in the output buffer. But
for the response, the output may be not empty and may wrap. In that case,
the response start-line is dumped at a random position in the buffer,
corrupting data. AFAIK, it is only an issue if the HTTP request pipelining
is used.
To fix the issue, we now take care to create a non-wapping buffer from the
output buffer.
This patch should fix issues #2779 and #2996. It must be backported as far as
2.9.
