User tried to update a PEM, generated automatically. Part of this PEM has LF
line endings, and another part (CA certificate), added by some API, has CRLF
line endings. This has revealed a bug in cli_snd_buf(), see more
details in issue GitHUB #2818. So, let's add an example of such PEM in our
SSL regtest.
cli_snd_buf() analyzez input line by line. Before this patch it has always
scanned a given line for the presence of '\r' followed by '\n'.
This is only needed for strings, that contain the commands itself like
"show ssl cert\n", "set ssl cert test.pem <<\n".
In case of strings, which contain the command's payload, like
"-----BEGIN CERTIFICATE-----\r\n", '\r\n' should be preserved
as is.
This patch fixes the GitHub issue #2818.
This patch should be backported in v3.1 and in v3.0.
This bug fixes the 3rd condition used by bbr_check_startup_high_loss() to decide
it has detected some high loss as mentioned by the BBR v3 RFC draft:
4.3.1.3. Exiting Startup Based on Packet Loss
...
There are at least BBRStartupFullLossCnt=6 discontiguous sequence ranges lost in that round trip.
where a <= operator was used in place of <.
Must be backported to 3.1.
bbr_congestion_event() role is to track the start time of recovery periods.
This was done using <ts> passed as parameter. But this parameter is the
time the newest lost packet has been sent.
The timestamp value to store in ->recovery_start_ts is <now_ms>.
Must be backported to 3.1.
->app_limited quic_drs struct member is not a boolean. This is
the index of the last transmitted packet marked as application-limited, or 0 if
the connection is not currently application-limited (see C.app_limited
definition in BBR v3 draft).
After these commits:
BUG/MINOR: quic: remove max_bw filter from delivery rate sampling
BUG/MINOR: quic: fix BBB max bandwidth oscillation issue
where some members were removed from bbr struct, the private data
size of QUIC cc algorithms may be reduced from 160 to 144 uint32_t.
Should be easily backported to 3.1 alonside the commits mentioned above.
Upon congestion events (for a instance packet loss),
bbr_adapt_lower_bounds_from_congestion() role is to adapt some BBR internal
variables in relation with the estimated bandwidth (BBR.bw).
According to the BBR v3 draft, this function should do nothing
if BBRIsProbingBW() pseudo-code returns true. That said, this function
is not defined by the BBR v3 draft. But according to this part mentioned before
defining the pseudo-code for BBRAdaptLowerBoundsFromCongestion():
4.5.10.3. When not Probing for Bandwidth
When not explicitly accelerating to probe for bandwidth (Drain, ProbeRTT,
ProbeBW_DOWN, ProbeBW_CRUISE), BBR responds to loss by slowing down to some extent.
This is because loss suggests that the available bandwidth and safe volume of
in-flight data may have decreased recently, and the flow needs to adapt, slowing
down toward the latest delivery process. BBR flows implement this response by
reducing the short-term model parameters, BBR.bw_lo and BBR.inflight_lo.
BBRIsProbingBW() should concern the accelerating probe for bandwidth states
which are BBR_ST_PROBE_BW_REFILL and BBR_ST_PROBE_BW_UP.
Adapt the code to match this latter assumption. At least this reduce
drastically the packet loss volumes at least during ProbeBW_CRUISE.
As an example, on a 100MBits/s internet link with ~94ms as RTT, before
this patch, 4329640 sent packets were needed with 1617119 lost packets (!!!) to
download a 3GB object. After this patch, 2843952 sent packets vs 144134 lost packets
are needed. There may be some packet loss issue. I suspect the maximum bandwidth
which may be overestimated. More this is the case, more the packet loss is big.
That said, at this time, it remains below 5% depending on the size of the objects,
5% being for more than 2GB objects.
Must be backported to 3.1.
Add a test to ensure that values of a local variable used by
bbr_inflight_hi_from_lost_packet() is not be impacted by underflow issues
when subtracting too big numbers and make this function return a correct value.
Must be backported to 3.1.
This filter is no more needed after this commit:
BUG/MINOR: quic: fix BBB max bandwidth oscillation issue.
Indeed, one added this filter at delivery rate sampling level to filter
the BBR max bandwidth estimations and was inspired from ngtcp2 code source when
trying to fix the oscillation issue. But this BBR max bandwidth oscillation issue
was fixed by the aforementioned commit.
Furthermore this code tends to always increment the BBR max bandwidth. From my point
of view, this is not a good idea at all.
Must be backported to 3.1.
This bug arrived with this commit:
6404b7a18a BUG/MINOR: quic: fix bbr_inflight() calls with wrong gain value
This patch partially reverts after having checked the BBR v3 draft.
This bug was invisible when testing long BBR flows.
Must be backported to 3.1.
Remove the code in relation with BBR.ack_phase as per this commit:
ee98c12ad6
I do now kwow at this time why such a request was pushed on GH for the BBR v3 draft
pseudo-code. That said, the use of such an ack phase seemed confusing, adding much
more information about a BBR flow state than needed. Indeed, the ack phase
state is modified several times in the BBR draft pseudo-code but only used to
decide if the max bandwidth filter virtual clock had to be incremented by
BBRAdvanceMaxBwFilter().
In addition to this, when discussing about haproxy BBR implementation with
Neal Cardwell on the BBR development google group about an oscillation issue
of the max bandwidth (BBR.max_bw), I concluded that this was due to the fact
that its filter virutal clock was too often update, due to the ack phase wich
was stalled in BBR_ACK_PHASE_ACKS_PROBE_STOPPING state for too long. This is
where Neal asked me to test the aforementioned commit. This definitively
makes the max bandwidth (BBR.max_bw) oscillation issue disappear.
Another solution would have been to add a new ack phase enum afer
BBR_ACK_PHASE_ACKS_PROBE_STOPPING. BBR_ACK_PHASE_ACKS_PROBE_STOPPED
would have been a good candidate.
Remove the code in relation with BBR.ack_phase.
Must be backported to 3.1.
The windowed filters are used only the BBR implementation for QUIC to filter
the maximum bandwidth samples for its estimation over a virtual time interval
tracked by counting the cyclical progression through ProbeBW cycles. ngtcp2
and quiche use such windowed filters in their BBR implementation. But in a
slightly different way. When updating the 2nd or 3rd filter samples, this
is done based on their values in place of the time they have been sampled.
It seems more logical to rely on the sample timestamps even if this has no
implication because when a sample is updated using another sample because it
has the same value, they have both the same timestamps!
This patch modifies two statements which compare two consecutive filter samples
based on their values (smp[]->v) by statements which compare them based on the
virtual time they have been sampled (smp[]->t). This fully complies which the
code used by the Linux kernel in lib/win_minmax.c.
Alo take the opportunity of this patch to shorten some statements using <smp>
local variable value to update smp[2] sample in place of initializing its two
members with the <smp> member values.
This patch SHOULD be easily backported to 3.1 where BBR was first implemented.
It's been some time since we are compatible with multiple SSL libraries,
let's rename the "OpenSSL library" strings in "SSL library" strings in
haproxy -vv, in order to be more generic.
Add the FIPS mode in haproxy -vv, it need to be activated on the system
with openssl.cnf or by compiling the SSL library with the right options.
Can't work with OpenSSL >= 3.0 because fips a "provider" to load, works
with AWS-LC, WolfSSL and OpenSSL 1.1.1.
Provide alternative code during stats dump when stress mode is active.
The objective is to force the applet to yield on every output line. This
allows to easily test reentrant code paths, in particular while adding
and removing server instances.
To support this, output is interrupted every time the output buffer (or
its equivalent) is not empty. Use COND_STRESS() macro to provide default
and stress alternative conditions.
Previous patch introduced stress mode to be able to easily test
alternative code paths.
The first point would be to force interruption of stats dump on every
line and check reentrant patchs, in particular while adding and removing
servers instances.
The purpose of this patch is to be able to use applet_putchk_stress()
during stats dump while not impacting other applets. To support this,
extract applet_putchk() into an internal _applet_putchk() which have a
new argument stress. Define two helpers applet_putchk() and
applet_putchk_stress(), the latter to set the stress argument to true.
For the moment, applet_putchk_stress() is not used. This will be the
subject of the next patch.
Define a new build mode DEBUG_STRESS. This will be used to stress some
code parts which cannot be reproduce easily with an alternative
suboptimal code.
First, a global <mode_stress> is set either to 1 or 0 depending on
DEBUG_STRESS compilation. A new global keyword "stress-level" is also
defined. It allows to specify a level from 0 to 9, to increase the
stress incurred on the code.
Helper macro STRESS_RUN* are defined for each stress level. This allows
to easily specify an instruction in default execution and a stress
counterpart if running on the corresponding stress level.
Released version 3.2-dev1 with the following main changes :
- MINOR: pattern: split pat_ref_set()
- MINOR: pattern: add pat_ref_gen_set() function
- MINOR: pattern: add pat_ref_gen_find_elt() function
- MINOR: pattern: add pat_ref_gen_delete() function
- MEDIUM: pattern: consider gen_id in pat_ref_set_from_node()
- MEDIUM: pattern: always consider gen_id for pat_ref lookup operations
- MINOR: version: this is development again (3.2)
- DEV: patchbot: prepare for new version 3.2-dev
- BUG/MEDIUM: sock: Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set
- MINOR: proxy: Add support of 421-Misdirected-Request in retry-on status
- BUG/MINOR: log: fix lf_text() behavior with empty string
- MINOR: log: always consider "+M" option in lf_text_len()
- BUG/MINOR: improve BBR throughput on very fast links
- MINOR: event_hdl: add PAT_REF events
- MINOR: pattern: publish event_hdl events on pat_ref updates
- MINOR: hlua: add patref class
- MINOR: hlua: add core.get_patref method
- MINOR: hlua_fcn: implement index and pair metamethods for patref class
- MINOR: hlua_fcn: wrap pat_ref struct for patref class
- MINOR: pattern: add pat_ref_may_commit() helper function
- MINOR: hlua_fcn: add Patref:commit() method
- MINOR: hlua_fcn: add Patref:prepare() method
- MINOR: hlua_fcn: add Patref:purge() method
- MINOR: hlua_fcn: add Patref:giveup()
- MINOR: hlua_fcn: add Patref:add()
- MINOR: hlua_fcn: add Patref:del()
- MINOR: hlua_fcn: add Patref:set()
- MINOR: hlua_fcn: add Patref:add_bulk()
- MINOR: hlua_fcn: add Patref:event_sub()
- DOC: lua: prefer Patref:{set,add}() over legacy methods for acl and maps
- BUG/MINOR: hlua_fcn: fix Patref:set() force parameter
- BUG/MEDIUM: event_hdl: fix uninitialized value in async mode when no data is provided
- BUG/MEDIUM: quic: prevent stream freeze on pacing
- BUG/MEDIUM: http-ana: Reset request flag about data sent to perform a L7 retry
- BUG/MINOR: h1-htx: Use default reason if not set when formatting the response
- BUILD: quic: fix a build error about an non initialized timestamp
- CI: github: allow coredumps on aws-lc and wolfssl jobs
- BUG/MINOR: listener: fix potential null pointer dereference in listener_release()
- MINOR: hlua: fix ambiguous hlua usage in hlua_filter_delete()
- BUG/MINOR: signal: register default handler for SIGINT in signal_init()
- BUG/MINOR: startup: close pidfd and free global.pidfile in handle_pidfile()
- BUG/MINOR: startup: fix pidfile creation
- MINOR: tools: add a new macro DEFVAL() to provide a default argument
- MINOR: tasklet: set TASK_WOKEN_OTHER on tasklets by default
- BUG/MINOR: quic: fix bbr_inflight() calls with wrong gain value
- BUG/MEDIUM: init: make sure only daemonized processes change their session
- BUG/MINOR: init: do not call fork_poller() for non-forked processes
- BUG/MEDIUM: mux-quic: remove pacing status when everything is sent
- BUG/MINOR: quic: remove startup alert if conn socket-owner unsupported
- BUG/MINOR: quic: remove startup alert if GSO unsupported
- MINOR: stktable: implement "recv-only" table option
- CLEANUP: stktable: replace nopurge attribute with flag
- CLEANUP: stktable: add some stktable flags polishing
- BUG/MEDIUM: mux-h2: make sure not to touch dummy streams when sending WU
- MINOR: mux-quic: clean up zero-copy done_ff callback
- BUG/MINOR: config: Fix parsing of accept-invalid-http-{request,response}
- BUG/MINOR: mworker: don't save program PIDs in oldpids
- BUG/MINOR: mworker: fix -D -W -sf/-st modes
- BUG/MINOR: startup: fix error path for master, if can't open pidfile
- CLEANUP: startup: make if condition to kill old pids more readable
- DOC: config: fix confusing init-state examples
- MINOR: mux-h1: use explicit __objt_server on idle conn reinsert
- MINOR: mux-h2: use explicit __objt_server on idle conn reinsert
- MINOR: mux-spop: use explicit __objt_server on idle conn reinsert
- MINOR: mux-fcgi: use explicit __objt_server on idle conn reinsert
- MINOR: quic: convert startup check in a freestanding function
- MINOR: quic: split startup check function
- MINOR: quic: implement build options report
- BUG/MINOR: debug: COUNT_IF() should return true/false
- MINOR: mux-h2/traces: add a missing trace on negative initial window size
- CLEANUP: mux-h2/traces: reword certain ambiguous traces
- MINOR: mux-h2/glitches: add a description to the H2 glitches
- BUG/MINOR: mux-h2: fix expression when detecting excess of CONTINUATION frames
- BUILD: debug: fix build issues in COUNT_IF() with -Wunused-value
- MINOR: tools: make fddebug() automatically emit the location
- MINOR: ssl: add notBefore and notAfter utility functions
- MEDIUM: ssl/cli: "show ssl sni" list the loaded SNI in frontends
- BUG/MEDIUM: startup: don't daemonize if started with -c
- BUG/MEDIUM: startup: report status if daemonized process fails
- BUG/MEDIUM: mworker: report status, if daemonized master fails
- BUG/MINOR: mworker: detach from tty when received READY from worker
- BUG/MINOR: namespace: handle a possible strdup() failure
- BUG/MINOR: ssl_crtlist: handle a possible strdup() failure
- BUG/MINOR: resolvers: handle a possible strdup() failure
- CI: use "/tmp" as default value for TMPDIR when searching logs
- DOC: management: fix typos and paragraph ordering in 'show ssl sni'
- CLEANUP: ssl: fix comment in 'show ssl sni'
- MINOR: ssl/cli: add negative filters to "show ssl sni"
- BUG/MINOR: stats: decrement srv refcount on stats-file release
- MINOR: list: define a watcher type
- BUG/MEDIUM: stats/server: use watcher to track server during stats dump
- MINOR: server: remove prev_deleted server list
- BUG/MINOR: http-fetch: Ignore empty argument string for query()
- BUG/MINOR: server-state: Fix expiration date of srvrq_check tasks
- BUG/MINOR: hlua_fcn: restore server pairs iterator pointer consistency
Since 9c91b30 ("MINOR: server: remove prev_deleted server list"), hlua
server pair iterator may use and return invalid (stale) server pointer
if multiple servers were deleted between two iterations.
Indeed, the server refcount mechanism (using srv_take()) is no longer
sufficient as the prev_deleted mitigation was removed.
To ensure server pointer consistency between two yields, the new watcher
mechanism must be used (as it already the case for stats dumping).
Thus in this patch we slightly change the server iteration logic:
hlua_server_list_iterator_context struct now stores the next valid server
pointer, and a watcher is added to ensure this pointer is never stale.
Then in hlua_listable_servers_pairs_iterator(), this next pointer is used
to create the Lua server object, and the next valid pointer is obtained by
leveraging watcher_next().
No backport needed unless 9c91b30 ("MINOR: server: remove prev_deleted
server list") is. Please note that dynamic servers were not supported in
Lua prior to 2.8, so it doesn't make sense to backport this patch further
than 2.8.
"hold.timeout" was used as expiration date for srvrq_check tasks. But it is
not accurrate. The expiration date must be based on the resolution timeouts
instead (resolve and retry).
The purpose of srvrq_check task is to clean up the server resolution status
when outdated info are inherited from the state file. Using "hold.timeout"
is not accurrate here because hold timeouts concern the resolution response
items not the resolution status of servers. It may be set to a huge value or
0. The expiration date of these tasks must be based on the resolution
timeouts instead.
So now the ("timeout resolve" + resolve_retries * "timeout retry") value is
used.
This patch should fix the issue #2816. It must be backported to all stable
versions.
query() sample fetch function takes an optional argument string. During
configuration parsing, empty string must be ignored. It is especially
important when the sample is used with empty parenthesis. The argument is
optional and it is a list of options to configure the behavior of the sample
fetch. So it is logical to ignore empty strings.
This patch should fix the issue #2815. It must be backported to 3.1.
This patch is a direct follow-up to the previous one. Thanks to watcher
type, it is not safe to assume that servers manipulated via stats dump
were not targetted by a "delete server" CLI command. As such,
prev_deleted list server member is now unneeded. This patch thus removes
any reference to it.
If a server A is deleted while a stats dump is currently on it, deletion
is delayed thanks to reference counting. Server A is nonetheless removed
from the proxy list. However, this list is a single linked list. If the
next server B is deleted and freed immediately, server A would still
point to it. This problem has been solved by the prev_deleted list in
servers.
This model seems correct, but it is difficult to ensure completely its
validity. In particular, it implies when stats dump is resumed, server A
elements will be accessed despite the server being in a half-deleted
state.
Thus, it has been decided to completely ditch the refcount mechanism for
stats dump. Instead, use the watcher element to register every stats
dump currently tracking a server instance. Each time a server is deleted
on the CLI, each stats dump element which may points to it are updated
to access the next server instance, or NULL if this is the last server.
This ensures that a server which was deleted via CLI but not completely
freed is never accessed on stats dump resumption.
Currently, no race condition related to dynamic servers and stats dump
is known. However, as described above, the previous model is deemed too
fragile, as such this patch is labelled as bug-fix. It should be
backported up to 2.6, after a reasonable period of observation. It
relies on the following patch :
MINOR: list: define a watcher type
Define a new watcher type into list module. This type is similar to bref
and can be used to register an element which is currently tracking a
dynamic target. Contrary to bref, if the target is freed, every watcher
element are updated to point to a next valid entry or NULL.
This type will simplify handling of dynamic servers deletion, in
particular while stats dump are performed.
This patch is not a bug-fix. However, it is mandatory to fix a race
condition in dynamic servers. Thus, it should be backported along the
next commit up to 2.6.
Servers instance may be removed at runtime. This can occurs during a
stat dump which currently references this server instance. This case is
protected by server refcount to prevent the server immediate release.
CLI output may be interrupted prior to stats dump completion, for
example if client CLI has been disconnected before the full response
transfer. As such, srv_drop() must be called in every stats dump release
callback.
srv_drop() was missing for stats-file dump release callback. This could
cause a race condition which would prevent a server instance to be fully
removed. Fix this by adding srv_drop() invokation into
cli_io_handler_release_dump_stat_file().
This should be backported up to 3.0.
The 'show ssl sni' output can be confusing when using crt-list, because
the wildcards can be completed with negative filters, and they need to
be associated to the same line.
Having a negative filter on its line alone does not make much sense,
this patch adds a new 'Negative Filter' column that show the exception
applied on a wildcard from a crt-list line.
Some master process' initialization steps are conditioned by receiving the
READY message from worker (pidfile creation, forwarding READY message to the
launching parent). So, master process can not do these initialization routines
before.
If the master process fails, while creating pid or forwarding the READY to the
parent in daemon mode, he exits with a proper alert message. In daemon mode we
no longer see such message, as process is already detached from the tty.
To fix this, as these alerts could be very useful, let's detach the master
process from the tty after his last initialization steps in _send_status.
As daemonization fork happens now very early and before the master-worker
fork, if master or worker processes fail during the initialization, some
critical errors can't be reported to stdout. The launching (parent) process in
such cases exits with 0. This makes an impression, that master and his worker
have successfully started at background, which really complicates the
operations.
In the previous commit a pipe was added to make daemonized child communicate
with his parent. Let's add the same logic to master-worker mode. Up to
receiving the READY message from the worker, master will "forward" it via the
pipe to the launching process. Launching process can obtain master's exit
status, if the master fails to start and nothing has been written in the pipe.
This fix should be backported only in 3.1.
Due to master-worker rework, daemonization fork happens now before parsing
and applying the configuration. This makes impossible to report correctly all
warnings and alerts to shell's stdout. Daemonzied process fails, while being
already in background, exit code reported by shell via '$?' equals to 0, as
it's the exit code of his parent.
To fix this, let's create a pipe between parent and daemonized child. The
child will send into this pipe a "READY" message, when it finishes his
initialization. The parent will wait on the "read" end of the pipe until
receiving something. If read() fails, parent obtains the status of the
exited child with waitpid(). So, the parent can correctly report the error to
the stdout and he can exit with child's exitcode.
This fix should be backported only in 3.1.
Due to master-worker refactoring, daemonization fork happens now very early,
before parsing and verifying the configuration. For the moment there is no
any specific syntax, which needs for the daemon mode to be really applied in
order to perform the tests.
So, it's better not to do the daemonization fork, if 'daemon' keyword is
presented in the config (or -D option), when we started with -c (MODE_CHECK).
Like this, during the config verification, the process will always stay in
foreground and all warning or errors will be delivered to the stdout.
This fix should be backported only in 3.1.
The "show ssl sni" command, allows one to dump the list of SNI in an
haproxy process, or a designated frontend.
It lists the SNI with the type, filename, and dates of expiration and
activation
fddebug() is sometimes quite helpful, but annoying to use when following
a call path because it's a pain to always repeat the function name and
call place. Let's have it automatically prepend the function name, the
file name and the line number, and make its arguments optional, replacing
them by a simple LF when all absent. This way, simply placing:
fddebug();
is sufficient to emit a location follocing "[%s@%s:%d]\n". This function
must not be used in production (and even call places with it shouldn't be
committed) and it should only be used by developers, so the simplest the
better.
Commit 7f64bb79fd ("BUG/MINOR: debug: COUNT_IF() should return true/false")
allowed the COUNT_IF() macro to return the evaluated value. This is handy
to place it in "if ()" conditions and count them at the same time. When
glitches are disabled, the condition is just returned as-is, but most call
places do not use the result, making some compilers complain. In addition,
while reviewing this, it was noticed that when DEBUG_STRICT=0, the macro
would still be replaced by a "do { } while (0)" statement, which not only
does not evaluate the expression, but also cannot return anything. Ditto
for COUNT_IF_HOT().
Let's make sure both are always properly evaluated now.
Latest commit f0eca8fe7 ("MINOR: mux-h2/glitches: add a description to
the H2 glitches") misplaced the optional glitch description field, with
it appearing at the end of the if () condition and always reporting
an excess of CONTINUATION frames from the first exceeding one.
This needs to be backported along with that commit once it gets backported.
Since we can now list them using "debug counters" and now support a
description, better add the description to all glitches. This patch may
be backported to 3.1, but before this the following patches must also
be picked:
86823c828 MINOR: mux-h2/traces: add a missing trace on negative initial window size
7c8e9420a CLEANUP: mux-h2/traces: reword certain ambiguous traces
When a negative initial windows size is reported, we're going to close
the connection, so it's important to report a trace to explain why!
This should be backported at least to 3.1 and possibly 3.0 (adapting the
context since there's no glitches there).
The COUNT_IF() macro was initially meant to return true/false to be used
in if() conditions but had an extra do { } while(0) that prevents it from
doing so. Let's get rid of the do { } while(0) before the code generalizes
to too many places. There's no impact on existing code, but may have to be
backported if future fixes rely on it.
Define a new function quic_register_build_options(). Its purpose is to
register a build options string for QUIC features which is reported when
using haproxy -vv.
This will allow to easily determine if connection socket-owner mode and
GSO are supported or not. Here is the new filtered output :
$ ./haproxy -vv|grep '^QUIC:'
QUIC: connection socket-owner mode support : yes
QUIC: GSO emission support : yes
Two features are tested on startup via quic_test_socketopts() :
connection socket-owner mode support and GSO. Extract both test in their
separated functions called by quic_test_socketopts().
This patch will allow to reuse easily QUIC features detection for build
options report via haproxy -vv.
