haproxy-2.3.git
4 years agoBUG/MINOR: lua: warn when registering action, conv, sf, cli or applet multiple times
Thierry Fournier [Sat, 28 Nov 2020 19:41:07 +0000 (20:41 +0100)]
BUG/MINOR: lua: warn when registering action, conv, sf, cli or applet multiple times

Lua allows registering multiple sample-fetches, converters, action, cli,
applet/services with the same name. This is absolutely useless since only
the first registration will be used. This patch sends a warning if the case
is encountered.

This pach could be backported until 1.8, with the 3 associated patches:
 - MINOR: actions: Export actions lookup functions
 - MINOR: actions: add a function returning a service pointer from its name
 - MINOR: cli: add a function to look up a CLI service description

(cherry picked from commit f67442efdb509d5d15f530a536b13f29fa7f48b7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: cli: add a function to look up a CLI service description
Thierry Fournier [Sat, 28 Nov 2020 19:10:08 +0000 (20:10 +0100)]
MINOR: cli: add a function to look up a CLI service description

This function will be useful to check if the keyword is already registered.
Also add a define for the max number of args.

This will be needed by a next patch to fix a bug and will have to be
backported.

(cherry picked from commit a51a1fd17420a96bb766afbae354e041fc9e1d9b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: actions: add a function returning a service pointer from its name
Thierry Fournier [Sat, 28 Nov 2020 18:32:14 +0000 (19:32 +0100)]
MINOR: actions: add a function returning a service pointer from its name

This function simply calls action_lookup() on the private service_keywords,
to look up a service name. This will be used to detect double registration
of a same service from Lua.

This will be needed by a next patch to fix a bug and will have to be
backported.

(cherry picked from commit 87e539906b13ed1d86684d9d3eab82b550fae02d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: actions: Export actions lookup functions
Thierry Fournier [Sat, 28 Nov 2020 16:40:24 +0000 (17:40 +0100)]
MINOR: actions: Export actions lookup functions

These functions will be useful to check if a keyword is already registered.
This will be needed by a next patch to fix a bug, and will need to be
backported.

(cherry picked from commit 7a71a6d9d262d7a0f7c3d208ab339d469958011d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: lua: Some lua init operation are processed unsafe
Thierry Fournier [Sat, 28 Nov 2020 15:08:02 +0000 (16:08 +0100)]
BUG/MINOR: lua: Some lua init operation are processed unsafe

Operation luaL_openlibs() and lua_prepend path are processed whithout
the safe context, so in case of failure Haproxy aborts or stops without
error message.

This patch could be backported until 1.8

(cherry picked from commit 2f05cc6f86ee6beda9c42a6bb99a9a96fad37b68)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: lua: Post init register function are not executed beyond the first one
Thierry Fournier [Sat, 28 Nov 2020 10:02:58 +0000 (11:02 +0100)]
BUG/MINOR: lua: Post init register function are not executed beyond the first one

Just because if the first init is a success we return success in place
of continuing the loop.

This patch could be backported until 1.8

(cherry picked from commit 13d08b73eb99741ca5903e8414b85a1d0b919594)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: lua: lua-load doesn't check its parameters
Thierry Fournier [Sun, 29 Nov 2020 00:06:24 +0000 (01:06 +0100)]
BUG/MINOR: lua: lua-load doesn't check its parameters

"lua-load" doesn't check if the expected parameter is present. It tries to
open() directly the argument at second position. So if the filename is
omitted, it tries to load an empty filename.

This patch could be backported until 1.8

(cherry picked from commit 77a88943d6a0d7852c25a03541d60ccf44aa5c6d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: lua: missing "\n" in error message
Thierry Fournier [Sat, 28 Nov 2020 23:55:53 +0000 (00:55 +0100)]
BUG/MINOR: lua: missing "\n" in error message

Just replace ".n" by "\n"

This could be backported until 1.9, but it is not so important.

(cherry picked from commit de6145f747e38a5da455ccb8a84ebfc1cd2f517f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: mux-h2/stats: not all GOAWAY frames are errors
Willy Tarreau [Tue, 1 Dec 2020 09:47:18 +0000 (10:47 +0100)]
BUG/MINOR: mux-h2/stats: not all GOAWAY frames are errors

The stats on haproxy.org reported ~12k GOAWAY for ~34k connections, with
only 2 protocol errorss. It turns out that the GOAWAY frame counter added
in commit a8879238c ("MINOR: mux-h2: report detected error on stats")
matches a bit too many situations. First it counts those which are not
sent as well as failed retries, second it counts as errors the cases of
attempts to cleanly close, while it's titled "GOAWAY sent on detected
error". Let's address this by moving the counter up one line and excluding
the clean codes.

This can be backported to 2.3.

(cherry picked from commit f965b2ad136b6487571f9a8fb0b3e58f778c002c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: mux-h2/stats: make stream/connection proto errors more accurate
Willy Tarreau [Tue, 1 Dec 2020 09:22:43 +0000 (10:22 +0100)]
BUG/MINOR: mux-h2/stats: make stream/connection proto errors more accurate

Since commit a8879238c ("MINOR: mux-h2: report detected error on stats")
we now have some error stats on stream/connection level protocol errors,
but some were improperly marked as stream while they're connection, and
2 or 3 relevant ones were missing and have now been added.

This could be backported to 2.3.

(cherry picked from commit a307528fe287f085705ef08a798c1698e474c395)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: local log format regression.
Emeric Brun [Fri, 27 Nov 2020 15:24:34 +0000 (16:24 +0100)]
BUG/MEDIUM: local log format regression.

Since 2.3 default local log format always adds hostame field.
This behavior change was due to log/sink re-work, because according
to rfc3164 the hostname field is mandatory.

This patch re-introduce a legacy "local" format which is analog
to rfc3164 but with hostname stripped. This is the new
default if logs are generated by haproxy.

To stay compliant with previous configurations, the option
"log-send-hostname" acts as if the default format is switched
to rfc3164.

This patch addresses the github issue #963

This patch should be backported in branches >= 2.3.

(cherry picked from commit 0237c4e3f5deb062e8714d02588c86d48fa2703c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: task: close a possible data race condition on a tasklet's list link
Willy Tarreau [Mon, 30 Nov 2020 13:58:53 +0000 (14:58 +0100)]
BUG/MEDIUM: task: close a possible data race condition on a tasklet's list link

In issue #958 Ashley Penney reported intermittent crashes on AWS's ARM
nodes which would not happen on x86 nodes. After investigation it turned
out that the Neoverse N1 CPU cores used in the Graviton2 CPU are much
more aggressive than the usual Cortex A53/A72/A55 or any x86 regarding
memory ordering.

The issue that was triggered there is that if a tasklet_wakeup() call
is made on a tasklet scheduled to run on a foreign thread and that
tasklet is just being dequeued to be processed, there can be a race at
two places:
  - if MT_LIST_TRY_ADDQ() happens between MT_LIST_BEHEAD() and
    LIST_SPLICE_END_DETACHED() if the tasklet is alone in the list,
    because the emptiness tests matches ;

  - if MT_LIST_TRY_ADDQ() happens during LIST_DEL_INIT() in
    run_tasks_from_lists(), then depending on how LIST_DEL_INIT() ends
    up being implemented, it may even corrupt the adjacent nodes while
    they're being reused for the in-tree storage.

This issue was introduced in 2.2 when support for waking up remote
tasklets was added. Initially the attachment of a tasklet to a list
was enough to know its status and this used to be stable information.
Now it's not sufficient to rely on this anymore, thus we need to use
a different information.

This patch solves this by adding a new task flag, TASK_IN_LIST, which
is atomically set before attaching a tasklet to a list, and is only
removed after the tasklet is detached from a list. It is checked
by tasklet_wakeup_on() so that it may only be done while the tasklet
is out of any list, and is cleared during the state switch when calling
the tasklet. Note that the flag is not set for pure tasks as it's not
needed.

However this introduces a new special case: the function
tasklet_remove_from_tasklet_list() needs to keep both states in sync
and cannot check both the state and the attachment to a list at the
same time. This function is already limited to being used by the thread
owning the tasklet, so in this case the test remains reliable. However,
just like its predecessors, this function is wrong by design and it
should probably be replaced with a stricter one, a lazy one, or be
totally removed (it's only used in checks to avoid calling a possibly
scheduled event, and when freeing a tasklet). Regardless, for now the
function exists so the flag is removed only if the deletion could be
done, which covers all cases we're interested in regarding the insertion.
This removal is safe against a concurrent tasklet_wakeup_on() since
MT_LIST_DEL() guarantees the atomic test, and will ultimately clear
the flag only if the task could be deleted, so the flag will always
reflect the last state.

This should be carefully be backported as far as 2.2 after some
observation period. This patch depends on previous patch
"MINOR: task: remove __tasklet_remove_from_tasklet_list()".

(cherry picked from commit 4d6c594998a47d2c62ff74fba36f5798bea1a228)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: task: remove __tasklet_remove_from_tasklet_list()
Willy Tarreau [Mon, 30 Nov 2020 13:52:11 +0000 (14:52 +0100)]
MINOR: task: remove __tasklet_remove_from_tasklet_list()

This function is only used at a single place directly within the
scheduler in run_tasks_from_lists() and it really ought not be called
by anything else, regardless of what its comment says. Let's delete
it, move the two lines directly into the call place, and take this
opportunity to factor the atomic decrement on tasks_run_queue. A comment
was added on the remaining one tasklet_remove_from_tasklet_list() to
mention the risks in using it.

(cherry picked from commit 2da4c316c2cb7b01f54ed1959e91d1799d13959c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: lists: Lock the element while we check if it is in a list.
Olivier Houchard [Wed, 25 Nov 2020 19:38:00 +0000 (20:38 +0100)]
BUG/MEDIUM: lists: Lock the element while we check if it is in a list.

In MT_LIST_TRY_ADDQ() and MT_LIST_TRY_ADD() we can't just check if the
element is already in a list, because there's a small race condition, it
could be added  between the time we checked, and the time we actually set
its next and prev, so we have to lock it first.

This is required to address issue #958.

This should be backported to 2.3, 2.2 and 2.1.

(cherry picked from commit 1f05324cbe92a7dde71f44dc740eb8240539746f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: plock: use an ARMv8 instruction barrier for the pause instruction
Your Name [Sat, 28 Nov 2020 15:37:14 +0000 (15:37 +0000)]
MINOR: plock: use an ARMv8 instruction barrier for the pause instruction

As suggested by @AGSaidi in issue #958, on ARMv8 its convenient to use
an "isb" instruction in pl_cpu_relax() to improve fairness. Without it
I've met a few watchdog conditions on valid locks with 16 threads,
indicating that some threads couldn't manage to get it in 2 seconds. I
never happened again with it. In addition, the performance increased
by slightly more than 5% thanks to the reduced contention.

This should be backported as far as 2.2, possibly even 2.0.

(cherry picked from commit 1e237d037b3a45ec92d1dfa80dfd2c6bd7fc3af9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years ago[RELEASE] Released version 2.3.2 v2.3.2
William Lallemand [Sat, 28 Nov 2020 15:51:33 +0000 (16:51 +0100)]
[RELEASE] Released version 2.3.2

Released version 2.3.2 with the following main changes :
    - BUILD: http-htx: fix build warning regarding long type in printf
    - CLEANUP: cfgparse: remove duplicate registration for transparent build options
    - BUG/MEDIUM: filters: Forward all filtered data at the end of http filtering
    - BUG/MINOR: http-ana: Don't wait for the body of CONNECT requests
    - DOC: add missing 3.10 in the summary
    - BUG/MINOR: ssl: segv on startup when AKID but no keyid
    - BUG/MEDIUM: http-ana: Don't eval http-after-response ruleset on empty messages
    - BUG/MEDIUM: ssl/crt-list: bundle support broken in crt-list
    - BUG/MEDIUM: ssl: error when no certificate are found
    - BUG/MINOR: ssl/crt-list: load bundle in crt-list only if activated
    - BUG/MEDIUM: ssl/crt-list: fix error when no file found
    - BUILD: makefile: enable crypt(3) for OpenBSD
    - DOC: clarify how to create a fallback crt
    - CLEANUP: connection: do not use conn->owner when the session is known
    - BUG/MAJOR: connection: reset conn->owner when detaching from session list
    - BUG/MINOR: http_htx: Fix searching headers by substring
    - DOC: better describes how to configure a fallback crt
    - BUG/MAJOR: filters: Always keep all offsets up to date during data filtering
    - MEDIUM: cache: Change caching conditions
    - DOC: cache: Add new caching limitation information
    - REGTESTS: Add sample_fetches/cook.vtc
    - REGTESTS: converter: add url_dec test
    - MINOR: http_act: Add -m flag for del-header name matching method
    - BUILD: Make DEBUG part of .build_opts
    - BUILD: Show the value of DEBUG= in haproxy -vv
    - BUG/MEDIUM: http_act: Restore init of log-format list
    - BUG/MAJOR: peers: fix partial message decoding
    - DOC: better document the config file format and escaping/quoting rules
    - DOC: Clarify %HP description in log-format
    - BUG/MINOR: tcpcheck: Don't forget to reset tcp-check flags on new kind of check
    - MINOR: tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main
    - BUG/MAJOR: tcpcheck: Allocate input and output buffers from the buffer pool
    - DOC: config: Move req.hdrs and req.hdrs_bin in L7 samples fetches section
    - BUG/MINOR: http-fetch: Fix smp_fetch_body() when called from a health-check

4 years agoBUG/MINOR: http-fetch: Fix smp_fetch_body() when called from a health-check
Christopher Faulet [Wed, 25 Nov 2020 07:08:08 +0000 (08:08 +0100)]
BUG/MINOR: http-fetch: Fix smp_fetch_body() when called from a health-check

res.body may be called from a health-check. It is probably never used. But it is
possibe. In such case, there is no channel. Thus we must not use it
unconditionally to set the flag SMP_F_MAY_CHANGE on the smp.

Now the condition test the channel first. In addtion, the flag is not set if the
payload is fully received.

This patch must be backported as far as 2.2.

(cherry picked from commit a9ffc416377e0df9859526dc3c1d769c6a68636f)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoDOC: config: Move req.hdrs and req.hdrs_bin in L7 samples fetches section
Christopher Faulet [Tue, 24 Nov 2020 16:13:24 +0000 (17:13 +0100)]
DOC: config: Move req.hdrs and req.hdrs_bin in L7 samples fetches section

req.hdrs and req.hdrs_bin are L7 sample fetches, not L6. They were in the wrong
section.

This patch may be backported as far as 1.8.

(cherry picked from commit 687a68e2d04900e84317d066de80891a7f848747)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUG/MAJOR: tcpcheck: Allocate input and output buffers from the buffer pool
Christopher Faulet [Wed, 25 Nov 2020 12:47:00 +0000 (13:47 +0100)]
BUG/MAJOR: tcpcheck: Allocate input and output buffers from the buffer pool

Historically, the input and output buffers of a check are allocated by hand
during the startup, with a specific size (not necessarily the same than
other buffers). But since the recent refactoring of the checks to rely
exclusively on the tcp-checks and to use the underlying mux layer, this part
is totally buggy. Indeed, because these buffers are now passed to a mux,
they maybe be swapped if a zero-copy is possible. In fact, for now it is
only possible in h2_rcv_buf(). Thus the bug concretely only exists if a h2
health-check is performed. But, it is a latent bug for other muxes.

Another problem is the size of these buffers. because it may differ for the
other buffer size, it might be source of bugs.

Finally, for configurations with hundreds of thousands of servers, having 2
buffers per check always allocated may be an issue.

To fix the bug, we now allocate these buffers when required using the buffer
pool. Thus not-running checks don't waste memory and muxes may swap them if
possible. The only drawback is the check buffers have now always the same
size than buffers used by the streams. This deprecates indirectly the
"tune.chksize" global option.

In addition, the http-check regtest have been update to perform some h2
health-checks.

Many thanks to @VigneshSP94 for its help on this bug.

This patch should solve the issue #936. It relies on the commit "MINOR:
tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main".
Both must be backport as far as 2.2.

bla

(cherry picked from commit b381a505c1010bb11abbe7b31e8d2307c4dab541)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoMINOR: tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main
Christopher Faulet [Wed, 25 Nov 2020 12:34:51 +0000 (13:34 +0100)]
MINOR: tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main

The special handling of in-progress send rules at the begining of
tcpcheck_main() function can be removed. Instead, at the begining of the
tcpcheck_eval_send() function, we test is there is some data in the output
buffer. In this case, it means we are evaluating an unfinished send rule and
we can jump to the sending part, skipping the formatting part.

This patch is mandatory for a major fix on the checks and must be backported
as far as 2.2.

(cherry picked from commit 39066c27384653b41b95370fc4dec4469a637a23)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUG/MINOR: tcpcheck: Don't forget to reset tcp-check flags on new kind of check
Christopher Faulet [Wed, 25 Nov 2020 15:43:12 +0000 (16:43 +0100)]
BUG/MINOR: tcpcheck: Don't forget to reset tcp-check flags on new kind of check

When a new kind of check is found during the parsing of a proxy section (via
an option directive), we must reset tcpcheck flags for this proxy. It is
mandatory to not inherit some flags from a previously declared check (for
instance in the default section).

This patch must be backported as far as 2.2.

(cherry picked from commit 1faf18ae3976cddfb17afa95ea447205330c821e)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoDOC: Clarify %HP description in log-format
Maciej Zdeb [Thu, 26 Nov 2020 10:45:52 +0000 (10:45 +0000)]
DOC: Clarify %HP description in log-format

%HP is used to report HTTP request URI in logs, which might be relative
or absolute. Description in documentation should not suggest that it
behaves exactly the same as "path" sample fetch.

This is even more important after 30ee1efe676e8264af16bab833c621d60a72a4d7
because right now, when HTTP2 is a standard, %HP usually returns absolute
URI.

This might be backported as far as 2.1

(cherry picked from commit 21acc33266cc04eac5e70c22839d622284a6f46a)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoDOC: better document the config file format and escaping/quoting rules
Willy Tarreau [Wed, 25 Nov 2020 18:58:20 +0000 (19:58 +0100)]
DOC: better document the config file format and escaping/quoting rules

It's always a pain to figure how to proceed when special characters need
to be embedded inside arguments of an expression. Let's document the
configuration file format and how unquoting/unescaping works at each
level (top level and argument level) so that everyone hopefully finds
suitable reminders or examples for complex cases.

This is related to github issue #200 and addresses issues #712 and #966.

(cherry picked from commit 6f1129d14dace99687f8681bf825dfda2905502a)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUG/MAJOR: peers: fix partial message decoding
Willy Tarreau [Thu, 26 Nov 2020 16:06:04 +0000 (17:06 +0100)]
BUG/MAJOR: peers: fix partial message decoding

Another bug in the peers message parser was uncovered by last commit
1dfd4f106 ("BUG/MEDIUM: peers: fix decoding of multi-byte length in
stick-table messages"): the function return on incomplete message does
not check if the channel has a pending close before deciding to return
0. It did not hurt previously because the loop calling co_getblk() once
per character would have depleted the buffer and hit the end, causing
<0 to be returned and matching the condition. But now that we process
at once what is available this cannot be relied on anymore and it's
now clearly visible that the final check is missing.

What happens when this strikes is that if a peer connection breaks in
the middle of a message, the function will return 0 (missing data) but
the caller doesn't check for the closed buffer, subscribes to reads,
and the applet handler is immediately called again since some data are
still available. This is detected by the loop prevention and the process
dies complaining that an appctx is spinning.

This patch simply adds the check for closed channel. It must be
backported to the same versions as the fix above.

(cherry picked from commit 345ebcfc010e397cb718400a32b4db845dda7a2f)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUG/MEDIUM: http_act: Restore init of log-format list
Maciej Zdeb [Mon, 23 Nov 2020 16:03:09 +0000 (16:03 +0000)]
BUG/MEDIUM: http_act: Restore init of log-format list

Restore init of log-format list in parse_http_del_header which was
accidently deleted by commit ebdd4c55da4360bde7878604ea528c2031a26541
(implementation of different header matching methods for
http-request/response del-header).

This is related to GitHub issue #909

(cherry picked from commit 6dee9969b9b1ff131b49f09000234a21f194b014)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUILD: Show the value of DEBUG= in haproxy -vv
Tim Duesterhus [Sat, 21 Nov 2020 17:07:59 +0000 (18:07 +0100)]
BUILD: Show the value of DEBUG= in haproxy -vv

Previously this was not visible after building.

(cherry picked from commit c8d19702f46867900d7b4de240c168f1c1bb594e)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUILD: Make DEBUG part of .build_opts
Tim Duesterhus [Sat, 21 Nov 2020 17:07:58 +0000 (18:07 +0100)]
BUILD: Make DEBUG part of .build_opts

This forces a recompilation if the value of DEBUG= changes.

(cherry picked from commit 81e948e05160099272b01e31c72796477a4ab472)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoMINOR: http_act: Add -m flag for del-header name matching method
Maciej Zdeb [Fri, 20 Nov 2020 13:58:48 +0000 (13:58 +0000)]
MINOR: http_act: Add -m flag for del-header name matching method

This patch adds -m flag which allows to specify header name
matching method when deleting headers from http request/response.
Currently beg, end, sub, str and reg are supported.

This is related to GitHub issue #909

(cherry picked from commit ebdd4c55da4360bde7878604ea528c2031a26541)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoREGTESTS: converter: add url_dec test
William Dauchy [Sun, 15 Nov 2020 13:04:43 +0000 (14:04 +0100)]
REGTESTS: converter: add url_dec test

while looking at `url_dec` implementation I realised there was not yet a
simple test to avoid future regressions.
This one is testing simple case, including the "+" behaviour depending
on the argument passed to `url_dec`

Signed-off-by: William Dauchy <wdauchy@gmail.com>
(cherry picked from commit a2a46ee5724ca10640caad2fd2cc8648e11c1d9c)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoREGTESTS: Add sample_fetches/cook.vtc
Tim Duesterhus [Fri, 13 Nov 2020 18:36:47 +0000 (19:36 +0100)]
REGTESTS: Add sample_fetches/cook.vtc

Add a reg-test verifying the fix in dea7c209f8a77b471323dd97bdc1ac4d7a17b812.

Some parts of the configuration used in the were taken from the initial bug
report from Maciej.

Should be backported together with dea7c209f8a77b471323dd97bdc1ac4d7a17b812
(all stable versions).

Co-authored-by: Maciej Zdeb <maciej@zdeb.pl>
(cherry picked from commit afe36e457fbc21d403004d664ff0d926f0dea401)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoDOC: cache: Add new caching limitation information
Remi Tricot-Le Breton [Thu, 26 Nov 2020 14:51:29 +0000 (15:51 +0100)]
DOC: cache: Add new caching limitation information

Responses that do not have an explicit expiration time or a validator
will not be cached anymore.

Must be backported if cc9bf2e ("MEDIUM: cache: Change caching
conditions") is backported.

(cherry picked from commit d493bc863d85182b663d4a8869ec805d74e712e8)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoMEDIUM: cache: Change caching conditions
Remi Tricot-Le Breton [Thu, 12 Nov 2020 10:14:41 +0000 (11:14 +0100)]
MEDIUM: cache: Change caching conditions

Do not cache responses that do not have an explicit expiration time
(s-maxage or max-age Cache-Control directives or Expires header) or a
validator (ETag or Last-Modified headers) anymore, as suggested in
RFC 7234#3.
The TX_FLAG_IGNORE flag is used instead of the TX_FLAG_CACHEABLE so as
not to change the behavior of the checkcache option.

(cherry picked from commit cc9bf2e5fe1fe6f15de9e78b6aaea2cd6be5ca4f)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUG/MAJOR: filters: Always keep all offsets up to date during data filtering
Christopher Faulet [Tue, 24 Nov 2020 08:49:01 +0000 (09:49 +0100)]
BUG/MAJOR: filters: Always keep all offsets up to date during data filtering

When at least one data filter is registered on a channel, the offsets of all
filters must be kept up to date. For data filters but also for others. It is
safer to do it in that way. Indirectly, this patch fixes 2 hidden bugs
revealed by the commit 22fca1f2c ("BUG/MEDIUM: filters: Forward all filtered
data at the end of http filtering").

The first one, the worst of both, happens at the end of http filtering when
at least one data filtered is registered on the channel. We call the
http_end() callback function on the filters, when defined, to finish the
http filtering. But it is performed for all filters. Before the commit
22fca1f2c, the only risk was to call the http_end() callback function
unexpectedly on a filter. Now, we may have an overflow on the offset
variable, used at the end to forward all filtered data. Of course, from the
moment we forward an arbitrary huge amount of data, all kinds of bad things
may happen. So offset computation is performed for all filters and
http_end() callback function is called only for data filters.

The other one happens when a data filter alter the data of a channel, it
must update the offsets of all previous filters. But the offset of non-data
filters must be up to date, otherwise, here too we may have an integer
overflow.

Another way to fix these bugs is to always ignore non-data filters from the
offsets computation. But this patch is safer and probably easier to
maintain.

This patch must be backported in all versions where the above commit is. So
as far as 2.0.

(cherry picked from commit 401e6dbff3ee0b1932f6a16e3f280246752a7edf)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoDOC: better describes how to configure a fallback crt
Joao Morais [Tue, 24 Nov 2020 11:24:30 +0000 (08:24 -0300)]
DOC: better describes how to configure a fallback crt

A default certificate is always the first one declared in the bind line,
either from `crt` or from `crt-line` option. This commit updates the
description of how to configure a fallback certificate, clarifying that
it needs to be the first one of the bind line.

Should be merged as far as the first SNI filter implementation.

(cherry picked from commit aa8fcc4692b8c2afda455199a694067fea9e9262)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: http_htx: Fix searching headers by substring
Maciej Zdeb [Fri, 20 Nov 2020 12:12:24 +0000 (12:12 +0000)]
BUG/MINOR: http_htx: Fix searching headers by substring

Function __http_find_header is used to search headers by name using specified
matching method. Matching by substring returned unexpected results due to wrong
length of substring supplied to strnistr function.

Fixed also the boolean condition by inverting it, as we're interested in
headers that contains the substring.

This patch should be backported as far as 2.2

(cherry picked from commit 302b9f8d7a3805bfd25ecf267a8ddd730c6887b3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MAJOR: connection: reset conn->owner when detaching from session list
Willy Tarreau [Fri, 20 Nov 2020 16:22:44 +0000 (17:22 +0100)]
BUG/MAJOR: connection: reset conn->owner when detaching from session list

Baptiste reported a new crash affecting 2.3 which can be triggered
when using H2 on the backend, with http-reuse always and with a tens
of clients doing close only. There are a few combined cases which cause
this to happen, but each time the issue is the same, an already freed
session is dereferenced in session_unown_conn().

Two cases were identified to cause this:
  - a connection referencing a session as its owner, which is detached
    from the session's list and is destroyed after this session ends.
    The test on conn->owner before calling session_unown_conn() is not
    sufficent as the pointer is not null but is not valid anymore.

  - a connection that never goes idle and that gets killed form the
    mux, where session_free() is called first, then conn_free() calls
    session_unown_conn() which scans the just freed session for older
    connections. This one is only triggered with DEBUG_UAF

The reason for this session to be present here is that it's needed during
the connection setup, to be passed to conn_install_mux_be() to mux->init()
as the owning session, but it's never deleted aftrewards. Furthermore, even
conn_session_free() doesn't delete this pointer after freeing the session
that lies there. Both do definitely result in a use-after-free that's more
easily triggered under DEBUG_UAF.

This patch makes sure that the owner is always deleted after detaching
or killing the session. However it is currently not possible to clear
the owner right after a synchronous init because the proxy protocol
apparently needs it (a reg test checks this), and if we leave it past
the connection setup with the session not attached anywhere, it's hard
to catch the right moment to detach it. This means that the session may
remain in conn->owner as long as the connection has never been added to
nor removed from the session's idle list. Given that this patch needs to
remain simple enough to be backported, instead it adds a workaround in
session_unown_conn() to detect that the element is already not attached
anywhere.

This fix absolutely requires previous patch "CLEANUP: connection: do not
use conn->owner when the session is known" otherwise the situation will
be even worse, as some places used to rely on conn->owner instead of the
session.

The fix could theorically be backported as far as 1.8. However, the code
in this area has significantly changed along versions and there are more
risks of breaking working stuff than fixing real issues there. The issue
was really woken up in two steps during 2.3-dev when slightly reworking
the idle conns with commit 08016ab82 ("MEDIUM: connection: Add private
connections synchronously in session server list") and when adding
support for storing used H2 connections in the session and adding the
necessary call to session_unown_conn() in the muxes. But the same test
managed to crash 2.2 when built in DEBUG_UAF and patched like this,
proving that we used to already leave dangling pointers behind us:

|  diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
|  index f8f235c1a..dd30b5f80 100644
|  --- a/include/haproxy/connection.h
|  +++ b/include/haproxy/connection.h
|  @@ -458,6 +458,10 @@ static inline void conn_free(struct connection *conn)
|                          sess->idle_conns--;
|                  session_unown_conn(sess, conn);
|          }
|  +       else {
|  +               struct session *sess = conn->owner;
|  +               BUG_ON(sess && sess->origin != &conn->obj_type);
|  +       }
|
|          sockaddr_free(&conn->src);
|          sockaddr_free(&conn->dst);

It's uncertain whether an existing code path there can lead to dereferencing
conn->owner when it's bad, though certain suspicious memory corruption bugs
make one think it's a likely candidate. The patch should not be hard to
adapt there.

Backports to 2.1 and older are left to the appreciation of the person
doing the backport.

A reproducer consists in this:

  global
    nbthread 1

  listen l
    bind :9000
    mode http
    http-reuse always
    server s 127.0.0.1:8999 proto h2

  frontend f
    bind :8999 proto h2
    mode http
    http-request return status 200

Then this will make it crash within 2-3 seconds:

  $ h1load -e -r 1 -c 10 http://0:9000/

If it does not, it might be that DEBUG_UAF was not used (it's harder then)
and it might be useful to restart.

(cherry picked from commit 3aab17bd56614f05cfbec553e618b774ed07cd45)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoCLEANUP: connection: do not use conn->owner when the session is known
Willy Tarreau [Fri, 20 Nov 2020 16:08:15 +0000 (17:08 +0100)]
CLEANUP: connection: do not use conn->owner when the session is known

At a few places we used to rely on conn->owner to retrieve the session
while the session is already known. This is not correct because at some
of these points the reason the connection's owner was still the session
(instead of NULL) is a mistake. At one place a comparison is even made
between the session and conn->owner assuming it's valid without checking
if it's NULL. Let's clean this up to use the session all the time.

Note that this will be needed for a forthcoming fix and will have to be
backported.

(cherry picked from commit 38b4d2eb22fec0f11af50f8a9977ccae0e7c66c6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoDOC: clarify how to create a fallback crt
Joao Morais [Sat, 21 Nov 2020 10:42:20 +0000 (07:42 -0300)]
DOC: clarify how to create a fallback crt

HAProxy uses CN and SAN of the certificates to match incoming SNI, and
use the matching certificate in the TLS handshake. `crt-list` goes
further and allows to configure SNI filters to explicitly define the
FQDNs that should match a certificate.

The first declared certificate of the `crt-list` option follows the same
rules, and it's also used as a fallback - the certificate that should be
used if SNI isn't provided or the provided one cannot match any
certificate or SNI filter. If a provided SNI matches the CN or SAN of
the first certificate, the first certificate would be used even if a
matching SNI filter is declared later.

This change clarifies this scenario and documents a filter that can be
used to convert the first declared certificate as a proper fallback.

Should be merged as far as the first SNI filter implementation.

(cherry picked from commit e51fab0a4aba977b111c9d0926d6adbaf62204a1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUILD: makefile: enable crypt(3) for OpenBSD
Matthieu Guegan [Fri, 20 Nov 2020 09:50:39 +0000 (10:50 +0100)]
BUILD: makefile: enable crypt(3) for OpenBSD

Allow OpenBSD to support encrypted passwords in Userlists.

OpenBSD's crypt(3) function is provided directly by libc and does not
require -lcrypt.
Signed-off-by: Matthieu Guegan <matthieu.guegan@deindeal.ch>
(cherry picked from commit 496374e59246349fe0c84af68c5490ba5b6bad33)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: ssl/crt-list: fix error when no file found
William Lallemand [Fri, 20 Nov 2020 17:26:09 +0000 (18:26 +0100)]
BUG/MEDIUM: ssl/crt-list: fix error when no file found

When a file from a crt-list was not found, this one was ignored silently
letting HAProxy starts without it.

This bug was introduced by 47da821 ("MEDIUM: ssl: emulates the
multi-cert bundles in the crtlist").

This commit adds a found variable which is checked once we tried every
bundle combination so we can exits with an error if none were found.

Must be backported in 2.3.

(cherry picked from commit 77e1c6fb0a5c5704315f05162f9a964bd8689c41)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: ssl/crt-list: load bundle in crt-list only if activated
William Lallemand [Fri, 20 Nov 2020 17:23:40 +0000 (18:23 +0100)]
BUG/MINOR: ssl/crt-list: load bundle in crt-list only if activated

Don't try to load a bundle from a crt-list if the bundle support was
disabled with ssl-load-extra-files.

Must be backported to 2.3.

(cherry picked from commit 7340457158b20fa89d9eba0e231b3a122f5620d3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: ssl: error when no certificate are found
William Lallemand [Fri, 20 Nov 2020 14:36:13 +0000 (15:36 +0100)]
BUG/MEDIUM: ssl: error when no certificate are found

When a non-existing file was specified in the configuration, haproxy
does not exits with an error which is not normal.

This bug was introduced by dfa93be ("MEDIUM: ssl: emulate multi-cert
bundles loading in standard loading") which does nothing if the stat
failed.

This patch introduce a "found" variable which is checked at the end of
the function so we exit with an error if no find were found.

Must be backported to 2.3.

(cherry picked from commit 06ce84a10079388e95e5959dbcb50fcc563c702b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: ssl/crt-list: bundle support broken in crt-list
William Lallemand [Fri, 20 Nov 2020 13:23:38 +0000 (14:23 +0100)]
BUG/MEDIUM: ssl/crt-list: bundle support broken in crt-list

In issue #970 it was reported that the bundle loading does not work
anymore with crt-list.

This bug was introduced by 47da821 ("MEDIUM: ssl: emulates the
multi-cert bundles in the crtlist") which incorrectly uses "path"
instead of "crt_path" in the name resolution.

Must be backported to 2.3.

(cherry picked from commit 86c2dd60f1a0b8332a6ffc6e95dca27470fe44f7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: http-ana: Don't eval http-after-response ruleset on empty messages
Christopher Faulet [Wed, 18 Nov 2020 15:44:02 +0000 (16:44 +0100)]
BUG/MEDIUM: http-ana: Don't eval http-after-response ruleset on empty messages

It is not possible on response comming from a server, but an errorfile may be
empty. In this case, the http-after-response ruleset must not be evaluated
because it is totally unexpected to manipulate headers on an empty HTX message.

This patch must be backported everywhere the http-after-response rules are
supported, i.e as far as 2.2.

(cherry picked from commit aab1b67383993b93bd70144825bc4350e3986a10)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: ssl: segv on startup when AKID but no keyid
William Lallemand [Thu, 19 Nov 2020 15:24:13 +0000 (16:24 +0100)]
BUG/MINOR: ssl: segv on startup when AKID but no keyid

In bug #959 it was reported that haproxy segfault on startup when trying
to load a certifcate which use the X509v3 AKID extension but without the
keyid field.

This field is not mandatory and could be replaced by the serial or the
DirName.

For example:

   X509v3 extensions:
       X509v3 Basic Constraints:
           CA:FALSE
       X509v3 Subject Key Identifier:
           42:7D:5F:6C:3E:0D:B7:2C:FD:6A:8A:32:C6:C6:B9:90:05:D1:B2:9B
       X509v3 Authority Key Identifier:
           DirName:/O=HAProxy Technologies/CN=HAProxy Test Intermediate CA
           serial:F2:AB:C1:41:9F:AB:45:8E:86:23:AD:C5:54:ED:DF:FA

This bug was introduced by 70df7b ("MINOR: ssl: add "issuers-chain-path" directive").

This patch must be backported as far as 2.2.

(cherry picked from commit f69cd6873756510e6e4ef82624c16719da6f2c80)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoDOC: add missing 3.10 in the summary
William Lallemand [Wed, 18 Nov 2020 09:41:24 +0000 (10:41 +0100)]
DOC: add missing 3.10 in the summary

3.10. Log forwarding was missing in the summary.

(cherry picked from commit 0217b7b24bb33d746d2bf625f5e894007517d1b0)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoBUG/MINOR: http-ana: Don't wait for the body of CONNECT requests
Christopher Faulet [Mon, 16 Nov 2020 15:03:35 +0000 (16:03 +0100)]
BUG/MINOR: http-ana: Don't wait for the body of CONNECT requests

CONNECT requests are bodyless messages but with no EOM blocks. Thus, conditions
to stop waiting for the message payload are not suited to this kind of
messages. Indeed, the message finishes on an EOH block. But the tunnel mode at
the stream level is only set in HTTP_XFER_BODY analyser. So, the stream is
blocked, waiting for a body that does not exist till a timeout expires.

To fix this bug, we just stop waiting for a body for CONNECT requests. Another
solution is to rely on HTX_SL_F_BODYLESS/HTTP_MSGF_BODYLESS flags. But this one
is less intrusive.

This message must be backported as far as 2.0. For the 2.0, only the HTX part
must be fixed.

(cherry picked from commit 63c69a9b4ef1136c66967463b9e4b3538e35c016)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: filters: Forward all filtered data at the end of http filtering
Christopher Faulet [Mon, 16 Nov 2020 09:10:38 +0000 (10:10 +0100)]
BUG/MEDIUM: filters: Forward all filtered data at the end of http filtering

When http filtering ends, if there are some filtered data not forwarded yet, we
forward them, in flt_http_end(). Most of time, this doesn't happen, except when
a tunnel is established using a CONNECT. In this case, there is not EOM on the
request and there is no body. Thus the headers are never forwarded, blocking the
stream.

This patch must be backported as far as 2.0. Prior versions don't suffer of this
bug because there is no HTX support. On the 2.0, the change is only applicable
on HTX streams. A special test must be performed to make sure.

(cherry picked from commit 22fca1f2c84334096e38d78ffc674be19c3e0292)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoCLEANUP: cfgparse: remove duplicate registration for transparent build options
Jerome Magnin [Wed, 30 Sep 2020 16:05:38 +0000 (18:05 +0200)]
CLEANUP: cfgparse: remove duplicate registration for transparent build options

Since commit 37bafdcbb ("MINOR: sock_inet: move the IPv4/v6 transparent mode code
to sock_inet"), build options for transparent proxying are registered twice.
This patch removes the older one.

(cherry picked from commit eff2e0a958816ace5ea71c4e958dff647b5bf170)
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoBUILD: http-htx: fix build warning regarding long type in printf
Willy Tarreau [Fri, 6 Nov 2020 13:24:02 +0000 (14:24 +0100)]
BUILD: http-htx: fix build warning regarding long type in printf

Commit a66adf41e ("MINOR: http-htx: Add understandable errors for the
errorfiles parsing") added a warning when loading malformed error files,
but this warning may trigger another build warning due to the %lu format
used. Let's simply cast it for output since it's just used for end user
output.

This must be backported to 2.0 like the commit above.

(cherry picked from commit 431a12cafeeec7300b7cea7e19b892d4e8c4900d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years ago[RELEASE] Released version 2.3.1 v2.3.1
William Lallemand [Fri, 13 Nov 2020 20:20:03 +0000 (21:20 +0100)]
[RELEASE] Released version 2.3.1

Released version 2.3.1 with the following main changes :
    - BUG/MINOR: ssl: don't report 1024 bits DH param load error when it's higher
    - MINOR: http-htx: Add understandable errors for the errorfiles parsing
    - DOC: config: Fix a typo on ssl_c_chain_der
    - BUG/MEDIUM: ssl/crt-list: correctly insert crt-list line if crt already loaded
    - BUG/MINOR: pattern: a sample marked as const could be written
    - BUG/MINOR: lua: set buffer size during map lookups
    - BUG/MINOR: stats: free dynamically stats fields/lines on shutdown
    - BUG/MINOR: peers: Do not ignore a protocol error for dictionary entries.
    - BUG/MINOR: peers: Missing TX cache entries reset.
    - BUG/MEDIUM: peers: fix decoding of multi-byte length in stick-table messages
    - BUG/MINOR: http-fetch: Extract cookie value even when no cookie name
    - BUG/MINOR: http-fetch: Fix calls w/o parentheses of the cookie sample fetches
    - BUG/MEDIUM: check: reuse srv proto only if using same mode
    - MINOR: check: report error on incompatible proto
    - MINOR: check: report error on incompatible connect proto
    - BUG/MINOR: http-htx: Handle warnings when parsing http-error and http-errors
    - BUG/MAJOR: spoe: Be sure to remove all references on a released spoe applet
    - MINOR: spoe: Don't close connection in sync mode on processing timeout
    - BUG/MINOR: tcpcheck: Don't warn on unused rules if check option is after
    - MINOR: init: Fix the prototype for per-thread free callbacks
    - MINOR: config/mux-h2: Return ERR_ flags from init_h2() instead of a status
    - MINOR: cfgparse: tighten the scope of newnameserver variable, free it on error.
    - REGTEST: ssl: test wildcard and multi-type + exclusions
    - REGTEST: ssl: mark reg-tests/ssl/ssl_crt-list_filters.vtc as broken
    - MINOR: peers: Add traces to peer_treat_updatemsg().
    - REGTEST: make ssl_client_samples and ssl_server_samples require to 2.2

4 years agoREGTEST: make ssl_client_samples and ssl_server_samples require to 2.2
Christopher Faulet [Fri, 13 Nov 2020 16:10:51 +0000 (17:10 +0100)]
REGTEST: make ssl_client_samples and ssl_server_samples require to 2.2

Some missing sample fetches was backported to 2.2 making these tests compatible
with the 2.2.

(cherry picked from commit c300747decc4a554a52d07acfd674ab7855ae15f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: peers: Add traces to peer_treat_updatemsg().
Frédéric Lécaille [Tue, 10 Nov 2020 15:18:03 +0000 (16:18 +0100)]
MINOR: peers: Add traces to peer_treat_updatemsg().

Add minimalistic traces for peers with only one event to diagnose potential
issues when decode peer update messages.

(cherry picked from commit d865935f3212f994d8868200d9a84315dbce1518)
[wt: also merge traces from f9e51beec and 1dfd4f10]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoREGTEST: ssl: mark reg-tests/ssl/ssl_crt-list_filters.vtc as broken
William Lallemand [Tue, 10 Nov 2020 21:40:24 +0000 (22:40 +0100)]
REGTEST: ssl: mark reg-tests/ssl/ssl_crt-list_filters.vtc as broken

This regtest requires a version of OpenSSL which supports the
ClientHello callback which is only the case of recents SSL libraries
(openssl 1.1.1).

This was reported in issue #944.

(cherry picked from commit 8f04e1849d40243ba7c103b06bd2c061840ceabc)
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoREGTEST: ssl: test wildcard and multi-type + exclusions
William Lallemand [Fri, 6 Nov 2020 13:46:36 +0000 (14:46 +0100)]
REGTEST: ssl: test wildcard and multi-type + exclusions

This test checks that the bug #818 and #810 are fixed.

It test if there is no inconsistency with multiple certificate types and
that the exclusion of the certificate is correctly working with a negative
filter.

(cherry picked from commit 3ff9591ea21848257f9ae7a7a700654649465c85)
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoMINOR: cfgparse: tighten the scope of newnameserver variable, free it on error.
Eric Salama [Fri, 13 Nov 2020 14:56:36 +0000 (15:56 +0100)]
MINOR: cfgparse: tighten the scope of newnameserver variable, free it on error.

This should fix issue GH #931.

Also remove a misleading comment.

This commit can be backported as far as 1.9

(cherry picked from commit 9139ec34ed7aae4fe63926ca5c87a5b32b376488)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: config/mux-h2: Return ERR_ flags from init_h2() instead of a status
Christopher Faulet [Fri, 6 Nov 2020 14:23:39 +0000 (15:23 +0100)]
MINOR: config/mux-h2: Return ERR_ flags from init_h2() instead of a status

post-check function callbacks must return ERR_* flags. Thus, init_h2() is fixed
to return ERR_NONE on success or (ERR_ALERT|ERR_FATAL) on error.

This patch may be backported as far as 2.2.

(cherry picked from commit 5214099233d99b8288fb392dfe911bc9ea86e9af)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: init: Fix the prototype for per-thread free callbacks
Christopher Faulet [Fri, 6 Nov 2020 14:19:19 +0000 (15:19 +0100)]
MINOR: init: Fix the prototype for per-thread free callbacks

Functions registered to release memory per-thread have no return value. But the
registering function and the function pointer in per_thread_free_fct structure
specify it should return an integer. This patch fixes it.

This patch may be backported as far as 2.0.

(cherry picked from commit 83fefbcdff9f562d719fcb5da6b4a11e20e13c7c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: tcpcheck: Don't warn on unused rules if check option is after
Christopher Faulet [Fri, 13 Nov 2020 07:55:57 +0000 (08:55 +0100)]
BUG/MINOR: tcpcheck: Don't warn on unused rules if check option is after

When tcp-check or http-check rules are used, if the corresponding check option
(option tcp-check and option httpchk) is declared after the ruleset, a warning
is emitted about an unused check ruleset while there is no problem in reality.

This patch must be backported as far as 2.2.

(cherry picked from commit c751b4508d9245baa54a19f7658aa7307d76e447)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: spoe: Don't close connection in sync mode on processing timeout
Christopher Faulet [Tue, 10 Nov 2020 13:31:39 +0000 (14:31 +0100)]
MINOR: spoe: Don't close connection in sync mode on processing timeout

In sync mode, if an applet receives a ack while the processing delay has already
expired, there is not frame waiting for this ack. But there is no reason to
close the connection in this case. The ack may be ignored and the connection may
be reused to process another frame. The only reason to trigger an error and
close the connection is when the wrong ack is received while there is still a
frame waiting for its ack. In sync mode, this should never happen.

This patch may be backported in all versions supporting the SPOE.

(cherry picked from commit c7ba91039a7b0703971efd791ca2ca609afedb96)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MAJOR: spoe: Be sure to remove all references on a released spoe applet
Christopher Faulet [Tue, 10 Nov 2020 17:45:34 +0000 (18:45 +0100)]
BUG/MAJOR: spoe: Be sure to remove all references on a released spoe applet

When a SPOE applet is used to send a frame, a reference on this applet is saved
in the spoe context of the offladed stream. But, if the applet is released
before receving the corresponding ack, we must be sure to remove this
reference. This was performed for fragmented frames only. But it must also be
performed for a spoe contexts in the applet waiting_queue and in the thread
waiting_queue (used in async mode).

This bug leads to a memory corruption when an offloaded stream try to update the
state of a released applet because it still have a reference on it. There are
many ways to trigger this bug. The easiest is probably during reloads. On the
old process, all applets are woken up to be released ASAP.

Many thanks to Maciej Zdeb to report the bug and to work on it for 2
months. Without his help, it would have been much more difficult to fix the
bug. It is always a huge pleasure to see how some users are enthousiast and
helpful. Thanks again Maciej !

This patch must be backported to all versions where the spoe is supported (>=
1.7).

(cherry picked from commit cf181c76e341f2d49f6cae0ca8200158058073f1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: http-htx: Handle warnings when parsing http-error and http-errors
Christopher Faulet [Fri, 13 Nov 2020 09:58:01 +0000 (10:58 +0100)]
BUG/MINOR: http-htx: Handle warnings when parsing http-error and http-errors

First of all, this patch is tagged as a bug. But in fact, it only fixes a bug in
the 2.2. On the 2.3 and above, it only add the ability to display warnings, when
an http-error directive is parsed from a proxy section and when an errorfile
directive is parsed from a http-errors section.

But on the 2.2, it make sure to display the warning emitted on a content-length
mismatch when an errorfile is parsed. The following is only applicable to the
2.2.

commit "BUG/MINOR: http-htx: Just warn if payload of an errorfile doesn't match
the C-L" (which is only present in 2.2, 2.1 and 2.0 trees, i.e see commit
7bf3d81d3cf4b9f4587 in 2.2 tree), is changing the behavior of `http_str_to_htx`
function. It may now emit warnings. And, it is the caller responsibility to
display it.

But the warning is missing when an 'http-error' directive is parsed from
a proxy section. It is also missing when an 'errorfile' directive is
parsed from a http-errors section.

This bug only exists on the 2.2. On earlier versions, these directives
are not supported and on later ones, an error is triggered instead of a
warning.

Thanks to William Dauchy that spotted the bug.

This patch must be backported as far as 2.2.

(cherry picked from commit 3005d28eb825f700820a8d38d6d7fa6ab51a1f80)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: check: report error on incompatible connect proto
Amaury Denoyelle [Fri, 13 Nov 2020 11:34:58 +0000 (12:34 +0100)]
MINOR: check: report error on incompatible connect proto

Report an error when using an explicit proto for a connect rule with
non-compatible mode in regards with the selected check type (tcp-check
vs http-check).

(cherry picked from commit 90eb93f79204336e38fdb859f61ab75ce44a4367)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: check: report error on incompatible proto
Amaury Denoyelle [Fri, 13 Nov 2020 11:34:57 +0000 (12:34 +0100)]
MINOR: check: report error on incompatible proto

If the check mux has been explicitly defined but is incompatible with
the selected check type (tcp-check vs http-check), report a warning and
prevent haproxy startup.

(cherry picked from commit 7c148901837c5deed0f33bc037480faf10cb9a1f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: check: reuse srv proto only if using same mode
Amaury Denoyelle [Fri, 13 Nov 2020 11:34:56 +0000 (12:34 +0100)]
BUG/MEDIUM: check: reuse srv proto only if using same mode

Only reuse the mux from server if the check is using the same mode.
For example, this prevents a tcp-check on a h2 server to select the h2
multiplexer instead of passthrough.

This bug was introduced by the following commit :
BUG/MEDIUM: checks: Use the mux protocol specified on the server line
It must be backported up to 2.2.

Fixes github issue #945.

(cherry picked from commit 0519bd4d04cc3455233d9a74c5d682a958fc02b2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: http-fetch: Fix calls w/o parentheses of the cookie sample fetches
Christopher Faulet [Fri, 13 Nov 2020 12:41:04 +0000 (13:41 +0100)]
BUG/MINOR: http-fetch: Fix calls w/o parentheses of the cookie sample fetches

req.cook, req.cook_val, req.cook_cnt and and their response counterparts may be
called without cookie name. In this case, empty parentheses may be used, or no
parentheses at all. In both, the result must be the same. But only the first one
works. The second one always returns a failure. This patch fixes this bug.

Note that on old versions (< 2.2), both cases fail.

This patch must be backported in all stable versions.

(cherry picked from commit 97fc8da2643531ade4163d6662f13f76fa59d677)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: http-fetch: Extract cookie value even when no cookie name
Maciej Zdeb [Fri, 13 Nov 2020 09:38:06 +0000 (09:38 +0000)]
BUG/MINOR: http-fetch: Extract cookie value even when no cookie name

HTTP sample fetches dealing with the cookies (req/res.cook,
req/res.cook_val and req/res.cook_cnt) must be prepared to be called
without cookie name. For the first two, the first cookie value is
returned, regardless its name. For the last one, all cookies are counted.

To do so, http_extract_cookie_value() may now be called with no cookie
name (cookie_name_l set to 0). In this case, the matching on the cookie
name is ignored and the first value found is returned.

Note this patch also fixes matching on cookie values in ACLs.

This should be backported in all stable versions.

(cherry picked from commit dea7c209f8a77b471323dd97bdc1ac4d7a17b812)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: peers: fix decoding of multi-byte length in stick-table messages
Willy Tarreau [Fri, 13 Nov 2020 13:10:20 +0000 (14:10 +0100)]
BUG/MEDIUM: peers: fix decoding of multi-byte length in stick-table messages

There is a bug in peer_recv_msg() due to an incorrect cast when trying
to decode the varint length of a stick-table message, causing lengths
comprised between 128 and 255 to consume one extra byte, ending in
protocol errors.  The root cause of this is that peer_recv_msg() tries
hard to reimplement all the parsing and control that is already done in
intdecode() just to measure the length before calling it. And it got it
wrong.

Let's just get rid of this unneeded code duplication and solely rely on
intdecode() instead. The bug was introduced in 2.0 as part of a cleanup
pass on this code with commit 95203f218 ("MINOR: peers: Move high level
receive code to reduce the size of I/O handler."), so this patch must
be backported to 2.0.

Thanks to Yves Lafon for reporting the problem.

(cherry picked from commit 1dfd4f106f15bc4e6e992f8babbc863c12975b5a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: peers: Missing TX cache entries reset.
Frédéric Lécaille [Thu, 12 Nov 2020 20:01:54 +0000 (21:01 +0100)]
BUG/MINOR: peers: Missing TX cache entries reset.

The TX part of a cache for a dictionary is made of an reserved array of ebtree nodes
which are pointers to dictionary entries. So when we flush the TX part of such a
cache, we must not only remove these nodes to dictionary entries from their ebtree.
We must also reset their values. Furthermore, the LRU key and the last lookup
result must also be reset.

(cherry picked from commit ea875e62e6b2f69c50533c5cd52eb5284c69723f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: peers: Do not ignore a protocol error for dictionary entries.
Frédéric Lécaille [Thu, 12 Nov 2020 18:53:11 +0000 (19:53 +0100)]
BUG/MINOR: peers: Do not ignore a protocol error for dictionary entries.

If we could not decode the ID of a dictionary entry from a peer update message,
we must inform the remote peer about such an error as this is done for
any other decoding error.

(cherry picked from commit f9e51beec118f1bbd558ed689fdad35046160529)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: stats: free dynamically stats fields/lines on shutdown
Amaury Denoyelle [Tue, 10 Nov 2020 13:24:30 +0000 (14:24 +0100)]
BUG/MINOR: stats: free dynamically stats fields/lines on shutdown

Register a new function on POST DEINIT to free stats fields/lines for
each domain.

This patch does not fix a critical bug but may be backported to 2.3.

(cherry picked from commit a2a6899bee5add180afb02ac2b1ce7cb360a96e3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: lua: set buffer size during map lookups
Thierry Fournier [Tue, 10 Nov 2020 19:38:20 +0000 (20:38 +0100)]
BUG/MINOR: lua: set buffer size during map lookups

This size is used by some pattern matching to determine if there
is sufficient room in the buffer to add final \0 if necessary.
If the size is not set, the conditions use uninitialized value.

Note: it seems this bug can't cause a crash.

Should be backported until 2.2 (at least)

(cherry picked from commit 91dc0c0d8fdc2fb091b49699ebb323d01aa1d9f6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: pattern: a sample marked as const could be written
Thierry Fournier [Tue, 10 Nov 2020 19:51:36 +0000 (20:51 +0100)]
BUG/MINOR: pattern: a sample marked as const could be written

The functions add final 0 to string if the final 0 is not set,
but don't check the flag CONST. This patch duplicates the strings
if the final zero is not set and the string is CONST.

Should be backported until 2.2 (at least)

(cherry picked from commit a68affeaa9377f88f773ef62a9bb2541dfb672d3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: ssl/crt-list: correctly insert crt-list line if crt already loaded
William Lallemand [Fri, 6 Nov 2020 15:24:07 +0000 (16:24 +0100)]
BUG/MEDIUM: ssl/crt-list: correctly insert crt-list line if crt already loaded

In issue #940, it was reported that the crt-list does not work correctly
anymore. Indeed when inserting a crt-list line which use a certificate
previously seen in the crt-list, this one won't be inserted in the SNI
list and will be silently ignored.

This bug was introduced by commit  47da821 "MEDIUM: ssl: emulates the
multi-cert bundles in the crtlist".

This patch also includes a reg-test which tests this issue.

This bugfix must be backported in 2.3.

(cherry picked from commit 50c03aac0417f7d70f98f31e513441c0fb743110)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

4 years agoDOC: config: Fix a typo on ssl_c_chain_der
Christopher Faulet [Fri, 6 Nov 2020 11:10:33 +0000 (12:10 +0100)]
DOC: config: Fix a typo on ssl_c_chain_der

There is a typo on the ssl_c_chain_der sample fetch
(s/ssl_c_der_chain/ssl_c_chain_der/). This implies a move of the fetch to keep
it at the right place.

This should be backported as far as 2.2 or anywhere the commit a598b500b
("MINOR: ssl: add ssl_{c,s}_chain_der fetch methods") is.

(cherry picked from commit 70d10d1fb657eb6f8ac8e86c9b6037b53cc907ac)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: http-htx: Add understandable errors for the errorfiles parsing
Christopher Faulet [Thu, 5 Nov 2020 21:43:41 +0000 (22:43 +0100)]
MINOR: http-htx: Add understandable errors for the errorfiles parsing

No details are provided when an error occurs during the parsing of an errorfile,
Thus it is a bit hard to diagnose where the problem is. Now, when it happens, an
understandable error message is reported.

This patch is not a bug fix in itself. But it will be required to change an
fatal error into a warning in last stable releases. Thus it must be backported
as far as 2.0.

(cherry picked from commit a66adf41ea28a0fa29437d1675f225b5cc589b59)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: ssl: don't report 1024 bits DH param load error when it's higher
Willy Tarreau [Thu, 5 Nov 2020 18:38:05 +0000 (19:38 +0100)]
BUG/MINOR: ssl: don't report 1024 bits DH param load error when it's higher

The default dh_param value is 2048 and it's preset to zero unless explicitly
set, so we must not report a warning about DH param not being loadble in 1024
bits when we're going to use 2048. Thanks to Dinko for reporting this.

This should be backported to 2.2.

(cherry picked from commit 6d27a92b83f75bab42bda08ed28b70fb95525fd9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years ago[RELEASE] Released version 2.3.0 v2.3.0
Willy Tarreau [Thu, 5 Nov 2020 16:04:53 +0000 (17:04 +0100)]
[RELEASE] Released version 2.3.0

Released version 2.3.0 with the following main changes :
    - CLEANUP: pattern: remove unused entry "tree" in pattern.val
    - BUILD: ssl: use SSL_CTRL_GET_RAW_CIPHERLIST instead of OpenSSL versions
    - BUG/MEDIUM: filters: Don't try to init filters for disabled proxies
    - BUG/MINOR: proxy/server: Skip per-proxy/server post-check for disabled proxies
    - BUG/MINOR: checks: Report a socket error before any connection attempt
    - BUG/MINOR: server: Set server without addr but with dns in RMAINT on startup
    - MINOR: server: Copy configuration file and line for server templates
    - BUG/MEDIUM: mux-pt: Release the tasklet during an HTTP upgrade
    - BUILD: ssl: use HAVE_OPENSSL_KEYLOG instead of OpenSSL versions
    - MINOR: debug: don't count free(NULL) in memstats
    - BUG/MINOR: filters: Skip disabled proxies during startup only
    - MINOR: mux_h2: capitalize frame type in stats
    - MINOR: mux_h2: add stat for total count of connections/streams
    - MINOR: stats: do not display empty stat module title on html
    - BUG/MEDIUM: stick-table: limit the time spent purging old entries
    - BUG/MEDIUM: listener: only enable a listening listener if needed
    - BUG/MEDIUM: listener: never suspend inherited sockets
    - BUG/MEDIUM: listener: make the master also keep workers' inherited FDs
    - MINOR: fd: add fd_want_recv_safe()
    - MEDIUM: listeners: make use of fd_want_recv_safe() to enable early receivers
    - REGTESTS: mark abns_socket as working now
    - CLEANUP: mux-h2: Remove the h1 parser state from the h2 stream
    - MINOR: sock: add a check against cross worker<->master socket activities
    - CI: github actions: limit OpenSSL no-deprecated builds to "default,bug,devel" reg-tests
    - BUG/MEDIUM: server: make it possible to kill last idle connections
    - MINOR: mworker/cli: the master CLI use its own applet
    - MINOR: ssl: define SSL_CTX_set1_curves_list to itself on BoringSSL
    - BUILD: ssl: use feature macros for detecting ec curves manipulation support
    - DOC: Add dns as an available domain to show stat
    - BUILD: makefile: usual reorder of objects for faster builds
    - DOC: update INSTALL to mention that TCC is supported
    - DOC: mention in INSTALL that haproxy 2.3 is a stable version
    - MINOR: version: mention that it's stable now

4 years agoMINOR: version: mention that it's stable now
Willy Tarreau [Thu, 5 Nov 2020 15:50:53 +0000 (16:50 +0100)]
MINOR: version: mention that it's stable now

This version will be maintained up to around Q1 2022.

4 years agoDOC: mention in INSTALL that haproxy 2.3 is a stable version
Willy Tarreau [Thu, 5 Nov 2020 16:00:07 +0000 (17:00 +0100)]
DOC: mention in INSTALL that haproxy 2.3 is a stable version

One day we'll have a script to update the status and support dates :-)

4 years agoDOC: update INSTALL to mention that TCC is supported
Willy Tarreau [Thu, 5 Nov 2020 15:56:37 +0000 (16:56 +0100)]
DOC: update INSTALL to mention that TCC is supported

TinyCC as found at https://repo.or.cz/tinycc.git does work to some extents
and is very convenient for developers, so let's mention it.

4 years agoBUILD: makefile: usual reorder of objects for faster builds
Willy Tarreau [Thu, 5 Nov 2020 15:42:25 +0000 (16:42 +0100)]
BUILD: makefile: usual reorder of objects for faster builds

Reordered the objets by reverse build times made the total build time
go down from 17.7s to 17.2s at -O2 using make -j8 on my PC, and from
~3.2 to ~2.7s on the build farm.

4 years agoDOC: Add dns as an available domain to show stat
Daniel Corbett [Sun, 1 Nov 2020 15:54:17 +0000 (10:54 -0500)]
DOC: Add dns as an available domain to show stat

Within management.txt, proxy was listed as the only available option. "dns"
is now supported so let's add that. This change also updates the command to list
the available options <dns|proxy> for "domain" as previously it only specified
<domain>, which could be confusing as a user may think this field accepts
dynamic options when it actually requires a specific keyword.

4 years agoBUILD: ssl: use feature macros for detecting ec curves manipulation support
Ilya Shipitsin [Tue, 3 Nov 2020 19:39:07 +0000 (00:39 +0500)]
BUILD: ssl: use feature macros for detecting ec curves manipulation support

Let us use SSL_CTX_set1_curves_list, defined by OpenSSL, as well as in
openssl-compat when SSL_CTRL_SET_CURVES_LIST is present (BoringSSL),
for feature detection instead of versions.

4 years agoMINOR: ssl: define SSL_CTX_set1_curves_list to itself on BoringSSL
Willy Tarreau [Thu, 5 Nov 2020 14:03:45 +0000 (15:03 +0100)]
MINOR: ssl: define SSL_CTX_set1_curves_list to itself on BoringSSL

OpenSSL 1.0.2 and onwards define SSL_CTX_set1_curves_list which is both a
function and a macro. OpenSSL 1.0.2 to 1.1.0 define SSL_CTRL_SET_CURVES_LIST
as a macro, which disappeared from 1.1.1. BoringSSL only has that one and
not the former macro but it does have the function. Let's keep the test on
the macro matching the function name by defining the macro to itself when
needed.

4 years agoMINOR: mworker/cli: the master CLI use its own applet
William Lallemand [Thu, 5 Nov 2020 09:28:53 +0000 (10:28 +0100)]
MINOR: mworker/cli: the master CLI use its own applet

Following the patch b4daee ("MINOR: sock: add a check against cross
worker<->master socket activities"), this patch adds a dedicated applet
for the master CLI. It ensures that the CLI connection can't be
used with the master rights in the case of bugs.

4 years agoBUG/MEDIUM: server: make it possible to kill last idle connections
Willy Tarreau [Thu, 5 Nov 2020 08:12:20 +0000 (09:12 +0100)]
BUG/MEDIUM: server: make it possible to kill last idle connections

In issue #933, @jaroslawr provided a report indicating that when using
many threads and many servers, it's very difficult to terminate the last
idle connections on each server. The issue has two causes in fact. The
first one is that during the calculation of the estimate of needed
connections, we round the computation up while in previous round it was
already rounded up, so we end up adding 1 to 1 which once divided by 2
remains 1. The second issue is that servers are not woken up anymore for
purging their connections if they don't have activity. The only reason
that was there to wake them up again was in case insufficient connections
were purged. And even then the purge task itself was not woken up. But
that is not enough for getting rid of the long tail of old connections
nor updating est_need_conns.

This patch makes sure to properly wake up as long as at least one idle
connection remains, and not to round up the needed connections anymore.

Prior to this patch, a test involving many connections which suddenly
stopped would keep many idle connections, now they're effectively halved
every pool-purge-delay.

This needs to be backported to 2.2.

4 years agoCI: github actions: limit OpenSSL no-deprecated builds to "default,bug,devel" reg...
Ilya Shipitsin [Tue, 3 Nov 2020 19:41:39 +0000 (00:41 +0500)]
CI: github actions: limit OpenSSL no-deprecated builds to "default,bug,devel" reg-tests

4 years agoMINOR: sock: add a check against cross worker<->master socket activities
Willy Tarreau [Wed, 4 Nov 2020 13:58:36 +0000 (14:58 +0100)]
MINOR: sock: add a check against cross worker<->master socket activities

Given that the previous issues caused spurious worker socket wakeups in
the master for inherited FDs that couldn't be closed, let's add a strict
test in the I/O callback to make sure that an accept() event is always
caught by the appropriate type of process (master for master listeners,
worker for worker listeners).

4 years agoCLEANUP: mux-h2: Remove the h1 parser state from the h2 stream
Christopher Faulet [Tue, 3 Nov 2020 17:25:52 +0000 (18:25 +0100)]
CLEANUP: mux-h2: Remove the h1 parser state from the h2 stream

Since the h2 multiplexer no longer relies on the legacy HTTP representation, and
uses exclusively the HTX, the H1 parser state (h1m) is no longer used by the h2
streams. Thus it can be removed.

This patch may be backported as far as 2.1.

4 years agoREGTESTS: mark abns_socket as working now
Willy Tarreau [Tue, 3 Nov 2020 17:43:48 +0000 (18:43 +0100)]
REGTESTS: mark abns_socket as working now

William noticed that the real issue in the abns test was that it was
failing to rebind some listeners, issues which were addressed in the
previous commits (in some cases, the master would even accept traffic
on the worker's socket which was not properly disabled, causing some
of the strange error messages).

The other issue documented in commit 2ea15a080 was the lack of
reliability when the test was run in parallel. This is caused by the
abns socket which uses a hard-coded address, making all tests in
parallel to step onto each others' toes. Since vtest cannot provide
abns sockets, we're instead concatenating the number of the listening
port that vtest allocated for another frontend to the abns path, which
guarantees to make them unique in the system.

The test works fine in all cases now, even with 100 in parallel.

4 years agoMEDIUM: listeners: make use of fd_want_recv_safe() to enable early receivers
Willy Tarreau [Wed, 4 Nov 2020 12:59:04 +0000 (13:59 +0100)]
MEDIUM: listeners: make use of fd_want_recv_safe() to enable early receivers

We used to refrain from calling fd_want_recv() if fd_updt was not allocated
but it's not the right solution as this does not allow the FD to be set.
Instead, let's use the new fd_want_recv_safe() which will update the FD and
create an update entry only if possible. In addition, the equivalent test
before calling fd_stop_recv() was removed as totally useless since there's
not fd_updt creation in this case.

4 years agoMINOR: fd: add fd_want_recv_safe()
Willy Tarreau [Wed, 4 Nov 2020 12:52:22 +0000 (13:52 +0100)]
MINOR: fd: add fd_want_recv_safe()

This does the same as fd_want_recv() except that it does check for
fd_updt[] to be allocated, as this may be called during early listener
initialization. Previously we used to check fd_updt[] before calling
fd_want_recv() but this is not correct since it does not update the
FD flags. This method will be safer.

4 years agoBUG/MEDIUM: listener: make the master also keep workers' inherited FDs
Willy Tarreau [Tue, 3 Nov 2020 17:38:05 +0000 (18:38 +0100)]
BUG/MEDIUM: listener: make the master also keep workers' inherited FDs

In commit 374e9af35 ("MEDIUM: listener: let do_unbind_listener() decide
whether to close or not") it didn't appear necessary to have the master
process keep open the workers' inherited FDs. But this is actually
necessary to handle the reload on "bind fd@foo" situations, otherwise
the FD may be reassigned and the new socket cannot be set up, sometimes
causing "socket operation on non-socket" or other types of errors.

William found that this was the cause for the consistent failures of the
abns regtest, which already used to fail very often before this and was
as such marked as broken.

Interestingly I didn't have this issue with my test configs because
the FD number I used was higher and within the range of other listening
sockets. But this means that one of these wouldn't work as expected.

No backport is needed, this was introduced as part of the listeners
rework in 2.3.

4 years agoBUG/MEDIUM: listener: never suspend inherited sockets
Willy Tarreau [Wed, 4 Nov 2020 13:14:55 +0000 (14:14 +0100)]
BUG/MEDIUM: listener: never suspend inherited sockets

It is not acceptable to suspend an inherited socket because we'd kill
its listening state, making it possibly unrecoverable for future
processes. The situation which can trigger this is when there is an
abns socket in a config and an inherited FD on another listener. Upon
soft reload, the abns fails to bind, a SIGTTOU is sent to the old
process which suspends everything, including the inherited FD, then
the new process can bind and tell the old one to quit. Except that the
new FD was not set back to the listen state, which is detected by
listener_accept() which can pause it. It's only upon second reload
that the FD works again.

The solution is to refrain from suspending such FDs since we don't own
them. And the next process will get them right anyway from its config.
For now only TCP and UDP face this issue so it's better to address this
on a protocol basis

No backport is needed, this is related to the new listeners in 2.3.

4 years agoBUG/MEDIUM: listener: only enable a listening listener if needed
Willy Tarreau [Wed, 4 Nov 2020 12:54:00 +0000 (13:54 +0100)]
BUG/MEDIUM: listener: only enable a listening listener if needed

The test on listener->state == LI_LISTEN is not sufficient to decide
if we need to enable a listener. Indeed, there is a very special case
which is the inherited FD shared, which has to reflect the real socket
state even after the previous test, and as such needs to remain in
LI_LISTEN state. In this case we don't want a worker to start the
master's listener nor conversely. Let's add a specific test for this.

4 years agoBUG/MEDIUM: stick-table: limit the time spent purging old entries
Willy Tarreau [Tue, 3 Nov 2020 16:47:41 +0000 (17:47 +0100)]
BUG/MEDIUM: stick-table: limit the time spent purging old entries

An interesting case was reported with threads and moderately sized
stick-tables. Sometimes the watchdog would trigger during the purge.
It turns out that the stick tables were sized in the 10s of K entries
which is the order of magnitude of the possible number of connections,
and that threads were used over distinct NUMA nodes. While at first
glance nothing looks problematic there, actually there is a risk that
a thread trying to purge the table faces 100% of entries still in use
by a connection with (ts->ref_cnt > 0), and ends up scanning the whole
table, while other threads on the other NUMA node are causing the
cache lines to bounce back and forth and considerably slow down its
progress to the point of possibly spending hundreds of milliseconds
there, multiplied by the number of queued threads all failing on the
same point.

Interestingly, smaller tables would not trigger it because the scan
would be faster, and larger ones would not trigger it because plenty
of entries would be idle!

The most efficient solution is to increase the table size to be large
enough for this never to happen, but this is not reliable. We could
have a parallel list of idle entries but that would significantly
increase the storage and processing cost only to improve a few rare
corner cases.

This patch takes a more pragmatic approach, it considers that it will
not visit more than twice the number of nodes to be deleted, which
means that it accepts to fail up to 50% of the time. Given that very
small batches are programmed each time (1/256 of the table size), this
means the operation will finish quickly (128 times faster than now),
and will reduce the inter-thread contention. If this needs to be
reconsidered, it will probably mean that the batch size needs to be
fixed differently.

This needs to be backported to stable releases which extensively use
threads, typically 2.0.

Kudos to Nenad Merdanovic for figuring the root cause triggering this!

4 years agoMINOR: stats: do not display empty stat module title on html
Amaury Denoyelle [Tue, 3 Nov 2020 14:04:46 +0000 (15:04 +0100)]
MINOR: stats: do not display empty stat module title on html

If a stat module is not available on the current proxy scope, do not
display its title on the related html box. This is clearer for the user.

4 years agoMINOR: mux_h2: add stat for total count of connections/streams
Amaury Denoyelle [Tue, 3 Nov 2020 14:04:45 +0000 (15:04 +0100)]
MINOR: mux_h2: add stat for total count of connections/streams

Add counters for total number of http2 connections/stream since haproxy
startup. Contrary to open_conn/stream, they are never reset to zero.

4 years agoMINOR: mux_h2: capitalize frame type in stats
Amaury Denoyelle [Tue, 3 Nov 2020 14:04:44 +0000 (15:04 +0100)]
MINOR: mux_h2: capitalize frame type in stats

http/2 frame type names are capitalized in the rfc, use the same
notation on the stats labels.