]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoMAJOR: htx: Remove the EOM block type and use HTX_FL_EOM instead
Christopher Faulet [Wed, 2 Dec 2020 18:12:22 +0000 (19:12 +0100)] 
MAJOR: htx: Remove the EOM block type and use HTX_FL_EOM instead

The EOM block may be removed. The HTX_FL_EOM flags is enough. Most of time,
to know if the end of the message is reached, we just need to have an empty
HTX message with HTX_FL_EOM flag set. It may also be detected when the last
block of a message with HTX_FL_EOM flag is manipulated.

Removing EOM blocks simplifies the HTX message filling. Indeed, there is no
more edge problems when the message ends but there is no more space to write
the EOM block. However, some part are more tricky. Especially the
compression filter or the FCGI mux. The compression filter must finish the
compression on the last DATA block. Before it was performed on the EOM
block, an extra DATA block with the checksum was added. Now, we must detect
the last DATA block to be sure to finish the compression. The FCGI mux on
its part must be sure to reserve the space for the empty STDIN record on the
last DATA block while this record was inserted on the EOM block.

The H2 multiplexer is probably the part that benefits the most from this
change. Indeed, it is now fairly easier to known when to set the ES flag.

The HTX documentaion has been updated accordingly.

4 years agoMINOR: htx: Add a function to know if a block is the only one in a message
Christopher Faulet [Wed, 2 Dec 2020 16:40:54 +0000 (17:40 +0100)] 
MINOR: htx: Add a function to know if a block is the only one in a message

The htx_is_unique_blk() function may now be used to know if a block is the
only one in an HTX message, excluding all unused blocks. Note the purpose of
this function is not to know if a block is the last one of an HTTP message.
This means no more data part from the message are expected, except tunneled
data. It only says if a block is alone in an HTX message.

4 years agoREGTESTS: Don't run http_msg_full_on_eom script on the 2.4 anymore
Christopher Faulet [Tue, 15 Dec 2020 13:53:03 +0000 (14:53 +0100)] 
REGTESTS: Don't run http_msg_full_on_eom script on the 2.4 anymore

The EOM block will be removed on the 2.4, thus this script will be broken on
this version. Now it is skipped for this version. It remains valid for 2.3
and 2.2.

4 years agoMINOR: htx: Rename HTX_FL_EOI flag into HTX_FL_EOM
Christopher Faulet [Fri, 20 Nov 2020 16:43:16 +0000 (17:43 +0100)] 
MINOR: htx: Rename HTX_FL_EOI flag into HTX_FL_EOM

The HTX_FL_EOI flag is not well named. For now, it is not very used. But
that will change. It will replace the EOM block. Thus, it is renamed.

4 years agoBUG/MAJOR: mux-h1/mux-h2/htx: Fix HTTP tunnel management at the mux level
Christopher Faulet [Fri, 22 Jan 2021 14:28:03 +0000 (15:28 +0100)] 
BUG/MAJOR: mux-h1/mux-h2/htx: Fix HTTP tunnel management at the mux level

Tunnel management between the H1 and H2 multiplexers is a bit blurred. And
the HTX is not enough well defined on this point to make things clear. In
fact, Establishing a tunnel between an H2 client and an H1 server, or the
opposite is buggy because the both multiplexers don't handle the EOM block
the same way when a tunnel is established. In fact, the H2 multiplexer is
pretty strict and add an END_STREAM flag when an EOM block is found, while
the H1 multiplexer is more flexible.

The purpose of this patch is to make the EOM block usage pretty clear and to
fix the HTTP multiplexers to really handle HTTP tunnels in the right
way. Now, an EOM block is used to mark the end of an HTTP message,
semantically speaking. That means it may be followed by tunneled data. Thus,
CONNECT requests are now finished by an EOM block, just after the EOH block.

On the H1 multiplexer side, a tunnel is now only established on the response
path. So a CONNECT request remains in a DONE state waiting for the 2xx
response. On the H2 multiplexer side, a flag is used to know an HTTP tunnel
is requested, to not immediately add the END_STREAM flag on the EOM block.

All these changes are sensitives and not backportable because of recent
changes. The same problem exists on earlier versions and should be
addressed. But it will only be possible with a specific patchset.

This patch relies on the following ones :

  * MEDIUM: mux-h1: Properly handle tunnel establishments and aborts
  * MEDIUM: mux-h2: Close streams when processing data for an aborted tunnel
  * MEDIUM: mux-h2: Block client data on server side waiting tunnel establishment
  * MINOR: mux-h2: Add 2 flags to help to properly handle tunnel mode
  * MINOR: mux-h1: Split H1C_F_WAIT_OPPOSITE flag to separate input/output sides
  * MINOR: mux-h1/mux-fcgi: Don't set TUNNEL mode if payload length is unknown

4 years agoMEDIUM: mux-h1: Properly handle tunnel establishments and aborts
Christopher Faulet [Fri, 22 Jan 2021 14:12:30 +0000 (15:12 +0100)] 
MEDIUM: mux-h1: Properly handle tunnel establishments and aborts

In the same way than the H2 mux, we now bloc data sending on the server side
if a tunnel is not fully established. In addition, if some data are still
pending for a aborted tunnel, an error is triggered and the server
connection is closed.

To do so, we rely on the H1C_F_WAIT_INPUT flag to bloc the output
processing. This patch contributes to fix the tunnel mode between the H1 and
the H2 muxes.

4 years agoMEDIUM: mux-h2: Close streams when processing data for an aborted tunnel
Christopher Faulet [Fri, 22 Jan 2021 11:13:15 +0000 (12:13 +0100)] 
MEDIUM: mux-h2: Close streams when processing data for an aborted tunnel

In the previous patch ("MEDIUM: mux-h2: Block client data on server side
waiting tunnel establishment"), we added a way to block client data for not
fully established tunnel on the server side. This one closes the stream with
an ERR_CANCEL erorr if there are some pending tunneled data while the tunnel
was aborted. This may happen on the client side if a non-empty DATA frame or
an empty DATA frame without the ES flag is received. This may also happen on
the server side if there is a DATA htx block. However in this last case, we
first wait the response is fully forwarded.

This patch contributes to fix the tunnel mode between the H1 and the H2
muxes.

4 years agoMEDIUM: mux-h2: Block client data on server side waiting tunnel establishment
Christopher Faulet [Fri, 22 Jan 2021 10:59:07 +0000 (11:59 +0100)] 
MEDIUM: mux-h2: Block client data on server side waiting tunnel establishment

On the server side, when a tunnel is not fully established, we must block
tunneled data, waiting for the server response. It is mandatory because the
server may refuse the tunnel. This happens when a DATA htx block is
processed in tunnel mode (H2_SF_BODY_TUNNEL flag set) but before the
response HEADERS frame is received (H2_SF_HEADERS_RCVD flag no set). In this
case, the H2_SF_BLK_MBUSY flag is set to mark the stream as busy. This flag
is removed when the tunnel is fully established or aborted.

This patch contributes to fix the tunnel mode between the H1 and the H2
muxes.

4 years agoMINOR: mux-h2: Add 2 flags to help to properly handle tunnel mode
Christopher Faulet [Fri, 22 Jan 2021 10:46:30 +0000 (11:46 +0100)] 
MINOR: mux-h2: Add 2 flags to help to properly handle tunnel mode

H2_SF_BODY_TUNNEL and H2_SF_TUNNEL_ABRT flags are added to properly handle
the tunnel mode in the H2 mux. The first one is used to detect tunnel
establishment or fully established tunnel. The second one is used to abort a
tunnel attempt. It is the first commit having as a goal to fix tunnel
establishment between H1 and H2 muxes.

There is a subtlety in h2_rcv_buf(). CS_FL_EOS flag is added on the
conn-stream when ES is received on a tunneled stream. It really reflects the
conn-stream state and is mandatory for next commits.

4 years agoMINOR: mux-h1: Split H1C_F_WAIT_OPPOSITE flag to separate input/output sides
Christopher Faulet [Wed, 13 Jan 2021 17:47:57 +0000 (18:47 +0100)] 
MINOR: mux-h1: Split H1C_F_WAIT_OPPOSITE flag to separate input/output sides

The H1C_F_WAIT_OPPOSITE flag is now splitted in 2 flags, H1C_F_WAIT_INPUT
and H1C_F_WAIT_OUTPUT, depending on the side is waiting. The change is a
prerequisite to fix the tunnel mode management in HTTP muxes.

H1C_F_WAIT_INPUT must be used to bloc the output side and to wait for an
event from the input side. H1C_F_WAIT_OUTPUT does the opposite. It bloc the
input side and wait for an event from the output side.

4 years agoMINOR: mux-h1/mux-fcgi: Don't set TUNNEL mode if payload length is unknown
Christopher Faulet [Tue, 8 Dec 2020 09:38:22 +0000 (10:38 +0100)] 
MINOR: mux-h1/mux-fcgi: Don't set TUNNEL mode if payload length is unknown

Responses with no C-L and T-E headers are no longer switched in TUNNEL mode
and remains in DATA mode instead. The H1 and FCGI muxes are updated
accordingly. This change reflects the real message state. It is not a true
tunnel. Data received are still part of the message.

It is not a bug. However, this message may be backported after some
observation period (at least as far as 2.2).

4 years agoBUG/MINOR: h2/mux-h2: Reject 101 responses with a PROTOCOL_ERROR h2s error
Christopher Faulet [Mon, 7 Dec 2020 17:24:43 +0000 (18:24 +0100)] 
BUG/MINOR: h2/mux-h2: Reject 101 responses with a PROTOCOL_ERROR h2s error

As stated in the RFC7540, section 8.1.1, the HTTP/2 removes support for the
101 informational status code. Thus a PROTOCOL_ERROR is now returned to the
server if a 101-switching-protocols response is received. Thus, the server
connection is aborted.

This patch may be backported as far as 2.0.

4 years agoMEDIUM: http-ana: Refuse invalid 101-switching-protocols responses
Christopher Faulet [Fri, 8 Jan 2021 15:02:05 +0000 (16:02 +0100)] 
MEDIUM: http-ana: Refuse invalid 101-switching-protocols responses

A 101-switching-protocols response must contain a Connection header with the
Upgrade option. And this response must only be received from a server if the
client explicitly requested a protocol upgrade. Thus, the request must also
contain a Connection header with the Upgrade option. If not, a
502-bad-gateway response is returned to the client. This way, a tunnel is
only established if both sides are agree.

It is closer to what the RFC says, but it remains a bit flexible because
there is no check on the Upgrade header itself. However, that's probably
enough to ensure a tunnel is not established when not requested.

This one is not tagged as a bug. But it may be backported, at least to
2.3. It relies on :

  * MINOR: htx/http-ana: Save info about Upgrade option in the Connection header

4 years agoMINOR: htx/http-ana: Save info about Upgrade option in the Connection header
Christopher Faulet [Fri, 8 Jan 2021 14:53:01 +0000 (15:53 +0100)] 
MINOR: htx/http-ana: Save info about Upgrade option in the Connection header

Add an HTX start-line flag and its counterpart into the HTTP message to
track the presence of the Upgrade option into the Connection header. This
way, without parsing the Connection header again, it will be easy to know if
a client asks for a protocol upgrade and if the server agrees to do so. It
will also be easy to perform some conformance checks when a
101-switching-protocols is received.

4 years agoBUG/MAJOR: mux-h1: Properly handle TCP to H1 upgrades
Christopher Faulet [Thu, 21 Jan 2021 16:50:19 +0000 (17:50 +0100)] 
BUG/MAJOR: mux-h1: Properly handle TCP to H1 upgrades

It is the second part and the most important of the fix.

Since the mux-h1 refactoring, and more specifically since the commit
c4bfa59f1 ("MAJOR: mux-h1: Create the client stream as later as possible"),
the upgrade from a TCP client connection to H1 is broken. Indeed, now the H1
mux is responsible to create the frontend conn-stream once the request
headers are fully received. But, to properly support TCP to H1 upgrades, we
must inherit from the existing conn-stream. To do so, if the conn-stream
already exists when the client H1 connection is created, we create a H1
stream in ST_ATTACHED state, but not ST_READY, and the conn-stream is
attached to it. Because the ST_READY state is not set, no data are xferred
to the data layer when h1_rcv_buf() is called and shutdowns are inhibited
except on client aborts. This way, the request is parsed the same way than
for a classical H1 connection. Once the request headers are fully received
and parsed, the data stream is upgraded and the ST_READY state is set.

A tricky case appears when an H2 upgrade is performed because the H2 preface
is matched. In this case, the conn-stream must be detached and destroyed
before switching to the H2 mux and releasing the current H1 mux. We must
also take care to detach and destroy the conn-stream when a timeout
occurres.

This patch relies on the following series of patches :

* BUG/MEDIUM: stream: Don't immediatly ack the TCP to H1 upgrades
* MEDIUM: http-ana: Do nothing in wait-for-request analyzer if not htx
* MINOR: stream: Add a function to validate TCP to H1 upgrades
* MEDIUM: mux-h1: Add ST_READY state for the H1 connections
* MINOR: mux-h1: Wake up instead of subscribe for reads after H1C creation
* MINOR: mux-h1: Try to wake up data layer first before calling its wake callback
* MINOR: stream-int: Take care of EOS in the SI wake callback function
* BUG/MINOR: stream: Don't update counters when TCP to H2 upgrades are performed

This fix is specific for 2.4. No backport needed.

4 years agoBUG/MEDIUM: stream: Don't immediatly ack the TCP to H1 upgrades
Christopher Faulet [Thu, 21 Jan 2021 16:31:04 +0000 (17:31 +0100)] 
BUG/MEDIUM: stream: Don't immediatly ack the TCP to H1 upgrades

Instead of switching the stream to HTX mode, the request channel is only
reset (the request buffer is xferred to the mux) and the SF_IGNORE flag is
set on the stream. This flag prevent any processing in case of abort. Once
the upgrade confirmed, the flag is removed, in stream_upgrade_from_cs().

It is only the first part of the fix. The next one ("BUG/MAJOR: mux-h1:
Properly handle TCP to H1 upgrades") is also required. Both rely on the
following series of patches :

* MEDIUM: http-ana: Do nothing in wait-for-request analyzer if not htx
* MINOR: stream: Add a function to validate TCP to H1 upgrades
* MEDIUM: mux-h1: Add ST_READY state for the H1 connections
* MINOR: mux-h1: Wake up instead of subscribe for reads after H1C creation
* MINOR: mux-h1: Try to wake up data layer first before calling its wake callback
* MINOR: stream-int: Take care of EOS in the SI wake callback function
* BUG/MINOR: stream: Don't update counters when TCP to H2 upgrades are performed

This fix is specific for 2.4. No backport needed.

4 years agoMEDIUM: http-ana: Do nothing in wait-for-request analyzer if not htx
Christopher Faulet [Thu, 21 Jan 2021 16:32:58 +0000 (17:32 +0100)] 
MEDIUM: http-ana: Do nothing in wait-for-request analyzer if not htx

If http_wait_for_request() analyzer is called with a non-htx stream, nothing
is performed and we return immediatly. For now, it is totally unexpected.
But it will be true during TCP to H1 upgrades, once fixed. Indeed, there
will be a transition period during these upgrades. First the mux will be
upgraded and the not the stream, and finally the stream will be upgraded by
the mux once ready. In the meantime, the stream will still be in raw
mode. Nothing will be performed in wait-for-request analyzer because it will
be the mux responsibility to handle errors.

This patch is required to fix the TCP to H1 upgrades.

4 years agoMINOR: stream: Add a function to validate TCP to H1 upgrades
Christopher Faulet [Thu, 21 Jan 2021 16:36:12 +0000 (17:36 +0100)] 
MINOR: stream: Add a function to validate TCP to H1 upgrades

TCP to H1 upgrades are buggy for now. When such upgrade is performed, a
crash is experienced. The bug is the result of the recent H1 mux
refactoring, and more specifically because of the commit c4bfa59f1 ("MAJOR:
mux-h1: Create the client stream as later as possible"). Indeed, now the H1
mux is responsible to create the frontend conn-stream once the request
headers are fully received. Thus the TCP to H1 upgrade is a problem because
the frontend conn-stream already exists.

To fix the bug, we must keep this conn-stream and the associate stream and
use it in the H1 mux. To do so, the upgrade will be performed in two
steps. First, the mux is upgraded from mux-pt to mux-h1. Then, the mux-h1
performs the stream upgrade, once the request headers are fully received and
parsed. To do so, stream_upgrade_from_cs() must be used. This function set
the SF_HTX flags to switch the stream to HTX mode, it removes the SF_IGNORE
flags and eventually it fills the request channel with some input data.

This patch is required to fix the TCP to H1 upgrades and is intimately
linked with the next commits.

4 years agoMEDIUM: mux-h1: Add ST_READY state for the H1 connections
Christopher Faulet [Thu, 21 Jan 2021 16:44:35 +0000 (17:44 +0100)] 
MEDIUM: mux-h1: Add ST_READY state for the H1 connections

An alive H1 connection may be in one of these 3 states :

  * ST_IDLE : not active and is waiting to be reused (no h1s and no cs)
  * ST_EMBRYONIC : active with a h1s but without any cs
  * ST_ATTACHED : active with a h1s and a cs

ST_IDLE and ST_ATTACHED are possible for frontend and backend
connection. ST_EMBRYONIC is only possible on the client side, when we are
waiting for the request headers. The last one is the expected state for an
active connection processing data. These states are mutually exclusives.

Now, there is a new state, ST_READY. It may only be set if ST_ATTACHED is
also set and when the CS is considered as fully active. For now, ST_READY is
set in the same time of ST_ATTACHED. But it will be used to fix TCP to H1
upgrades. Idea is to have an H1 connection in ST_ATTACHED state but not
ST_READY yet and have more or less the same behavior than an H1 connection
in ST_EMBRYONIC state. And when the upgrade is fully achieved, the ST_READY
state may be set and the data layer may be notified accordingly.

So for now, this patch should not change anything. TCP to H1 upgrades are
still buggy. But it is mandatory to make it work properly.

4 years agoMINOR: mux-h1: Wake up H1C after its creation if input buffer is not empty
Christopher Faulet [Thu, 21 Jan 2021 16:45:45 +0000 (17:45 +0100)] 
MINOR: mux-h1: Wake up H1C after its creation if input buffer is not empty

When a H1 connection is created, we now wakeup the H1C tasklet if there are
some data in the input buffer. If not we only subscribe for reads.

This patch is required to fix the TCP to H1 upgrades.

4 years agoMINOR: mux-h1: Try to wake up data layer first before calling its wake callback
Christopher Faulet [Thu, 21 Jan 2021 16:49:01 +0000 (17:49 +0100)] 
MINOR: mux-h1: Try to wake up data layer first before calling its wake callback

Instead of calling the data layer wake callback function, we now first try
to wake it up. If the data layer is subscribed for receives or for sends,
its tasklet is woken up. The wake callback function is only called as the
last chance to notify the data layer.

4 years agoMEDIUM: stream-int: Take care of EOS if the SI wake callback function
Christopher Faulet [Thu, 21 Jan 2021 15:22:01 +0000 (16:22 +0100)] 
MEDIUM: stream-int: Take care of EOS if the SI wake callback function

Because si_cs_process() is also the SI wake callback function, it may be
called from the mux layer. Thus, in such cases, it is performed outside any
I/O event and si_cs_recv() is not called. If a read0 is reported by the mux,
via the CS_FL_EOS flag, the event is not handled, because only si_cs_recv()
take care of this flag for now.

It is not a bug, because this does not happens for now. All muxes set this
flag when the data layer retrieve data (via mux->rcv_buf()). But it is safer
to be prepared to handle it from the wake callback. And in fact, it will be
useful to fix the HTTP upgrades of TCP connections (especially TCP>H1>H2
upgrades).

To be sure to not handle the same event twice, it is only handled if the
shutr is not already set on the input channel.

4 years agoREGTESTS: set_ssl_server_cert.vtc: check the sha1 from the server
William Lallemand [Thu, 28 Jan 2021 15:00:22 +0000 (16:00 +0100)] 
REGTESTS: set_ssl_server_cert.vtc: check the sha1 from the server

Check the sha1 from the server side with the sample ssl_c_sha1 sample
fetch in order to evict a possible problem with "show/set ssl cert".

4 years agoREGTESTS: set_ssl_server_cert.vtc: check the Sha1 Fingerprint
William Lallemand [Thu, 28 Jan 2021 13:59:19 +0000 (14:59 +0100)] 
REGTESTS: set_ssl_server_cert.vtc: check the Sha1 Fingerprint

Check the sha1 fingerprint once the certificate was changed with "show
ssl cert". This way the test is more reliable.

4 years agoREGTESTS: set_ssl_server_cert.vtc: remove the abort command
William Lallemand [Thu, 28 Jan 2021 13:56:31 +0000 (14:56 +0100)] 
REGTESTS: set_ssl_server_cert.vtc: remove the abort command

Temporarily remove the abort command as it seems to cause problems when
trying to do a "show ssl cert" after it.

4 years agoBUG/MEDIUM: backend: never reuse a connection for tcp mode
Amaury Denoyelle [Tue, 26 Jan 2021 16:35:46 +0000 (17:35 +0100)] 
BUG/MEDIUM: backend: never reuse a connection for tcp mode

The reuse of idle connections should only happen for a proxy with the
http mode. In case of a backend with the tcp mode, the reuse selection
and insertion in session list are skipped.

This behavior is present since commit :
MEDIUM: connection: Add private connections synchronously in session server list
It could also be further exagerated by :
MEDIUM: backend: add reused conn to sess if mux marked as HOL blocking

It can be backported up to 2.3.

4 years agoBUG/MEDIUM: session: only retrieve ready idle conn from session
Amaury Denoyelle [Tue, 26 Jan 2021 13:14:37 +0000 (14:14 +0100)] 
BUG/MEDIUM: session: only retrieve ready idle conn from session

A bug was introduced by the early insertion of idle connections at the
end of connect_server. It is possible to reuse a connection not yet
ready waiting for an handshake (for example with proxy protocol or ssl).
A wrong duplicate xprt_handshake_io_cb tasklet is thus registered as a
side-effect.

This triggers the BUG_ON statement of xprt_handshake_subscribe :
    BUG_ON(ctx->subs && ctx->subs != es);

To counter this, a check is now present in session_get_conn to only
return a connection without the flag CO_FL_WAIT_XPRT. This might cause
sometimes the creation of dedicated server connections when in theory
reuse could have been used, but probably only occurs rarely in real
condition.

This behavior is present since commit :
    MEDIUM: connection: Add private connections synchronously in session server list
It could also be further exagerated by :
    MEDIUM: backend: add reused conn to sess if mux marked as HOL blocking

It can be backported up to 2.3.

NOTE : This bug seems to be only reproducible with mode tcp, for an
unknown reason. However, reuse should never happen when not in http
mode. This improper behavior will be the subject of a dedicated patch.

This bug can easily be reproducible with the following config (a
webserver is required to accept proxy protocol on port 31080) :

    global

    defaults
      mode tcp
      timeout connect 1s
      timeout server 1s
      timeout client 1s

    listen li
      bind 0.0.0.0:4444
      server bla1 127.0.0.1:31080 check send-proxy-v2

with the inject client :
    $ inject -u 10000 -d 10 -G 127.0.0.1:4444

This should fix the github issue #1058.

4 years agoBUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()
William Lallemand [Wed, 27 Jan 2021 13:58:51 +0000 (14:58 +0100)] 
BUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()

Use chunk_inistr() for a chunk initialisation in
ssl_sock_load_sctl_from_file() instead of a manual initialisation which
was not initialising head.

Fix issue #1073.

Must be backported as far as 2.2

4 years agoCLEANUP: ssl: remove dead code in ckch_inst_new_load_srv_store()
William Lallemand [Wed, 27 Jan 2021 13:42:40 +0000 (14:42 +0100)] 
CLEANUP: ssl: remove dead code in ckch_inst_new_load_srv_store()

The new ckch_inst_new_load_srv_store() function which mimics the
ckch_inst_new_load_store() function includes some dead code which was
used only in the former function.

Fix issue #1081.

4 years agoBUG/MINOR: stats: Add a break after filling ST_F_MODE field for servers
Christopher Faulet [Wed, 27 Jan 2021 12:32:25 +0000 (13:32 +0100)] 
BUG/MINOR: stats: Add a break after filling ST_F_MODE field for servers

The previous patch was pushed too quickly (399bf72f6 "BUG/MINOR: stats:
Remove a break preventing ST_F_QCUR to be set for servers"). It was not an
extra break but a misplaced break statement. Thus, now a break statement
must be added after filling the ST_F_MODE field in stats_fill_sv_stats().

No backport needed except if the above commit is backported.

4 years agoBUG/MINOR: stats: Remove a break preventing ST_F_QCUR to be set for servers
Christopher Faulet [Wed, 27 Jan 2021 11:48:32 +0000 (12:48 +0100)] 
BUG/MINOR: stats: Remove a break preventing ST_F_QCUR to be set for servers

There is an extra break statement wrongly placed in stats_fill_sv_stats()
function, just before filling the ST_F_QCUR field. It prevents this field to
be set to the right value for servers.

No backport needed except if commit 3a9a4992 ("MEDIUM: stats: allow to
select one field in `stats_fill_sv_stats`") is backported.

4 years agoCI: Fix DEBUG_STRICT definition for Coverity
Tim Duesterhus [Tue, 26 Jan 2021 18:24:25 +0000 (19:24 +0100)] 
CI: Fix DEBUG_STRICT definition for Coverity

The DEBUG_STRICT define needs to be passed as part of `DEBUG`, not as a bare
parameter.

4 years agoBUILD: Include stdlib.h in compiler.h if DEBUG_USE_ABORT is set
Tim Duesterhus [Tue, 26 Jan 2021 18:24:24 +0000 (19:24 +0100)] 
BUILD: Include stdlib.h in compiler.h if DEBUG_USE_ABORT is set

Building with `"DEBUG=-DDEBUG_STRICT=1 -DDEBUG_USE_ABORT=1"` previously emitted the warning:

    In file included from include/haproxy/api.h:35:0,
                     from src/mux_pt.c:13:
    include/haproxy/buf.h: In function â€˜br_init’:
    include/haproxy/bug.h:42:90: warning: implicit declaration of function â€˜abort’ [-Wimplicit-function-declaration]
     #define ABORT_NOW() do { extern void ha_backtrace_to_stderr(); ha_backtrace_to_stderr(); abort(); } while (0)
                                                                                              ^
    include/haproxy/bug.h:56:21: note: in expansion of macro â€˜ABORT_NOW’
     #define CRASH_NOW() ABORT_NOW()
                         ^
    include/haproxy/bug.h:68:4: note: in expansion of macro â€˜CRASH_NOW’
        CRASH_NOW();                                           \
        ^
    include/haproxy/bug.h:62:35: note: in expansion of macro â€˜__BUG_ON’
     #define _BUG_ON(cond, file, line) __BUG_ON(cond, file, line)
                                       ^
    include/haproxy/bug.h:61:22: note: in expansion of macro â€˜_BUG_ON’
     #define BUG_ON(cond) _BUG_ON(cond, __FILE__, __LINE__)
                          ^
    include/haproxy/buf.h:875:2: note: in expansion of macro â€˜BUG_ON’
      BUG_ON(size < 2);
      ^

This patch fixes that issue. The `DEBUG_USE_ABORT` option exists for use with
static analysis tools. No backport needed.

4 years agoCLEANUP: ssl: make load_srv_{ckchs,cert} match their bind counterpart
William Lallemand [Tue, 26 Jan 2021 11:01:46 +0000 (12:01 +0100)] 
CLEANUP: ssl: make load_srv_{ckchs,cert} match their bind counterpart

This patch makes things more consistent between the bind_conf functions
and the server ones:

- ssl_sock_load_srv_ckchs() loads the SSL_CTX in the server
  (ssl_sock_load_ckchs() load the SNIs in the bind_conf)

- add the server parameter to ssl_sock_load_srv_ckchs()

- changes made to the ckch_inst are done in
  ckch_inst_new_load_srv_store()

4 years agoCLEANUP: ssl: remove SSL_CTX function parameter
William Lallemand [Tue, 26 Jan 2021 10:27:42 +0000 (11:27 +0100)] 
CLEANUP: ssl: remove SSL_CTX function parameter

Since the server SSL_CTX is now stored in the ckch_inst, it is not
needed anymore to pass an SSL_CTX to ckch_inst_new_load_srv_store() and
ssl_sock_load_srv_ckchs().

4 years agoCLEANUP: ssl/cli: rework free in cli_io_handler_commit_cert()
William Lallemand [Tue, 26 Jan 2021 09:18:57 +0000 (10:18 +0100)] 
CLEANUP: ssl/cli: rework free in cli_io_handler_commit_cert()

The new feature allowing the change of server side certificates
introduced duplicated free code. Rework the code in
cli_io_handler_commit_cert() to be more consistent.

4 years agoMINOR: ssl: Remove client_crt member of the server's ssl context
Remi Tricot-Le Breton [Mon, 25 Jan 2021 16:19:45 +0000 (17:19 +0100)] 
MINOR: ssl: Remove client_crt member of the server's ssl context

The client_crt member is not used anymore since the server's ssl context
initialization now behaves the same way as the bind lines one (using
ckch stores and instances).

4 years agoMEDIUM: ssl: Enable backend certificate hot update
Remi Tricot-Le Breton [Mon, 25 Jan 2021 16:19:44 +0000 (17:19 +0100)] 
MEDIUM: ssl: Enable backend certificate hot update

When trying to update a backend certificate, we should find a
server-side ckch instance thanks to which we can rebuild a new ssl
context and a new ckch instance that replace the previous ones in the
server structure. This way any new ssl session will be built out of the
new ssl context and the newly updated certificate.

This resolves a subpart of GitHub issue #427 (the certificate part)

4 years agoMEDIUM: ssl: Load client certificates in a ckch for backend servers
Remi Tricot-Le Breton [Mon, 25 Jan 2021 16:19:43 +0000 (17:19 +0100)] 
MEDIUM: ssl: Load client certificates in a ckch for backend servers

In order for the backend server's certificate to be hot-updatable, it
needs to fit into the implementation used for the "bind" certificates.
This patch follows the architecture implemented for the frontend
implementation and reuses its structures and general function calls
(adapted for the server side).
The ckch store logic is kept and a dedicated ckch instance is used (one
per server). The whole sni_ctx logic was not kept though because it is
not needed.
All the new functions added in this patch are basically server-side
copies of functions that already exist on the frontend side with all the
sni and bind_cond references removed.
The ckch_inst structure has a new 'is_server_instance' flag which is
used to distinguish regular instances from the server-side ones, and a
new pointer to the server's structure in case of backend instance.
Since the new server ckch instances are linked to a standard ckch_store,
a lookup in the ckch store table will succeed so the cli code used to
update bind certificates needs to be covered to manage those new server
side ckch instances.

4 years agoMINOR: ssl: Certificate chain loading refactorization
Remi Tricot-Le Breton [Mon, 25 Jan 2021 16:19:42 +0000 (17:19 +0100)] 
MINOR: ssl: Certificate chain loading refactorization

Move the certificate chain loading code into a dedicated function that
will then be useable elsewhere.

4 years agoMINOR: ssl: Server ssl context prepare function refactoring
Remi Tricot-Le Breton [Mon, 25 Jan 2021 16:19:41 +0000 (17:19 +0100)] 
MINOR: ssl: Server ssl context prepare function refactoring

Split the server's ssl context initialization into the general ssl
related initializations and the actual initialization of a single
SSL_CTX structure. This way the context's initialization will be
usable by itself from elsewhere.

4 years agoREORG: backend: simplify conn_backend_get
Amaury Denoyelle [Tue, 26 Jan 2021 13:35:26 +0000 (14:35 +0100)] 
REORG: backend: simplify conn_backend_get

Reorganize the conditions for the reuse of idle/safe connections :
- reduce code by using variable to store reuse mode and idle/safe conns
  counts
- consider that idle/safe/avail lists are properly allocated if
  max_idle_conns not null. An allocation failure prevents haproxy
  startup.

4 years agoCLEANUP: backend: remove an obsolete comment on conn_backend_get
Amaury Denoyelle [Tue, 26 Jan 2021 13:35:25 +0000 (14:35 +0100)] 
CLEANUP: backend: remove an obsolete comment on conn_backend_get

This comment was valid for haproxy 1.8 but now it is obsolete.

4 years agoCLEANUP: srv: fix comment for pool-max-conn
Amaury Denoyelle [Tue, 26 Jan 2021 13:35:24 +0000 (14:35 +0100)] 
CLEANUP: srv: fix comment for pool-max-conn

Adjust comment for the unlimited value of pool-max-conn which is -1.

4 years agoMINOR: reg-tests: add http-reuse test
Amaury Denoyelle [Tue, 26 Jan 2021 13:35:23 +0000 (14:35 +0100)] 
MINOR: reg-tests: add http-reuse test

Add a serie of 4 tests for the various http-reuse modes : never, safe,
aggressive and always.

4 years agoBUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name
Amaury Denoyelle [Tue, 26 Jan 2021 13:35:22 +0000 (14:35 +0100)] 
BUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name

Leak for parsing of option usesrc of the source keyword.

This can be backported to 1.8.

4 years agoBUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown
Christopher Faulet [Mon, 25 Jan 2021 11:02:00 +0000 (12:02 +0100)] 
BUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown

It is only a problem on the response path because the request payload length
it always known. But when a filter is registered to analyze the response
payload, the filtering may hang if the server closes just after the headers.

The root cause of the bug comes from an attempt to allow the filters to not
immediately forward the headers if necessary. A filter may choose to hold
the headers by not forwarding any bytes of the payload. For a message with
no payload but a known payload length, there is always a EOM block to
forward. Thus holding the EOM block for bodyless messages is a good way to
also hold the headers. However, messages with an unknown payload length,
there is no EOM block finishing the message, but only a SHUTR flag on the
channel to mark the end of the stream. If there is no payload when it
happens, there is no payload at all to forward. In the filters API, it is
wrongly detected as a condition to not forward the headers.

Because it is not the most used feature and not the obvious one, this patch
introduces another way to hold the message headers at the begining of the
forwarding. A filter flag is added to explicitly says the headers should be
hold. A filter may choose to set the STRM_FLT_FL_HOLD_HTTP_HDRS flag and not
forwad anything to hold the headers. This flag is removed at each call, thus
it must always be explicitly set by filters. This flag is only evaluated if
no byte has ever been forwarded because the headers are forwarded with the
first byte of the payload.

reg-tests/filters/random-forwarding.vtc reg-test is updated to also test
responses with unknown payload length (with and without payload).

This patch must be backported as far as 2.0.

4 years agoMINOR: abort() on my_unreachable() when DEBUG_USE_ABORT is set.
Tim Duesterhus [Mon, 25 Jan 2021 16:51:36 +0000 (17:51 +0100)] 
MINOR: abort() on my_unreachable() when DEBUG_USE_ABORT is set.

Hopefully this helps static analysis tools detecting that the code after that
call is unreachable.

See GitHub Issue #1075.

4 years agoMINOR: contrib/prometheus-exporter: use fill_sv_stats for server dump
William Dauchy [Mon, 25 Jan 2021 16:29:04 +0000 (17:29 +0100)] 
MINOR: contrib/prometheus-exporter: use fill_sv_stats for server dump

use `stats_fill_sv_stats` when possible to avoid duplicating code.

the following metrics have a change of behaviour:

haproxy_server_limit_sessions
haproxy_server_queue_limit
haproxy_server_check_failures_total
haproxy_server_check_up_down_total
haproxy_server_downtime_seconds_total
haproxy_server_current_throttle
haproxy_server_idle_connections_limit

depending on cases, if the limit was not configured or enabled, NaN is
returned instead. It should not be an issue for users, even better than
before as it provides more precise info.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMEDIUM: stats: allow to select one field in `stats_fill_sv_stats`
William Dauchy [Mon, 25 Jan 2021 16:29:03 +0000 (17:29 +0100)] 
MEDIUM: stats: allow to select one field in `stats_fill_sv_stats`

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
This patch follows what has already been done on frontend and backend
side.
From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the server.

A few things to note though:
- state require prior calculation, so I moved that to a sort of helper
  `stats_fill_be_stats_computestate`.
- all ST_F*TIME fields requires some minor compute, so I moved it at te
  beginning of the function under a condition.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: use fill_be_stats for backend dump
William Dauchy [Mon, 25 Jan 2021 16:29:02 +0000 (17:29 +0100)] 
MINOR: contrib/prometheus-exporter: use fill_be_stats for backend dump

use `stats_fill_be_stats` when possible to avoid duplicating code; make
use of field selector to get the needed field only.

the only difference is on `haproxy_backend_downtime_seconds_total` as
stats.c is testing `px->srv`. This behaviour is present since commit
7344f4789321ef8ce2ce17cf73dabd672f7c8c6 ("MINOR: stats: only report
backend's down time if it has servers"). The end result is a NaN instead
of a zero when no server are present.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMEDIUM: stats: allow to select one field in `stats_fill_be_stats`
William Dauchy [Mon, 25 Jan 2021 16:29:01 +0000 (17:29 +0100)] 
MEDIUM: stats: allow to select one field in `stats_fill_be_stats`

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
This patch follows what has already been done on frontend side.
From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the backend

A few things to note though:
- status and uweight field requires prior compute, so I moved that to a
  sort of helper `stats_fill_be_stats_computesrv`.
- all ST_F*TIME fields requires some minor compute, so I moved it at te
  beginning of the function under a condition.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoDOC: Improve documentation of the various hdr() fetches
Tim Duesterhus [Sat, 23 Jan 2021 16:50:21 +0000 (17:50 +0100)] 
DOC: Improve documentation of the various hdr() fetches

GitHub issue #796 notes that many administrators miss the fact that the `hdr()`
fetch (without the `f`) splits the header value at commas. This is only
mentioned at the end of a long paragraph.

This patch attempts to improve the documentation by:
- Explaning the "comma issue" as early as possible.
- Adding newlines to split the explanation into distinct sections.
- Reducing duplication by making the `res` siblings refer to their `req`
  counterparts.

This patch may be backported as long as it applies cleanly. During the
refactoring I needed to adjust several explanations for consistency and not all
of them might be available in older branches.

4 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Fri, 22 Jan 2021 21:11:59 +0000 (02:11 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 16th iteration of typo fixes

4 years agoCLEANUP: stats: improve field selection for frontend http fields
William Dauchy [Fri, 22 Jan 2021 20:09:48 +0000 (21:09 +0100)] 
CLEANUP: stats: improve field selection for frontend http fields

while working on backend/servers I realised I could have written that in
a better way and avoid one extra break. This is slightly improving
readiness.
also while being here, fix function declaration which was not 100%
accurate.

this patch does not change the behaviour of the code.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: better output of Not-a-Number
William Dauchy [Fri, 22 Jan 2021 20:09:47 +0000 (21:09 +0100)] 
MINOR: contrib/prometheus-exporter: better output of Not-a-Number

Not necessarily mandatory but I saw a few prometheus client parsing only
`NaN`. Also most librarries do output `NaN`

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoBUG/MINOR: stats: Init the metric variable when frontend stats are filled
Christopher Faulet [Mon, 25 Jan 2021 14:16:41 +0000 (15:16 +0100)] 
BUG/MINOR: stats: Init the metric variable when frontend stats are filled

In stats_fill_fe_stats(), some fields are conditionnal (ST_F_HRSP_* for
instance). But unlike unimplemented fields, for those fields, the <metric>
variable is used to fill the <stats> array, but it is not initialized. This
bug as no impact, because these fields are not used. But it is better to fix
it now to avoid future bugs.

To fix it, the metric is now defined and initialized into the for loop.

The bug was introduced by the commit 0ef54397 ("MEDIUM: stats: allow to
select one field in `stats_fill_fe_stats`"). No backport is needed except if
the above commit is backported. It fixes the issue #1063.

4 years agoBUILD: ssl: guard Client Hello callbacks with HAVE_SSL_CLIENT_HELLO_CB macro instead...
Ilya Shipitsin [Fri, 22 Jan 2021 19:09:14 +0000 (00:09 +0500)] 
BUILD: ssl: guard Client Hello callbacks with HAVE_SSL_CLIENT_HELLO_CB macro instead of openssl version

let us introduce new macro HAVE_SSL_CLIENT_HELLO_CB and guard
callback functions with it

4 years agoBUG/MINOR: stats: Continue to fill frontend stats on unimplemented metric
Christopher Faulet [Fri, 22 Jan 2021 16:33:22 +0000 (17:33 +0100)] 
BUG/MINOR: stats: Continue to fill frontend stats on unimplemented metric

A regression was introduced by the commit 0ef54397b ("MEDIUM: stats: allow
to select one field in `stats_fill_fe_stats`"). stats_fill_fe_stats()
function fails on unimplemented metrics for frontends. However, not all
stats metrics are used by frontends. For instance ST_F_QCUR. As a
consequence, the frontends stats are always skipped.

To fix the bug, we just skip unimplemented metric for frontends. An error is
triggered only if a specific field is given and is unimplemented.

No backport is needed except if the above commit is backported.

4 years ago[RELEASE] Released version 2.4-dev6 v2.4-dev6
Willy Tarreau [Fri, 22 Jan 2021 15:19:46 +0000 (16:19 +0100)] 
[RELEASE] Released version 2.4-dev6

Released version 2.4-dev6 with the following main changes :
    - MINOR: converter: adding support for url_enc
    - BUILD: SSL: guard TLS13 ciphersuites with HAVE_SSL_CTX_SET_CIPHERSUITES
    - BUILD: ssl: guard EVP_PKEY_get_default_digest_nid with ASN1_PKEY_CTRL_DEFAULT_MD_NID
    - BUILD: ssl: guard openssl specific with SSL_READ_EARLY_DATA_SUCCESS
    - BUILD: Makefile: exclude broken tests by default
    - CLEANUP: cfgparse: replace "realloc" with "my_realloc2" to fix to memory leak on error
    - BUG/MINOR: hlua: Fix memory leak in hlua_alloc
    - MINOR: contrib/prometheus-exporter: export build_info
    - DOC: fix some spelling issues over multiple files
    - CLEANUP: Fix spelling errors in comments
    - SCRIPTS: announce-release: fix typo in help message
    - CI: github: add a few more words to the codespell ignore list
    - DOC: Add maintainers for the Prometheus exporter
    - BUG/MINOR: sample: fix concat() converter's corruption with non-string variables
    - BUG/MINOR: server: Memory leak of proxy.used_server_addr during deinit
    - CLEANUP: sample: remove uneeded check in json validation
    - MINOR: reg-tests: add a way to add service dependency
    - BUG/MINOR: sample: check alloc_trash_chunk return value in concat()
    - BUG/MINOR: reg-tests: fix service dependency script
    - MINOR: reg-tests: add base prometheus test
    - Revert "BUG/MINOR: dns: SRV records ignores duplicated AR records"
    - BUG/MINOR: sample: Memory leak of sample_expr structure in case of error
    - BUG/MINOR: check: Don't perform any check on servers defined in a frontend
    - BUG/MINOR: init: enforce strict-limits when using master-worker
    - MINOR: contrib/prometheus-exporter: avoid connection close header
    - MINOR: contrib/prometheus-exporter: use fill_info for process dump
    - BUG/MINOR: init: Use a dynamic buffer to set HAPROXY_CFGFILES env variable
    - MINOR: config: Add failifnotcap() to emit an alert on proxy capabilities
    - MINOR: server: Forbid server definitions in frontend sections
    - BUG/MINOR: threads: Fixes the number of possible cpus report for Mac.
    - CLEANUP: pattern: rename pat_ref_commit() to pat_ref_commit_elt()
    - MINOR: pattern: add the missing generation ID manipulation functions
    - MINOR: peers: Add traces for peer control messages.
    - BUG/MINOR: dns: SRV records ignores duplicated AR records (v2)
    - BUILD: peers: fix build warning about unused variable
    - BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
    - MINOR: cache: Do not store responses with an unknown encoding
    - BUG/MINOR: peers: Possible appctx pointer dereference.
    - MINOR: build: discard echoing in help target
    - MINOR: cache: Remove the `hash` part of the accept-encoding secondary key
    - CLEANUP: cache: Use proper data types in secondary_key_cmp()
    - CLEANUP: Rename accept_encoding_hash_cmp to accept_encoding_bitmap_cmp
    - BUG/MINOR: peers: Wrong "new_conn" value for "show peers" CLI command.
    - MINOR: contrib: Make the wireshark peers dissector compile for more distribs.
    - BUG/MINOR: mux_h2: missing space between "st" and ".flg" in the "show fd" helper
    - CLEANUP: tools: make resolve_sym_name() take a const pointer
    - CLEANUP: cli: make "show fd" use a const connection to access other fields
    - MINOR: cli: make "show fd" also report the xprt and xprt_ctx
    - MINOR: xprt: add a new show_fd() helper to complete some "show fd" dumps.
    - MINOR: ssl: provide a "show fd" helper to report important SSL information
    - MINOR: xprt/mux: export all *_io_cb functions so that "show fd" resolves them
    - MINOR: mux-h2: make the "show fd" helper also decode the h2s subscriber when known
    - MINOR: mux-h1: make the "show fd" helper also decode the h1s subscriber when known
    - MINOR: mux-fcgi: make the "show fd" helper also decode the fstrm subscriber when known
    - CI: Pin VTest to a known good commit
    - MINOR: cli: give the show_fd helpers the ability to report a suspicious entry
    - MINOR: cli/show_fd: report some easily detectable suspicious states
    - MINOR: ssl/show_fd: report some FDs as suspicious when possible
    - MINOR: mux-h2/show_fd: report as suspicious an entry with too many calls
    - MINOR: mux-h1/show_fd: report as suspicious an entry with too many calls
    - BUG/MINOR: mworker: define _GNU_SOURCE for strsignal()
    - BUG/MEDIUM: tcpcheck: Don't destroy connection in the wake callback context
    - BUG/MEDIUM: mux-h2: Xfer rxbuf to the upper layer when creating a front stream
    - MINOR: http: Add HTTP 501-not-implemented error message
    - MINOR: muxes: Add exit status for errors about not implemented features
    - MINOR: mux-h1: Be prepared to return 501-not-implemented error during parsing
    - MEDIUM: mux-h1: Return a 501-not-implemented for upgrade requests with a body
    - DOC: Remove space after comma in converter signature
    - DOC: Rename '<var name>' to '<var>' in converter signature
    - MINOR: stats: duplicate 3 fields in bytes in info
    - MINOR: stats: add new start time field
    - MINOR: contrib/prometheus-exporter: merge info description from stats
    - MEDIUM: stats: allow to select one field in `stats_fill_fe_stats`
    - MINOR: contrib/prometheus-exporter: use fill_fe_stats for frontend dump
    - MINOR: contrib/prometheus-exporter: Don't needlessly set empty label for metrics
    - MINOR: contrib/prometheus-exporter: Split the PROMEX_FL_STATS_METRIC flag
    - MINOR: contrib/prometheus-exporter: Add promex_metric struct defining a metric
    - MEDIUM: contrib/prometheus-exporter: Rework matrices defining Promex metrics
    - BUG/MINOR: stream: Don't update counters when TCP to H2 upgrades are performed
    - BUG/MEDIUM: mux-h2: fix read0 handling on partial frames
    - MINOR: debug: always export the my_backtrace function
    - MINOR: debug: extract the backtrace dumping code to its own function
    - MINOR: debug: create ha_backtrace_to_stderr() to dump an instant backtrace
    - MEDIUM: debug: now always print a backtrace on CRASH_NOW() and friends
    - MINOR: debug: let ha_dump_backtrace() dump a bit further for some callers
    - BUILD: debug: fix build warning by consuming the write() result
    - MINOR: lua: remove unused variable
    - BUILD/MINOR: lua: define _GNU_SOURCE for LLONG_MAX

4 years agoBUILD/MINOR: lua: define _GNU_SOURCE for LLONG_MAX
Bertrand Jacquin [Thu, 21 Jan 2021 21:14:07 +0000 (21:14 +0000)] 
BUILD/MINOR: lua: define _GNU_SOURCE for LLONG_MAX

Lua requires LLONG_MAX defined with __USE_ISOC99 which is set by
_GNU_SOURCE, not necessarely defined by default on old compiler/glibc.

  $ make V=1 TARGET=linux-glibc-legacy USE_THREAD= USE_ACCEPT4= USE_PCRE=1 USE_OPENSSL=1 USE_ZLIB=1  USE_LUA=1
  ..
  cc -Iinclude  -O2 -g -Wall -Wextra -Wdeclaration-after-statement -fwrapv  -Wno-strict-aliasing -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter  -Wno-missing-field-initializers              -DUSE_EPOLL  -DUSE_NETFILTER -DUSE_PCRE    -DUSE_POLL       -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO -DUSE_OPENSSL -DUSE_LUA -DUSE_FUTEX   -DUSE_ZLIB  -DUSE_CPU_AFFINITY   -DUSE_DL -DUSE_RT      -DUSE_PRCTL -DUSE_THREAD_DUMP     -I/usr/include/openssl101e/  -DUSE_PCRE -I/usr/include  -DCONFIG_HAPROXY_VERSION=\"2.4-dev5-73246d-83\" -DCONFIG_HAPROXY_DATE=\"2021/01/21\" -c -o src/hlua.o src/hlua.c
  In file included from /usr/local/include/lua.h:15,
                   from /usr/local/include/lauxlib.h:15,
                   from src/hlua.c:16:
  /usr/local/include/luaconf.h:581:2: error: #error "Compiler does not support 'long long'. Use option '-DLUA_32BITS'   or '-DLUA_C89_NUMBERS' (see file 'luaconf.h' for details)"
  ..
  cc -Iinclude  -O2 -g -Wall -Wextra -Wdeclaration-after-statement -fwrapv  -Wno-strict-aliasing -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter  -Wno-missing-field-initializers              -DUSE_EPOLL  -DUSE_NETFILTER -DUSE_PCRE    -DUSE_POLL       -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO -DUSE_OPENSSL -DUSE_LUA -DUSE_FUTEX   -DUSE_ZLIB  -DUSE_CPU_AFFINITY   -DUSE_DL -DUSE_RT      -DUSE_PRCTL -DUSE_THREAD_DUMP     -I/usr/include/openssl101e/  -DUSE_PCRE -I/usr/include  -DCONFIG_HAPROXY_VERSION=\"2.4-dev5-73246d-83\" -DCONFIG_HAPROXY_DATE=\"2021/01/21\" -c -o src/hlua_fcn.o src/hlua_fcn.c
  In file included from /usr/local/include/lua.h:15,
                   from /usr/local/include/lauxlib.h:15,
                   from src/hlua_fcn.c:17:
  /usr/local/include/luaconf.h:581:2: error: #error "Compiler does not support 'long long'. Use option '-DLUA_32BITS'   or '-DLUA_C89_NUMBERS' (see file 'luaconf.h' for details)"
  ..

Cc: Thierry Fournier <tfournier@arpalert.org>
4 years agoMINOR: lua: remove unused variable
Bertrand Jacquin [Thu, 21 Jan 2021 19:14:46 +0000 (19:14 +0000)] 
MINOR: lua: remove unused variable

hlua_init() uses 'idx' only in openssl related code, while 'i' is used
in shared code and is safe to be reused. This commit replaces the use of
'idx' with 'i'

  $ make V=1 TARGET=linux-glibc USE_LUA=1 USE_OPENSSL=
  ..
  cc -Iinclude  -O2 -g -Wall -Wextra -Wdeclaration-after-statement -fwrapv -Wno-address-of-packed-member -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered -Wno-missing-field-initializers -Wno-cast-function-type  -Wtype-limits -Wshift-negative-value -Wshift-overflow=2 -Wduplicated-cond -Wnull-dereference   -DUSE_EPOLL  -DUSE_NETFILTER     -DUSE_POLL  -DUSE_THREAD  -DUSE_BACKTRACE   -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO  -DUSE_LUA -DUSE_FUTEX -DUSE_ACCEPT4    -DUSE_CPU_AFFINITY -DUSE_TFO -DUSE_NS -DUSE_DL -DUSE_RT      -DUSE_PRCTL -DUSE_THREAD_DUMP    -I/usr/include/lua5.3 -I/usr/include/lua5.3  -DCONFIG_HAPROXY_VERSION=\"2.4-dev5-37286a-78\" -DCONFIG_HAPROXY_DATE=\"2021/01/21\" -c -o src/hlua.o src/hlua.c
  src/hlua.c: In function 'hlua_init':
  src/hlua.c:9145:6: warning: unused variable 'idx' [-Wunused-variable]
   9145 |  int idx;
        |      ^~~

4 years agoBUILD: debug: fix build warning by consuming the write() result
Willy Tarreau [Fri, 22 Jan 2021 14:58:26 +0000 (15:58 +0100)] 
BUILD: debug: fix build warning by consuming the write() result

When writing commit a8459b28c ("MINOR: debug: create
ha_backtrace_to_stderr() to dump an instant backtrace") I just forgot
that some distros are a bit extremist about the syscall return values.

  src/debug.c: In function `ha_backtrace_to_stderr':
  src/debug.c:147:3: error: ignoring return value of `write', declared with attribute warn_unused_result [-Werror=unused-result]
     write(2, b.area, b.data);
     ^~~~~~~~~~~~~~~~~~~~~~~~
    CC      src/h1_htx.o

Let's apply the usual tricks to shut them up. No backport is needed.

4 years agoMINOR: debug: let ha_dump_backtrace() dump a bit further for some callers
Willy Tarreau [Fri, 22 Jan 2021 13:48:34 +0000 (14:48 +0100)] 
MINOR: debug: let ha_dump_backtrace() dump a bit further for some callers

The dump state is now passed to the function so that the caller can adjust
the behavior. A new series of 4 values allow to stop *after* dumping main
instead of before it or any of the usual loops. This allows to also report
BUG_ON() that could happen very high in the call graph (e.g. startup, or
the scheduler itself) while still understanding what the call path was.

4 years agoMEDIUM: debug: now always print a backtrace on CRASH_NOW() and friends
Willy Tarreau [Fri, 22 Jan 2021 13:15:46 +0000 (14:15 +0100)] 
MEDIUM: debug: now always print a backtrace on CRASH_NOW() and friends

The purpose is to enable the dumping of a backtrace on BUG_ON(). While
it's very useful to know that a condition was met, very often some
caller context is missing to figure how the condition could happen.
From now on, on systems featuring backtrace, a backtrace of the calling
thread will also be dumped to stderr in addition to the unexpected
condition. This will help users of DEBUG_STRICT as they'll most often
find this backtrace in their logs even if they can't find their core
file.

A new "debug dev bug" expert-mode CLI command was added to test the
feature.

4 years agoMINOR: debug: create ha_backtrace_to_stderr() to dump an instant backtrace
Willy Tarreau [Fri, 22 Jan 2021 13:12:27 +0000 (14:12 +0100)] 
MINOR: debug: create ha_backtrace_to_stderr() to dump an instant backtrace

This function calls the ha_dump_backtrace() function with a locally
allocated buffer and sends the output slightly indented to fd #2. It's
meant to be used as an emergency backtrace dump.

4 years agoMINOR: debug: extract the backtrace dumping code to its own function
Willy Tarreau [Fri, 22 Jan 2021 12:52:41 +0000 (13:52 +0100)] 
MINOR: debug: extract the backtrace dumping code to its own function

The backtrace dumping code was located into the thread dump function
but it looks particularly convenient to be able to call it to produce
a dump in other situations, so let's move it to its own function and
make sure it's called last in the function so that we can benefit from
tail merging to save one entry.

4 years agoMINOR: debug: always export the my_backtrace function
Willy Tarreau [Fri, 22 Jan 2021 11:12:29 +0000 (12:12 +0100)] 
MINOR: debug: always export the my_backtrace function

In order to simplify the code and remove annoying ifdefs everywhere,
let's always export my_backtrace() and make it adapt to the situation
and return zero if not supported. A small update in the thread dump
function was needed to make sure we don't use its results if it fails
now.

4 years agoBUG/MEDIUM: mux-h2: fix read0 handling on partial frames
Willy Tarreau [Wed, 20 Jan 2021 09:53:13 +0000 (10:53 +0100)] 
BUG/MEDIUM: mux-h2: fix read0 handling on partial frames

Since commit aade4edc1 ("BUG/MEDIUM: mux-h2: Don't handle pending read0
too early on streams"), we've met a few cases where an early connection
close wouldn't be properly handled if some data were pending in a frame
header, because the test now considers the buffer's contents before
accepting to report the close, but given that frame headers or preface
are consumed at once, the buffer cannot make progress when it's stuck
at intermediary lengths.

In order to address this, this patch introduces two flags in the h2c
connection to store any reported shutdown and failed parsing. The idea
is that we cannot rely on conn_xprt_read0_pending() in the parser since
it wouldn't consider data pending in the buffer nor intermediary layers,
but we know for certain that after a read0 is reported by the transport
layer in presence of an RD_SH on the connection, no more progress will
be made there. This alone is not sufficient to decide to end processing,
we can only do this once these final data have been submitted to a parser.
Therefore, now when a parser fails on missing data, we check if a read0
has already been reported on this connection, and if so we set a new
END_REACHED flag on the connection to indicate a failure to process the
final data. The h2c_read0_pending() function now simply reports this
flag's status. This way we're certain that the input shutdown is only
considered after the demux attempted to parse the last frame.

Maybe over the long term the subscribe() API should be improved to
synchronously fail when trying to subscribe for an even that will not
happen. This may be an elegant solution that could possibly work across
multiple layers and even muxes, and be usable at a few specific places
where that's needed.

Given the patch above was backported as far as 2.0, this one should be
backported there as well. It is possible that the fcgi mux has the same
issue, but this was not analysed yet.

Thanks to Pierre Cheynier for providing detailed traces allowing to
quickly narrow the problem down, and to Olivier for his analysis.

4 years agoBUG/MINOR: stream: Don't update counters when TCP to H2 upgrades are performed
Christopher Faulet [Thu, 21 Jan 2021 16:10:44 +0000 (17:10 +0100)] 
BUG/MINOR: stream: Don't update counters when TCP to H2 upgrades are performed

When a TCP to H2 upgrade is performed, the SF_IGNORE flag is set on the
stream before killing it. This happens when a TCP/SSL client connection is
routed to a HTTP backend and the h2 alpn detected. The SF_IGNORE flag was
added for this purpose, to skip some processing when the stream is aborted
before a mux upgrade. Some counters updates were skipped this way. But some
others are still updated.

Now, all counters update at the end of process_stream(), before releasing
the stream, are ignored if SF_IGNORE flag is set. Note this stream is
aborted because we switch from a mono-stream to a multi-stream
multiplexer. It works differently for TCP to H1 upgrades.

This patch should be backported as far as 2.0 after some observation period.

4 years agoMEDIUM: contrib/prometheus-exporter: Rework matrices defining Promex metrics
Christopher Faulet [Wed, 20 Jan 2021 14:20:53 +0000 (15:20 +0100)] 
MEDIUM: contrib/prometheus-exporter: Rework matrices defining Promex metrics

The global and stats matrices are replaced by a simpler ones. Now we have
only 2 arrays of prometheus metrics. Their flags are used to match on the
entity type. This simplify a bit the metrics definition. For now, labels and
descriptions are still outside of these arrays, because the labels must be
reworked to be more dynamic and the descrptions must be replaced by stats
ones as far as possible.

4 years agoMINOR: contrib/prometheus-exporter: Add promex_metric struct defining a metric
Christopher Faulet [Wed, 20 Jan 2021 14:19:12 +0000 (15:19 +0100)] 
MINOR: contrib/prometheus-exporter: Add promex_metric struct defining a metric

This structure will be used to define a Prometheus metric, i.e its name, its
type (gauge or counter) and its flags. The flags will be used to know for
which entities the metric is defined (global, frontend, backend and/or server).

4 years agoMINOR: contrib/prometheus-exporter: Split the PROMEX_FL_STATS_METRIC flag
Christopher Faulet [Wed, 20 Jan 2021 14:02:50 +0000 (15:02 +0100)] 
MINOR: contrib/prometheus-exporter: Split the PROMEX_FL_STATS_METRIC flag

PROMEX_FL_STATS_METRIC flag is splitted in 3 flags to easily identify the
processed entity type (frontend, backend or server). Thus, now we are using
PROMEX_FL_FRONT_METRIC, PROMEX_FL_BACK_METRIC or PROMEX_FL_SRV_METRIC. These
flags will be used to know if a metric is defined and must be exported for a
given entity type.

4 years agoMINOR: contrib/prometheus-exporter: Don't needlessly set empty label for metrics
Christopher Faulet [Wed, 20 Jan 2021 14:28:22 +0000 (15:28 +0100)] 
MINOR: contrib/prometheus-exporter: Don't needlessly set empty label for metrics

There is no reason to define empty labels for metrics. By default, all labels
are initialized to an empty ist.

4 years agoMINOR: contrib/prometheus-exporter: use fill_fe_stats for frontend dump
William Dauchy [Sun, 17 Jan 2021 17:27:46 +0000 (18:27 +0100)] 
MINOR: contrib/prometheus-exporter: use fill_fe_stats for frontend dump

use `stats_fill_fe_stats` when possible to avoid duplicating code; make
use of field selector to get the needed field only.

this should not introduce any difference of output.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMEDIUM: stats: allow to select one field in `stats_fill_fe_stats`
William Dauchy [Sun, 17 Jan 2021 17:27:45 +0000 (18:27 +0100)] 
MEDIUM: stats: allow to select one field in `stats_fill_fe_stats`

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the frontend.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: contrib/prometheus-exporter: merge info description from stats
William Dauchy [Fri, 15 Jan 2021 21:41:39 +0000 (22:41 +0100)] 
MINOR: contrib/prometheus-exporter: merge info description from stats

Now that units are coherent we can merge the info description from
haproxy stats.
Description were not always the same, but I guess we may eventually
improve them in the future.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: stats: add new start time field
William Dauchy [Fri, 15 Jan 2021 21:41:38 +0000 (22:41 +0100)] 
MINOR: stats: add new start time field

Another patch in order to try to reconciliate haproxy stats and
prometheus. Here I'm adding a proper start time field in order to make
proper use of uptime field.
That being done we can move the calculation in `fill_info`

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: stats: duplicate 3 fields in bytes in info
William Dauchy [Fri, 15 Jan 2021 21:41:37 +0000 (22:41 +0100)] 
MINOR: stats: duplicate 3 fields in bytes in info

in order to prepare a possible merge of fields between haproxy stats and
prometheus, duplicate 3 fields:
  INF_MEMMAX
  INF_POOL_ALLOC
  INF_POOL_USED
Those were specifically named in MB unit which is not what prometheus
recommends. We therefore used them but changed the unit while doing the
calculation. It created a specific case for that, up to the description.
This patch:
- removes some possible confusion, i.e. using MB field for bytes
- will permit an easier merge of fields such as description

First consequence for now, is that we can remove the calculation on
prometheus side and move it on `fill_info`.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoDOC: Rename '<var name>' to '<var>' in converter signature
Tim Duesterhus [Thu, 21 Jan 2021 16:40:50 +0000 (17:40 +0100)] 
DOC: Rename '<var name>' to '<var>' in converter signature

The space appears to trip up the dconv parser and `<var>` is used for other
converters.

4 years agoDOC: Remove space after comma in converter signature
Tim Duesterhus [Thu, 21 Jan 2021 16:40:49 +0000 (17:40 +0100)] 
DOC: Remove space after comma in converter signature

This space appears to trip up the dconv parser and is inconsistent with
other converts.

4 years agoMEDIUM: mux-h1: Return a 501-not-implemented for upgrade requests with a body
Christopher Faulet [Mon, 7 Dec 2020 17:17:33 +0000 (18:17 +0100)] 
MEDIUM: mux-h1: Return a 501-not-implemented for upgrade requests with a body

If an HTTP protocol upgrade request with a payload is received, a
501-not-implemented error is now returned to the client. It is valid from
the RFC point of view but will be incompatible with the way the H2
websockets will be handled by HAProxy. And it is probably a very uncommon
way to do perform protocol upgrades.

4 years agoMINOR: mux-h1: Be prepared to return 501-not-implemented error during parsing
Christopher Faulet [Mon, 7 Dec 2020 10:26:13 +0000 (11:26 +0100)] 
MINOR: mux-h1: Be prepared to return 501-not-implemented error during parsing

With this patch, the H1 mux is now able to return 501-not-implemented errors
to client during the request parsing. However, no such errors are returned
for now.

4 years agoMINOR: muxes: Add exit status for errors about not implemented features
Christopher Faulet [Mon, 7 Dec 2020 10:24:37 +0000 (11:24 +0100)] 
MINOR: muxes: Add exit status for errors about not implemented features

The MUX_ES_NOTIMPL_ERR exit status is added to allow the multiplexers to
report errors about not implemented features. This will be used by the H1
mux to return 501-not-implemented errors.

4 years agoMINOR: http: Add HTTP 501-not-implemented error message
Christopher Faulet [Mon, 7 Dec 2020 10:22:24 +0000 (11:22 +0100)] 
MINOR: http: Add HTTP 501-not-implemented error message

Add the support for the 501-not-implemented status code with the
corresponding default message. The documentation is updated accordingly
because it is now part of status codes HAProxy may emit via an errorfile or
a deny/return HTTP action.

4 years agoBUG/MEDIUM: mux-h2: Xfer rxbuf to the upper layer when creating a front stream
Christopher Faulet [Tue, 15 Dec 2020 15:56:50 +0000 (16:56 +0100)] 
BUG/MEDIUM: mux-h2: Xfer rxbuf to the upper layer when creating a front stream

Just like the H1 muliplexer, when a new frontend H2 stream is created, the
rxbuf is xferred to the stream at the upper layer.

Originally, it is not a bug fix, but just an api standardization. And in
fact, it fixes a crash when a h2 stream is aborted after the request parsing
but before the first call to process_stream(). It crashes since the commit
8bebd2fe5 ("MEDIUM: http-ana: Don't process partial or empty request
anymore"). It is now totally unexpected to have an HTTP stream without a
valid request. But here the stream is unable to get the request because the
client connection was aborted. Passing it during the stream creation fixes
the bug. But the true problem is that the stream-interfaces are still
relying on the connection state while only the muxes should do so.

This fix is specific for 2.4. No backport needed.

4 years agoBUG/MEDIUM: tcpcheck: Don't destroy connection in the wake callback context
Christopher Faulet [Mon, 18 Jan 2021 14:47:03 +0000 (15:47 +0100)] 
BUG/MEDIUM: tcpcheck: Don't destroy connection in the wake callback context

When a tcpcheck ruleset uses multiple connections, the existing one must be
closed and destroyed before openning the new one. This part is handled in
the tcpcheck_main() function, when called from the wake callback function
(wake_srv_chk). But it is indeed a problem, because this function may be
called from the mux layer. This means a mux may call the wake callback
function of the data layer, which may release the connection and the mux. It
is easy to see how it is hazardous. And actually, depending on the
scheduling, it leads to crashes.

Thus, we must avoid to release the connection in the wake callback context,
and move this part in the check's process function instead. To do so, we
rely on the CHK_ST_CLOSE_CONN flags. When a connection must be replaced by a
new one, this flag is set on the check, in tcpcheck_main() function, and the
check's task is woken up. Then, the connection is really closed in
process_chk_conn() function.

This patch must be backported as far as 2.2, with some adaptations however
because the code is not exactly the same.

4 years agoBUG/MINOR: mworker: define _GNU_SOURCE for strsignal()
Bertrand Jacquin [Thu, 21 Jan 2021 01:31:46 +0000 (01:31 +0000)] 
BUG/MINOR: mworker: define _GNU_SOURCE for strsignal()

glibc < 2.10 requires _GNU_SOURCE in order to make use of strsignal(),
otherwise leading to SEGV at runtime.

  $ make V=1 TARGET=linux-glibc-legacy USE_THREAD= USE_ACCEPT4=
  ..
  src/mworker.c: In function 'mworker_catch_sigchld':
  src/mworker.c:285: warning: implicit declaration of function 'strsignal'
  src/mworker.c:285: warning: pointer/integer type mismatch in conditional expression
  ..

  $ make V=1 reg-tests REGTESTS_TYPES=slow,default
  ..
  ###### Test case: reg-tests/mcli/mcli_start_progs.vtc ######
  ## test results in: "/tmp/haregtests-2021-01-19_15-18-07.n24989/vtc.29077.28f6153d"
  ---- h1    Bad exit status: 0x008b exit 0x0 signal 11 core 128
  ---- h1    Assert error in haproxy_wait(), src/vtc_haproxy.c line 792:  Condition(*(&h->fds[1]) >= 0) not true.  Errno=0 Success
  ..

  $ gdb ./haproxy /tmp/core.0.haproxy.30270
  ..
  Core was generated by `/root/haproxy/haproxy -d -W -S fd@8 -dM -f /tmp/haregtests-2021-01-19_15-18-07.'.
  Program terminated with signal 11, Segmentation fault.
  #0  0x00002aaaab387a10 in strlen () from /lib64/libc.so.6
  (gdb) bt
  #0  0x00002aaaab387a10 in strlen () from /lib64/libc.so.6
  #1  0x00002aaaab354b69 in vfprintf () from /lib64/libc.so.6
  #2  0x00002aaaab37788a in vsnprintf () from /lib64/libc.so.6
  #3  0x00000000004a76a3 in memvprintf (out=0x7fffedc680a0, format=0x5a5d58 "Current worker #%d (%d) exited with code %d (%s)\n", orig_args=0x7fffedc680d0)
      at src/tools.c:3868
  #4  0x00000000004bbd40 in print_message (label=0x58abed "ALERT", fmt=0x5a5d58 "Current worker #%d (%d) exited with code %d (%s)\n", argp=0x7fffedc680d0)
      at src/log.c:1066
  #5  0x00000000004bc07f in ha_alert (fmt=0x5a5d58 "Current worker #%d (%d) exited with code %d (%s)\n") at src/log.c:1109
  #6  0x0000000000534b7b in mworker_catch_sigchld (sh=<value optimized out>) at src/mworker.c:293
  #7  0x0000000000556af3 in __signal_process_queue () at src/signal.c:88
  #8  0x00000000004f6216 in signal_process_queue () at include/haproxy/signal.h:39
  #9  run_poll_loop () at src/haproxy.c:2859
  #10 0x00000000004f63b7 in run_thread_poll_loop (data=<value optimized out>) at src/haproxy.c:3028
  #11 0x00000000004faaac in main (argc=<value optimized out>, argv=0x7fffedc68498) at src/haproxy.c:904

See: https://man7.org/linux/man-pages/man3/strsignal.3.html

Must be backported as far as 2.0.

4 years agoMINOR: mux-h1/show_fd: report as suspicious an entry with too many calls
Willy Tarreau [Thu, 21 Jan 2021 08:13:35 +0000 (09:13 +0100)] 
MINOR: mux-h1/show_fd: report as suspicious an entry with too many calls

An FD entry that maps to an H1 connection whose stream was woken
up more than 1M times is now flagged as suspicious.

4 years agoMINOR: mux-h2/show_fd: report as suspicious an entry with too many calls
Willy Tarreau [Thu, 21 Jan 2021 08:13:35 +0000 (09:13 +0100)] 
MINOR: mux-h2/show_fd: report as suspicious an entry with too many calls

An FD entry that maps to an H2C connection whose last stream was woken
up more than 1M times is now flagged as suspicious.

4 years agoMINOR: ssl/show_fd: report some FDs as suspicious when possible
Willy Tarreau [Thu, 21 Jan 2021 07:53:50 +0000 (08:53 +0100)] 
MINOR: ssl/show_fd: report some FDs as suspicious when possible

If a subscriber's tasklet was called more than one million times, if
the ssl_ctx's connection doesn't match the current one, or if the
connection appears closed in one direction while the SSL stack is
still subscribed, the FD is reported as suspicious. The close cases
may occasionally trigger a false positive during very short and rare
windows. Similarly the 1M calls will trigger after 16GB are transferred
over a given connection. These are rare enough events to be reported as
suspicious.

4 years agoMINOR: cli/show_fd: report some easily detectable suspicious states
Willy Tarreau [Thu, 21 Jan 2021 08:07:29 +0000 (09:07 +0100)] 
MINOR: cli/show_fd: report some easily detectable suspicious states

A file descriptor which maps to a connection but has more than one
thread in its mask, or an FD handle that doesn't correspond to the FD,
or wiht no mux context, or an FD with no thread in its mask, or with
more than 1 million events is flagged as suspicious.

4 years agoMINOR: cli: give the show_fd helpers the ability to report a suspicious entry
Willy Tarreau [Thu, 21 Jan 2021 07:26:06 +0000 (08:26 +0100)] 
MINOR: cli: give the show_fd helpers the ability to report a suspicious entry

Now the show_fd helpers at the transport and mux levels return an integer
which indicates whether or not the inspected entry looks suspicious. When
an entry is reported as suspicious, "show fd" will suffix it with an
exclamation mark ('!') in the dump, that is supposed to help detecting
them.

For now, helpers were adjusted to adapt to the new API but none of them
reports any suspicious entry yet.

4 years agoCI: Pin VTest to a known good commit
Tim Duesterhus [Wed, 20 Jan 2021 18:13:29 +0000 (19:13 +0100)] 
CI: Pin VTest to a known good commit

As of January, 11th the macOS builds fail due to regression introduced in
VTest. This patch pins VTest to the newest good commit.

This patch should be reverted once VTest's 'master' is stable again.

see vtest/VTest#26

4 years agoMINOR: mux-fcgi: make the "show fd" helper also decode the fstrm subscriber when...
Willy Tarreau [Wed, 20 Jan 2021 16:10:46 +0000 (17:10 +0100)] 
MINOR: mux-fcgi: make the "show fd" helper also decode the fstrm subscriber when known

When dumping a live fcgi stream, also take the opportunity for reporting
the subscriber including the event, tasklet, handler and context.

4 years agoMINOR: mux-h1: make the "show fd" helper also decode the h1s subscriber when known
Willy Tarreau [Wed, 20 Jan 2021 16:05:58 +0000 (17:05 +0100)] 
MINOR: mux-h1: make the "show fd" helper also decode the h1s subscriber when known

When dumping a live h1 stream, also take the opportunity for reporting
the subscriber including the event, tasklet, handler and context. Example:

   3030 : st=0x21(R:rA W:Ra) ev=0x04(heOpi) [Lc] tmask=0x4 umask=0x0 owner=0x7f97805c1f70 iocb=0x65b847(sock_conn_iocb) back=1 cflg=0x00002300 sv=s1/recv mux=H1 ctx=0x7f97805c21b0 h1c.flg=0x80000200 .sub=1 .ibuf=0@(nil)+0/0 .obuf=0@(nil)+0/0 h1s=0x7f97805c2380 h1s.flg=0x4010 .req.state=MSG_DATA .res.state=MSG_RPBEFORE .meth=POST status=0 .cs.flg=0x00000000 .cs.data=0x7f97805c1720 .subs=0x7f97805c1748(ev=1 tl=0x7f97805c1990 tl.calls=2 tl.ctx=0x7f97805c1720 tl.fct=si_cs_io_cb) xprt=RAW

4 years agoMINOR: mux-h2: make the "show fd" helper also decode the h2s subscriber when known
Willy Tarreau [Wed, 20 Jan 2021 15:27:01 +0000 (16:27 +0100)] 
MINOR: mux-h2: make the "show fd" helper also decode the h2s subscriber when known

When dumping a valid h2 stream, also dump the subscriber, its events,
tasklet context and handler. Example:

    128 : st=0x21(R:rA W:Ra) ev=0x01(heopI) [lc] tmask=0x1 umask=0x0 owner=0x7f40380d7370 iocb=0x65b71b(sock_conn_iocb) back=0 cflg=0x00001300 fe=recv mux=H2 ctx=0x1ad23e0 h2c.st0=FRP .err=0 .maxid=3 .lastid=-1 .flg=0x10000 .nbst=2 .nbcs=2 .fctl_cnt=0 .send_cnt=0 .tree_cnt=2 .orph_cnt=0 .sub=1 .dsi=3 .dbuf=16366@0x1ea9380+16441/16448 .msi=-1 .mbuf=[1..1|32],h=[0@(nil)+0/0],t=[0@(nil)+0/0] last_h2s=0x20a8340 .id=3 .st=OPN .flg=0x4100 .rxbuf=0@(nil)+0/0 .cs=0x20a8440(.flg=0x00100000 .data=0x20a8738) .subs=0x20a8760(ev=1 tl=0x20a89b0 tl.calls=22 tl.ctx=0x20a8738 tl.fct=si_cs_io_cb) xprt=SSL xprt_ctx=0x1aaf4c0 xctx.st=0 .xprt=RAW .wait.ev=1 .subs=0x1ad28e0(ev=1 tl=0x1ab3c70 tl.calls=176 tl.ctx=0x1ad23e0 tl.fct=h2_io_cb) .sent_early=0 .early_in=0

4 years agoMINOR: xprt/mux: export all *_io_cb functions so that "show fd" resolves them
Willy Tarreau [Wed, 20 Jan 2021 13:55:01 +0000 (14:55 +0100)] 
MINOR: xprt/mux: export all *_io_cb functions so that "show fd" resolves them

In FD dumps it's often very important to figure what upper layer function
is going to be called. Let's export the few I/O callbacks that appear as
tasklet functions so that "show fd" can resolve them instead of printing
a pointer relative to main. For example:

   1028 : st=0x21(R:rA W:Ra) ev=0x01(heopI) [lc] tmask=0x2 umask=0x2 owner=0x7f00b889b200 iocb=0x65b638(sock_conn_iocb) back=0 cflg=0x00001300 fe=recv mux=H2 ctx=0x7f00c8824de0 h2c.st0=FRH .err=0 .maxid=795 .lastid=-1 .flg=0x0000 .nbst=0 .nbcs=0 .fctl_cnt=0 .send_cnt=0 .tree_cnt=0 .orph_cnt=0 .sub=1 .dsi=795 .dbuf=0@(nil)+0/0 .msi=-1 .mbuf=[1..1|32],h=[0@(nil)+0/0],t=[0@(nil)+0/0] xprt=SSL xprt_ctx=0x7f00c86d0750 xctx.st=0 .xprt=RAW .wait.ev=1 .subs=0x7f00c88252e0(ev=1 tl=0x7f00a07d1aa0 tl.calls=1047 tl.ctx=0x7f00c8824de0 tl.fct=h2_io_cb) .sent_early=0 .early_in=0

4 years agoMINOR: ssl: provide a "show fd" helper to report important SSL information
Willy Tarreau [Wed, 20 Jan 2021 13:41:29 +0000 (14:41 +0100)] 
MINOR: ssl: provide a "show fd" helper to report important SSL information

The SSL context contains a lot of important details that are currently
missing from debug outputs. Now that we detect ssl_sock, we can perform
some sanity checks, print the next xprt, the subscriber callback's context,
handler and number of calls. The process function is also resolved. This
now gives for example on an H2 connection:

   1029 : st=0x21(R:rA W:Ra) ev=0x01(heopI) [lc] tmask=0x2 umask=0x2 owner=0x7fc714881700 iocb=0x65b528(sock_conn_iocb) back=0 cflg=0x00001300 fe=recv mux=H2 ctx=0x7fc734545e50 h2c.st0=FRH .err=0 .maxid=217 .lastid=-1 .flg=0x0000 .nbst=0 .nbcs=0 .fctl_cnt=0 .send_cnt=0 .tree_cnt=0 .orph_cnt=0 .sub=1 .dsi=217 .dbuf=0@(nil)+0/0 .msi=-1 .mbuf=[1..1|32],h=[0@(nil)+0/0],t=[0@(nil)+0/0] xprt=SSL xprt_ctx=0x7fc73478f230 xctx.st=0 .xprt=RAW .wait.ev=1 .subs=0x7fc734546350(ev=1 tl=0x7fc7346702e0 tl.calls=278 tl.ctx=0x7fc734545e50 tl.fct=main-0x144efa) .sent_early=0 .early_in=0

4 years agoMINOR: xprt: add a new show_fd() helper to complete some "show fd" dumps.
Willy Tarreau [Wed, 20 Jan 2021 14:30:56 +0000 (15:30 +0100)] 
MINOR: xprt: add a new show_fd() helper to complete some "show fd" dumps.

Just like we did for the muxes, now the transport layers will have the
ability to provide helpers to report more detailed information about their
internal context. When the helper is not known, the pointer continues to
be dumped as-is if it's not NULL. This way a transport with no context nor
dump function will not add a useless "xprt_ctx=(nil)" but the pointer will
be emitted if valid or if a helper is defined.