]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoMEDIUM: ssl: add a rwlock for SSL server session cache
William Lallemand [Mon, 8 Feb 2021 09:43:44 +0000 (10:43 +0100)] 
MEDIUM: ssl: add a rwlock for SSL server session cache

When adding the server side support for certificate update over the CLI
we encountered a design problem with the SSL session cache which was not
locked.

Indeed, once a certificate is updated we need to flush the cache, but we
also need to ensure that the cache is not used during the update.
To prevent the use of the cache during an update, this patch introduce a
rwlock for the SSL server session cache.

In the SSL session part this patch only lock in read, even if it writes.
The reason behind this, is that in the session part, there is one cache
storage per thread so it is not a problem to write in the cache from
several threads. The problem is only when trying to write in the cache
from the CLI (which could be on any thread) when a session is trying to
access the cache. So there is a write lock in the CLI part to prevent
simultaneous access by a session and the CLI.

This patch also remove the thread_isolate attempt which is eating too
much CPU time and was not protecting from the use of a free ptr in the
session.

4 years agoBUILD: ssl: guard SSL_CTX_set_msg_callback with SSL_CTRL_SET_MSG_CALLBACK macro
Ilya Shipitsin [Mon, 8 Feb 2021 11:55:06 +0000 (16:55 +0500)] 
BUILD: ssl: guard SSL_CTX_set_msg_callback with SSL_CTRL_SET_MSG_CALLBACK macro

both SSL_CTX_set_msg_callback and SSL_CTRL_SET_MSG_CALLBACK defined since
ea262260469e49149cb10b25a87dfd6ad3fbb4ba, we can safely switch to that guard
instead of OpenSSL version

4 years agoCLEANUP: tools: typo in `strl2irc` mention
William Dauchy [Sat, 6 Feb 2021 19:47:51 +0000 (20:47 +0100)] 
CLEANUP: tools: typo in `strl2irc` mention

`str2irc` does not exist

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoCLEANUP: check: fix some typo in comments
William Dauchy [Sat, 6 Feb 2021 19:47:50 +0000 (20:47 +0100)] 
CLEANUP: check: fix some typo in comments

a few obvious english typo in comments, some of which introduced by
myself quite recently

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Sat, 6 Feb 2021 17:29:08 +0000 (22:29 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 17th iteration of typo fixes

4 years agoMEDIUM: contrib/prometheus-exporter: export base stick table stats
William Dauchy [Sun, 7 Feb 2021 19:42:38 +0000 (20:42 +0100)] 
MEDIUM: contrib/prometheus-exporter: export base stick table stats

I saw some people falling back to unix socket to collect some data they
could not find in prometheus exporter. One of them is base info from
stick tables (used/size).
I do not plan to extend it more for now; keys are quite a mess to
handle.

This should resolve github issue #1008.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: use stats desc when possible followup
William Dauchy [Sun, 7 Feb 2021 19:42:37 +0000 (20:42 +0100)] 
MINOR: contrib/prometheus-exporter: use stats desc when possible followup

Remove remaining descrition which are common to stats.c.

This patch is a followup of commit
82b2ce2f967d967139adb7afab064416fadad615 ("MINOR:
contrib/prometheus-exporter: use stats desc when possible"). I probably
messed up with one of my rebase because I'm pretty sure I removed them
at some point, but who knows what happened.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoBUG/MINOR: mux-h1: Don't emit extra CRLF for empty chunked messages
Christopher Faulet [Mon, 8 Feb 2021 08:34:35 +0000 (09:34 +0100)] 
BUG/MINOR: mux-h1: Don't emit extra CRLF for empty chunked messages

Because of a buggy tests when processing the EOH HTX block, an extra CRLF is
added for empty chunked messages. This bug was introduced by the commit
d1ac2b90c ("MAJOR: htx: Remove the EOM block type and use HTX_FL_EOM
instead").

This fix is specific for 2.4. No backport needed.

4 years agoBUILD: ssl: guard SSL_CTX_add_server_custom_ext with special macro
Ilya Shipitsin [Sat, 6 Feb 2021 13:59:22 +0000 (18:59 +0500)] 
BUILD: ssl: guard SSL_CTX_add_server_custom_ext with special macro

special guard macros HAVE_SSL_CTX_ADD_SERVER_CUSTOM_EXT was defined earlier
exactly for guarding SSL_CTX_add_server_custom_ext, let us use it wherever
appropriate

4 years agoBUILD: ssl: fix typo in HAVE_SSL_CTX_ADD_SERVER_CUSTOM_EXT macro
Ilya Shipitsin [Sat, 6 Feb 2021 13:55:27 +0000 (18:55 +0500)] 
BUILD: ssl: fix typo in HAVE_SSL_CTX_ADD_SERVER_CUSTOM_EXT macro

HAVE_SSL_CTX_ADD_SERVER_CUSTOM_EXT was introduced in ec609098718b9c1cd803ca57442b2b98c9ba4a16
however it was defined as HAVE_SL_CTX_ADD_SERVER_CUSTOM_EXT (missing "S")
let us fix typo

4 years ago[RELEASE] Released version 2.4-dev7 v2.4-dev7
Willy Tarreau [Fri, 5 Feb 2021 14:17:33 +0000 (15:17 +0100)] 
[RELEASE] Released version 2.4-dev7

Released version 2.4-dev7 with the following main changes :
    - BUG/MINOR: stats: Continue to fill frontend stats on unimplemented metric
    - BUILD: ssl: guard Client Hello callbacks with HAVE_SSL_CLIENT_HELLO_CB macro instead of openssl version
    - BUG/MINOR: stats: Init the metric variable when frontend stats are filled
    - MINOR: contrib/prometheus-exporter: better output of Not-a-Number
    - CLEANUP: stats: improve field selection for frontend http fields
    - CLEANUP: assorted typo fixes in the code and comments
    - DOC: Improve documentation of the various hdr() fetches
    - MEDIUM: stats: allow to select one field in `stats_fill_be_stats`
    - MINOR: contrib/prometheus-exporter: use fill_be_stats for backend dump
    - MEDIUM: stats: allow to select one field in `stats_fill_sv_stats`
    - MINOR: contrib/prometheus-exporter: use fill_sv_stats for server dump
    - MINOR: abort() on my_unreachable() when DEBUG_USE_ABORT is set.
    - BUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown
    - BUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name
    - MINOR: reg-tests: add http-reuse test
    - CLEANUP: srv: fix comment for pool-max-conn
    - CLEANUP: backend: remove an obsolete comment on conn_backend_get
    - REORG: backend: simplify conn_backend_get
    - MINOR: ssl: Server ssl context prepare function refactoring
    - MINOR: ssl: Certificate chain loading refactorization
    - MEDIUM: ssl: Load client certificates in a ckch for backend servers
    - MEDIUM: ssl: Enable backend certificate hot update
    - MINOR: ssl: Remove client_crt member of the server's ssl context
    - CLEANUP: ssl/cli: rework free in cli_io_handler_commit_cert()
    - CLEANUP: ssl: remove SSL_CTX function parameter
    - CLEANUP: ssl: make load_srv_{ckchs,cert} match their bind counterpart
    - BUILD: Include stdlib.h in compiler.h if DEBUG_USE_ABORT is set
    - CI: Fix DEBUG_STRICT definition for Coverity
    - BUG/MINOR: stats: Remove a break preventing ST_F_QCUR to be set for servers
    - BUG/MINOR: stats: Add a break after filling ST_F_MODE field for servers
    - CLEANUP: ssl: remove dead code in ckch_inst_new_load_srv_store()
    - BUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()
    - BUG/MEDIUM: session: only retrieve ready idle conn from session
    - BUG/MEDIUM: backend: never reuse a connection for tcp mode
    - REGTESTS: set_ssl_server_cert.vtc: remove the abort command
    - REGTESTS: set_ssl_server_cert.vtc: check the Sha1 Fingerprint
    - REGTESTS: set_ssl_server_cert.vtc: check the sha1 from the server
    - MEDIUM: stream-int: Take care of EOS if the SI wake callback function
    - MINOR: mux-h1: Try to wake up data layer first before calling its wake callback
    - MINOR: mux-h1: Wake up H1C after its creation if input buffer is not empty
    - MEDIUM: mux-h1: Add ST_READY state for the H1 connections
    - MINOR: stream: Add a function to validate TCP to H1 upgrades
    - MEDIUM: http-ana: Do nothing in wait-for-request analyzer if not htx
    - BUG/MEDIUM: stream: Don't immediatly ack the TCP to H1 upgrades
    - BUG/MAJOR: mux-h1: Properly handle TCP to H1 upgrades
    - MINOR: htx/http-ana: Save info about Upgrade option in the Connection header
    - MEDIUM: http-ana: Refuse invalid 101-switching-protocols responses
    - BUG/MINOR: h2/mux-h2: Reject 101 responses with a PROTOCOL_ERROR h2s error
    - MINOR: mux-h1/mux-fcgi: Don't set TUNNEL mode if payload length is unknown
    - MINOR: mux-h1: Split H1C_F_WAIT_OPPOSITE flag to separate input/output sides
    - MINOR: mux-h2: Add 2 flags to help to properly handle tunnel mode
    - MEDIUM: mux-h2: Block client data on server side waiting tunnel establishment
    - MEDIUM: mux-h2: Close streams when processing data for an aborted tunnel
    - MEDIUM: mux-h1: Properly handle tunnel establishments and aborts
    - BUG/MAJOR: mux-h1/mux-h2/htx: Fix HTTP tunnel management at the mux level
    - MINOR: htx: Rename HTX_FL_EOI flag into HTX_FL_EOM
    - REGTESTS: Don't run http_msg_full_on_eom script on the 2.4 anymore
    - MINOR: htx: Add a function to know if a block is the only one in a message
    - MAJOR: htx: Remove the EOM block type and use HTX_FL_EOM instead
    - MINOR: mux-h1: Add a flag on H1 streams with a response known to be bodyless
    - MEDIUM: mux-h1: Don't emit any payload for bodyless responses
    - MINOR: mux-h1: Don't emit C-L and T-E headers for 204 and 1xx responses
    - MINOR: mux-h1: Don't add Connection close/keep-alive header for 1xx messages
    - MINOR: h2/mux-h2: Add flags to notify the response is known to have no body
    - MEDIUM: mux-h2: Don't emit DATA frame for bodyless responses
    - MEDIUM: http-ana: Deal with L7 retries in HTTP analysers
    - MINOR: h1: reject websocket handshake if missing key
    - MEDIUM: h1: generate WebSocket key on response if needed
    - MINOR: mux_h2: define H2_SF_EXT_CONNECT_SENT stream flag
    - MEDIUM: h2: parse Extended CONNECT reponse to htx
    - MEDIUM: mux_h2: generate Extended CONNECT from htx upgrade
    - MEDIUM: h1: add a WebSocket key on handshake if needed
    - MEDIUM: mux_h2: generate Extended CONNECT response
    - MEDIUM: h2: parse Extended CONNECT request to htx
    - MEDIUM: h2: send connect protocol h2 settings
    - MINOR: vtc: add test for h1/h2 protocol upgrade translation
    - MINOR: vtc: add websocket test
    - REGTESTS: Fix required versions for several scripts
    - REGTEST: Don't use the websocket to validate http-check
    - MINOR: mux-h1/trace: add traces at level ERROR for all kind of errors
    - MINOR: mux-fcgi/trace: add traces at level ERROR for all kind of errors
    - MINOR: h1: Raise the chunk size limit up to (2^52 - 1)
    - BUG/MEDIUM: listener: do not accept connections faster than we can process them
    - REGTESTS: set_ssl_server_cert.vtc: set as broken
    - Revert "BUG/MEDIUM: listener: do not accept connections faster than we can process them"
    - BUG/MINOR: backend: check available list allocation for reuse
    - CI: Fix the coverity builds
    - DOC: management: fix "show resolvers" alphabetical ordering
    - MINOR: tools: add print_time_short() to print a condensed duration value
    - MINOR: activity: make profiling more manageable
    - MINOR: activity: declare a new structure to collect per-function activity
    - MEDIUM: tasks/activity: collect per-task statistics when profiling is enabled
    - MINOR: activity: also report collected tasks stats in "show profiling"
    - MINOR: activity: flush scheduler stats on "set profiling tasks on"
    - MINOR: activity: add a new "show tasks" command to list currently active tasks
    - MINOR: listener: export accept_queue_process
    - MINOR: session: export session_expire_embryonic()
    - MINOR: muxes: export the timeout and shutr task handlers
    - MINOR: checks: export a few functions that appear often in trace dumps
    - MINOR: peers: export process_peer_sync() to improve traces
    - MINOR: stick-tables: export process_table_expire()
    - MINOR: mux-h1: Remove first useless test on count in h1_process_output()
    - BUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list
    - MINOR: http-fetch: Don't check if argument list is set in sample fetches
    - MINOR: http-conv: Don't check if argument list is set in sample converters
    - MINOR: sample: Don't check if argument list is set in sample fetches
    - MINOR: ssl-sample: Don't check if argument list is set in sample fetches
    - MINOR: mux-h2: Don't tests the start-line when sending HEADERS frame
    - MINOR: mux-h2: Slightly improve request HEADERS frames sending
    - MINOR: contrib/prometheus-exporter: declare states for objects
    - MAJOR: contrib/prometheus-exporter: move ftd/bkd/srv states to labels
    - MEDIUM: contrib/prometheus-exporter: Use dynamic labels instead of static ones
    - MINOR: listener: export manage_global_listener_queue()
    - BUG/MINOR: activity: take care of late wakeups in "show tasks"
    - REGTESTS: set_ssl_server_cert.vtc: remove SSL caching and set as working
    - REGTESTS: set_ssl_server_cert: cleanup the SSL caching option
    - MINOR: checks: Add function to get the result code corresponding to a status
    - MAJOR: contrib/prometheus-exporter: move health check status to labels
    - MINOR: contrib/prometheus-exporter: improve service status description field
    - MINOR: stats: improve pending connections description
    - MINOR: stats: improve max stats descriptions
    - MINOR: contrib/prometheus-exporter: use stats desc when possible
    - MINOR: contrib/prometheus-exporter: add uweight field
    - MINOR: contrib/prometheus-exporter: add recv logs_logs_total field
    - CLEANUP: contrib/prometheus-exporter: remove unused includes
    - CLEANUP: contrib/prometheus-exporter: align and reorder fields
    - CLEANUP: contrib/prometheus-exporter: remove description in README
    - DOC: contrib/prometheus-exporter: Add missing metrics in README
    - BUG/MINOR: contrib/prometheus-exporter: Add missing label for ST_F_HRSP_1XX
    - BUG/MINOR: contrib/prometheus-exporter: Restart labels dump at the right pos
    - BUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store
    - BUG/MEDIUM: ssl: check a connection's status before computing a handshake
    - BUG/MINOR: mux_h2: fix incorrect stat titles
    - MINOR: ssl/cli: flush the server session cache upon 'commit ssl cert'
    - BUG/MINOR: cli: fix set server addr/port coherency with health checks
    - MINOR: server: Don't set the check port during the update from a state file
    - MINOR: dns: Don't set the check port during a server dns resolution
    - MEDIUM: check: remove checkport checkaddr flag
    - MEDIUM: server: adding support for check_port in server state
    - BUG/MINOR: check: consitent way to set agentaddr
    - MEDIUM: check: align agentaddr and agentport behaviour
    - DOC: server: Add missing params in comment of the server state line parsing
    - BUG/MINOR: xxhash: make sure armv6 uses memcpy()
    - REGTESTS: mark http-check-send.vtc as 2.4-only
    - REGTESTS: mark sample_fetches/hashes.vtc as 2.4-only
    - BUG/MINOR: ssl: do not try to use early data if not configured
    - REGTESTS: unbreak http-check-send.vtc
    - MINOR: cli/show_fd: report local and report ports when known
    - BUILD: Makefile: move REGTESTST_TYPE default setting
    - BUG/MEDIUM: mux-h2: handle remaining read0 cases
    - CLEANUP: http-htx: Set buffer area to NULL instead of malloc(0)
    - BUG/MINOR: sock: Unclosed fd in case of connection allocation failure
    - BUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED

4 years agoBUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED
Willy Tarreau [Fri, 5 Feb 2021 11:16:01 +0000 (12:16 +0100)] 
BUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED

The demux loop could quit on missing data but the H2_CF_END_REACHED flag
would not be set in this case. This fixes a remaining situation where
previous commit f09612289 ("BUG/MEDIUM: mux-h2: handle remaining read0
cases") could not be sufficient and still leave CLOSE_WAIT. It's harder
to reproduce but was still observed in prod.

Now we quit via the end of the loop which already takes care of shutr.

This should be backported along with the patch above as far as 2.0.

4 years agoBUG/MINOR: sock: Unclosed fd in case of connection allocation failure
Remi Tricot-Le Breton [Thu, 14 Jan 2021 14:26:24 +0000 (15:26 +0100)] 
BUG/MINOR: sock: Unclosed fd in case of connection allocation failure

If allocating a connection object failed right after a successful accept
on a listener, the new file descriptor was not properly closed.

This fixes GitHub issue #905.
It can be backported to 2.3.

4 years agoCLEANUP: http-htx: Set buffer area to NULL instead of malloc(0)
Christopher Faulet [Fri, 5 Feb 2021 09:29:29 +0000 (10:29 +0100)] 
CLEANUP: http-htx: Set buffer area to NULL instead of malloc(0)

During error files conversion to HTX message, in http_str_to_htx(), if a
file is empty, the corresponding buffer's area is initialized with a
malloc(0) and its size is set to 0. There is no problem here. The behaviour
is totally defined. But it is not really intuitive. Instead, we can simply
set the area to NULL.

This patch should fix the issue #1022.

4 years agoBUG/MEDIUM: mux-h2: handle remaining read0 cases
Willy Tarreau [Fri, 5 Feb 2021 10:41:46 +0000 (11:41 +0100)] 
BUG/MEDIUM: mux-h2: handle remaining read0 cases

Commit 3d4631fec ("BUG/MEDIUM: mux-h2: fix read0 handling on partial
frames") tried to address an issue introduced in commit aade4edc1 where
read0 wasn't properly handled in the middle of a frame. But the fix was
incomplete for two reasons:

  - first, it would set H2_CF_RCVD_SHUT in h2_recv() after detecting
    a read0 but the condition was guarded by h2_recv_allowed() which
    explicitly excludes read0 ;

  - second, h2_process would only call h2_process_demux() when there
    were still data in the buffer, but closing after a short pause to
    leave a buffer empty wouldn't be caught in this case.

This patch fixes this by properly taking care of the received shutdown
and by also waking up h2_process_demux() on an empty buffer if the demux
is not blocked.

Given the patches above were tagged for backporting to 2.0, this one
should be as well.

4 years agoBUILD: Makefile: move REGTESTST_TYPE default setting
William Lallemand [Fri, 5 Feb 2021 10:27:54 +0000 (11:27 +0100)] 
BUILD: Makefile: move REGTESTST_TYPE default setting

In patch 3bad3d5 ("BUILD: Makefile: exclude broken tests by default"),
the default setting of the REGTESTST_TYPE variable was set in the
Makefile instead of the run-regtests.sh script.

Doing it in the Makefile was breaking the use of this environment
varible with make ( REGTESTS_TYPES=slow,default make reg-tests )

This patch move the default setting from the Makefile to
run-regtests.sh. It also change the documentation in `make
reg-tests-help` about the default value.

This patch should be backported where 3bad3d5 is backported.

4 years agoMINOR: cli/show_fd: report local and report ports when known
Willy Tarreau [Fri, 5 Feb 2021 09:54:52 +0000 (10:54 +0100)] 
MINOR: cli/show_fd: report local and report ports when known

FD dumps are not always easy to match against netstat dumps, and often
require an lsof as a third dump. Let's emit the socket family, and the
local and remore ports when the FD is an IPv4/IPv6 socket, this will
significantly ease the matching.

4 years agoREGTESTS: unbreak http-check-send.vtc
Willy Tarreau [Fri, 5 Feb 2021 09:13:15 +0000 (10:13 +0100)] 
REGTESTS: unbreak http-check-send.vtc

As noticed by Christopher, I messed up the version fix in commit
cb4ed02ef ("REGTESTS: mark http-check-send.vtc as 2.4-only"), as while
looking up the commit introducing the change I accidently reverted it.
Let's reinsert the contents of the file prior to that fix, except the
version, of course.

4 years agoBUG/MINOR: ssl: do not try to use early data if not configured
Willy Tarreau [Wed, 3 Feb 2021 10:21:38 +0000 (11:21 +0100)] 
BUG/MINOR: ssl: do not try to use early data if not configured

The CO_FL_EARLY_SSL_HS flag was inconditionally set on the connection,
resulting in SSL_read_early_data() always being used first in handshake
calculations. While this seems to work well (probably that there are
fallback paths inside openssl), it's particularly confusing and makes
the debugging quite complicated. It possibly is not optimal by the way.

This flag ought to be set only when early_data is configured on the bind
line. Apparently there used to be a good reason for doing it this way in
1.8 times, but it really does not make sense anymore. It may be OK to
backport this to 2.3 if this helps with troubleshooting, but better not
go too far as it's unlikely to fix any real issue while it could introduce
some in old versions.

4 years agoREGTESTS: mark sample_fetches/hashes.vtc as 2.4-only
Willy Tarreau [Thu, 4 Feb 2021 17:07:59 +0000 (18:07 +0100)] 
REGTESTS: mark sample_fetches/hashes.vtc as 2.4-only

Commit 9eea56009 ("REGTESTS: add tests for the xxh3 converter") introduced
the xxh3 to the tests thus made it incompatible with 2.3 and older, let's
upgrade the version requirement.

4 years agoREGTESTS: mark http-check-send.vtc as 2.4-only
Willy Tarreau [Thu, 4 Feb 2021 17:06:13 +0000 (18:06 +0100)] 
REGTESTS: mark http-check-send.vtc as 2.4-only

Since commit 39ff8c519 ("REGTESTS: complete http-check test"), it breaks
on pre-2.4, let's update the required version.

4 years agoBUG/MINOR: xxhash: make sure armv6 uses memcpy()
Willy Tarreau [Thu, 4 Feb 2021 16:02:39 +0000 (17:02 +0100)] 
BUG/MINOR: xxhash: make sure armv6 uses memcpy()

There was a special case made to allow ARMv6 to use unaligned accesses
via a cast in xxHash when __ARM_FEATURE_UNALIGNED is defined. But while
ARMv6 (and v7) does support unaligned accesses, it's only for 32-bit
pointers, not 64-bit ones, leading to bus errors when the compiler emits
an ldrd instruction and the input (e.g. a pattern) is not aligned, as in
issue #1035.

Note that v7 was properly using the packed approach here and was safe,
however haproxy versions 2.3 and older use the old r39 xxhash code which
has the same issue for armv7. A slightly different fix is required there,
by using a different definition of packed for 32 and 64 bits.

The problem is really visible when running v7 code on a v8 kernel because
such kernels do not implement alignment trap emulation, and the process
dies when this happens. This is why in the issue above it was only detected
under lxc. The emulation could have been disabled on v7 as well by writing
zero to /proc/cpu/alignment though.

This commit is a backport of xxhash commit a470f2ef ("update default memory
access for armv6").

Thanks to @srkunze for the report and tests, @stgraber for his help on
setting up an easy reproducer outside of lxc, and @Cyan4973 for the
discussion around the best way to fix this. Details and alternate patches
available on https://github.com/Cyan4973/xxHash/issues/490.

4 years agoDOC: server: Add missing params in comment of the server state line parsing
Christopher Faulet [Thu, 4 Feb 2021 09:12:05 +0000 (10:12 +0100)] 
DOC: server: Add missing params in comment of the server state line parsing

srv_use_ssl and srv_check_port parameters were not mentionned in the comment
of the function parsing a server state line.

4 years agoMEDIUM: check: align agentaddr and agentport behaviour
William Dauchy [Wed, 3 Feb 2021 21:30:09 +0000 (22:30 +0100)] 
MEDIUM: check: align agentaddr and agentport behaviour

in the same manner of agentaddr, we now:
- permit to set agentport through `port` keyword, like it is the case
  for agentaddr through `addr`
- set the priority on `agent-port` keyword when used
- add a flag to be able to test when the value is set like for agentaddr

it makes the behaviour between `addr` and `port` more consistent.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoBUG/MINOR: check: consitent way to set agentaddr
William Dauchy [Wed, 3 Feb 2021 21:30:08 +0000 (22:30 +0100)] 
BUG/MINOR: check: consitent way to set agentaddr

small consistency problem with `addr` and `agent-addr` options:
for the both options, the last one parsed is always used to set the
agent-check addr.  Thus these two lines don't have the same behavior:

  server ... addr <addr1> agent-addr <addr2>
  server ... agent-addr <addr2> addr <addr1>

After this patch `agent-addr` will always be the priority option over
`addr`. It means we test the flag before setting agentaddr.
We also fix all the places where we did not set the flag to be coherent
everywhere.

I was not really able to determine where this issue is coming from. So
it is probable we may backport it to all stable version where the agent
is supported.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMEDIUM: server: adding support for check_port in server state
William Dauchy [Wed, 3 Feb 2021 21:30:06 +0000 (22:30 +0100)] 
MEDIUM: server: adding support for check_port in server state

We can currently change the check-port using the cli command `set server
check-port` but there is a consistency issue when using server state.
This patch aims to fix this problem but will be also a good preparation
work to get rid of checkport flag, so we are able to know when checkport
was set by config.

I am fully aware this is not making github #953 moving forward, I
however think this might be acceptable while waiting for a proper
solution and resolve consistency problem faced with port settings.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMEDIUM: check: remove checkport checkaddr flag
William Dauchy [Wed, 3 Feb 2021 21:30:07 +0000 (22:30 +0100)] 
MEDIUM: check: remove checkport checkaddr flag

While trying to fix some consistency problem with the config file/cli
(e.g. check-port cli command does not set the flag), we realised
checkport flag was not necessarily needed. Indeed tcpcheck uses service
port as the last choice if check.port is zero. So we can assume if
check.port is zero, it means it was never set by the user, regardless if
it is by the cli or config file.  In the longterm this will avoid to
introduce a new consistency issue if we forget to set the flag.

in the same manner of checkport flag, we don't really need checkaddr
flag. We can assume if checkaddr is not set, it means it was never set
by the user or config.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: dns: Don't set the check port during a server dns resolution
Christopher Faulet [Thu, 4 Feb 2021 09:39:56 +0000 (10:39 +0100)] 
MINOR: dns: Don't set the check port during a server dns resolution

When a server dns resolution is performed, there is no reason to set an
unconfigured check port with the server port. Because by default, if the
check port is not set, the server's one is used. Thus we can remove this
useless assignment. It is mandatory for next improvements.

4 years agoMINOR: server: Don't set the check port during the update from a state file
Christopher Faulet [Thu, 4 Feb 2021 09:17:15 +0000 (10:17 +0100)] 
MINOR: server: Don't set the check port during the update from a state file

When the server state is loaded from a server-state file, there is no reason
to set an unconfigured check port with the server port. Because by default,
if the check port is not set, the server's one is used. Thus we can remove
this useless assignment. It is mandatory for next improvements.

4 years agoBUG/MINOR: cli: fix set server addr/port coherency with health checks
William Dauchy [Wed, 3 Feb 2021 21:30:05 +0000 (22:30 +0100)] 
BUG/MINOR: cli: fix set server addr/port coherency with health checks

while reading `update_server_addr_port` I found out some things which
can be seen as incoherency. I hope I did not overlooked anything:

- one comment is stating check's address should be updated if it uses
  the server one; however the condition checks if `SRV_F_CHECKADDR` is
  set; this flag is set when a check address is set; result is that we
  override the check address where I was not expecting it. In fact we
  don't need to update anything here as server addr is used when check
  addr is not set.
- same goes for check agent addr
- for port, it is a bit different, we update the check port if it is
  unset. This is harmless because we also use server port if check port
  is unset. However it creates some incoherency before/after using this
  command, as check port should stay unset througout the life of the
  process unless it is is set by `set server check-port` command.

quite hard to locate the origin of this this issue but the function was
introduced in commit d458adcc52b74608e2fe6a2a95f09ce5e94932b7 ("MINOR:
new update_server_addr_port() function to change both server's ADDR and
service PORT"). I was however not able to determine whether this is due
to a change of behavior along the years. So this patch can potentially
be backported up to v1.8 but we must be careful while doing so, as the
code has changed a lot. That being said, the bug being not very
impacting I would be fine keeping it for 2.4 only.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: ssl/cli: flush the server session cache upon 'commit ssl cert'
William Lallemand [Wed, 3 Feb 2021 17:51:01 +0000 (18:51 +0100)] 
MINOR: ssl/cli: flush the server session cache upon 'commit ssl cert'

Flush the SSL session cache when updating a certificate which is used on a
server line. This prevent connections to be established with a cached
session which was using the previous SSL_CTX.

This patch also replace the ha_barrier with a thread_isolate() since there
are more operations to do. The reg-test was also updated to remove the
'no-ssl-reuse' keyword which is now uneeded.

4 years agoBUG/MINOR: mux_h2: fix incorrect stat titles
Amaury Denoyelle [Wed, 3 Feb 2021 15:27:22 +0000 (16:27 +0100)] 
BUG/MINOR: mux_h2: fix incorrect stat titles

Duplicate titles for the stats H2_ST_{OPEN,TOTAL}_{CONN,STREAM}. These
entries are used on csv for the heading.

This must be backported up to 2.3.

This fixes the github issue #1102.

4 years agoBUG/MEDIUM: ssl: check a connection's status before computing a handshake
Willy Tarreau [Tue, 2 Feb 2021 14:42:25 +0000 (15:42 +0100)] 
BUG/MEDIUM: ssl: check a connection's status before computing a handshake

As spotted in issue #822, we're having a problem with error detection in
the SSL layer. The problem is that on an overwhelmed machine, accepted
connections can start to pile up, each of them requiring a slow handshake,
and during all this time if the client aborts, the handshake will still be
calculated.

The error controls are properly placed, it's just that the SSL layer
reads records exactly of the advertised size, without having the ability
to encounter a pending connection error. As such if injecting many TLS
connections to a listener with a huge backlog, it's fairly possible to
meet this situation:

  12:50:48.236056 accept4(8, {sa_family=AF_INET, sin_port=htons(62794), sin_addr=inet_addr("127.0.0.1")}, [128->16], SOCK_NONBLOCK) = 1109
  12:50:48.236071 setsockopt(1109, SOL_TCP, TCP_NODELAY, [1], 4) = 0
  (process other connections' handshakes)

  12:50:48.257270 getsockopt(1109, SOL_SOCKET, SO_ERROR, [ECONNRESET], [4]) = 0
  (proof that error was detectable there but this code was added for the PoC)

  12:50:48.257297 recvfrom(1109, "\26\3\1\2\0", 5, 0, NULL, NULL) = 5
  12:50:48.257310 recvfrom(1109, "\1\0\1\3"..., 512, 0, NULL, NULL) = 512

  (handshake calculation taking 700us)

  12:50:48.258004 sendto(1109, "\26\3\3\0z"..., 1421, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = -1 EPIPE (Broken pipe)
  12:50:48.258036 close(1109)             = 0

The situation was amplified by the multi-queue accept code, as it resulted
in many incoming connections to be accepted long before they could be
handled. Prior to this they would have been accepted and the handshake
immediately started, which would have resulted in most of the connections
waiting in the the system's accept queue, and dying there when the client
aborted, thus the error would have been detected before even trying to
pass them to the handshake code.

As a result, with a listener running on a very large backlog, it's possible
to quickly accept tens of thousands of connections and waste time slowly
running their handshakes while they get replaced by other ones.

This patch adds an SO_ERROR check on the connection's FD before starting
the handshake. This is not pretty as it requires to access the FD, but it
does the job.

Some improvements should be made over the long term so that the transport
layers can report extra information with their ->rcv_buf() call, or at the
very least, implement a ->get_conn_status() function to report various
flags such as shutr, shutw, error at various stages, allowing an upper
layer to inquire for the relevance of engaging into a long operation if
it's known the connection is not usable anymore. An even simpler step
could probably consist in implementing this in the control layer.

This patch is simple enough to be backported as far as 2.0.

Many thanks to @ngaugler for his numerous tests with detailed feedback.

4 years agoBUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store
William Lallemand [Mon, 1 Feb 2021 14:31:00 +0000 (15:31 +0100)] 
BUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store

The "abort ssl cert" command is buggy and removes the current ckch store,
and instances, leading to SNI removal. It must only removes the new one.

This patch also adds a check in set_ssl_cert.vtc and
set_ssl_server_cert.vtc.

Must be backported as far as 2.2.

4 years agoBUG/MINOR: contrib/prometheus-exporter: Restart labels dump at the right pos
Christopher Faulet [Mon, 1 Feb 2021 14:05:21 +0000 (15:05 +0100)] 
BUG/MINOR: contrib/prometheus-exporter: Restart labels dump at the right pos

For some metrics, several lines are produced per entity, one per label
value. For instance, the health-check status (ST_F_CHECK_STATUS) or the
entity status (ST_F_STATUS). The dump may be stopped in the middle of the
labels processing if the output buffer is full. This means the next time, we
must take care to restart on the right label value.

For now, this part is buggy and we always restart to dump all the label
values again from the beginning. To be sure to restart at the right
position, the field <ctx.stats.st_code> in the applet context is used to
save the last position. Of course, we take care to reset this value when
necessary.

This fix is specific for 2.4. No backport needed.

4 years agoBUG/MINOR: contrib/prometheus-exporter: Add missing label for ST_F_HRSP_1XX
Christopher Faulet [Mon, 1 Feb 2021 13:55:37 +0000 (14:55 +0100)] 
BUG/MINOR: contrib/prometheus-exporter: Add missing label for ST_F_HRSP_1XX

Since the labels are dynamically created for each metric, the "code" label
of the ST_F_HRSP_1XX field is missing. To fix the bug, this metric is
handled in the same way the other ST_F_HRSP_* field are. We only take care
to dump the metric header only once.

This bug was introduced by the commit 5a2f93873 ("MEDIUM:
contrib/prometheus-exporter: Use dynamic labels instead of static ones"). No
backport needed.

4 years agoDOC: contrib/prometheus-exporter: Add missing metrics in README
Christopher Faulet [Mon, 1 Feb 2021 13:50:30 +0000 (14:50 +0100)] 
DOC: contrib/prometheus-exporter: Add missing metrics in README

Some metrics were missing (haproxy_process_uptime_seconds and
haproxy_process_build_info). To ease the review against the service output,
the same order is used in the README.

4 years agoCLEANUP: contrib/prometheus-exporter: remove description in README
William Dauchy [Mon, 1 Feb 2021 12:12:00 +0000 (13:12 +0100)] 
CLEANUP: contrib/prometheus-exporter: remove description in README

Now that we got ride of description in prometheus code, let's assume we
no longer need to maintain it in README, and diret user to the output of
prometheus to get more info.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoCLEANUP: contrib/prometheus-exporter: align and reorder fields
William Dauchy [Mon, 1 Feb 2021 12:11:59 +0000 (13:11 +0100)] 
CLEANUP: contrib/prometheus-exporter: align and reorder fields

- align safe_idle_connections_current field
  fix minor typo added in commit
  37286a5ac595069a491c3e8a7a7e4faf3d9ea8ad ("MEDIUM:
  contrib/prometheus-exporter: Rework matrices defining Promex metrics")
- reorder info fields to be able to compare them easily
- add missing ignored info fields as comment

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoCLEANUP: contrib/prometheus-exporter: remove unused includes
William Dauchy [Mon, 1 Feb 2021 12:11:58 +0000 (13:11 +0100)] 
CLEANUP: contrib/prometheus-exporter: remove unused includes

unless I'm wrong, those includes are no longer needed.  The only recent
one I remember is ssl-sock include since commit
5d9b8f3c9347a1a10b86f81d70b22c3cab0e6925 ("MINOR:
contrib/prometheus-exporter: use fill_info for process dump") where we
make use of the code from stats.c

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: add recv logs_logs_total field
William Dauchy [Mon, 1 Feb 2021 12:11:57 +0000 (13:11 +0100)] 
MINOR: contrib/prometheus-exporter: add recv logs_logs_total field

this field was added by commit 45c457a62941a7c4a86ce4327d7755edcd4b230e
("MINOR: log: adds counters on received syslog messages.")

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: add uweight field
William Dauchy [Mon, 1 Feb 2021 12:11:56 +0000 (13:11 +0100)] 
MINOR: contrib/prometheus-exporter: add uweight field

this field was added in commit bd7151002437af1a034a9fdbb582b3cbef5a78d1
("MINOR: stats: report server's user-configured weight next to effective
weight")

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: use stats desc when possible
William Dauchy [Mon, 1 Feb 2021 12:11:55 +0000 (13:11 +0100)] 
MINOR: contrib/prometheus-exporter: use stats desc when possible

It is a followup work of commit a191b77e54c26a97064cb42ab4927d4f5c95b896
("MINOR: contrib/prometheus-exporter: merge info description from
stats") but for all other stats fields; we however keep a way to
override them when needed (e.g. units, specific cases)

this is another step which will avoid duplicating work between stats.c
and prometheus.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: stats: improve max stats descriptions
William Dauchy [Mon, 1 Feb 2021 12:11:54 +0000 (13:11 +0100)] 
MINOR: stats: improve max stats descriptions

In order to unify prometheus and stats description, we need to remove
some field reference which are specific to stats implementation:
- `scur` in max current sessions (also reword current session)
- `rate` in max sessions
- `req_rate` in max requests
- `conn_rate` in max connections

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: stats: improve pending connections description
William Dauchy [Mon, 1 Feb 2021 12:11:53 +0000 (13:11 +0100)] 
MINOR: stats: improve pending connections description

In order to unify prometheus and stats description, we need to clarify
the description for pending connections.
- remove the BE reference in counters struct, as it is also used in
  servers
- remove reference of `qcur` field in description as it is specific to
  stats implemention
- try to reword cur and max pending connections description

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: improve service status description field
William Dauchy [Mon, 1 Feb 2021 12:11:52 +0000 (13:11 +0100)] 
MINOR: contrib/prometheus-exporter: improve service status description field

Since we changed the behaviour of this metric, improve the description
to better explain what is the meaning of the new gauge value; it also
reflects the description we did for health check status.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMAJOR: contrib/prometheus-exporter: move health check status to labels
William Dauchy [Mon, 1 Feb 2021 12:11:51 +0000 (13:11 +0100)] 
MAJOR: contrib/prometheus-exporter: move health check status to labels

this patch is a breaking change between v2.3 and v2.4: we move from
using gauge value for health check states to labels values. The diff is
quite small thanks to the preparation work from Christopher to allow
more flexibility in labels, see commit
5a2f938732126f43bbf4cea5482c01552b0d0314 ("MEDIUM:
contrib/prometheus-exporter: Use dynamic labels instead of static ones")

this is a follow up of commit c6464591a365bfcf509b322bdaa4d608c9395d75
("MAJOR: contrib/prometheus-exporter: move ftd/bkd/srv states to
labels"). The main goal being to be better aligned with prometheus use
cases in terms of queries. More specifically to health checks, Pierre C.
mentioned the possible quirks he had to put in place in order to make
use of those metrics through prometheus:

  <aggregator_function> by(proxy, check_status) (count_values by(proxy,
  instance) ("check_status", haproxy_server_check_status))

I am perfectly aware this introduces a lot more metrics but I don't see
how we can improve the usability without it. The main issue remains in
the cardinality of the states which are > 20. Prometheus recommends to
stay below a cardinality of 10 for a given metric but I consider our
case very specific, because highly linked to the level of precision
haproxy exposes.

Even before this patch I saw several large production setup (a few
hundreds of MB in output) which are making use of the scope parameter to
simply ignore the server metrics, so that the scrapping can be faster,
and memory consumed on client side not too high. So I believe we should
eventually continue in that direction and offer more granularity of
filtering of the output. That being said it is already possible to
filter out the data on prometheus client side.

this is related to github issue #1029

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: checks: Add function to get the result code corresponding to a status
Christopher Faulet [Mon, 1 Feb 2021 12:11:50 +0000 (13:11 +0100)] 
MINOR: checks: Add function to get the result code corresponding to a status

The function get_check_status_result() can now be used to get the result
code (CHK_RES_*) corresponding to a check status (HCHK_STATUS_*). It will be
used by the Prometheus exporter when reporting the check status of a server.

4 years agoREGTESTS: set_ssl_server_cert: cleanup the SSL caching option
William Lallemand [Mon, 1 Feb 2021 13:57:31 +0000 (14:57 +0100)] 
REGTESTS: set_ssl_server_cert: cleanup the SSL caching option

Replace the tune.ssl.cachesize 0 and the no-tls-tickets by a
no-ssl-reuse option on the server line.

4 years agoREGTESTS: set_ssl_server_cert.vtc: remove SSL caching and set as working
William Lallemand [Mon, 1 Feb 2021 13:37:36 +0000 (14:37 +0100)] 
REGTESTS: set_ssl_server_cert.vtc: remove SSL caching and set as working

In a previous commit this test was disabled because I though the
feature was broken, but in fact this is the test which is broken.

Indeed the connection between the server and the client was not
renegociated and was using the SSL cache or a ticket. To be work
correctly these 2 features must be disabled or a new connection must be
established after the ticket timeout, which is too long for a regtest.

Also a "nbthread 1" was added as it was easier to reproduce the problem
with it.

4 years agoBUG/MINOR: activity: take care of late wakeups in "show tasks"
Willy Tarreau [Fri, 29 Jan 2021 14:04:16 +0000 (15:04 +0100)] 
BUG/MINOR: activity: take care of late wakeups in "show tasks"

During the call to thread_isolate(), some other threads might have
performed some task_wakeup() which will have a call date past the
one we retrieved. It could be avoided by taking the current date
once we're alone but this would significantly affect the latency
measurements by adding the isolation time. Instead we're now only
accounting positive times, so that late wakeups normally appear
with a zero latency.

No backport is needed, this is 2.4.

4 years agoMINOR: listener: export manage_global_listener_queue()
Willy Tarreau [Fri, 29 Jan 2021 13:29:06 +0000 (14:29 +0100)] 
MINOR: listener: export manage_global_listener_queue()

This one pops up in tasks lists when running against a saturated
listener.

4 years agoMEDIUM: contrib/prometheus-exporter: Use dynamic labels instead of static ones
Christopher Faulet [Thu, 28 Jan 2021 10:24:17 +0000 (11:24 +0100)] 
MEDIUM: contrib/prometheus-exporter: Use dynamic labels instead of static ones

Instead of using static labels for metrics, we now use an array of labels,
filled for each metrics if necessary and passed to the dump function. This
way, it is easier to extend the promex service. For now, there are at most 8
labels per metrics. This limit may be raised by changing PROMEX_MAX_LABELS
value. And to ease labels addition, a label is defined as a key/value
pair. The formatting is handled by the dump function.

For the proxies and servers, the first entry of the array is always the
proxy name. In addition, for the servers, the second entry is always the
server name.

4 years agoMAJOR: contrib/prometheus-exporter: move ftd/bkd/srv states to labels
William Dauchy [Wed, 27 Jan 2021 21:40:17 +0000 (22:40 +0100)] 
MAJOR: contrib/prometheus-exporter: move ftd/bkd/srv states to labels

this patch is a breaking change between v2.3 and v2.4: we move from
using gauge value for frontend/backend/servers states to labels values.

the main motivation being I realised it is very difficult to make use of
it without hard coded quirks on prometheus client side; especially
because the main use is often to group by state, which is harder when
the state is the value of the metric.

in order to achieve that we iterate on the status metric to generate
labels, and so as many metrics.

this is the first step to resolve github issue #1029
A second step should address health check states.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: declare states for objects
William Dauchy [Wed, 27 Jan 2021 21:40:16 +0000 (22:40 +0100)] 
MINOR: contrib/prometheus-exporter: declare states for objects

in preparation to change state gauge values as labels, declare them as
enum associated with the string definition

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: mux-h2: Slightly improve request HEADERS frames sending
Christopher Faulet [Fri, 29 Jan 2021 10:49:16 +0000 (11:49 +0100)] 
MINOR: mux-h2: Slightly improve request HEADERS frames sending

In h2s_bck_make_req_headers() function, in the loop on the HTX blocks, the
most common blocks, the headers, are now handled in first, before the
start-line. The same change was already performed on the response HEADERS
frames. Thus the code is more consistent now.

4 years agoMINOR: mux-h2: Don't tests the start-line when sending HEADERS frame
Christopher Faulet [Fri, 29 Jan 2021 10:39:43 +0000 (11:39 +0100)] 
MINOR: mux-h2: Don't tests the start-line when sending HEADERS frame

When a HEADERS frame is sent, it is always when an HTX start-line block is
found. Thus, in h2s_bck_make_req_headers() and h2s_frt_make_resp_headers()
functions, it is useless to tests the start-line. Instead of being too
defensive, we use BUG_ON() now because it must not happen and must be
handled as a bug.

This patch should fix the issue #1086.

4 years agoMINOR: ssl-sample: Don't check if argument list is set in sample fetches
Christopher Faulet [Fri, 29 Jan 2021 10:30:37 +0000 (11:30 +0100)] 
MINOR: ssl-sample: Don't check if argument list is set in sample fetches

The list is always defined by definition. Thus there is no reason to test
it.

4 years agoMINOR: sample: Don't check if argument list is set in sample fetches
Christopher Faulet [Fri, 29 Jan 2021 10:29:28 +0000 (11:29 +0100)] 
MINOR: sample: Don't check if argument list is set in sample fetches

The list is always defined by definition. Thus there is no reason to test
it.

4 years agoMINOR: http-conv: Don't check if argument list is set in sample converters
Christopher Faulet [Fri, 29 Jan 2021 10:25:02 +0000 (11:25 +0100)] 
MINOR: http-conv: Don't check if argument list is set in sample converters

The list is always defined by definition. Thus there is no reason to test
it.

4 years agoMINOR: http-fetch: Don't check if argument list is set in sample fetches
Christopher Faulet [Fri, 29 Jan 2021 10:22:15 +0000 (11:22 +0100)] 
MINOR: http-fetch: Don't check if argument list is set in sample fetches

The list is always defined by definition. Thus there is no reason to test
it. There is also plenty of checks on arguments types while it is already
validated during the configuration parsing. But one thing at a time.

This patch should fix the issue #1087.

4 years agoBUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list
Christopher Faulet [Fri, 29 Jan 2021 09:27:47 +0000 (10:27 +0100)] 
BUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list

The sample fetch functions must always be called with a valid argument
list. When called by hand, if there is no argument to pass, empty_arg_list must
be used.

In the stick-table code, there are some calls to smp_fetch_src() with NULL as
argument list. It is changed to use empty_arg_list instead. It is not really a
bug because smp_fetch_src() does not use the argument list. But it is an API
bug.

This patch may be backported to all stable branches as a cleanup.

4 years agoMINOR: mux-h1: Remove first useless test on count in h1_process_output()
Christopher Faulet [Fri, 29 Jan 2021 09:22:28 +0000 (10:22 +0100)] 
MINOR: mux-h1: Remove first useless test on count in h1_process_output()

h1_process_output() function is never called with no data to send (count ==
0). Thus, the first test on count, at the beginning of the function is
useless and may be removed. This way, by reading the code, it is obvious the
<chn_htx> variable is always defined.

This patch should fix the issue #1085.

4 years agoMINOR: stick-tables: export process_table_expire()
Willy Tarreau [Fri, 29 Jan 2021 11:39:32 +0000 (12:39 +0100)] 
MINOR: stick-tables: export process_table_expire()

This handler can take quite some time as it deletes a large number of
entries under a lock, let's export it so that it's immediately visible
in "show profiling".

4 years agoMINOR: peers: export process_peer_sync() to improve traces
Willy Tarreau [Fri, 29 Jan 2021 11:38:42 +0000 (12:38 +0100)] 
MINOR: peers: export process_peer_sync() to improve traces

This one will probably pop up from time to time in "show profiling",
better have it resolve.

4 years agoMINOR: checks: export a few functions that appear often in trace dumps
Willy Tarreau [Fri, 29 Jan 2021 11:35:24 +0000 (12:35 +0100)] 
MINOR: checks: export a few functions that appear often in trace dumps

The check I/O handler, process_chk_conn and server_warmup are often
present in complex backtraces as they're impacted by locking or I/O
issues. Let's export them so that they resolve cleanly.

4 years agoMINOR: muxes: export the timeout and shutr task handlers
Willy Tarreau [Fri, 29 Jan 2021 11:33:46 +0000 (12:33 +0100)] 
MINOR: muxes: export the timeout and shutr task handlers

These ones appear often in "show tasks" so it's handy to make them
resolve.

4 years agoMINOR: session: export session_expire_embryonic()
Willy Tarreau [Fri, 29 Jan 2021 11:27:57 +0000 (12:27 +0100)] 
MINOR: session: export session_expire_embryonic()

This is only to make it resolve nicely in "show tasks".

4 years agoMINOR: listener: export accept_queue_process
Willy Tarreau [Fri, 29 Jan 2021 11:25:23 +0000 (12:25 +0100)] 
MINOR: listener: export accept_queue_process

This is only to make it resolve in "show tasks".

4 years agoMINOR: activity: add a new "show tasks" command to list currently active tasks
Willy Tarreau [Fri, 29 Jan 2021 10:32:55 +0000 (11:32 +0100)] 
MINOR: activity: add a new "show tasks" command to list currently active tasks

This finally adds the long-awaited solution to inspect the run queues
and figure what is eating the CPU or causing latencies. We can even see
the experienced latencies when profiling is enabled. Example on a
saturated process:

> show tasks
Running tasks: 14983 (4 threads)
  function                     places     %    lat_tot   lat_avg
  process_stream                 4948   33.0   5.840m    70.82ms
  h1_io_cb                       2535   16.9      -         -
  main+0x9e670                   2508   16.7   2.930m    70.10ms
  ssl_sock_io_cb                 2499   16.6      -         -
  si_cs_io_cb                    2493   16.6      -         -

4 years agoMINOR: activity: flush scheduler stats on "set profiling tasks on"
Willy Tarreau [Fri, 29 Jan 2021 10:56:21 +0000 (11:56 +0100)] 
MINOR: activity: flush scheduler stats on "set profiling tasks on"

If a user enables profiling by hand, it makes sense to reset the stats
counters to provide fresh new measurements. Therefore it's worth using
this as the standard method to reset counters.

4 years agoMINOR: activity: also report collected tasks stats in "show profiling"
Willy Tarreau [Thu, 28 Jan 2021 23:07:40 +0000 (00:07 +0100)] 
MINOR: activity: also report collected tasks stats in "show profiling"

"show profiling" will now dump the stats collected by the scheduler if
profiling was previously enabled. This will immediately make it obvious
what functions are responsible for others' high latencies or which ones
are suffering from others, and should help spot issues like undesired
wakeups.

Example:

Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
Tasks activity:
  function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
  si_cs_io_cb                 5569479   23.37s    4.196us      -         -
  h1_io_cb                    5558654   13.60s    2.446us      -         -
  process_stream               250841   1.476s    5.882us   3.499s    13.95us
  main+0x9e670                    198      -         -      5.526ms   27.91us
  task_run_applet                  17   1.509ms   88.77us   205.8us   12.11us
  srv_cleanup_idle_connections     12   44.51us   3.708us   25.71us   2.142us
  main+0x158c80                     9   48.72us   5.413us      -         -
  srv_cleanup_toremove_connections  5   165.1us   33.02us   123.6us   24.72us

4 years agoMEDIUM: tasks/activity: collect per-task statistics when profiling is enabled
Willy Tarreau [Thu, 28 Jan 2021 23:07:40 +0000 (00:07 +0100)] 
MEDIUM: tasks/activity: collect per-task statistics when profiling is enabled

Now when the profiling is enabled, the scheduler wlil update per-function
task-level statistics on number of calls, cpu usage and lateny, that could
later be checked using "show profiling". This will immediately make it
obvious what functions are responsible for others' high latencies or which
ones are suffering from others, and should help spot issues like undesired
wakeups. For now the stats are only collected but not reported (though they
are readable from sched_activity[] under gdb).

4 years agoMINOR: activity: declare a new structure to collect per-function activity
Willy Tarreau [Thu, 28 Jan 2021 18:19:26 +0000 (19:19 +0100)] 
MINOR: activity: declare a new structure to collect per-function activity

The new sched_activity structure will be used to collect task-level
activity based on the target function. The principle is to declare a
large enough array to make collisions rare (256 entries), and hash
the function pointer using a reduced XXH to decide where to store the
stats. On first computation an entry is definitely assigned to the
array and it's done atomically. A special entry (0) is used to store
collisions ("others"). The goal is to make it easy and inexpensive for
the scheduler code to use these to store #calls, cpu_time and lat_time
for each task.

4 years agoMINOR: activity: make profiling more manageable
Willy Tarreau [Thu, 28 Jan 2021 20:44:22 +0000 (21:44 +0100)] 
MINOR: activity: make profiling more manageable

In 2.0, commit d2d3348ac ("MINOR: activity: enable automatic profiling
turn on/off") introduced an automatic mode to enable/disable profiling.
The problem is that the automatic mode automatically changes to on/off,
which implied that the forced on/off modes aren't sticky anymore. It's
annoying when debugging because as soon as the load decreases, profiling
stops.

This makes a small change which ought to have been done first, which
consists in having two states for "auto" (auto-on, auto-off) to
distinguish them from the forced states. Setting to "auto" in the config
defaults to "auto-off" as before, and setting it on the CLI switches to
auto but keeps the current operating state.

This is simple enough to be backported to older releases if needed.

4 years agoMINOR: tools: add print_time_short() to print a condensed duration value
Willy Tarreau [Fri, 29 Jan 2021 09:47:52 +0000 (10:47 +0100)] 
MINOR: tools: add print_time_short() to print a condensed duration value

When reporting some values in debugging output we often need to have
some condensed, stable-length values. This function prints a duration
from nanosecond to years with at least 4 digits of accuracy using the
most suitable unit, always on 7 chars.

4 years agoDOC: management: fix "show resolvers" alphabetical ordering
Willy Tarreau [Fri, 29 Jan 2021 11:01:46 +0000 (12:01 +0100)] 
DOC: management: fix "show resolvers" alphabetical ordering

Not sure why it was located between "show ssl" and "show table"...
This should be backported.

4 years agoCI: Fix the coverity builds
Tim Duesterhus [Thu, 28 Jan 2021 17:58:53 +0000 (18:58 +0100)] 
CI: Fix the coverity builds

In an attempt to fix the use of DEBUG_STRICT commit
7f0f4786d1927f1450392e871480e3122796024e unfortunately broke the Coverity
builds completely.

It turns out that Coverity does not properly handle quoting within
`COVERITY_SCAN_BUILD_COMMAND`, instead breaking up single arguments at
whitespace, thus passing `-DDEBUG_USE_ABORT=1` to `make` as-is.

Fix this issue by hijacking the Makefile within the Coverity workflow. We
simply replace the default value of the `DEBUG` option with whatever values we
need. The build command now only includes the TARGET and USE_* flags, each of
which works without any spaces.

4 years agoBUG/MINOR: backend: check available list allocation for reuse
Amaury Denoyelle [Thu, 28 Jan 2021 16:33:26 +0000 (17:33 +0100)] 
BUG/MINOR: backend: check available list allocation for reuse

Do not consider reuse connection if available list is not allocated for
the target server. This will prevent a crash when using a standalone
server for an external purpose like socket_tcp/socket_ssl on hlua code.
For the idle/safe lists, they are considered allocated if
srv.max_idle_conns is not null.

Note that the hlua code is currently safe thanks to the additional
checks on proxy http mode and stream reuse policy not never. However,
this might not be sufficient for future code.

This patch should be backported in every branches containing the
following patch :
  7f68d815af356fbe1b2e1080a88b9935581c91d2 (2.4 tree)
  REORG: backend: simplify conn_backend_get

4 years agoRevert "BUG/MEDIUM: listener: do not accept connections faster than we can process...
Willy Tarreau [Thu, 28 Jan 2021 17:07:24 +0000 (18:07 +0100)] 
Revert "BUG/MEDIUM: listener: do not accept connections faster than we can process them"

This reverts commit 62e8aaa1bd5ca96089eaa88487c700c4af4617f4.

While is works extremely well to address SSL handshake floods, it prevents
establishment of new connections during regular traffic above 50-60 Gbps,
because for an unknown reason the queue seems to have ~1.7 active tasks
per connection all the time, which makes no sense as these ought to be
waiting on subscribed events. It might uncover a deeper issue but at least
for now a different solution is needed. cf issue #822.

The test is trivial to run, just start a config with tune.runqueue-depth 10
and inject on 1GB objects with more than 10 connections. Try to connect to
the stats socket, it only works once, then the listeners are not dequeued.

4 years agoREGTESTS: set_ssl_server_cert.vtc: set as broken
William Lallemand [Thu, 28 Jan 2021 17:08:36 +0000 (18:08 +0100)] 
REGTESTS: set_ssl_server_cert.vtc: set as broken

It looks like this test is broken with a low nbthread value (1 for
example). Disable this test in the CI until the problem is solved.

4 years agoBUG/MEDIUM: listener: do not accept connections faster than we can process them
Willy Tarreau [Wed, 27 Jan 2021 16:22:29 +0000 (17:22 +0100)] 
BUG/MEDIUM: listener: do not accept connections faster than we can process them

In github issue #822, user @ngaugler reported some performance problems when
dealing with many concurrent SSL connections on restarts, after migrating
from 1.6 to 2.2, indicating a long time required to re-establish connections.

The Run_queue metric in the traces showed an abnormally high number of tasks
in the run queue, likely indicating we were accepting faster than we could
process. And this is indeed one of the differences between 1.6 and 2.2, the
accept I/O loop and the TLS handshakes are totally independent, so much that
they can even run on different threads. In 1.6 the SSL handshake was handled
almost immediately after the accept(), so this was limiting the input rate.
With large maxconn values, as long as there are incoming connections, new
I/Os are scheduled and many of them pass before the handshake, being tagged
for low latency processing.

The result is that handshakes get postponed, and are further postponed as
new connections are accepted. When they are finally able to be processed,
some of them fail as the client is gone, and the client had already queued
new ones. This causes an excess number of apparent connections and total
number of handshakes to be processed, just because we were accepting
connections on a temporarily saturated machine.

The solution is to temporarily pause new incoming connections when the
load already indicates that more tasks are already queued than will be
handled in a poll loop. The difficulty with this usually is to be able
to come back to re-enable the operation, but given that the metric is
the run queue, we just have to queue the global_listener_queue task so
that it gets picked by any thread once the run queues get flushed.

Before this patch, injecting with SSL reneg with 10000 concurrent
connections resulted in 350k tasks in the run queue, and a majority of
handshake timeouts noticed by the client. With the patch, the run queue
fluctuates between 1-3x runqueue-depth, the process is constantly busy, the
accept rate is maximized and clients observe no error anymore.

It would be desirable to backport this patch to 2.3 and 2.2 after some more
testing, provided the accept loop there is compatible.

4 years agoMINOR: h1: Raise the chunk size limit up to (2^52 - 1)
Christopher Faulet [Wed, 27 Jan 2021 14:17:13 +0000 (15:17 +0100)] 
MINOR: h1: Raise the chunk size limit up to (2^52 - 1)

The allowed chunk size was historically limited to 2GB to avoid risk of
overflow. This restriction is no longer necessary because the chunk size is
immediately stored into a 64bits integer after the parsing. Thus, it is now
possible to raise this limit. However to never fed possibly bogus values
from languages that use floats for their integers, we don't get more than 13
hexa-digit (2^52 - 1). 4 petabytes is probably enough !

This patch should fix the issue #1065. It may be backported as far as
2.1. For the 2.0, the legacy HTTP part must be reviewed. But there is
honestely no reason to do so.

4 years agoMINOR: mux-fcgi/trace: add traces at level ERROR for all kind of errors
Christopher Faulet [Wed, 27 Jan 2021 11:06:54 +0000 (12:06 +0100)] 
MINOR: mux-fcgi/trace: add traces at level ERROR for all kind of errors

A number of traces could be added or changed to report errors with
TRACE_ERROR. The goal is to be able to enable error tracing only to detect
anomalies.

4 years agoMINOR: mux-h1/trace: add traces at level ERROR for all kind of errors
Christopher Faulet [Wed, 27 Jan 2021 10:27:50 +0000 (11:27 +0100)] 
MINOR: mux-h1/trace: add traces at level ERROR for all kind of errors

A number of traces could be added or changed to report errors with
TRACE_ERROR. The goal is to be able to enable error tracing only to detect
anomalies.

4 years agoREGTEST: Don't use the websocket to validate http-check
Christopher Faulet [Tue, 5 Jan 2021 14:42:51 +0000 (15:42 +0100)] 
REGTEST: Don't use the websocket to validate http-check

Now, some conformance tests are performed when an HTTP connection is
upgraded to websocket. This make the http-check-send.vtc script failed for
the backend <be6_ws>. Because the purpose of this health-check is to pass a
"Connection: Upgrade" header on an http-check send rule, we may use a dummy
protocal instead.

4 years agoREGTESTS: Fix required versions for several scripts
Christopher Faulet [Tue, 15 Dec 2020 16:13:39 +0000 (17:13 +0100)] 
REGTESTS: Fix required versions for several scripts

The following scripts require HAProxy 2.4 :

 * cache/caching_rules.vtc
 * cache/post_on_entry.vtc
 * cache/vary.vtc
 * checks/1be_40srv_odd_health_checks.vtc
 * checks/40be_2srv_odd_health_checks.vtc
 * checks/4be_1srv_health_checks.vtc
 * converter/fix.vtc
 * converter/mqtt.vtc
 * http-messaging/protocol_upgrade.vtc
 * http-messaging/websocket.vtc
 * http-set-timeout/set_timeout.vtc
 * log/log_uri.vtc

However it may change is features are backported.

4 years agoMINOR: vtc: add websocket test
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:12 +0000 (17:53 +0100)] 
MINOR: vtc: add websocket test

Test the conformance of websocket rfc6455 in haproxy. In particular, if
a missing key is detected on a h1 message, haproxy must close the
connection.

Note that the case h2 client/h1 srv is not tested as I did not find a
way to calculate the key on the server side.

4 years agoMINOR: vtc: add test for h1/h2 protocol upgrade translation
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:11 +0000 (17:53 +0100)] 
MINOR: vtc: add test for h1/h2 protocol upgrade translation

This test send HTTP/1.1 Get+Upgrade or HTTP/2 Extended Connect through a
haproxy instance.

4 years agoMEDIUM: h2: send connect protocol h2 settings
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:10 +0000 (17:53 +0100)] 
MEDIUM: h2: send connect protocol h2 settings

In order to announce support for the Extended CONNECT h2 method by
haproxy, always send the ENABLE_CONNECT_PROTOCOL h2 settings. This new
setting has been described in the rfc 8441.

After receiving ENABLE_CONNECT_PROTOCOL, the client is free to use the
Extended CONNECT h2 method. This can notably be useful for the support
of websocket handshake on http/2.

4 years agoMEDIUM: h2: parse Extended CONNECT request to htx
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:09 +0000 (17:53 +0100)] 
MEDIUM: h2: parse Extended CONNECT request to htx

Support for the rfc 8441 Bootstraping WebSockets with HTTP/2

Convert an Extended CONNECT HTTP/2 request into a htx representation.
The htx message uses the GET method with an Upgrade header field to be
fully compatible with the equivalent HTTP/1.1 Upgrade mechanism.

The Extended CONNECT is of the following form :

:method = CONNECT
:protocol = websocket
:scheme = https
:path = /chat
:authority = server.example.com

The new pseudo-header :protocol has been defined and is used to identify
an Extended CONNECT method. Contrary to standard CONNECT, Extended
CONNECT must have :scheme, :path and :authority defined.

4 years agoMEDIUM: mux_h2: generate Extended CONNECT response
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:08 +0000 (17:53 +0100)] 
MEDIUM: mux_h2: generate Extended CONNECT response

Support for the rfc 8441 Bootstraping WebSockets with HTTP/2

Convert a 101 htx response message to a 200 HTTP/2 response.

4 years agoMEDIUM: h1: add a WebSocket key on handshake if needed
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:07 +0000 (17:53 +0100)] 
MEDIUM: h1: add a WebSocket key on handshake if needed

Add the header Sec-Websocket-Key when generating a h1 handshake websocket
without this header. This is the case when doing h2-h1 conversion.

The key is randomly generated and base64 encoded. It is stored on the session
side to be able to verify response key and reject it if not valid.

4 years agoMEDIUM: mux_h2: generate Extended CONNECT from htx upgrade
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:06 +0000 (17:53 +0100)] 
MEDIUM: mux_h2: generate Extended CONNECT from htx upgrade

Support for the rfc 8441 Bootstraping WebSockets with HTTP/2

Generate an HTTP/2 Extended CONNECT request from a htx Upgrade message.
This conversion is done when seeing the header Connection: Upgrade. A
CONNECT request is written with the :protocol pseudo-header set from the
Upgrade htx header value.

The protocol is saved in the h2s structure. This is needed on the
response side because the protocol is not present on HTTP/2 response but
is needed if the client side is using HTTP/1.1 with 101 status code.

4 years agoMEDIUM: h2: parse Extended CONNECT reponse to htx
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:05 +0000 (17:53 +0100)] 
MEDIUM: h2: parse Extended CONNECT reponse to htx

Support for the rfc 8441 Bootstraping WebSockets with HTTP/2

Convert a 200 status reply from an Extended CONNECT request into a htx
representation. The htx message is set to 101 status code to be fully
compatible with the equivalent HTTP/1.1 Upgrade mechanism.

This conversion is only done if the stream flags H2_SF_EXT_CONNECT_SENT
has been set. This is true if an Extended CONNECT request has already
been seen on the stream.

Besides the 101 status, the additional headers Connection/Upgrade are
added to the htx message. The protocol is set from the value stored in
h2s. Typically it will be extracted from the client request. This is
only used if the client is using h1 as only the HTTP/1.1 101 Response
contains the Upgrade header.

4 years agoMINOR: mux_h2: define H2_SF_EXT_CONNECT_SENT stream flag
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:04 +0000 (17:53 +0100)] 
MINOR: mux_h2: define H2_SF_EXT_CONNECT_SENT stream flag

This flag is used to signal that an Extended CONNECT has been sent by
the server mux on the current stream.
This will allow to convert the response to a 101 htx status message.

4 years agoMEDIUM: h1: generate WebSocket key on response if needed
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:03 +0000 (17:53 +0100)] 
MEDIUM: h1: generate WebSocket key on response if needed

Add the Sec-Websocket-Accept header on a websocket handshake response.
This header may be missing if a h2 server is used with a h1 client.

The response key is calculated following the rfc6455. For this, the
handshake request key must be stored in the h1 session, as a new field
name ws_key. Note that this is only done if the message has been
prealably identified as a Websocket handshake request.

4 years agoMINOR: h1: reject websocket handshake if missing key
Amaury Denoyelle [Fri, 11 Dec 2020 16:53:02 +0000 (17:53 +0100)] 
MINOR: h1: reject websocket handshake if missing key

If a request is identified as a WebSocket handshake, it must contains a
websocket key header or else it can be reject, following the rfc6455.

A new flag H1_MF_UPG_WEBSOCKET is set on such messages.  For the request
te be identified as a WebSocket handshake, it must contains the headers:
  Connection: upgrade
  Upgrade: websocket

This commit is a compagnon of
"MEDIUM: h1: generate WebSocket key on response if needed" and
"MEDIUM: h1: add a WebSocket key on handshake if needed".

Indeed, it ensures that a WebSocket key is added only from a http/2 side
and not for a http/1 bogus peer.

4 years agoMEDIUM: http-ana: Deal with L7 retries in HTTP analysers
Christopher Faulet [Mon, 12 Oct 2020 13:18:50 +0000 (15:18 +0200)] 
MEDIUM: http-ana: Deal with L7 retries in HTTP analysers

The code dealing with the copy of requests in the L7-buffer and the
retransmits during L7 retries has been moved in the HTTP analysers. The copy
is now performed in the REQ_HTTP_XFER_BODY analyser and the L7 retries is
performed in the RES_WAIT_HTTP analyser. This way, si_cs_recv() and
si_cs_send() don't care of it anymore. It is much more natural to deal with
L7 retry in HTTP analysers.

4 years agoMEDIUM: mux-h2: Don't emit DATA frame for bodyless responses
Christopher Faulet [Wed, 2 Dec 2020 14:17:31 +0000 (15:17 +0100)] 
MEDIUM: mux-h2: Don't emit DATA frame for bodyless responses

Some responses must not contain data. Reponses to HEAD requests and 204/304
responses. But there is no warranty that this will be really respected by
the senders or even if it is possible. For instance, the method may be
rewritten by an http-request rule (HEAD->GET). Thus, it is not really
possible to always strip these data from the response at the receive
stage. And the response may be emitted by an applet or an internal service
not strictly following the spec. All that to say that we may be prepared to
handle payload for bodyless responses on the sending path.

In addition, unlike the HTTP/1, it is not really clear that the trailers is
part of the payload or not. Thus, some clients may expect to have the
trailers, if any, in the response to a HEAD request. For instance, the GRPC
status is placed in a trailer and clients rely on it. But what happens for
204 responses then.  Read the following thread for details :

  https://lists.w3.org/Archives/Public/ietf-http-wg/2020OctDec/0040.html

So, thanks to previous patches, it is now possible to know on the sending
path if a response must be bodyless or not. So, for such responses, no DATA
frame is emitted, except eventually the last empty one carring the ES
flag. However, the TRAILERS frames are still emitted. The h2s_skip_data()
function is added to take care to remove HTX DATA blocks without emitting
any DATA frame expect the last one, if there is no trailers.