]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
8 months agoMINOR: mux-h2/traces: print the size of the DATA frames
Willy Tarreau [Thu, 3 Oct 2024 14:04:04 +0000 (16:04 +0200)] 
MINOR: mux-h2/traces: print the size of the DATA frames

DATA frames produce a special trace with the amount of transferred data
in arg4, but this was not reported by h2_trace(). This commit just adds
it.

8 months agoBUG/MINOR: mux-h2/traces: present the correct buffer for trailers errors traces
Willy Tarreau [Wed, 2 Oct 2024 13:39:52 +0000 (15:39 +0200)] 
BUG/MINOR: mux-h2/traces: present the correct buffer for trailers errors traces

The local "rxbuf" buffer was passed to the trace instead of h2s->rxbuf
that is used when decoding trailers. The impact is essentially the
impossibility to present some buffer contents in some rare cases. It
may be backported but it's unlikely that anyone will ever notice the
difference.

8 months agoBUILD: cache: silence an uninitialized warning at -Og with gcc-12.2
Willy Tarreau [Thu, 10 Oct 2024 04:59:12 +0000 (06:59 +0200)] 
BUILD: cache: silence an uninitialized warning at -Og with gcc-12.2

Building with gcc-12.2 -Og yields this incorrect warning in cache.c:

  In function 'release_entry_unlocked',
      inlined from 'http_action_store_cache' at src/cache.c:1449:4:
  src/cache.c:330:9: warning: 'object' may be used uninitialized [-Wmaybe-uninitialized]
    330 |         release_entry(cache, entry, 1);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  src/cache.c: In function 'http_action_store_cache':
  src/cache.c:1200:29: note: 'object' was declared here
   1200 |         struct cache_entry *object, *old;
        |                             ^~~~~~

This is wrong, the only way to reach the function is with first!=NULL
and the gotos that reach there are all those made with first==NULL.
Let's just preset object to NULL to silence it.

9 months agoMINOR: cfgparse: simulate long configuration parsing with force-cfg-parser-pause
William Lallemand [Fri, 11 Oct 2024 15:38:46 +0000 (17:38 +0200)] 
MINOR: cfgparse: simulate long configuration parsing with force-cfg-parser-pause

This command is pausing the configuration parser for <timeout>
milliseconds. This is useful for development or for testing timeouts of
init scripts, particularly to simulate a very long reload. It requires
the expose-experimental-directives to be set.

9 months agoBUG/MEDIUM: mux-quic: ensure timeout server is active for short requests
Amaury Denoyelle [Thu, 10 Oct 2024 15:07:36 +0000 (17:07 +0200)] 
BUG/MEDIUM: mux-quic: ensure timeout server is active for short requests

If a small request is received on QUIC MUX frontend, it can be
transmitted directly with the FIN on attach operation. rcv_buf is
skipped by the stream layer. Thus, it is necessary to ensure that there
is similar behavior when FIN is reported either on attach or rcv_buf.

One difference was that se_expect_data() was called only for rcv_buf but
not on attach. This most obvious effect is that stream timeout was
deactivated for this request : client timeout was disabled on EOI but
server one not armed due to previous se_expect_no_data(). This prevents
the early closure of too long requests.

To fix this, add an invokation of se_expect_data() on attach operation.

This bug can simply be detected using httpterm with delay request (for
example /?t=10000) and using smaller client/server timeouts. The bug is
present if the request is not aborted on timeout but instead continue
until its proper HTTP 200 termination.

This has been introduced by the following commit :
  85eabfbf672c57e4ed082da1b96c95348b331320
  MEDIUM: mux-quic: Don't expect data from server as long as request is unfinished

This must be backported up to 2.8.

9 months agoMINOR: sample: postresolve sink names in debug() converter
Aurelien DARRAGON [Thu, 10 Oct 2024 09:37:36 +0000 (11:37 +0200)] 
MINOR: sample: postresolve sink names in debug() converter

debug() converter used to resolve sink names during parsing time. Because
of this, we were unable to specify sink names that were defined after
the debug() converter was placed.

Like in the previous commit, let's implement proper postparsing for the
debug() converter, in order to be able to use sink names that are about
to be defined later in the config file.

9 months agoMINOR: trace: postresolve sink names
Aurelien DARRAGON [Tue, 8 Oct 2024 17:02:20 +0000 (19:02 +0200)] 
MINOR: trace: postresolve sink names

A previous known limitation about traces was that parsing was performed on
the fly, meaning that when using "sink" keyword, only sinks that were
either internal or previously defined in the config could be used. Indeed,
it was not possible to use a ring section defined AFTER the traces section
when using the 'sink' keyword from traces.

This limitation was also mentioned in the config file.

Let's get rid of that limitation by implementing proper postparsing for
the sink parameter in traces section. To do this, make use of the new
sink_find_early() helper to start referencing sink by their names even
if they don't exist yet (if they are about to be defined later in the
config)

Traces commands on the cli are not concerned by this change.

9 months agoMEDIUM: sink: implement sink_find_early()
Aurelien DARRAGON [Thu, 10 Oct 2024 09:31:59 +0000 (11:31 +0200)] 
MEDIUM: sink: implement sink_find_early()

sink_find_early() is a convenient function that can be used instead of
sink_find() during parsing time in order to try to find a matching
sink even if the sink is not defined yet.

Indeed, if the sink is not defined, sink_find_early() will try to create
it and mark it as forward-declared. It will also save informations from
the caller to better identify it in case of errors.

If the sink happens to be found in the config, it will transition from
forward-declared type to its final type. Else, it means that the sink
was not found in the config, in this case, during postresolve, we raise
an error to indicate that the sink was not found in the configuration.

It should help solve postresolving issue with rings, because for now only
log targets implement proper ring postresolving.. but rings may be used
at different places in the code, such as debug() converter or in "traces"
section.

9 months agoMINOR: ssl: disable server side default CRL check with WolfSSL
Damien Claisse [Tue, 8 Oct 2024 13:32:49 +0000 (13:32 +0000)] 
MINOR: ssl: disable server side default CRL check with WolfSSL

Patch 64a77e3ea5 disabled CRL check when no CRL file was provided, but
it only did it on bind side. Add the same fix in server context
initialization side.
This allows to enable peer verification (verify required) on a server
using TLS, without having to provide a CRL file.

9 months agoBUG/MEDIUM: quic: properly decount out-of-order ACK on stream release
Amaury Denoyelle [Wed, 9 Oct 2024 09:59:32 +0000 (11:59 +0200)] 
BUG/MEDIUM: quic: properly decount out-of-order ACK on stream release

Out-of-order STREAM ACK are buffered in its related streambuf tree. On
insertion, overlapping or contiguous ranges are merged together. The
total size of buffered ack range is stored in <room> streambuf member
and reported to QUIC MUX layer on streambuf release. The objective is to
ensure QUIC MUX layer can allocate Tx buffers conveniently to preserve a
good transfer throughput.

Streamdesc is the overall container of many streambufs. It may also been
released when its upper QCS instance is freed, after all stream data
have been emitted. In this case, the active streambuf is also released
via custom code. However, in this code path, <room> was not reported to
the QUIC MUX layer.

This bug caused wrong estimation for the QUIC MUX txbuf window, with
bytes reamining even after all ACK reception. This may cause transfer
freeze on other connection streams, with RESET_STREAM emission on
timeout client.

To fix this, reuse the existing qc_stream_buf_release() function on
streamdesc release. This ensures that notify_room is correctly used.

No need to backport.

9 months agoBUG/MINOR: quic: fix discarding of already stored out-of-order ACK
Amaury Denoyelle [Wed, 9 Oct 2024 10:03:36 +0000 (12:03 +0200)] 
BUG/MINOR: quic: fix discarding of already stored out-of-order ACK

To properly decount out-of-order acked data range, contiguous or
overlapping ranges are first merged before their insertion in a tree.

The first step ensure that a newly reported range is not completely
covered by the existing tree ranges. However, one of the condition was
incorrect. Fix this to ensure that the final range tree does not contain
duplicated entry.

The impact of this bug is unknown. However, it may have allowed the
insertion of overlapping ranges, which could in turn cause an error in
QUIC MUX txbuf window, with a possible transfer freeze.

No need to backport.

9 months agoBUG/MEDIUM: hlua: properly handle sample func errors in hlua_run_sample_{fetch,conv}()
Aurelien DARRAGON [Tue, 8 Oct 2024 09:42:14 +0000 (11:42 +0200)] 
BUG/MEDIUM: hlua: properly handle sample func errors in hlua_run_sample_{fetch,conv}()

To execute sample fetches and converters from lua. hlua API leverages the
sample API. Prior to executing the sample func, the arg checker is called
from hlua_run_sample_{fetch,conv}() to detect potential errors.

However, hlua_run_sample_{fetch,conv}() both pass NULL as <err> argument,
but it is wrong for two reasons. First we miss an opportunity to report
precise error messages to help the user know what went wrong during the
check.. and more importantly, some val check functions consider that the
<err> pointer is never NULL. This is the case for example with
check_crypto_hmac(). Because of this, when such val check functions
encounter an error, they will crash the process because they will try
to de-reference NULL.

This bug was discovered and reported by GH user @JB0925 on #2745.

Perhaps val check functions should make sure that the provided <err>
pointer is != NULL prior to de-referencing it. But since there are
multiple occurences found in the code and the API isn't clear about that,
it is easier to fix the hlua part (caller) for now.

To fix the issue, let's always provide a valid <err> pointer when
leveraging val_arg() check function pointer, and make use of it in case
or error to report relevant message to the user before freeing it.

It should be backported to all stable versions.

9 months agoBUG/MEDIUM: hlua: make hlua_ctx_renew() safe
Aurelien DARRAGON [Tue, 8 Oct 2024 09:34:10 +0000 (11:34 +0200)] 
BUG/MEDIUM: hlua: make hlua_ctx_renew() safe

hlua_ctx_renew() is called from unsafe places where the caller doesn't
expect it to LJMP.. however hlua_ctx_renew() makes use of Lua library
function that could potentially raise errors, such as lua_newthread(),
and it does nothing to catch errors. Because of this, haproxy could
unexpectedly crash. This was discovered and reported by GH user
@JB0925 on #2745.

To fix the issue, let's simply make hlua_ctx_renew() safe by applying
the same logic implemented for hlua_ctx_init() or hlua_ctx_destroy(),
which is catching Lua errors by leveraging SET_SAFE_LJMP_PARENT() helper.

It should be backported to all stable versions.

9 months agoREGTESTS: add some tests for 'do-log' action
Aurelien DARRAGON [Thu, 3 Oct 2024 07:26:21 +0000 (09:26 +0200)] 
REGTESTS: add some tests for 'do-log' action

Now that 'do-log' action may be used for all existing action contexts,
let's add some tests in reg-tests/log/log_profile.vtc to ensure it works
as expected. quic-ini is not tested as it may not be builtin depending on
build options..

9 months agoMINOR: action: add do-log action
Aurelien DARRAGON [Fri, 20 Sep 2024 07:34:13 +0000 (09:34 +0200)] 
MINOR: action: add do-log action

Thanks to the two previous commits, we can now expose the do-log action
on all available action contexts, including the new quic-init context.

Each context is responsible for exposing the do-log action by registering
the relevant log steps, saving the idendifier, and then store it in the
rule's context so that do_log_action() automatically uses it to produce
the log during runtime.

To use the feature, it is simply needed to use "do-log" (without argument)
on an action directive, example:

   tcp-request connection do-log

As mentioned before, each context where the action is exposed has its own
log step identifier. Currently known identifiers are:

  quic-initial:           quic-init
  tcp-request connection: tcp-req-conn
  tcp-request session:    tcp-req-sess
  tcp-request content:    tcp-req-cont
  tcp-response content:   tcp-res-cont
  http-request:           http-req
  http-response:          http-res
  http-after-response:    http-after-res

Thus, these "additional" logging steps can be used as-is under log-profile
section (after "on" keyword). However, although the parser will accept
them, it makes no sense to use them with the "log-steps" proxy keyword,
since the only path for these origins to trigger a log generation is
through the explicit use of "do-log" action.

This need was described in GH #401, it should help to conditionally
trigger logs using ACL at specific key points.. and may either be used
alone or combined with "log-steps" to add additional log "trackers" during
transaction handling.

Documentation was updated and some examples were added.

9 months agoMINOR: log: add do_log_parse_act() helper func
Aurelien DARRAGON [Tue, 1 Oct 2024 15:55:55 +0000 (17:55 +0200)] 
MINOR: log: add do_log_parse_act() helper func

Function may be used from places where per-context actions are usually
registered (tcp_act.c, http_act.c, quic_rules.c.. to name a few) in
order to expose the do_log() action.

9 months agoMINOR: log: add do_log() logging helper
Aurelien DARRAGON [Thu, 26 Sep 2024 08:09:09 +0000 (10:09 +0200)] 
MINOR: log: add do_log() logging helper

do_log() is quite similar to sess_log() or strm_log(), excepts that it
may be called at any time during session handling in an opportunistic
way as long as the session exists (the stream may or may not exist).

Also, it will try to emit the log as INFO by default, unless set-log-level
is used on the stream, or error origin flag is set.

9 months agoMEDIUM: quic: decount out-of-order ACK data range for MUX txbuf window
Amaury Denoyelle [Wed, 2 Oct 2024 12:44:41 +0000 (14:44 +0200)] 
MEDIUM: quic: decount out-of-order ACK data range for MUX txbuf window

This commit is the last one of a serie whose objective is to restore
QUIC transfer throughput performance to the state prior to the recent
QUIC MUX buffer allocator rework.

This gain is obtained by reporting received out-of-order ACK data range
to the QUIC MUX which can then decount room in its txbuf window. This is
implemented in QUIC streamdesc layer by adding a new invokation of
notify_room callback. This is done into qc_stream_buf_store_ack() which
handle out-of-order ACK data range.

Previous commit has introduced merging of overlapping ACK data range. As
such, it's easy to only report the newly acknowledged data range.

As with in-order ACKs, this new notification is only performed on
released streambuf. As such, when a streambuf instance is released,
notify_room notification now also reports the total length of
out-of-order ACK data range currently stored. This value is stored in a
new streambuf member <room> to avoid unnecessary tree lookup.

This <room> member also serves on in-order ACK notification to reduce
the notified room. This prevents to report invalid values when overlap
ranges are treated first out-of-order and then in-order, which would
cause an invalid QUIC MUX txbuf window value.

After this change has been implemented, performance has been
significantly improved, both with ngtcp2-client rate usage and on
interop goodput test. These values are now similar to the rate observed
on older haproxy version before QUIC MUX buffer allocator rework.

9 months agoMEDIUM: quic: merge contiguous/overlapping buffered ack stream range
Amaury Denoyelle [Wed, 2 Oct 2024 08:23:21 +0000 (10:23 +0200)] 
MEDIUM: quic: merge contiguous/overlapping buffered ack stream range

Transfer throughput was deteriorated since recent rework of QUIC MUX
txbuf allocator. This was partially restorated with the commit to
decount individual in-order ACK from the MUX buffer window.

To fully retrieve the old performance level, all ACKs must be decounted
when handled by QUIC streamdesc layer, event out-of-order ranges.
However, this is not easily implemented as several ranges may exist in
parallel with overlap on the underlying data. It would cause
miscalculation for QUIC MUX buffer window if such ranges were blindly
reported.

The proper solution is to first implement merge of contiguous or
overlapping ACK data ranges to reduce the number of stored ranges to the
minimal. This is the purpose of this patch. This is implemented in a new
static function named qc_stream_buf_store_ack() into streamdesc layer.

The merge algorithm is simple enough. First, it ensures the newly added
range is not already fully covered by a preexisting entry. Then, it
checks if there is contiguity/overlap with one or several ranges
starting at the same of a greater offset. If true, the newly added entry
is extended to cover them all, and all contiguous/overlapped ranges are
removed. Finally, if there is contiguity or overlap with an entry
starting at a smaller offset, no new range is instantiated and instead
the smaller offset is extended.

Now that contiguous or overlapped ranges cannot exits anymore, ACK data
ranges tree instiatiation can used EB_ROOT_UNIQUE.

Outside of the longer term objective which is to decount out-of-order
ACKs from MUX txbuf window, this commit could also improve some
performance and/or memory usage for connections where stream data
fragmentation and packet reording is high.

9 months agoMINOR: quic: implement dedicated type for out-of-order stream ACK
Amaury Denoyelle [Tue, 1 Oct 2024 15:34:55 +0000 (17:34 +0200)] 
MINOR: quic: implement dedicated type for out-of-order stream ACK

QUIC streamdesc layer is responsible to handle reception of ACK for
streams. It removes stream data from the underlying buffers on ACK
reception.

Streamdesc layer treats ACK in order at the stream level. Out of order
ACKs are buffered in a tree until they can be handled on older data
acknowledgement reception. Previously, qf_stream instance which comes
from the quic_tx_packet was used as tree node to buffer such ranges.

Introduce a new type dedicated to represent out of order stream ack data
range. This type is named qc_stream_ack. It contains minimal infos only
relative to the acknowledged stream data range.

This allows to reduce size of frequently used quic_frame with the
removal of tree node from qf_stream. Another side effect of this change
is that now quic_frame are always released immediately on ACK reception,
both in-order and out-of-order. This allows to also release the
quic_tx_packet instance which should reduce memory consumption.

The drawback of this change is that qc_stream_ack instance must be
allocated on out-of-order ACK reception. As such, qc_stream_desc_ack()
may fail if an error happens on allocation. For the moment, such error
is silenly recovered up to qc_treat_rx_pkts() with the dropping of the
received packet containing the ACK frame. In the future, it may be
useful to close the connection as this error may only happens on low
memory usage.

9 months agoMEDIUM: quic: decount acknowledged data for MUX txbuf window
Amaury Denoyelle [Wed, 18 Sep 2024 08:32:39 +0000 (10:32 +0200)] 
MEDIUM: quic: decount acknowledged data for MUX txbuf window

Recently, a new allocation mechanism was implemented for Tx buffers used
by QUIC MUX. Now, underlying congestion window size is used to determine
if it is still possible or not to allocate a new buffer when necessary.

This mechanism has render the QUIC stack more flexible. However, it also
has brought some performance degradation, with transfer time longer in
certain environment. It was first discovered on the measurement results
of the interop. It can also easily be reproduced using the following
ngtcp2-client example which forces a very small congestion window due to
frequent loss :

 $ ngtcp2-client -q --no-quic-dump --no-http-dump --exit-on-all-streams-close -r 0.1 127.0.0.1 20443 "https://[::]:20443/?s=10m"

This performance decrease is caused by the allocator which is now too
strict. It may cause buffer underrun frequently at the MUX layer when
the congestion window is too small, as new buffers cannot be allocated
until the current one is fully acknowledged. This resuls in transfers
with very bad throughput utilisation. The objective of this new serie of
patches is to relax some restrictions to permit QUIC MUX to allocate new
buffers more quickly, while preserving the initial limitation based on
congestion window size.

An interesting method for this is to notify QUIC MUX about newly
available room on individual ACK reception, without waiting for the full
bffer acknowledgement. This is easily implemented by adding a new
notify_room invokation in QUIC streamdesc layer on ACK reception.
However, ACK reception are handled in-order at the stream level. Out of
order ACKs are buffered and are not decounted for now. This will be
implemented in a future commit.

Note that for a single buffer instance, data can in parallel be written
by QUIC MUX and removed on ACK reception. This could cause room
notification to QUIC MUX layer to report invalid values. As such, ACK
reception are only accounted for released buffers. This ensures that
such buffers won't received any new data. In the same time, buffer room
is notified on release operation as it does not need acknowledgement.

This commit has permit to improve performance for the ngtcp2-client
scenario above. However, it is not yet sufficient enough for interop
goodput test.

9 months agoMINOR: quic: strengthen qc_release_frm()
Amaury Denoyelle [Thu, 3 Oct 2024 16:11:08 +0000 (18:11 +0200)] 
MINOR: quic: strengthen qc_release_frm()

quic_frame is the type used to represent frames emitted in a QUIC Tx
packet. Each frame is attached to a packet, and can also be linked to
other frames from the the same packet, or duplicated frames for
retransmission. As such, quic_frame free operation is a tedious process.

qc_release_frm() has been implemented to ensure quic_frame is always
properly freed after detaching from all its list attach point. One
particular point is to ensure that when a frame is released, the frame
origin and all origin copies, including the current <frm> are flagged as
acked and detached from the reflist. Add a BUG_ON() to ensure this loop
is properly conducted when dealing with the current <frm> instance.

9 months agoBUG/MINOR: stats: Fix the name for the total number of streams created
Christopher Faulet [Fri, 4 Oct 2024 13:44:39 +0000 (15:44 +0200)] 
BUG/MINOR: stats: Fix the name for the total number of streams created

Because of a copy/paste error, CurrStreams was reused by mistake. It should
be "CumStreams"

No backports needed.

9 months agoBUG/MAJOR: mux-quic: do not crash on empty STREAM frame emission
Amaury Denoyelle [Fri, 4 Oct 2024 08:10:37 +0000 (10:10 +0200)] 
BUG/MAJOR: mux-quic: do not crash on empty STREAM frame emission

Most of the time STREAM frames emitted by QUIC MUX have some data in it.
However, it is possible to use an empty frame when a delayed FIN must be
transferred.

Recently, QUIC MUX send callback notification has been refactored. Now,
this callback is blindly called by quic_conn lower layer each time a
STREAM frame is built into a newly Tx packet. QUIC MUX is responsible to
ensure the notified frame corresponds to newly emitted data or
retransmission. Offsets are used for this comparison, but this requires
special care for empty FIN frames.

Sadly, the comparison written to determine if an empty FIN frame was
sent for the first time or retransmitted is not correct. This caused
such frame to always be dismissed as retransmission in QUIC MUX sent
callback. This prevented the related QCS instance to be removed from the
send_list, causing qcc_io_send() to retry a new emission. This was
finally interrupted by the BUG_ON() assertion to prevent an infinite
loop.

Fix this crash by updating the condition in QUIC MUX send callback. For
empty STREAM frame, it is sufficient to check if QC_SF_FIN_STREAM was
already removed or not to detect a retransmission. Indeed, empty STREAM
frames are never used outside of delayed FIN reporting.

No need to backport. This crash was introduced in the current dev branch
by the following commit.
  d7f4e5abf0b7129329d0ea716c104474fd934bc6
  MEDIUM: quic: strengthen MUX send notification

9 months ago[RELEASE] Released version 3.1-dev9 v3.1-dev9
Willy Tarreau [Thu, 3 Oct 2024 15:47:33 +0000 (17:47 +0200)] 
[RELEASE] Released version 3.1-dev9

Released version 3.1-dev9 with the following main changes :
    - MINOR: tools: add minimal file name management
    - CLEANUP: stick-table: make the file location point to a global file name
    - MINOR: proxy: use the global file names for conf->file
    - CLEANUP: cfgparse: factor proxy vs log-forward collisions
    - BUG/MINOR: cfgparse: detect another uncaught case of duplicate defaults
    - MINOR: proxy: add a list of orphaned defaults sections
    - MEDIUM: cfgparse: drop duplicate named defaults sections after use
    - OPTIM: cfgparse: speed up duplicate server detection
    - MEDIUM: cfgparse: warn about deprecated use of duplicate server names
    - BUG/MINOR: server: shut down streams under thread isolation
    - BUG/MINOR: proxy: also make the cli and resolvers use the global name
    - REGTESTS: log: fix log-profile.vtc
    - MEDIUM: mailers: warn about deprecated legacy mailers
    - BUG/MEDIUM: cli: Be sure to catch immediate client abort
    - DEV: flags/applet: decode appctx flags
    - BUG/MEDIUM: cli: Deadlock when setting frontend maxconn
    - MINOR: log: fix indent in strm_log()
    - MINOR: log: introduce extra log profile steps
    - MINOR: log: handle extra log origins in _process_send_log_override()
    - MINOR: log: introduce log_orig flags
    - MINOR: log: explicitly handle extra log origins as error when relevant
    - MINOR: log: support extra log origins for '%OG' alias
    - MINOR: proxy: add log_steps struct member
    - MINOR: log: introduce "log-steps" proxy keyword
    - MINOR: log: add log_orig_proxy() helper function
    - MEDIUM: log: consider log-steps proxy setting for existing log origins
    - DOC: config: document proxy "log-steps" keyword
    - REGTESTS: add a test for proxy "log-steps"
    - Revert "BUG/MINOR: server: shut down streams under thread isolation"
    - MINOR: task: define two new one-shot events for use with WOKEN_OTHER or MSG
    - BUG/MEDIUM: stream: make stream_shutdown() async-safe
    - BUG/MINOR: server: make sure the HMAINT state is part of MAINT
    - BUG/MINOR: queue: make sure that maintenance redispatches server queue
    - MINOR: server: make srv_shutdown_sessions() call pendconn_redistribute()
    - BUILD: tools: only include execinfo.h for the real backtrace() function
    - MINOR: tools: do not attempt to use backtrace() on linux without glibc
    - OPTIM: channel: speed up co_getline()'s search of the end of line
    - OPTIM: stconn: Don't pretend mux have more data to deliver on EOI/EOS/ERROR
    - BUG/MINOR: mcli: Pretend the mux have more data to deliver between two commands
    - MINOR: action: Export release_expr_int_action() release function
    - MINOR: stream: Rely on a per-stream max connection retries value
    - MINOR: stream: Support dynamic changes of the number of connection retries
    - MINOR: stream/stats: Expose the current number of streams in stats
    - MINOR: stream/stats: Expose the total number of streams ever created in stats
    - BUG/MINOR: cfgparse-global: fix allowed args number for setenv
    - MINOR: cfgparse-global: add dedicated parser for *env keywords
    - MINOR: mux-quic: complete Tx infos for QCS dump
    - MINOR: quic: ensure txbuf realloc is only performed on empty buffer
    - MINOR: mux-quic: strengthen qcs_send_metadata() usage
    - MINOR: quic: remove unneeded notification of txbuf room
    - MINOR: quic: refactor MUX send notification
    - MEDIUM: quic: strengthen MUX send notification
    - MINOR: quic: refactor STREAM room notification
    - MINOR: quic: do not remove qc_stream_desc automatically on ACK handling
    - MINOR: quic: store streambuf in a streamdesc tree
    - MINOR: quic: move buffered ACK to streambuf
    - MEDIUM: quic: handle out-of-order ACK at streamdesc layer
    - MEDIUM: quic: refactor buffered STREAM ACK consuming
    - BUG/MEDIUM: queue: always dequeue the backend when redistributing the last server
    - MINOR: config/trace: Add a 'traces' section to declare debug traces
    - MINOR: trace: Be able to chain commands for a source in one line
    - MINOR: tcpcheck: Add support for an option host header value for httpchk option
    - BUG/MINOR: mux-h1: Fix condition to set EOI on SE during zero-copy forwarding
    - MINOR: mux-h1: Use a dedicated function to conditionnaly set EOI flag on SE
    - BUG/MINOR: http-ana: Disable fast-fwd for unfinished req waiting for upgrade
    - BUG/MINOR: mux-quic: fix crash on qcc_init() early return
    - BUG/MINOR: quic: fix trace on releasing STREAM frame after ack

9 months agoBUG/MINOR: quic: fix trace on releasing STREAM frame after ack
Amaury Denoyelle [Wed, 2 Oct 2024 15:04:19 +0000 (17:04 +0200)] 
BUG/MINOR: quic: fix trace on releasing STREAM frame after ack

Fix NULL argument pass to qc_release_frm(). This allows to give more
context on the traces inside it. Note that no crash occured as QUIC
traces always check validity on first arg before derefencing it.

No backport needed.

9 months agoBUG/MINOR: mux-quic: fix crash on qcc_init() early return
Amaury Denoyelle [Wed, 2 Oct 2024 08:21:02 +0000 (10:21 +0200)] 
BUG/MINOR: mux-quic: fix crash on qcc_init() early return

qcc_release() may be used in case qcc_init() cannot complete. In this
case, connection instance is NULL. As such, it cannot be dereferenced
without testing it first.

This should fix github coverity report #2739.

No backport needed.

9 months agoBUG/MINOR: http-ana: Disable fast-fwd for unfinished req waiting for upgrade
Christopher Faulet [Wed, 2 Oct 2024 07:57:34 +0000 (09:57 +0200)] 
BUG/MINOR: http-ana: Disable fast-fwd for unfinished req waiting for upgrade

If a request is waiting for a protocol upgrade but it is not finished, the
data fast-forwarding is disabled. Otherwise, the request analyzers will miss
the end of the message.

This case is possible since the commit 01fb1a54 ("BUG/MEDIUM: mux-h1/mux-h2:
Reject upgrades with payload on H2 side only"). Indeed, before, a protocol
upgrade was not allowed for request with payload. But it is now possible and
this comes with a side-effect. It is not really satisfying but for now there
is no other way to sync the muxes and the applicative stream. It seems to be
a reasonnable fix for now, waiting for a deeper refactoring.

This patch must be backported with the commit above.

9 months agoMINOR: mux-h1: Use a dedicated function to conditionnaly set EOI flag on SE
Christopher Faulet [Wed, 2 Oct 2024 07:36:15 +0000 (09:36 +0200)] 
MINOR: mux-h1: Use a dedicated function to conditionnaly set EOI flag on SE

The same conditions are evaluated in h1_process_demux() and h1_fastfwd() to
know if SE_FL_EOI flag must be set or not on the sedesc. So now, a dedicated
function is used.

9 months agoBUG/MINOR: mux-h1: Fix condition to set EOI on SE during zero-copy forwarding
Christopher Faulet [Wed, 2 Oct 2024 07:25:28 +0000 (09:25 +0200)] 
BUG/MINOR: mux-h1: Fix condition to set EOI on SE during zero-copy forwarding

During zero-copy data forwarding, the producer must set the EOI flag on the SE
when end of the message is reached. It is already done but there is a case where
this flag is set while it should not. When a request wants to perform a protocol
upgrade and it is waiting for the server response, the flag must not be set
because the HTTP message is finished but some data are possibly still expected,
depending on the server response. On a 101-switching-protocol, more data will be
sent because the producer is switch to TUNNEL state.

So, now, the right condition is used. In DONE state, SE_FL_EOI flag is set on the sedesc iff:

  - it is the response
  - it is the request and the response is also in DONNE state
  - it is a request but no a protocol upgrade nor a CONNECT

This patch must be backported as far as 2.9.

9 months agoMINOR: tcpcheck: Add support for an option host header value for httpchk option
Christopher Faulet [Tue, 1 Oct 2024 16:40:40 +0000 (18:40 +0200)] 
MINOR: tcpcheck: Add support for an option host header value for httpchk option

Support for headers and body hidden in the version for the "option httpchk"
directive was removed. However a Host header is mandatory for HTTP/1.1
requests and some servers may return an error if it is not set. For now, to
add it, an "http-check send" rule must be added. But it is not really handy
to use an extra config line for this purpose.

So now, it is possible to set the host header value, a log-format string, as
extra argument to "option httpchk" directive. It must be the fourth argument:

  option httpchk GET / HTTP/1.1 www.srv.com

While this patch is not a bug fix, it is simple enough to be backported if
necessary. On 2.9 and older, lf_init_expr() does not exist and LIST_INIT() must
be used instead.

9 months agoMINOR: trace: Be able to chain commands for a source in one line
Christopher Faulet [Tue, 1 Oct 2024 09:05:48 +0000 (11:05 +0200)] 
MINOR: trace: Be able to chain commands for a source in one line

In the configuration file or on the CLI, configuring traces for a specific
source is a bit painful because this must be done in several lines. Thanks
to this patch, it is now possible to fully configure traces for a source in
one line. For instance, the following on the CLI:

  trace h1 sink stderr; trace h1 level developer; trace h1 verbosity complete; trace h1 start now

can now be replaced by:

  trace h1 sink stderr level developer verbosity complete start now

The same is true for the 'trace' directives in the configuration file.

9 months agoMINOR: config/trace: Add a 'traces' section to declare debug traces
Christopher Faulet [Tue, 1 Oct 2024 06:48:38 +0000 (08:48 +0200)] 
MINOR: config/trace: Add a 'traces' section to declare debug traces

It is no longer supported to declare debug traces, via 'trace' directive, in
a global section. A 'traces' directive must be used instead. The syntax of
the 'trace' directive in these sections remains the same. But it is no
longer experimental.

The main reason for this change is to avoid to have a ring section defined
before a global one. Indeed, for now, forward declarations of ring sections
are not supported. So to configure traces, you had to add a ring section
before the global one defining the traces. Most of time, that meant to have
two global sections :

  global
    [...] # global settings

  ring <name>
    [...]

  global
    [...] # trace config

In addition, it will be possible to easily extend the traces section by
adding some new directives.

9 months agoBUG/MEDIUM: queue: always dequeue the backend when redistributing the last server
Willy Tarreau [Tue, 1 Oct 2024 16:57:51 +0000 (18:57 +0200)] 
BUG/MEDIUM: queue: always dequeue the backend when redistributing the last server

An interesting bug was revealed by commit 5541d4995d ("BUG/MEDIUM: queue:
deal with a rare TOCTOU in assign_server_and_queue()"). When shutting
down a server to redistribute its connections, no check is made on the
backend's queue. If we're turning off the last server and the backend
has pending connections, these ones will wait there till the queue
timeout. But worse, since the commit above, we can enter an endless loop
in the following situation:

  - streams are present in the backend's queue
  - streams are purged on the last server via srv_shutdown_streams()
  - that one calls pendconn_redistribute(srv) which does not purge
    the backend's pendconns
  - a stream performs some load balancing and enters assign_server_and_queue()
  - assign_server() is called in turn
  - the LB algo is non-deterministic and there are entries in the
    backend's queue. The function notices it and returns SRV_STATUS_FULL
  - assign_server_and_queue() calls pendconn_add() to add the connection
    to the backend's queue
  - on return, pendconn_must_try_again() is called, it figures there's
    no stream served anymore on the server nor the proxy, so it removes
    the pendconn from the queue and returns 1
  - assign_server_and_queue() loops back to the beginning to try again,
    while the conditions have not changed, resulting in an endless loop.

Ideally a change count should be used in the queues so that it's possible
to detect that some dequeuing happened and/or that a last stream has left.
But that wouldn't completely solve the problem that is that we must never
ever add to a queue when there's no server streams to dequeue the new
entries.

The current solution consists in making pendconn_redistribute() take care
of the proxy after the server in case there's no more server available on
the proxy. It at least ensures that no pending streams are left in the
backend's queue when shutting streams down or when the last server goes
down. The try_again loop remains necessary to deal with inevitable races
during pendconn additions. It could be limited to a few rounds, though,
but it should never trigger if the conditions are sufficient to permit
it to converge.

One way to reproduce the issue is to run a config with a single server
with maxconn 1 and plenty of threads, then run in loops series of:

 "disable server px/s;shutdown sessions server px/s;
  wait 100ms server-removable px/s; show servers conn px;
  enable server px/s"

on the CLI at ~10/s while injecting with around 40 concurrent conns at
40-100k RPS. In this case in 10s - 1mn the crash can appear with a
backtrace like this one for at least 1 thread:

  #0  pendconn_add (strm=strm@entry=0x17f2ce0) at src/queue.c:487
  #1  0x000000000064797d in assign_server_and_queue (s=s@entry=0x17f2ce0) at src/backend.c:1064
  #2  0x000000000064a928 in srv_redispatch_connect (s=s@entry=0x17f2ce0) at src/backend.c:1962
  #3  0x000000000064ac54 in back_handle_st_req (s=s@entry=0x17f2ce0) at src/backend.c:2287
  #4  0x00000000005ae1d5 in process_stream (t=t@entry=0x17f4ab0, context=0x17f2ce0, state=<optimized out>) at src/stream.c:2336

It's worth noting that other threads may often appear waiting after the
poller and one in server_atomic_sync() waiting for isolation, because
the event that is processed when shutting the server down is consumed
under isolation, and having less threads available to dequeue remaining
requests increases the probability to trigger the problem, though it is
not at all necessary (some less common traces never show them).

This should carefully be backported wherever the commit above was
backported.

9 months agoMEDIUM: quic: refactor buffered STREAM ACK consuming
Amaury Denoyelle [Mon, 30 Sep 2024 07:48:29 +0000 (09:48 +0200)] 
MEDIUM: quic: refactor buffered STREAM ACK consuming

For the moment, streamdesc layer can only deal with in-order ACK at the
stream level. Received out-of-order ACKs are buffered in a tree attached
to a streambuf instance.

Previously, caller of qc_stream_desc_ack() was responsible to implement
consumption of these buffered ACKs. Refactor this by implementing it
directly at the streamdesc layer within qc_stream_desc_ack(). This
simplifies quic_rx ACK handling and ensure buffered ACKs are consumed as
soon as possible.

9 months agoMEDIUM: quic: handle out-of-order ACK at streamdesc layer
Amaury Denoyelle [Mon, 30 Sep 2024 07:21:10 +0000 (09:21 +0200)] 
MEDIUM: quic: handle out-of-order ACK at streamdesc layer

qc_stream_desc_ack() is the entrypoint for streamdesc layer to handle a
new acknowledgement of previously emitted STREAM data.

Previously, it was only able to deal with in-order ACK offset. The
caller was responsible to buffer out-of-order ACKs. Change this by
dealing with the latter case directly in qc_stream_desc_ack(). This
notably simplify ACK handling in quic_rx module.

9 months agoMINOR: quic: move buffered ACK to streambuf
Amaury Denoyelle [Tue, 1 Oct 2024 09:13:41 +0000 (11:13 +0200)] 
MINOR: quic: move buffered ACK to streambuf

QUIC streamdesc layer is used to manage QUIC MUX stream txbuf data
storage until acknowledgment. Currently, it only supports in-order
acknowledgment at the stream level. This requires to be able to buffer
out-of-order ACKs until they can be handled.

Previously, these ACKs were stored in a tree to the streamdesc instance.
Move this indexed storage at the streambuf instance.

This commit is purely an architecture change. However, it will allow to
extend ACK management in future patches, such as the ability to merge
overlapping out-of-order ACKs.

9 months agoMINOR: quic: store streambuf in a streamdesc tree
Amaury Denoyelle [Tue, 1 Oct 2024 09:27:37 +0000 (11:27 +0200)] 
MINOR: quic: store streambuf in a streamdesc tree

qc_stream_desc layer is used by QUIC MUX to store emitted STREAM data
until their acknowledgement. Each stream with Tx capability can allocate
its own qc_stream_desc. In turn, each stream desc can have one or
multiple data buffers. This is useful when a MUX stream releases a
buffer and allocate a new one, to preserve bandwith without waiting to
receive all acknowledgement of the previous buffer.

Each buffer is encapsulated in a qc_stream_buf structure. Previously, it
was stored as a list into qc_stream_desc. Change this storage to use a
tree instead. Each buffer is indexed by their offset.

This commit does not introduce functional changes. However, this
rearchitecture will be necessary for future commit to extend ACK
management which require fetching individual buffer instance, not just
the first or last element of a streamdesc, by their offset.

9 months agoMINOR: quic: do not remove qc_stream_desc automatically on ACK handling
Amaury Denoyelle [Thu, 26 Sep 2024 14:14:40 +0000 (16:14 +0200)] 
MINOR: quic: do not remove qc_stream_desc automatically on ACK handling

qc_stream_desc_ack() is used to handle ACK received for STREAM frame. It
removes acknowledged data from their underlying buffer.

If all data were removed after ACK handling, qc_stream_desc instance
would automatically be freed at the end of qc_stream_desc_ack().
However, this renders the function complicated to use. Simplify this by
removing this automatic removal. Now, caller is responsible to check
after ACK handling if qc_stream_desc instance can be removed. This is
easily done using qc_stream_desc_done() helper.

9 months agoMINOR: quic: refactor STREAM room notification
Amaury Denoyelle [Wed, 25 Sep 2024 16:25:08 +0000 (18:25 +0200)] 
MINOR: quic: refactor STREAM room notification

qc_stream_desc is an intermediary layer between QUIC MUX and quic_conn.
It is a facility which permits to store data to emit and keep them for
retransmission until acknowledgment. This layer is responsible to notify
QUIC MUX each time a buffer is freed. This is necessary as MUX buffer
allocation is limited by the underlying congestion window size.

Refactor this to use a mechanism similar to send notification. A new
callback notify_room can now be registered to qc_stream_desc instance.
This is set by QUIC MUX to qmux_ctrl_room(). On MUX QUIC free, special
care is now taken to reset notify_room callback to NULL.

Thanks to this refactoring, further adjustment have been made to refine
the architecture. One of them is the removal of qc_stream_desc
QC_SD_FL_OOB_BUF, which is now converted to a MUX layer flag
QC_SF_TXBUF_OOB.

9 months agoMEDIUM: quic: strengthen MUX send notification
Amaury Denoyelle [Mon, 30 Sep 2024 12:39:15 +0000 (14:39 +0200)] 
MEDIUM: quic: strengthen MUX send notification

Previous commit implement a refactor of MUX send notification from
quic_conn layer. With this new architecture, a proper callback is
defined for each qc_stream_desc instance.

This architecture change allows to simplify notification from quic_conn
layer. First, ensure the MUX callback to properly ignore retransmission
of an already emitted frame. Luckily, this can be handled easily by
comparing offsets and FIN status. Also, each QCS instance can now be
unregistered from send notification just prior qc_stream_desc releasing.
This ensures a QCS is never manipulated from quic_conn after its
emission ending. Both these changes render the send notification more
robust. As a nice effect, flag QUIC_FL_CONN_TX_MUX_CONTEXT can be
removed as it is now unneeded.

9 months agoMINOR: quic: refactor MUX send notification
Amaury Denoyelle [Wed, 25 Sep 2024 15:55:10 +0000 (17:55 +0200)] 
MINOR: quic: refactor MUX send notification

For STREAM emission, MUX QUIC generates one or several frames and emit
them via qc_send_mux(). Lower layer may use them as-is, or split them to
lower chunk to fit in a QUIC packet. It is then responsible to notify
the MUX to report the amount of data sent.

Previously, this was done via a direct call from quic_conn to MUX using
qcc_streams_sent_done(). Modify this to have a better isolation accross
layers. Define a send callback handled by the qc_stream_desc instance.
This allows the MUX to register each QCS instance individually to the
renamved qmux_ctrl_send() which replaces qcc_streams_sent_done().

At quic_conn layer, qc_stream_desc_send() can be used now. This is a
wrapper to qc_stream_desc layer to invoke the send callback if
registered.

This mechanism of qc_stream_desc callback should be extended later to
implement other notifications accross the QUIC stack.

9 months agoMINOR: quic: remove unneeded notification of txbuf room
Amaury Denoyelle [Fri, 27 Sep 2024 13:31:21 +0000 (15:31 +0200)] 
MINOR: quic: remove unneeded notification of txbuf room

When a stream buffer is freed, qc_stream_desc notify MUX. This is useful
if MUX is waiting for Tx buffer allocation.

Remove this notification in qc_stream_desc(). This is because the
function is called when all stream data have been acknowledged and thus
notified. This function can also be called with some data
unacknowledged, but in this case this is only true just before
connection closure. As such, it is useful to notify the MUX in this
condition.

9 months agoMINOR: mux-quic: strengthen qcs_send_metadata() usage
Amaury Denoyelle [Tue, 1 Oct 2024 14:17:03 +0000 (16:17 +0200)] 
MINOR: mux-quic: strengthen qcs_send_metadata() usage

This function is reserved for QCS instance where no data was emitted.
A BUG_ON() ensures this by checking that streamdesc buf_list is empty.

However, this condition would not be enough if data were previously
emitted but already fully acknowledged. Thus, extend the condition by
also checking the streamdesc ack_offset is 0.

9 months agoMINOR: quic: ensure txbuf realloc is only performed on empty buffer
Amaury Denoyelle [Tue, 1 Oct 2024 08:55:40 +0000 (10:55 +0200)] 
MINOR: quic: ensure txbuf realloc is only performed on empty buffer

QUIC application protocol layer has the ability to either allocate a
standard buffer or a smaller one. The latter is useful when only small
data are transferred to prevent consuming too much of the QUIC MUX
buffer window.

This operation is performed using qc_stream_buf_realloc(). Add a new
BUG_ON() in it to ensure no data is present in the buffer. Indeed, this
would cause to data loss, or even crash when trying to acknowledge data.

Note that for the moment qc_stream_buf_realloc() is only use for HTTP/3
headers transmission, and this usage is conform to the new BUG_ON. This
commit is thus not a bug fix, but only to strengthen the API.

9 months agoMINOR: mux-quic: complete Tx infos for QCS dump
Amaury Denoyelle [Thu, 26 Sep 2024 08:49:54 +0000 (10:49 +0200)] 
MINOR: mux-quic: complete Tx infos for QCS dump

Complete debug info when a QCS instance is dumped either on traces or
show quic. Display the value of Tx offset both soft and real, along with
the current flow-control limit.

9 months agoMINOR: cfgparse-global: add dedicated parser for *env keywords
Valentine Krasnobaeva [Mon, 30 Sep 2024 12:27:08 +0000 (14:27 +0200)] 
MINOR: cfgparse-global: add dedicated parser for *env keywords

This commit prepares the config parser to support MODE_DISCOVERY and, thus,
refactored master-worker mode. The latter implies, that master process reads
only the 'DISCOVERY' tagged keywords from the global section and it must call
for this an appropriate keyword parser.

So, let's move the code, which parses *env keywords, from the global section
parser to its own keyword registered parser.

9 months agoBUG/MINOR: cfgparse-global: fix allowed args number for setenv
Valentine Krasnobaeva [Mon, 30 Sep 2024 13:29:47 +0000 (15:29 +0200)] 
BUG/MINOR: cfgparse-global: fix allowed args number for setenv

Keywords setenv and presetenv take 2 arguments: variable name and value.
So, the total number, that should be passed to alertif_too_many_args is 2
("setenv <name> <value>") instead of 3. For alertif_too_many_args the first
argument index is 0.

This should be backported in all stable versions.

9 months agoMINOR: stream/stats: Expose the total number of streams ever created in stats
Christopher Faulet [Fri, 27 Sep 2024 15:16:00 +0000 (17:16 +0200)] 
MINOR: stream/stats: Expose the total number of streams ever created in stats

A shared counter is added in the thread context to track the total number of
streams created on the thread. This number is then reported in stats. It
will be a useful information to diagnose some bugs.

9 months agoMINOR: stream/stats: Expose the current number of streams in stats
Christopher Faulet [Wed, 25 Sep 2024 07:59:11 +0000 (09:59 +0200)] 
MINOR: stream/stats: Expose the current number of streams in stats

A shared counter is added in the thread context to track the current number
of streams. This number is then reported in stats. It will be a useful
information to diagnose some bugs.

9 months agoMINOR: stream: Support dynamic changes of the number of connection retries
Christopher Faulet [Wed, 25 Sep 2024 13:10:08 +0000 (15:10 +0200)] 
MINOR: stream: Support dynamic changes of the number of connection retries

Thanks to the previous patch, it is now possible to add an action to
dynamically change the maxumum number of connection retires for a stream.
"set-retries" action may now be used to do so, from a "tcp-request content"
or a "http-request" rule. This action accepts an expression or an integer
between 0 and 100. The integer value is checked during the configuration
parsing and leads to an error if it is not in the expected range. However,
for the expression, the value is retrieve at runtime. So, invalid value are
just ignored.

Too high value is forbidden to avoid any trouble. 100 retries seems already
be an amazingly hight value. In addition, the option is only available on
backend or listen sections.

Because the max retries is limited to 100 at most, it can be stored as a
unsigned short. This save some space in the stream structure.

9 months agoMINOR: stream: Rely on a per-stream max connection retries value
Christopher Faulet [Wed, 25 Sep 2024 13:05:07 +0000 (15:05 +0200)] 
MINOR: stream: Rely on a per-stream max connection retries value

Instead of directly relying on the backend parameter to limit the number of
connection retries, we now use a per-stream value. This value is by default
inherited from the backend value when it is set. So for now, there is no
change except the stream value is used instead of the backend value. But
thanks to this change, it will be possible to dynamically change this value.

9 months agoMINOR: action: Export release_expr_int_action() release function
Christopher Faulet [Wed, 25 Sep 2024 13:03:43 +0000 (15:03 +0200)] 
MINOR: action: Export release_expr_int_action() release function

This function was only used by TCP actions and was private to tcp_act.c
file. However, it make sense to make it public to be used by any action
relying on an int-or-expression argument.

9 months agoBUG/MINOR: mcli: Pretend the mux have more data to deliver between two commands
Christopher Faulet [Fri, 27 Sep 2024 16:14:33 +0000 (18:14 +0200)] 
BUG/MINOR: mcli: Pretend the mux have more data to deliver between two commands

Since the commit "OPTIM: stconn: Don't pretend mux have more data to deliver
on EOI/EOS/ERROR", the SC no longer pretend its mux have more data to
deliver when one of EOI/EOS/ERROR flags are set on its sedesc.

However, for the master cli, it is an issue because any EOI/EOS at the end
of a command is in fact detected on the attempt to get the next command. To
do so, the stream is reset. Because if the commit above, the next received
is never performed. To fix the issue, when the stream is reset, the front SC
pretend its mux have more data to deliver.

This patch must only be bacported if the commit above is backported.

9 months agoOPTIM: stconn: Don't pretend mux have more data to deliver on EOI/EOS/ERROR
Christopher Faulet [Fri, 27 Sep 2024 13:05:11 +0000 (15:05 +0200)] 
OPTIM: stconn: Don't pretend mux have more data to deliver on EOI/EOS/ERROR

Doing some benchs on the 3.0, we encountered a small loss on requests/sec on
small objects compared to the 2.8 . After bisecting the issue, it appeared
that this was introduced when the mux-to-mux zero-copy data forwarding was
implemented in 2.9-dev8. Extra subscribes on receives at the end of the
message were responsible of the loss.

A basic configuration, sending H2 requests to a H1 server returning
responses without payload is enough to observe the issue. With the following
command, we can observe a huge increase of epoll_ctl calls on 2.9/3.x:

  h2load -c 100 -m 10 -n 100000 http://...

On 2.8 we have around 3200 calls to epoll_ctl against more than 20k on 3.1.

The fix seems obvious. After a receive, there is no reason to state a mux
have more data to deliver if EOI/EOS/ERROR flag was set on the
stream-endpoint descriptor. With this change, extra calls to epoll_ctl
disappear. However it is a sensitive part so it is important to keep an eye
on it and to not backport it.

Thanks to Willy and Emeric to have spot the issue.

9 months agoOPTIM: channel: speed up co_getline()'s search of the end of line
Willy Tarreau [Mon, 30 Sep 2024 09:19:20 +0000 (11:19 +0200)] 
OPTIM: channel: speed up co_getline()'s search of the end of line

Previously, co_getline() was essentially used for occasional parsing
in peers's banner or Lua, so it could afford to read one character at
a time. However now it's also used on the TCP log path, where it can
consume up to 40% CPU as mentioned in GH issue #2731. Let's speed it
up by using memchr() to look for the LF, and copying the data at once
using memcpy().

Previously it would take 2.44s to consume 1 GB of log on a single
thread of a Core i7-8650U, now it takes 1.56s (-36%).

9 months agoMINOR: tools: do not attempt to use backtrace() on linux without glibc
Willy Tarreau [Sun, 29 Sep 2024 07:46:10 +0000 (09:46 +0200)] 
MINOR: tools: do not attempt to use backtrace() on linux without glibc

The function is provided by glibc. Nothing prevents us from using our
own outside of glibc there (tested on aarch64 with musl). We still do
not enable it by default as we don't yet know if all archs work well,
but it's sufficient to pass USE_BACKTRACE=1 when building with musl to
verify it's OK.

9 months agoBUILD: tools: only include execinfo.h for the real backtrace() function
Willy Tarreau [Sun, 29 Sep 2024 07:37:16 +0000 (09:37 +0200)] 
BUILD: tools: only include execinfo.h for the real backtrace() function

No need to include this possibly non-existing file when using our own
backtrace() implementation, it's only needed for the libc-provided one.
Because of this it's currently not possible to build musl with backtrace
enabled.

9 months agoMINOR: server: make srv_shutdown_sessions() call pendconn_redistribute()
Willy Tarreau [Fri, 27 Sep 2024 17:01:38 +0000 (19:01 +0200)] 
MINOR: server: make srv_shutdown_sessions() call pendconn_redistribute()

When shutting down server sessions, the queue was not considered, which
is a problem if some element reached the queue at the moment the server
was going down, because there will be no more requests to kick them out
of it. Let's always make sure we scan the queue to kick these streams
out of it and that they can possibly find a more suitable server. This
may make a difference in the time it takes to shut down a server on the
CLI when lots of servers are in the queue.

It might be interesting to backport this to 3.0 but probably not much
further.

9 months agoBUG/MINOR: queue: make sure that maintenance redispatches server queue
Willy Tarreau [Fri, 27 Sep 2024 16:54:07 +0000 (18:54 +0200)] 
BUG/MINOR: queue: make sure that maintenance redispatches server queue

Turning a server to maintenance currently doesn't redispatch the server
queue unless there's an explicit "option redispatch" and no "option
persist", while the former has never really been the purpose of this
test. Better refine this so that forced maintenance also causes the
queue to be flushed, and possibly redispatched unless the proxy has
option persist. This way now when turning a server to maintenance,
the queue is immediately flushed and streams can decide what to do.

This can be backported, though there's no need to go far since it was
never directly reported and only noticed as part of debugging some
rare "shutdown sessions" strangeness, which it might participate to.

9 months agoBUG/MINOR: server: make sure the HMAINT state is part of MAINT
Willy Tarreau [Fri, 27 Sep 2024 16:38:35 +0000 (18:38 +0200)] 
BUG/MINOR: server: make sure the HMAINT state is part of MAINT

In 1.8 when adding "set server fqdn" with commit b418c1228c ("MINOR:
server: cli: Add server FQDNs to server-state file and stats socket."),
the HMAINT flag was not made part of the MAINT ones, so technically
speaking when changing the FQDN, the server is not completely considered
as in maintenance mode.

In its defense, the code location around that was completely messy, with
the aggregator flag being hidden between other values and purposely but
discretely ignoring one of the flags, so the comments were updated to
make the intent clearer (particularly regarding CMAINT which looked like
it was also forgotten while it was on purpose).

This can be backported anywhere.

9 months agoBUG/MEDIUM: stream: make stream_shutdown() async-safe
Willy Tarreau [Thu, 26 Sep 2024 16:30:36 +0000 (18:30 +0200)] 
BUG/MEDIUM: stream: make stream_shutdown() async-safe

The solution found in commit b500e84e24 ("BUG/MINOR: server: shut down
streams under thread isolation") to deal with inter-thread stream
shutdown doesn't work fine because there exists code paths involving
a server lock which can then deadlock on thread_isolate(). A better
solution then consists in deferring the shutdown to the stream itself
and just wake it up for that.

The only thing is that TASK_WOKEN_OTHER is a bit too generic and we
need to pass at least 2 types of events (SF_ERR_DOWN and SF_ERR_KILLED),
so we're now leveraging the new TASK_F_UEVT1 and _UEVT2 flags on the
task's state to convey these info. The caller only needs to wake the
task up with these flags set, and the stream handler will then finish
the job locally using stream_shutdown_self().

This needs to be carefully backported to all branches affected by the
dequeuing issue and containing any of the 5541d4995d ("BUG/MEDIUM:
queue: deal with a rare TOCTOU in assign_server_and_queue()"), and/or
b11495652e ("BUG/MEDIUM: queue: implement a flag to check for the
dequeuing").

9 months agoMINOR: task: define two new one-shot events for use with WOKEN_OTHER or MSG
Willy Tarreau [Thu, 26 Sep 2024 16:19:10 +0000 (18:19 +0200)] 
MINOR: task: define two new one-shot events for use with WOKEN_OTHER or MSG

TASK_WOKEN_MSG only says "someone sent you a message" but doesn't convey
any info about the message. TASK_WOKEN_OTHER says "you're woken for another
reason" but doesn't tell which one. Most often they're used as-is by the
task handlers to report very specific situations.

For some important control notifications, having the ability to modulate
the message a little bit is useful, so let's define two user event types
UEVT1 and UEVT2 to be used in conjunction with TASK_WOKEN_MSG or _OTHER
so that the application can know that a specific condition was explicitly
requested. It will be used this way:

  task_wakeup(s->task, TASK_WOKEN_MSG | TASK_F_UEVT1);
or:
  task_wakeup(s->task, TASK_WOKEN_OTHER | TASK_F_UEVT2);

Since events are cumulative, keep in mind not to consider a 3rd value
as the combination of EVT1+EVT2; these really mean that the two events
appeared (though in unspecified order).

9 months agoRevert "BUG/MINOR: server: shut down streams under thread isolation"
Willy Tarreau [Thu, 26 Sep 2024 16:36:17 +0000 (18:36 +0200)] 
Revert "BUG/MINOR: server: shut down streams under thread isolation"

This reverts commit b500e84e24fd19ccbcdf4fae5165aeb07e46bd67.

Thread isolation does not work well for this, there exists code paths
which already hold the server's lock and result in a deadlock. Let's
revert that and address it better without isolation.

9 months agoREGTESTS: add a test for proxy "log-steps"
Aurelien DARRAGON [Thu, 5 Sep 2024 14:48:09 +0000 (16:48 +0200)] 
REGTESTS: add a test for proxy "log-steps"

Now that proxy "log-steps" keyword was implemented and is usable since
("MEDIUM: log: consider log-steps proxy setting for existing log origins")
let's add some tests for it in reg-tests/log/log_profile.vtc.

9 months agoDOC: config: document proxy "log-steps" keyword
Aurelien DARRAGON [Thu, 5 Sep 2024 14:20:00 +0000 (16:20 +0200)] 
DOC: config: document proxy "log-steps" keyword

Now that "log-steps" proxy keyword is functional, let's add some
documentation and usage examples for it.

9 months agoMEDIUM: log: consider log-steps proxy setting for existing log origins
Aurelien DARRAGON [Wed, 4 Sep 2024 13:03:46 +0000 (15:03 +0200)] 
MEDIUM: log: consider log-steps proxy setting for existing log origins

During tcp/http transaction processing, haproxy may produce logs at
different steps during the processing (accept, connect, request,
response, close). But the behavior is hardly configurable because
haproxy will only emit a single log per transaction, and by default
it will try to produce the log once all log aliases or fetches used
in the logformat could be satisfied, which means the log is often
emitted during connection teardown, unless "option logasap" is used.

We were often asked to have a way to emit multiple logs for a single
transaction, like for instance emit log during accept, then request,
response and close for instance, see GH #401 for more context.

Thanks to "log-steps" keyword introduced by commit "MINOR: log:
introduce "log-steps" proxy keyword", it is now possible to explictly
configure when logs should be generated by haproxy when processing a
transaction. This commit adds the required checks so that log-steps
proxy option is properly considered for existing logs generated by
haproxy. If "log-steps" is not specified on the proxy, the old behavior
is preserved.

Note: a slight cpu overhead should only be visible when "log-steps"
keyword will be used due to the implementation relying on eb32 lookup
instead of basic bitfield check as described in "MINOR: proxy: add
log_steps struct member". However, the default behavior shouldn't be
affected.

When combining log-steps with log-profiles, user has the ability to
explicitly control how and when haproxy should generate logs during
requests handling.

9 months agoMINOR: log: add log_orig_proxy() helper function
Aurelien DARRAGON [Wed, 4 Sep 2024 10:15:21 +0000 (12:15 +0200)] 
MINOR: log: add log_orig_proxy() helper function

Function may be used on proxy where log-steps are used to check if a given
log origin should be handled or not.

9 months agoMINOR: log: introduce "log-steps" proxy keyword
Aurelien DARRAGON [Wed, 31 Jul 2024 15:06:57 +0000 (17:06 +0200)] 
MINOR: log: introduce "log-steps" proxy keyword

For now it is only available for proxies with frontend capability because
log-steps are only evaluated under sess_log() or strm_log() which
essentially focus on the frontend side when it comes to log settings so
it's better to keep it this way for better consistency, at least for now.

For now the setting does nothing (it is not considered during runtime),
it will be implemented and documented in upcoming commits.

9 months agoMINOR: proxy: add log_steps struct member
Aurelien DARRAGON [Tue, 20 Aug 2024 16:29:06 +0000 (18:29 +0200)] 
MINOR: proxy: add log_steps struct member

add proxy->conf.log_steps eb32 root tree which will be used to store the
log origin identifiers that should result in haproxy emitting a log as
configured by the user using upcoming "log-steps" proxy keyword.

It was chosen to use eb32 tree instead of simple bitfield because despite
the slight overhead it is more future-proof given that we already
implemented the prerequisites for seamless custom log origins registration
that will also be usable from "log-steps" proxy keyword.

9 months agoMINOR: log: support extra log origins for '%OG' alias
Aurelien DARRAGON [Tue, 10 Sep 2024 13:39:23 +0000 (15:39 +0200)] 
MINOR: log: support extra log origins for '%OG' alias

Following previous commits, let's improve log_orig_to_str() so that
extra log origins (registered through log_orig_register()) can be
translated to string from origin ID.

For that, it is required to add eb_32 tree node to log_origin struct in
order to enable quick integer lookup during runtime. Slow name lookup
using the list is acceptable for config parsing, but it is not the case
during runtime when log_orig_to_str() is expected to be used. Also, to
prevent duplicated info, get rid of ->id field and use ->tree.key instead

9 months agoMINOR: log: explicitly handle extra log origins as error when relevant
Aurelien DARRAGON [Wed, 31 Jul 2024 12:10:52 +0000 (14:10 +0200)] 
MINOR: log: explicitly handle extra log origins as error when relevant

Thanks to previous commit, we can know check for log_orig optional flags
in functions taking struct log_orig as parameter. Let's take this
opportunity to add the LOG_ORIG_FL_ERROR flag and check this flag at a
few places to handle the log message differently because if the flag is
set then the caller expects the log to be handled as an error explicitly.

e.g.: in _process_send_log_override(), if the flag is set, use the error
log format instead of the dedicated one.

9 months agoMINOR: log: introduce log_orig flags
Aurelien DARRAGON [Mon, 23 Sep 2024 15:22:45 +0000 (17:22 +0200)] 
MINOR: log: introduce log_orig flags

Rename 'enum log_orig' to 'enum log_orig_id', since this enum specifically
contains the log origin ids.

Add 'struct log_orig' which wraps 'enum log_orig' with optional flags
(no flags defined for now).

Add log_orig() helper func that takes id and flags as parameter and
returns log_orig struct initialized with input arguments.

Update functions taking log origin as parameter so they explicitly take
log orig id or log orig wrapper as argument depending on the level of
context expected by the function.

9 months agoMINOR: log: handle extra log origins in _process_send_log_override()
Aurelien DARRAGON [Wed, 31 Jul 2024 12:45:32 +0000 (14:45 +0200)] 
MINOR: log: handle extra log origins in _process_send_log_override()

Thanks to the previous commit, it is now possible to register additional
log origins that may be used from log-profile section as 'on' steps.

As such, let's make _process_send_log_override() function aware of them
by trying to lookup in the tree of extra logging steps in the default
switch-case catchall. If the log origin id matches with the id of the
extra logging step, we use the associated log format instead of the
"any" log format.

9 months agoMINOR: log: introduce extra log profile steps
Aurelien DARRAGON [Wed, 31 Jul 2024 09:15:13 +0000 (11:15 +0200)] 
MINOR: log: introduce extra log profile steps

add a way to register additional log origins using log_origin_register()
that may be used as log profile steps from log profile sections.

For now this does nothing as no extra origins are registered and extra log
origins are not yet considered for runtime logging paths.

When specifying an extra logging step for on <step> under log-profile
section, the logging step is stored within a binary tree for efficient
lookup during runtime. No performance impact should be expected if extra
log origins are not being used, and slight performance impact if extra
log origins are used.

Don't forget to update the documentation when new log origins are added
(both %OG log alias and on <step> log-profile keyword are concerned.

9 months agoMINOR: log: fix indent in strm_log()
Aurelien DARRAGON [Thu, 26 Sep 2024 08:11:41 +0000 (10:11 +0200)] 
MINOR: log: fix indent in strm_log()

8f34320e15 ("MINOR: log: provide log origin in logformat expressions
using '%OG'") caused wrong indent in strm_log()

9 months agoBUG/MEDIUM: cli: Deadlock when setting frontend maxconn
Oliver Dala [Wed, 25 Sep 2024 09:37:25 +0000 (11:37 +0200)] 
BUG/MEDIUM: cli: Deadlock when setting frontend maxconn

The proxy lock state isn't passed down to relax_listener
through dequeue_proxy_listeners, which causes a deadlock
in relax_listener when it tries to get that lock.

Backporting: Older versions didn't have relax_listener and directly called
resume_listener in dequeue_proxy_listeners. lpx should just be passed directly
to resume_listener then.

The bug was introduced in commit 001328873c352e5e4b1df0dcc8facaf2fc1408aa

[cf: This patch should fix the issue #2726. It must be backported as far as
2.4]

9 months agoDEV: flags/applet: decode appctx flags
Christopher Faulet [Tue, 24 Sep 2024 16:26:36 +0000 (18:26 +0200)] 
DEV: flags/applet: decode appctx flags

Decode APPCTX flags via appctx_show_flags() function.

9 months agoBUG/MEDIUM: cli: Be sure to catch immediate client abort
Christopher Faulet [Tue, 24 Sep 2024 15:50:09 +0000 (17:50 +0200)] 
BUG/MEDIUM: cli: Be sure to catch immediate client abort

A client abort while nothing was sent is properly handled except when this
immediately happens after the connection was accepted. The read0 event is
caught before the CLI applet is created. In that case, the shutdown is not
handled and the applet is no longer wakeup. In that case, the stream remains
blocked and no timeout are armed.

The bug was due to the fact that when the applet I/O handler was called for
the first time, the applet context was initialized and nothing more was
performed. A shutdown, if any, would be handled on the next call. In that
case, it was too late.

Now, afet the init step, we loop to eval the first command. There is no
command here but the shutdown will be tested.

This patch should fix the issue #2727. It must be backported to 3.0.

9 months agoMEDIUM: mailers: warn about deprecated legacy mailers
Aurelien DARRAGON [Mon, 23 Sep 2024 10:05:18 +0000 (12:05 +0200)] 
MEDIUM: mailers: warn about deprecated legacy mailers

As mentioned in 2.8 announce on the mailing list [1] and on the wiki [2],
use of legacy mailers is now deprecated and will not be supported anymore
starting with version 3.3. Use of Lua script (AKA Lua mailers) is now
encouraged (and fully supported since 2.8) for this purpose, as it offers
more flexibility (e.g: alerts can be customized) and is more future-proof.

Configurations relying on legacy mailers will now raise a warning.

Users willing to keep their existing mailers config in a working state
should simply add the following line to their global section:

   # mailers.lua file as provided in the git repository
   # adjust path as needed
   lua-load examples/lua/mailers.lua

[1]: https://www.mail-archive.com/haproxy@formilux.org/msg43600.html
[2]: https://github.com/haproxy/wiki/wiki/Breaking-changes

9 months agoREGTESTS: log: fix log-profile.vtc
Aurelien DARRAGON [Thu, 5 Sep 2024 14:48:32 +0000 (16:48 +0200)] 
REGTESTS: log: fix log-profile.vtc

Add missing wait for Slg4 introduced in f8299bc ("MINOR: log: "drop"
support for log-profile steps"), and missing barrier increase due to
the use of barrier sync, which could have resulted in the regtest
being timing-sentive and thus less-reliable.

Also, the "error" check in Slg4 wasn't even considered because it is
emitted by frontend 4, not frontend 2..

No backport needed unless f8299bc is.

9 months agoBUG/MINOR: proxy: also make the cli and resolvers use the global name
Willy Tarreau [Sat, 21 Sep 2024 18:08:06 +0000 (20:08 +0200)] 
BUG/MINOR: proxy: also make the cli and resolvers use the global name

As detected by ASAN on the CI, two places still using strdup() on the
proxy names were left by commit b325453c3 ("MINOR: proxy: use the global
file names for conf->file").

No backport is needed.

9 months agoBUG/MINOR: server: shut down streams under thread isolation
Willy Tarreau [Sat, 21 Sep 2024 17:35:35 +0000 (19:35 +0200)] 
BUG/MINOR: server: shut down streams under thread isolation

Since the beginning of thread support, the shutdown of streams attached
to a server was run under the server's lock, but that's not sufficient.
It indeed turns out that shutting down streams (either from the CLI using
"shutdown sessions server XXX" or due to "on-error shutdown-sessions")
iterates over all the streams to shut them down, but stream_shutdown()
has no way to protect its actions against concurrent actions from the
stream itself on another thread, and streams offer no such provisions
anyway.

The impact is some rare but possible crashes when shutting down streams
from the CLI in cmopetition with high server traffic. The probability
is low enough to mark it minor, though it was observed in the field.

At least since 2.4 the streams are arranged in per-thread lists, so it
likely would be possible using the event subsystem to delegate these
events to dedicated per-thread tasks which would address the problem.
But server streams don't get killed often enough to justify such extra
complexity, so better just run the loop under thread isolation.

It also shows that the internal API could probably be improved to
support a lighter thread exclusion instead of full isolation: various
places want to only exclude one thread and here it could work. But
again there's no point doing this for now.

This patch should be backported to all stable branches. It's important
to carefully check that this srv_shutdowns_streams() function is never
called itself under isolation in older versions (though at first glance
it looks OK).

9 months agoMEDIUM: cfgparse: warn about deprecated use of duplicate server names
Willy Tarreau [Fri, 20 Sep 2024 15:15:11 +0000 (17:15 +0200)] 
MEDIUM: cfgparse: warn about deprecated use of duplicate server names

As discussed below, there are too many problems and limitations caused
by still supporting duplicate server names. That's already particularly
complicated and dissuasive to use since it requires these servers to
have explicit IDs to be accept. Let's now warn on any duplicate, even
with explicit IDs and remind that this will become forbidden in 3.3.

Link: https://www.mail-archive.com/haproxy@formilux.org/msg45185.html
9 months agoOPTIM: cfgparse: speed up duplicate server detection
Willy Tarreau [Fri, 20 Sep 2024 15:12:04 +0000 (17:12 +0200)] 
OPTIM: cfgparse: speed up duplicate server detection

Surprisingly, the duplicate server name detection has never made use
of the names tree, so lookups were still in O(N^2). It took 1 second
to validate 50k servers spread into 25 backends at 2k per backend.

By simply using the tree (and since the current server already is in
the tree), we just have to walk using ebpt_prev_dup to visit previous
servers with the same name. We can then detect which ones conflict
without having an ID set and error. The config check time is now 1/4
of the previous one for 2k servers per backend, and more importantly
it will make it simpler to check for any duplicates later.

9 months agoMEDIUM: cfgparse: drop duplicate named defaults sections after use
Willy Tarreau [Fri, 20 Sep 2024 14:10:52 +0000 (16:10 +0200)] 
MEDIUM: cfgparse: drop duplicate named defaults sections after use

It has never been permitted to explicitly reference named defaults
sections for which there are duplicate names. This means that when
a duplicate defaults section is found, there's no point in keeping
it since it will never be used for lookups, so it can be dropped.

However, some such defaults sections might have some rules in them
that are implicitly referenced by proxies placed after them. In this
case they cannot be removed.

What is done here is that upon each new named section creation, if
another one is found with the same name, its config location is stored
into the new proxy's {prev_file,prev_line} pair, and the old section is
either destroyed if its refcount is null, or just unindexed. The dup
check when creating a new proxy now consists in checking the prev_line
instead of performing a dup lookup on the defaults section.

This will guarantee that we can't find duplicate defaults sections in
their tree anymore, while still keeping track of what's allocated and
releasing everything upon exit.

Beyond the consistency gain, there are nice savings for large configs
involving many defaults sections: a test with 300k sections saved
about 1.9 GB of RAM, and started 25% faster likely thanks to spending
less time allocating memory.

9 months agoMINOR: proxy: add a list of orphaned defaults sections
Willy Tarreau [Fri, 20 Sep 2024 13:59:04 +0000 (15:59 +0200)] 
MINOR: proxy: add a list of orphaned defaults sections

We'll soon delete unreferenced and duplicated named defaults sections
from the list of proxies. The problem with this is that this list (in
fact a name-based tree) is used to release all of them at the end. Let's
add a list of orphaned defaults sections, typically those containing
"http-check send" statements or various other rules, and that are
implicitly inherited by a proxy hence have a non-zero refcount while
also having a name. These now makes it possible to remove them from
the name index while still keeping their memory around for the lifetime
of the process, and cleaning it at the end.

9 months agoBUG/MINOR: cfgparse: detect another uncaught case of duplicate defaults
Willy Tarreau [Fri, 20 Sep 2024 12:28:15 +0000 (14:28 +0200)] 
BUG/MINOR: cfgparse: detect another uncaught case of duplicate defaults

The following sequence was not properly caught:

   defaults def
   backend back from def
   defaults def

But this one was:

   defaults def
   defaults def
   backend back from def

Let's check when defaults are declared that they're not already
referenced.

Better not backport this. While it will catch broken configs (possibly
some with backends pasted after the wrong defaults), these might still
work by accident. It may be reported as a diag warning though.

9 months agoCLEANUP: cfgparse: factor proxy vs log-forward collisions
Willy Tarreau [Fri, 20 Sep 2024 12:13:14 +0000 (14:13 +0200)] 
CLEANUP: cfgparse: factor proxy vs log-forward collisions

This simplifies the check added in 1a38684fbc ("MEDIUM: cfgparse:
detect collisions between defaults and log-forward"), by factoring it
with the other existing one.

The tests are ugly in that code because a first block tests pure
proxies, a second one proxies or defaults and inside that one we
have special cases for defaults. Let's just move the tests to the
"any proxy type" block.

9 months agoMINOR: proxy: use the global file names for conf->file
Willy Tarreau [Thu, 19 Sep 2024 13:35:11 +0000 (15:35 +0200)] 
MINOR: proxy: use the global file names for conf->file

Proxy file names are assigned a bit everywhere (resolvers, peers,
cli, logs, proxy). All these elements were enumerated and now use
copy_file_name(). The only ha_free() call was turned to drop_file_name().

As a bonus side effect, a 300k backend config saved 14 MB of RAM.

9 months agoCLEANUP: stick-table: make the file location point to a global file name
Willy Tarreau [Thu, 19 Sep 2024 13:06:09 +0000 (15:06 +0200)] 
CLEANUP: stick-table: make the file location point to a global file name

The file name used to point to the calling function's stack for stick
tables, which was OK during parsing but remained dangling afterwards.
At least it was already marked const so as not to accidentally free it.
Let's make it point to a file_name_node now.

9 months agoMINOR: tools: add minimal file name management
Willy Tarreau [Thu, 19 Sep 2024 12:56:01 +0000 (14:56 +0200)] 
MINOR: tools: add minimal file name management

In proxies, stick-tables, servers, etc... at plenty of places we store
a file name and a line number. Some file names are the result of strdup()
(e.g. in proxies), others not (e.g. stick-tables) and leave dangling
pointers at the end of parsing. The risk of double-free is not null
either.

In order to stop this, let's first add a simple tool that allows to
register short strings inside a global list, these strings happening
to be server names. The strings are either duplicated and stored upon
failure to find them, or just added to this storage. Since file names
are not expected to disappear before the end of the process, for now
we don't even implement refcounting, and we free them all at the end.
There's already a drop_file_name() function to reset the pointer like
ha_free() used to do, and even if not strictly needed it's a good
habit to get used to doing it.

The strings are returned as const so that they're stored as-is in
structs, and that nasty free() calls are easily caught. The pointer
points to the char[] storage inside the node itself. This way later
if we want to implement refcounting, it will be trivial to just look
up a string and change its associated node's refcount. If needed,
comparisons can also be made on pointers.

For now they're not used yet and are released on deinit().

9 months ago[RELEASE] Released version 3.1-dev8 v3.1-dev8
Willy Tarreau [Wed, 18 Sep 2024 20:29:08 +0000 (22:29 +0200)] 
[RELEASE] Released version 3.1-dev8

Released version 3.1-dev8 with the following main changes :
    - DOC: configuration: place the HAPROXY_HTTP_LOG_FMT example on the correct line
    - MINOR: mux-h1: Set EOI on SE during demux when both side are in DONE state
    - BUG/MEDIUM: mux-h1/mux-h2: Reject upgrades with payload on H2 side only
    - REGTESTS: h1/h2: Update script testing H1/H2 protocol upgrades
    - BUG/MEDIUM: clock: detect and cover jumps during execution
    - BUG/MINOR: pattern: prevent const sample from being tampered in pat_match_beg()
    - BUG/MEDIUM: pattern: prevent uninitialized reads in pat_match_{str,beg}
    - BUG/MEDIUM: pattern: prevent UAF on reused pattern expr
    - MEDIUM: ssl/cli: "dump ssl cert" allow to dump a certificate in PEM format
    - BUG/MAJOR: mux-h1: Wake SC to perform 0-copy forwarding in CLOSING state
    - BUG/MINOR: h1-htx: Don't flag response as bodyless when a tunnel is established
    - REGTESTS: fix random failures with wrong_ip_port_logging.vtc under load
    - BUG/MINOR: pattern: do not leave a leading comma on "set" error messages
    - REGTESTS: shorten a bit the delay for the h1/h2 upgrade test
    - MINOR: server: allow init-state for dynamic servers
    - DOC: server: document what to check for when adding new server keywords
    - MEDIUM: h1: Accept invalid T-E values with accept-invalid-http-response option
    - BUG/MINOR: polling: fix time reporting when using busy polling
    - BUG/MINOR: clock: make time jump corrections a bit more accurate
    - BUG/MINOR: clock: validate that now_offset still applies to the current date
    - BUG/MEDIUM: queue: implement a flag to check for the dequeuing
    - OPTIM: sample: don't check casts for samples of same type
    - OPTIM: vars: remove the unneeded lock in vars_prune_*
    - OPTIM: vars: inline vars_prune() to avoid many calls
    - MINOR: vars: remove the emptiness tests in callers before pruning
    - IMPORT: import cebtree (compact elastic binary trees)
    - OPTIM: vars: use a cebtree instead of a list for variable names
    - OPTIM: vars: use multiple name heads in the vars struct
    - BUG/MINOR: peers: local entries updates may not be advertised after resync
    - DOC: config: Explicitly list relaxing rules for accept-invalid-http-* options
    - MINOR: proxy: Rename accept-invalid-http-* options
    - DOC: configuration: Remove dangerous directives from the proxy matrix
    - BUG/MEDIUM: sc_strm/applet: Wake applet after a successfull synchronous send
    - BUG/MEDIUM: cache/stats: Wait to have the request before sending the response
    - BUG/MEDIUM: promex: Wait to have the request before sending the response
    - MINOR: clock: test all clock_gettime() return values
    - MEDIUM: clock: collect the monotonic time in clock_local_update_date()
    - MEDIUM: clock: opportunistically use CLOCK_MONOTONIC for the internal time
    - MEDIUM: clock: use the monotonic clock for idle time calculation
    - MEDIUM: clock: don't compute before_poll when using monotonic clock
    - BUG/MINOR: fix missing "log-format overrides previous 'option tcplog clf'..." detection
    - BUG/MINOR: fix missing "'option httpslog' overrides previous 'option tcplog clf'..." detection
    - BUG/MINOR: cfgparse-listen: fix option httpslog override warning message
    - BUG/MINOR: cfgparse: detect incorrect overlap of same backend names
    - MEDIUM: cfgparse: warn about proxies having the same names
    - DOC: management: add init-state to add server keywords
    - BUG/MINOR: mux-quic: report glitches to session
    - BUILD: cebtree: silence a bogus gcc warning on impossible code paths
    - MEDIUM: cfgparse: warn about colliding names between defaults and proxies
    - MEDIUM: cfgparse: detect collisions between defaults and log-forward

9 months agoMEDIUM: cfgparse: detect collisions between defaults and log-forward
Willy Tarreau [Wed, 18 Sep 2024 16:05:02 +0000 (18:05 +0200)] 
MEDIUM: cfgparse: detect collisions between defaults and log-forward

Sadly, when log-forward were introduced they took great care of avoiding
collision with regular proxies but defaults were missed (they need to be
explicitly checked for). So now we have to move them to a warning for 3.1
instead of rejecting them.

9 months agoMEDIUM: cfgparse: warn about colliding names between defaults and proxies
Willy Tarreau [Wed, 18 Sep 2024 15:37:17 +0000 (17:37 +0200)] 
MEDIUM: cfgparse: warn about colliding names between defaults and proxies

In order to complete the checks added in 303a66573d ("MEDIUM: cfgparse:
warn about proxies having the same names"), we also need to warn about
regular proxies having the same name as defaults sections as well as
defaults sections having the same name as proxies, since defaults
sections are inherently proxies, albeit stored in a separate list for
now.

9 months agoBUILD: cebtree: silence a bogus gcc warning on impossible code paths
Willy Tarreau [Tue, 17 Sep 2024 15:27:44 +0000 (17:27 +0200)] 
BUILD: cebtree: silence a bogus gcc warning on impossible code paths

gcc-12 and above report a wrong warning about a negative length being
passed to memcmp() on an impossible code path when built at -O0. The
pattern is the same at a few places, basically:

  int foo(int op, const void *a, const void *b, size_t size, size_t arg)
  {
      if (op == 1) // arg is a strict multiple of size
          return memcmp(a, b, arg - size);
      return 0;
  }
  ...
  int bar()
  {
     return foo(0, a, b, sizeof(something), 0);
  }

It *might* be possible to invent dummy values for the "len" argument
above in the real code, but that significantly complexifies it and as
usual can easily result in introducing undesired bugs.

Here we take a different approach consisting in shutting the
-Wstringop-overread warning on gcc>=12 at -O0 since that's the only
condition that triggers it. The issue was reported to and confirmed by
the gcc team here:  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114622

No backport needed, but this should be upstreamed into cebtree after
checking that all involved macros are available.

9 months agoBUG/MINOR: mux-quic: report glitches to session
Amaury Denoyelle [Wed, 18 Sep 2024 13:33:30 +0000 (15:33 +0200)] 
BUG/MINOR: mux-quic: report glitches to session

Glitch counter was implemented for QUIC/HTTP3. The counter is stored in
the QCC MUX connection instance. However, this is never reported at the
session level which is necessary if glitch counter is tracked via a
stick-table.

To fix this, use session_add_glitch_ctr() in various QUIC MUX functions
which may increment glitch counter.

This should be backported up to 3.0.

9 months agoDOC: management: add init-state to add server keywords
Damien Claisse [Tue, 17 Sep 2024 14:54:24 +0000 (14:54 +0000)] 
DOC: management: add init-state to add server keywords

Commit ce6a621ae allowed init-state to be used for dynamic servers but I
forgot to update management doc.

9 months agoMEDIUM: cfgparse: warn about proxies having the same names
Willy Tarreau [Tue, 17 Sep 2024 17:26:06 +0000 (19:26 +0200)] 
MEDIUM: cfgparse: warn about proxies having the same names

As discussed below, there are too many problems and uncaught bugs
in the parser when trying to support proxies having similar names
but different types. There's specific code to detect the presence
of stick-tables in a pair of such proxies for example. It's even
possible that certain combinations of backend+listen that were not
previously detected have some nasty side effects.

According to the proposal in the discussion, this is now deprecated
in 3.1 (thus we emit a warning) and will become forbidden in 3.3.

A backport might be useful, but reporting a diag_warning only, not a
classical warning, so as not to break setups running in zero-warning
mode.

It was verified with a config involving all 9 combinations of
(frontend,backend,listen) followed by one of the same three that all
collisions are now properly blocked and that only back+front are kept
and emit a warning.

Link: https://www.mail-archive.com/haproxy@formilux.org/msg45185.html
9 months agoBUG/MINOR: cfgparse: detect incorrect overlap of same backend names
Willy Tarreau [Tue, 17 Sep 2024 17:22:28 +0000 (19:22 +0200)] 
BUG/MINOR: cfgparse: detect incorrect overlap of same backend names

As reported below, it's possible to declare a backend then a proxy with
the same name, because for the proxy we check a frontend capability (the
first one to be tested):

   backend b
   listen b
        bind :8888

Let's check the two capabilities in this case and not just the frontend.

Better not backport this, as there's a risk of breakage of existing
setups that work by accident. It might make sense to report them as
diag warnings though.

Link: https://www.mail-archive.com/haproxy@formilux.org/msg45185.html