Emeric Brun [Fri, 29 Oct 2021 15:59:18 +0000 (17:59 +0200)]
MEDIUM: resolvers: rename dns extra counters to resolvers extra counters
This patch renames all dns extra counters and stats functions, types and
enums using the 'resolv' prefix/suffixes.
The dns extra counter domain id used on cli was replaced by "resolvers"
instead of "dns".
The typed extra counter prefix dumping resolvers domain "D." was
also renamed "N." because it points counters on a Nameserver.
This was done to finish the split between "resolver" and "dns" layers
and to avoid further misunderstanding when haproxy will handle dns
load balancing.
Emeric Brun [Fri, 29 Oct 2021 15:30:41 +0000 (17:30 +0200)]
MINOR: resolvers/dns: split dns and resolver counters in dns_counter struct
This patch add a union and struct into dns_counter struct to split
application specific counters.
The only current existing application is the resolver.c layer but
in futur we could handle different application such as dns load
balancing with others specific counters.
Emeric Brun [Fri, 29 Oct 2021 14:44:49 +0000 (16:44 +0200)]
BUG/MINOR: resolvers: throw log message if trash not large enough for query
Before this patch the sent error counter was increased
for each targeted nameserver as soon as we were unable to build
the query message into the trash buffer. But this counter is here
to count sent errors at dns.c transport layer and this error is not
related to a nameserver.
This patch stops to increase those counters and sent a log message
to signal the trash buffer size is not large enough to build the query.
Note: This case should not happen except if trash size buffer was
customized to a very low value.
The function was also re-worked to return -1 in this error case
as it was specified in comment. This function is currently
called at multiple point in resolver.c but return code
is still not yet handled. So to advert the user of the malfunction
the log message was added.
This patch should be backported on all versions including the
layer split between dns.c and resolver.c (v >= 2.4)
Emeric Brun [Fri, 29 Oct 2021 14:28:33 +0000 (16:28 +0200)]
BUG/MINOR: resolvers: fix sent messages were counted twice
The sent messages counter was increased at both resolver.c and dns.c
layers.
This patch let the dns.c layer count the sent messages since this
layer handle a retry if transport layer is not ready (EAGAIN on udp
or tcp session ring buffer full).
This patch should be backported on all versions using a split of those
layers for resolving (v >=2.4)
Amaury Denoyelle [Mon, 18 Oct 2021 12:40:29 +0000 (14:40 +0200)]
MINOR: server: add ws keyword
Implement parsing for the server keyword 'ws'. This is used to configure
the mode of selection for websocket protocol. The configuration
documentation has been updated.
A new regtest has been created to test the proper behavior of the
keyword.
Handle properly websocket streams if the server uses an ALPN with both
h1 and h2. Add a new field h2_ws in the server structure. If set to off,
reuse is automatically disable on backend and ALPN is forced to http1.x
if possible. Nothing is done if on.
Implement a mechanism to be able to use a different http version for
websocket streams. A new server member <ws> represents the algorithm to
select the protocol. This can overrides the server <proto>
configuration. If the connection uses ALPN for proto selection, it is
updated for websocket streams to select the right protocol.
Three mode of selection are implemented :
- auto : use the same protocol between non-ws and ws streams. If ALPN is
use, try to update it to "http/1.1"; this is only done if the server
ALPN contains "http/1.1".
- h1 : use http/1.1
- h2 : use http/2.0; this requires the server to support RFC8441 or an
error will be returned by haproxy.
Amaury Denoyelle [Thu, 28 Oct 2021 14:36:11 +0000 (16:36 +0200)]
MINOR: connection: add alternative mux_ops param for conn_install_mux_be
Add a new parameter force_mux_ops. This will be useful to specify an
alternative to the srv->mux_proto field. If non-NULL, it will be use to
force the mux protocol wether srv->mux_proto is set or not.
This argument will become useful to install a mux for non-standard
streams, most notably websocket streams.
Amaury Denoyelle [Mon, 18 Oct 2021 12:32:36 +0000 (14:32 +0200)]
MINOR: connection: implement function to update ALPN
Implement a new function to update the ALPN on an existing connection.
on an existing connection. The ALPN from the ssl context can be checked
to update the ALPN only if it is a subset of the context value.
This method will be useful to change a connection ALPN for websocket,
must notably if the server does not support h2 websocket through the
rfc8441 Extended Connect.
Amaury Denoyelle [Mon, 18 Oct 2021 12:45:49 +0000 (14:45 +0200)]
MINOR: stream/mux: implement websocket stream flag
Define a new stream flag SF_WEBSOCKET and a new cs flag CS_FL_WEBSOCKET.
The conn-stream flag is first set by h1/h2 muxes if the request is a
valid websocket upgrade. The flag is then converted to SF_WEBSOCKET on
the stream creation.
This will be useful to properly manage websocket streams in
connect_server().
Amaury Denoyelle [Mon, 18 Oct 2021 07:43:29 +0000 (09:43 +0200)]
BUG/MEDIUM: mux-h2: reject upgrade if no RFC8441 support
The RFC8441 was not respected by haproxy in regards with server support
for Extended CONNECT. The Extended CONNECT method was used to convert an
Upgrade header stream even if no SETTINGS_ENABLE_CONNECT_PROTOCOL was
received, which is forbidden by the RFC8441. In this case, the behavior
of the http/2 server is unspecified.
Fix this by flagging the connection on receiption of the RFC8441
settings SETTINGS_ENABLE_CONNECT_PROTOCOL. Extended CONNECT is thus only
be used if the flag is present. In the other case, the stream is
immediatly closed as there is no way to handle it in http/2. It results
in a http/1.1 502 or http/2 RESET_STREAM to the client side.
The protocol-upgrade regtest has been extended to test that haproxy does
not emit Extended CONNECT on servers without RFC8441 support.
Amaury Denoyelle [Mon, 18 Oct 2021 08:05:16 +0000 (10:05 +0200)]
MINOR: mux-h2: add trace on extended connect usage
Add a state trace to report that a protocol upgrade is converted using
the rfc8441 Extended connect method. This is useful in regards with the
recent changes to improve http/2 websockets.
Tim Duesterhus [Fri, 29 Oct 2021 16:06:55 +0000 (18:06 +0200)]
MINOR: jwt: Make invalid static JWT algorithms an error in `jwt_verify` converter
It is not useful to start a configuration where an invalid static string is
provided as the JWT algorithm. Better make the administrator aware of the
suspected typo by failing to start.
The early version of the script used to support passing non-branch
arguments but as it evolved we lost that option. Let's use "--" as a
delimiter after the branch(es) to pass optional file names to filter
on. This is convenient to list missing patches on a specific set of
files.
Willy Tarreau [Tue, 2 Nov 2021 17:05:41 +0000 (18:05 +0100)]
[RELEASE] Released version 2.5-dev12
Released version 2.5-dev12 with the following main changes :
- MINOR: httpclient: support payload within a buffer
- MINOR: httpclient/lua: support more HTTP methods
- MINOR: httpclient/lua: return an error when it can't generate the request
- CLEANUP: lua: Remove any ambiguities about lua txn execution context flags
- BUG/MEDIUM: lua: fix invalid return types in hlua_http_msg_get_body
- CLEANUP: connection: No longer export make_proxy_line_v1/v2 functions
- CLEANUP: tools: Use const address for get_net_port() and get_host_port()
- CLEANUP: lua: Use a const address to retrieve info about a connection
- MINOR: connection: Add function to get src/dst without updating the connection
- MINOR: session: Add src and dst addresses to the session
- MINOR: stream-int: Add src and dst addresses to the stream-interface
- MINOR: frontend: Rely on client src and dst addresses at stream level
- MINOR: log: Rely on client addresses at the appropriate level to log messages
- MINOR: session: Rely on client source address at session level to log error
- MINOR: http-ana: Rely on addresses at stream level to set xff and xot headers
- MINOR: http-fetch: Rely on addresses at stream level in HTTP sample fetches
- MINOR: mux-fcgi: Rely on client addresses at stream level to set default params
- MEDIUM: tcp-sample: Rely on addresses at the appropriate level in tcp samples
- MEDIUM: connection: Rely on addresses at stream level to make proxy line
- MEDIUM: backend: Rely on addresses at stream level to init server connection
- MEDIUM: connection: Assign session addresses when PROXY line is received
- MEDIUM: connection: Assign session addresses when NetScaler CIP proto is parsed
- MEDIUM: tcp-act: Set addresses at the apprioriate level in set-(src/dst) actions
- MINOR: tcp-act: Add set-src/set-src-port for "tcp-request content" rules
- DOC: config: Fix alphabetical order of fc_* samples
- MINOR: tcp-sample: Add samples to get original info about client connection
- REGTESTS: Add script to test client src/dst manipulation at different levels
- MINOR: stream: Use backend stream-interface dst address instead of target_addr
- BUILD: log: Fix compilation without SSL support
- DEBUG: protocol: yell loudly during registration of invalid sock_domain
- MINOR: protocols: add a new protocol type selector
- MINOR: protocols: make use of the protocol type to select the protocol
- MINOR: protocols: replace protocol_by_family() with protocol_lookup()
- MINOR: halog: Add -qry parameter allowing to preserve the query string in -uX
- CLEANUP: jwt: Remove the use of a trash buffer in jwt_jwsverify_hmac()
- CLEANUP: jwt: Remove the use of a trash buffer in jwt_jwsverify_rsa_ecdsa()
- DEV: coccinelle: Add realloc_leak.cocci
- CLEANUP: hlua: Remove obsolete branch in `hlua_alloc()`
- BUILD: atomic: prefer __atomic_compare_exchange_n() for __ha_cas_dw()
- BUILD: atomic: fix build on mac/arm64
- MINOR: atomic: remove the memcpy() call and dependency on string.h
- MINOR: httpclient: request streaming with a callback
- MINOR: httpclient/lua: handle the streaming into the lua applet
- REGTESTS: lua: test httpclient with body streaming
- DOC: halog: Move the `-qry` parameter into the correct section in help text
- MINOR: halog: Rename -qry to -query
- CLEANUP: halog: Use consistent indentation in help()
- BUG/MINOR: halog: Add missing newlines in die() messages
- MINOR: halog: Add support for extracting captures using -hdr
- DOC: Typo fixed "it" should be "is"
- BUG/MINOR: mux-h1: Save shutdown mode if the shutdown is delayed
- BUG/MEDIUM: mux-h1: Perform a connection shutdown when the h1c is released
- BUG/MEDIUM: resolvers: Don't recursively perform requester unlink
- BUG/MEDIUM: http-ana: Drain request data waiting the tarpit timeout expiration
- BUG/MINOR: http: Authorization value can have multiple spaces after the scheme
- BUG/MINOR: http: http_auth_bearer fetch does not work on custom header name
- BUG/MINOR: httpclient/lua: misplaced luaL_buffinit()
- BUILD/MINOR: cpuset freebsd build fix
- BUG/MINOR: httpclient: use a placeholder value for Host header
- BUG/MEDIUM: stream-int: Block reads if channel cannot receive more data
- BUG/MEDIUM: resolvers: Track api calls with a counter to free resolutions
- MINOR: stream: Improve dump of bogus streams
- DOC/peers: some grammar fixes for peers 2.1 spec
- MEDIUM: vars: make the var() sample fetch function really return type ANY
- MINOR: vars: add "set-var" for "tcp-request connection" rules.
Willy Tarreau [Tue, 2 Nov 2021 16:08:15 +0000 (17:08 +0100)]
MEDIUM: vars: make the var() sample fetch function really return type ANY
A long-standing issue was reported in issue #1215.
In short, var() was initially internally declared as returning a string
because it was not possible by then to return "any type". As such, users
regularly get trapped thinking that when they're storing an integer there,
then the integer matching method automatically applies. Except that this
is not possible since this is related to the config parser and is decided
at boot time where the variable's type is not known yet.
As such, what is done is that the output being declared as type string,
the string match will automatically apply, and any value will first be
converted to a string. This results in several issues like:
not working. This is because the string match on the second line will in
fact compare the string representation of the variable against strings
"lt" and "0", none of which matches.
The doc says that the matching method is mandatory, though that's not
the case in the code due to that default string type being permissive.
There's not even a warning when no explicit match is placed, because
this happens very deep in the expression evaluator and making a special
case just for "var" can reveal very complicated.
The set-var() converter already mandates a matching method, as the
following will be rejected:
... if { int(12),set-var(txn.truc) 12 }
while this one will work:
... if { int(12),set-var(txn.truc) -m int 12 }
As such, this patch this modifies var() to match the doc, returning the
type "any", and mandating the matching method, implying that this bogus
config which does not work:
http-request set-var(txn.foo) int(-1)
http-request deny if { var(txn.foo) -m int lt 0 }
This *will* break some configs (and even 3 of our regtests relied on
this), but except those which already match string exclusively, all
other ones are already broken and silently fail (and one of the 3
regtests, the one on FIX, was bogus regarding this).
In order to fix existing configs, one can simply append "-m str"
after a "var()" in an ACL or "if" expression:
Most commonly, patterns such as "le", "lt", "ge", "gt", "eq", "ne" in
front of a number indicate that the intent was to match an integer,
and in this case "-m int" would be desired:
tcp-response content reject if ! { var(res.size) gt 3800 }
ought to become:
tcp-response content reject if ! { var(res.size) -m int gt 3800 }
This must not be backported, but if a solution is found to at least
detect this exact condition in the generic expression parser and
emit a warning, this could probably help spot configuration bugs.
BUG/MEDIUM: resolvers: Track api calls with a counter to free resolutions
The kill list introduced in commit f766ec6b5 ("MEDIUM: resolvers: use a kill
list to preserve the list consistency") contains a bug. The deatch_row must
be initialized before calling resolv_process_responses() function. However,
this function is called for the dns code. The death_row is not visible from
the outside. So, it is possible to add a resolution in an uninitialized
death_row, leading to a crash.
But, with the current implementation, it is not possible to handle the
death_row in resolv_process_responses() function because, internally, the
kill list may be freed via a call to resolv_unlink_resolution(). At the end,
we are unable to determine all call chains to guarantee a safe use of the
kill list. It is a shameful observation, but unfortunatly true.
So, to make the fix simple, we track all calls to the public resolvers
api. A counter is incremented when we enter in the resolver code and
decremented when we leave it. This way, we are able to track the recursions
to init and release the kill list only once, at the edge.
Following functions are incrementing/decrementing the recurse counter:
BUG/MEDIUM: stream-int: Block reads if channel cannot receive more data
First of all, we must be careful here because this part was modified and
each time, this introduced a bug. But, in si_update_rx(), we must not
re-enables receives if the channel buffer cannot receive more
data. Otherwise the multiplexer will be wake up for nothing. Because the
stream is woken up when the multiplexer is waiting for more room to move on,
this may lead to a ping-pong loop between the stream and the mux.
Note that for now, it does not fix any known bug. All reported issues in
this area were fixed in another way.
This patch must be backported with a special care. Technically speaking, it
may be backported as far as 2.0.
BUG/MINOR: httpclient: use a placeholder value for Host header
A Host header must be present for http_update_host() to success.
htx_add_header(htx, ist("Host"), IST_NULL) was used but this is not a
good idea from a semantic point of view. It also tries to make a memcpy
with a len of 0, which is unrequired.
Some luaL_buffinit() call was done before the push of the variable name,
where it seems to work correctly with lua < 5.4.3, it brokes
systematically on this version.
This patch inverts the pushstring and the buffinit.
BUG/MINOR: http: http_auth_bearer fetch does not work on custom header name
The http_auth_bearer sample fetch can take a header name as parameter,
in which case it will try to extract a Bearer value out of the given
header name instead of the default "Authorization" one. In this case,
the extraction would not have worked because of a misuse of strncasecmp.
This patch fixes this by replacing the standard string functions by ist
ones.
It also properly manages the multiple spaces that could be found between
the scheme and its value.
No backport needed, that's part of JWT which is only in 2.5.
BUG/MEDIUM: http-ana: Drain request data waiting the tarpit timeout expiration
When a tarpit action is performed, we must be sure to drain data from the
request channel. Otherwise, the mux on the frontend side may be blocked
because the request channel buffer is full.
This may lead to Two bugs. The first one is a HOL blocking on the H2
multiplexer. A tarpitted stream may block all the others because data are
not drained for the whole tarpit timeout. The second bug is a ping-pong loop
between the multiplexer and the stream. The mux is waiting for more space in
the channel buffer, so it wakes up the stream. And the stream systematically
re-enables receives.
This last part is not pretty clean and it will be addressed with another
fix. But draning request data is a good way to fix both bugs in same time.
This patch must be backported as far as 2.0. The legacy HTTP mode is
probably affected, but I don't know if same bugs may be experienced in this
mode.
A loop on the resolution answer items is performed inside
resolv_detach_from_resolution_answer_items(). But by reading the code, it
seems possible to recursively unlink the same requester.
To avoid any loop at this stage, the requester clean up must be performed
before the call to resolv_detach_from_resolution_answer_items(). This way,
the second call to _resolv_unlink_resolution() does nothing and returns
immediately because the requester was already detached from the resolution.
This patch is related to the issue #1404. It must be backported as far as
2.2.
BUG/MEDIUM: mux-h1: Perform a connection shutdown when the h1c is released
When the H1 connection is released, a connection shutdown is now performed.
If it was already performed when the stream was detached, this action has no
effect. But it is mandatory, when an idle H1C is released. Otherwise the
xprt and the socket shutdown is never perfmed. It is especially important
for SSL client connections, because it is the only way to perform a clean
SSL shutdown.
Without this patch, SSL_shutdown is never called, preventing, among other
things, the SSL session caching.
This patch depends on the commit "BUG/MINOR: mux-h1: Save shutdown mode if
the shutdown is delayed". It should be backported as far as 2.0.
BUG/MINOR: mux-h1: Save shutdown mode if the shutdown is delayed
The connection shutdown may be delayed if there are pending outgoing
data. The action is performed once data are fully sent. In this case the
mode (dirty/clean) was lost and a clean shutdown was always performed. Now,
the mode is saved to be sure to perform the connection shutdown using the
right mode. To do so, H1C_F_ST_SILENT_SHUT flag is introduced.
Tim Duesterhus [Thu, 28 Oct 2021 15:24:02 +0000 (17:24 +0200)]
MINOR: halog: Add support for extracting captures using -hdr
This patch adds support for extracting captured header fields to halog. A field
can be extracted by passing the `-hdr <block>:<field>` output filter.
Both `<block>` and `<field>` are 1-indexed.
`<block>` refers to the index of the brace-delimited list of headers. If both
request and response headers are captured, then request headers are referenced
by `<block> = 1`, response headers are `2`. If only one direction is captured,
there will only be a single block `1`.
`<field>` refers to a single field within the selected block.
The output will contain one line, possibly empty, per log line processed.
Passing a non-existent `<block>` or `<field>` will result in an empty line.
Example:
capture request header a len 50
capture request header b len 50
capture request header c len 50
capture response header d len 50
capture response header e len 50
capture response header f len 50
`-srv 1:1` will extract request header `a`
`-srv 1:2` will extract request header `b`
`-srv 1:3` will extract request header `c`
`-srv 2:3` will extract response header `f`
Willy Tarreau [Thu, 28 Oct 2021 07:41:29 +0000 (09:41 +0200)]
MINOR: atomic: remove the memcpy() call and dependency on string.h
The memcpy() call in the aarch64 version of __ha_cas_dw() is sometimes
inlined and sometimes not, depending on the gcc version. It's only used
to copy two void*, so let's use direct assignment instead of memcpy().
It would also be possible to change the asm code to directly write there,
but it's not worth it.
With this change the code is 8kB smaller with gcc-5.4.
Willy Tarreau [Thu, 28 Oct 2021 06:53:33 +0000 (08:53 +0200)]
BUILD: atomic: prefer __atomic_compare_exchange_n() for __ha_cas_dw()
__atomic_compare_exchange() is incorrectly documented in the gcc builtins
doc, it says the desired value is "type *desired" while in reality it is
"const type *desired" as expected since that value must in no way be
modified by the operation. However it seems that clang has implemented
it as documented, and reports build warnings when fed a const.
This is quite problematic because it means we have to betry the callers,
pretending we won't touch their constants but not knowing what the
compiler would do with them, and possibly hiding future bugs.
Instead of forcing a cast, let's just switch to the better
__atomic_compare_exchange_n() that takes a value instead of a pointer.
At least with this one there is no doubt about how the input will be
used.
It was verified that the output object code is the same both in clang
and gcc with this change.
Tim Duesterhus [Sat, 23 Oct 2021 17:53:35 +0000 (19:53 +0200)]
DEV: coccinelle: Add realloc_leak.cocci
This coccinelle patch finds locations where the return value of `realloc()` is
assigned to the pointer passed to `realloc()`. This calls will leak memory if
`realloc()` returns `NULL`.
Tim Duesterhus [Mon, 18 Oct 2021 16:40:28 +0000 (18:40 +0200)]
CLEANUP: jwt: Remove the use of a trash buffer in jwt_jwsverify_hmac()
The OpenSSL documentation (https://www.openssl.org/docs/man1.1.0/man3/HMAC.html)
specifies:
> It places the result in md (which must have space for the output of the hash
> function, which is no more than EVP_MAX_MD_SIZE bytes). If md is NULL, the
> digest is placed in a static array. The size of the output is placed in
> md_len, unless it is NULL. Note: passing a NULL value for md to use the
> static array is not thread safe.
`EVP_MAX_MD_SIZE` appears to be defined as `64`, so let's simply use a stack
buffer to avoid the whole memory management.
Tim Duesterhus [Mon, 18 Oct 2021 10:12:02 +0000 (12:12 +0200)]
MINOR: halog: Add -qry parameter allowing to preserve the query string in -uX
Our use-case for this is a dynamic application that performs routing based on
the query string. Without this option all URLs will just point to the central
entrypoint of this location, making the output completely useless.
Willy Tarreau [Wed, 27 Oct 2021 15:41:07 +0000 (17:41 +0200)]
MINOR: protocols: replace protocol_by_family() with protocol_lookup()
At a few places we were still using protocol_by_family() instead of
the richer protocol_lookup(). The former is limited as it enforces
SOCK_STREAM and a stream protocol at the control layer. At least with
protocol_lookup() we don't have this limitationn. The values were still
set for now but later we can imagine making them configurable on the
fly.
Willy Tarreau [Wed, 27 Oct 2021 15:28:55 +0000 (17:28 +0200)]
MINOR: protocols: make use of the protocol type to select the protocol
Instead of using sock_type and ctrl_type to select a protocol, let's
make use of the new protocol type. For now they always match so there
is no change. This is applied to address parsing and to socket retrieval
from older processes.
Willy Tarreau [Wed, 27 Oct 2021 15:05:36 +0000 (17:05 +0200)]
MINOR: protocols: add a new protocol type selector
The protocol selection is currently performed based on the family,
control type and socket type. But this is often not enough, as both
only provide DGRAM or STREAM, leaving few variants. Protocols like
SCTP for example might be indistinguishable from TCP here. Same goes
for TCP extensions like MPTCP.
This commit introduces a new enum proto_type that is placed in each
and every protocol definition, that will usually more or less match
the sock_type, but being an enum, will support additional values.
Willy Tarreau [Wed, 27 Oct 2021 13:06:35 +0000 (15:06 +0200)]
DEBUG: protocol: yell loudly during registration of invalid sock_domain
The test on the sock_domain is a bit useless because the protocols are
registered at boot time, and the test silently fails and returns no
error. Use a BUG_ON() instead to make sure to catch such bugs in the
code if any.
MINOR: tcp-sample: Add samples to get original info about client connection
Because source and destination address of the client connection are now
updated at the appropriated level (connection, session or stream), original
info about the client connection are preserved. src/src_port/src_is_local
and dst/dst_port/dst_is_local return current info about the client
connection. It is the info at the highest available level. Most of time, the
stream. Any tcp/http rules may alter this info.
To get original info, "fc_" prefix must be added. For instance
"fc_src". Here, only "tcp-request connection" rules may alter source and
destination address/port.
MINOR: tcp-act: Add set-src/set-src-port for "tcp-request content" rules
This patch was reverted because it was inconsitent to change connection
addresses at stream level. Especially in HTTP because all requests was
affected by this change and not only the current one. In HTTP/2, it was
worse. Several streams was able to change the connection addresses at the
same time.
It is no longer an issue, thanks to recent changes. With multi-level client
source and destination addresses, it is possible to limit the change to the
current request. Thus this patch can be reintroduced.
If it possible to set source IP/Port from "tcp-request connection",
"tcp-request session" and "http-request" rules but not from "tcp-request
content" rules. There is no reason for this limitation and it may be a
problem for anyone wanting to call a lua fetch to dynamically set source
IP/Port from a TCP proxy. Indeed, to call a lua fetch, we must have a
stream. And there is no stream when "tcp-request connection/session" rules
are evaluated.
Thanks to this patch, "set-src" and "set-src-port" action are now supported
by "tcp_request content" rules.
MEDIUM: tcp-act: Set addresses at the apprioriate level in set-(src/dst) actions
When client source or destination addresses are changed via a tcp/http
action, we update addresses at the appropriate level. When "tcp-request
connection" rules are evaluated, we update addresses at the connection
level. When "tcp-request session" rules is evaluated, we update those at the
session level. And finally, when "tcp-request content" or "http-request"
rules are evaluated, we update the addresses at the stream level.
The same is performed when source or destination ports are changed.
Of course, for now, not all level are supported. But thanks to this patch,
it will be possible.
MEDIUM: connection: Assign session addresses when NetScaler CIP proto is parsed
Just like for the PROXY protocol, when the NetScaler Client IP insertion
header is received, the retrieved client source and destination addresses
are set at the session level. This leaves those at the connection level
intact.
MEDIUM: connection: Assign session addresses when PROXY line is received
When PROXY protocol line is received, the retrieved client source and
destination addresses are set at the session level. This leaves those at the
connection level intact.
MEDIUM: backend: Rely on addresses at stream level to init server connection
Client source and destination addresses at stream level are used to initiate
the connections to a server. For now, stream-interface addresses are never
set. So, thanks to the fallback mechanism, no changes are expected with this
patch. But its purpose is to rely on addresses at the appropriate level when
set instead of those at the connection level.
MEDIUM: connection: Rely on addresses at stream level to make proxy line
If the stream exists, the frontend stream-interface is used to get the
client source and destination addresses when the proxy line is built. For
now, stream-interface or session addresses are never set. So, thanks to the
fallback mechanism, no changes are expected with this patch. But its purpose
is to rely on addresses at the appropriate level when set instead of those
at the connection level.
MEDIUM: tcp-sample: Rely on addresses at the appropriate level in tcp samples
In src, src-port, dst and dst-port sample fetches, the client source and
destination addresses are retrieved from the appropriate level. It means
that, if the stream exits, we use the frontend stream-interface to get the
client source and destination addresses. Otherwise, the session is used. For
now, stream-interface or session addresses are never set. So, thanks to the
fallback mechanism, no changes are expected with this patch. But its purpose
is to rely on addresses at the appropriate level when set instead of those
at the connection level.
MINOR: mux-fcgi: Rely on client addresses at stream level to set default params
Client source and destination addresses at stream level are now used to emit
SERVER_NAME/SERVER_PORT and REMOTE_ADDR/REMOTE_PORT parameters. For now,
stream-interface addresses are never set. So, thanks to the fallback
mechanism, no changes are expected with this patch. But its purpose is to
rely on addresses at the stream level, when set, instead of those at the
connection level.
MINOR: http-fetch: Rely on addresses at stream level in HTTP sample fetches
Client source and destination addresses at stream level are now used to
compute base32+src and url32+src hashes. For now, stream-interface addresses
are never set. So, thanks to the fallback mechanism, no changes are expected
with this patch. But its purpose is to rely on addresses at the stream
level, when set, instead of those at the connection level.
MINOR: http-ana: Rely on addresses at stream level to set xff and xot headers
Client source and destination addresses at stream level are now used to emit
X-Forwarded-For and X-Original-To headers. For now, stream-interface addresses
are never set. So, thanks to the fallback mechanism, no changes are expected
with this patch. But its purpose is to rely on addresses at the stream level,
when set, instead of those at the connection level.
MINOR: session: Rely on client source address at session level to log error
When an embryonic session is killed, if no log format is defined for this
error, a generic error is emitted. When this happens, we now rely on the
session to get the client source address. For now, session addresses are
never set. So, thanks to the fallback mechanism, no changes are expected
with this patch. But its purpose is to rely on addresses at the session
level when set instead of those at the connection level.
MINOR: log: Rely on client addresses at the appropriate level to log messages
When a log message is emitted, if the stream exits, we use the frontend
stream-interface to retrieve the client source and destination
addresses. Otherwise, the session is used. For now, stream-interface or
session addresses are never set. So, thanks to the fallback mechanism, no
changes are expected with this patch. But its purpose is to rely on
addresses at the appropriate level when set instead of those at the
connection level.
MINOR: frontend: Rely on client src and dst addresses at stream level
For now, stream-interface or session addresses are never set. So, thanks to
the fallback mechanism, no changes are expected with this patch. But its
purpose is to rely on the client addresses at the stream level, when set,
instead of those at the connection level. The addresses are retrieved from
the frontend stream-interface.
MINOR: stream-int: Add src and dst addresses to the stream-interface
For now, these addresses are never set. But the idea is to be able to set, at
least first, the client source and destination addresses at the stream level
without updating the session or connection ones.
Of course, because these addresses are carried by the strream-interface, it
would be possible to set server source and destination addresses at this level
too.
Functions to fill these addresses have been added: si_get_src() and
si_get_dst(). If not already set, these functions relies on underlying
layers to fill stream-interface addresses. On the frontend side, the session
addresses are used if set, otherwise the client connection ones are used. On
the backend side, the server connection addresses are used.
And just like for sessions and conncetions, si_src() and si_dst() may be used to
get source and destination addresses or the stream-interface. And, if not set,
same mechanism as above is used.
MINOR: session: Add src and dst addresses to the session
For now, these addresses are never set. But the idea is to be able to set
client source and destination addresses at the session level without
updating the connection ones.
Functions to fill these addresses have been added: sess_get_src() and
sess_get_dst(). If not already set, these functions relies on
conn_get_src() and conn_get_dst() to fill session addresses.
And just like for conncetions, sess_src() and sess_dst() may be used to get
source and destination addresses. However, if not set, the corresponding
address from the underlying client connection is returned. When this
happens, the addresses is filled in the connection object.
MINOR: connection: Add function to get src/dst without updating the connection
conn_get_src() and conn_get_dst() functions are used to fill the source and
destination addresses of a connection. On success, ->src and ->dst
connection fields can be safely used.
For convenience, 2 new functions are added here: conn_src() and conn_dst().
These functions return the corresponding address, as a const and only if it
is already set. Otherwise NULL is returned.
vishnu [Sun, 24 Oct 2021 01:16:24 +0000 (06:46 +0530)]
BUG/MEDIUM: lua: fix invalid return types in hlua_http_msg_get_body
hlua_http_msg_get_body must return either a Lua string or nil. For some
HTTPMessage objects, HTX_BLK_EOT blocks are also present in the HTX buffer
along with HTX_BLK_DATA blocks. In such cases, _hlua_http_msg_dup will start
copying data into a luaL_Buffer until it encounters an HTX_BLK_EOT. But then
instead of pushing neither the luaL_Buffer nor `nil` to the Lua stack, the
function will return immediately. The end result will be that the caller of
the HTTPMessage.body() method from a Lua filter will see whatever object was
on top of the stack as return value. It may be either a userdata object if
HTTPMessage.body() was called with only two arguments, or the third argument
itself if called with three arguments. Hence HTTPMessage.body() would return
either nil, or HTTPMessage body as Lua string, or a userdata objects, or
number.
This fix ensure that HTTPMessage.body() will always return either a string
or nil.
Reviewed-by: Christopher Faulet <cfaulet@haproxy.com>
CLEANUP: lua: Remove any ambiguities about lua txn execution context flags
Flags used to set the execution context of a lua txn are used as an enum. It is
not uncommon but there are few flags otherwise. So to remove ambiguities, a
comment and a _NONE value are added to have a clear definition of supported
values.
This patch should fix the issue #1429. No backport needed.
MINOR: httpclient/lua: return an error when it can't generate the request
Add a check during the httpclient request generation which yield an lua
error when the generation didn't work. The most common case is the lack
of space in the buffer, it can because of too much headers or a too big
body.
Willy Tarreau [Fri, 22 Oct 2021 17:40:44 +0000 (19:40 +0200)]
[RELEASE] Released version 2.5-dev11
Released version 2.5-dev11 with the following main changes :
- DEV: coccinelle: Add strcmp.cocci
- CLEANUP: Apply strcmp.cocci
- CI: Add `permissions` to GitHub Actions
- CI: Clean up formatting in GitHub Action definitions
- MINOR: add ::1 to predefined LOCALHOST acl
- CLEANUP: assorted typo fixes in the code and comments
- CLEANUP: Consistently `unsigned int` for bitfields
- MEDIUM: resolvers: lower-case labels when converting from/to DNS names
- MEDIUM: resolvers: replace bogus resolv_hostname_cmp() with memcmp()
- MINOR: jwt: Empty the certificate tree during deinit
- MINOR: jwt: jwt_verify returns negative values in case of error
- MINOR: jwt: Do not rely on enum order anymore
- BUG/MEDIUM: stream: Keep FLT_END analyzers if a stream detects a channel error
- MINOR: httpclient/cli: access should be only done from expert mode
- DOC: management: doc about the CLI httpclient
- BUG/MEDIUM: tcpcheck: Properly catch early HTTP parsing errors
- BUG/MAJOR: dns: tcp session can remain attached to a list after a free
- BUG/MAJOR: dns: attempt to lock globaly for msg waiter list instead of use barrier
- CLEANUP: dns: always detach the appctx from the dns session on release
- DEBUG: dns: add a few more BUG_ON at sensitive places
- BUG/MAJOR: resolvers: add other missing references during resolution removal
- CLEANUP: resolvers: do not export resolv_purge_resolution_answer_records()
- BUILD: resolvers: avoid a possible warning on null-deref
- BUG/MEDIUM: resolvers: always check a valid item in query_list
- CLEANUP: always initialize the answer_list
- CLEANUP: resolvers: simplify resolv_link_resolution() regarding requesters
- CLEANUP: resolvers: replace all LIST_DELETE with LIST_DEL_INIT
- MEDIUM: resolvers: use a kill list to preserve the list consistency
- MEDIUM: resolvers: remove the last occurrences of the "safe" argument
- BUG/MEDIUM: checks: fix the starting thread for external checks
- MEDIUM: resolvers: replace the answer_list with a (flat) tree
- MEDIUM: resolvers: hash the records before inserting them into the tree
- BUG/MAJOR: buf: fix varint API post- vs pre- increment
- OPTIM: resolvers: move the eb32 node before the data in the answer_item
- MINOR: list: add new macro LIST_INLIST_ATOMIC()
- OPTIM: dns: use an atomic check for the list membership
- BUG/MINOR: task: do not set TASK_F_USR1 for no reason
- BUG/MINOR: mux-h2: do not prevent from sending a final GOAWAY frame
- MINOR: connection: add a new CO_FL_WANT_DRAIN flag to force drain on close
- MINOR: mux-h2: perform a full cycle shutdown+drain on close
- CLEANUP: resolvers: get rid of single-iteration loop in resolv_get_ip_from_response()
- MINOR: quic: Increase the size of handshake RX UDP datagrams
- BUG/MEDIUM: lua: fix memory leaks with realloc() on non-glibc systems
- MINOR: memprof: report the delta between alloc and free on realloc()
- MINOR: memprof: add one pointer size to the size of allocations
- BUILD: fix compilation on NetBSD
- MINOR: backend: add traces for idle connections reuse
- BUG/MINOR: backend: fix improper insert in avail tree for always reuse
- MINOR: backend: improve perf with tcp proxies skipping idle conns
- MINOR: connection: remove unneeded memset 0 for idle conns
Amaury Denoyelle [Wed, 20 Oct 2021 13:11:37 +0000 (15:11 +0200)]
MINOR: connection: remove unneeded memset 0 for idle conns
Remove the zeroing of an idle connection node on remove from a tree.
This is not needed and should improve slightly the performance of idle
connection usage. Besides, it breaks the memory poisoning feature.
Amaury Denoyelle [Wed, 20 Oct 2021 13:04:03 +0000 (15:04 +0200)]
MINOR: backend: improve perf with tcp proxies skipping idle conns
Skip the hash connection calcul when reuse must not be used in
connect_server() : this is the case for TCP proxies. This should result
in slightly better performance when using this use-case.
Amaury Denoyelle [Wed, 20 Oct 2021 13:22:20 +0000 (15:22 +0200)]
BUG/MINOR: backend: fix improper insert in avail tree for always reuse
In connect_server(), if http-reuse always is set, the backend connection
is inserted into the available tree as soon as created. However, the
hash connection field is only set later at the end of the function.
This seems to have no impact as the hash connection field is always
position before a lookup. However, this is not a proper usage of ebmb
API. Fix this by setting the hash connection field before the insertion
into the avail tree.
Amaury Denoyelle [Wed, 20 Oct 2021 13:22:01 +0000 (15:22 +0200)]
MINOR: backend: add traces for idle connections reuse
Add traces in connect_server() to debug idle connection reuse. These
are attached to stream trace module, as it's already in use in
backend.c with the macro TRACE_SOURCE.
Amaury Denoyelle [Thu, 21 Oct 2021 08:57:02 +0000 (10:57 +0200)]
BUILD: fix compilation on NetBSD
Use include file <sys/time.h> to fix compilation error with timeval in
some files. This is as reported as 'man 7 system_data_types'. The build
error is reported on NetBSD 9.2.
Willy Tarreau [Fri, 22 Oct 2021 14:33:53 +0000 (16:33 +0200)]
MINOR: memprof: add one pointer size to the size of allocations
The current model causes an issue when trying to spot memory leaks,
because malloc(0) or realloc(0) do not count as allocations since we only
account for the application-usable size. This is the problem that made
issue #1406 not to appear as a leak.
What we're doing now is to account for one extra pointer (the one that
memory allocators usually place before the returned area), so that a
malloc(0) will properly account for 4 or 8 bytes. We don't need something
exact, we just need something non-zero so that a realloc(X) followed by a
realloc(0) without a free() gives a small non-zero result.
It was verified that the results are stable including in the presence
of lots of malloc/realloc/free as happens when stressing Lua.
It would make sense to backport this to 2.4 as it helps in bug reports.
Willy Tarreau [Fri, 22 Oct 2021 14:26:12 +0000 (16:26 +0200)]
MINOR: memprof: report the delta between alloc and free on realloc()
realloc() calls are painful to analyse because they have two non-zero
columns and trying to spot a leaking one requires a bit of scripting.
Let's simply append the delta at the end of the line when alloc and
free are non-nul.
It would be useful to backport this to 2.4 to help with bug reports.
Willy Tarreau [Fri, 22 Oct 2021 14:00:02 +0000 (16:00 +0200)]
BUG/MEDIUM: lua: fix memory leaks with realloc() on non-glibc systems
In issue #1406, Lev Petrushchak reported a nasty memory leak on Alpine
since haproxy 2.4 when using Lua, that memory profiling didn't detect.
After inspecting the code and Lua's code, it appeared that Lua's default
allocator does an explicit free() on size zero, while since 2.4 commit d36c7fa5e ("MINOR: lua: simplify hlua_alloc() to only rely on realloc()"),
haproxy only calls realloc(ptr,0) that performs a free() on glibc but not
on other systems as it's not required by POSIX...
This patch reinstalls the explicit test for nsize==0 to call free().
Thanks to Lev for the very documented report, and to Tim for the links
to a musl thread on the same subject that confirms the diagnostic.
MINOR: quic: Increase the size of handshake RX UDP datagrams
Some browsers may send Initial packets with sizes greater than 1252 bytes
(QUIC_INITIAL_IPV4_MTU). Let us increase this size limit up to 2048 bytes.
Also use this size for "max_udp_payload_size" transport parameter to limit
the size of the datagrams we want to receive.
Willy Tarreau [Fri, 22 Oct 2021 06:34:14 +0000 (08:34 +0200)]
CLEANUP: resolvers: get rid of single-iteration loop in resolv_get_ip_from_response()
In issue 1424 Coverity reports that the loop increment is unreachable,
which is true, the list_for_each_entry() was replaced with a for loop,
but it was already not needed and was instead used as a convenient
construct for a single iteration lookup. Let's get rid of all this
now and replace the loop with an "if" statement.
Willy Tarreau [Thu, 21 Oct 2021 20:24:31 +0000 (22:24 +0200)]
MINOR: mux-h2: perform a full cycle shutdown+drain on close
While in H1 we can usually close quickly, in H2 a client might be sending
window updates or anything while we're sending a GOAWAY and the pending
data in the socket buffers at the moment the close() is performed on the
socket results in the output data being lost and an RST being emitted.
One example where this happens easily is with h2spec, which randomly
reports connection resets when waiting for a GOAWAY while haproxy sends
it, as seen in issue #1422. With h2spec it's not window updates that are
causing this but the fact that h2spec has to upload the payload that
comes with invalid frames to accommodate various implementations, and
does that in two different segments. When haproxy aborts on the invalid
frame header, the payload was not yet received and causes an RST to
be sent.
Here we're dealing with this two ways:
- we perform a shutdown(WR) on the connection to forcefully push pending
data on a front connection after the xprt is shut and closed ;
- we drain pending data
- then we close
This totally solves the issue with h2spec, and the extra cost is very
low, especially if we consider that H2 connections are not set up and
torn down often. This issue was never observed with regular clients,
most likely because this pattern does not happen in regular traffic.
After more testing it could make sense to backport this, at least to
avoid reporting errors on h2spec tests.
Willy Tarreau [Thu, 21 Oct 2021 19:31:42 +0000 (21:31 +0200)]
MINOR: connection: add a new CO_FL_WANT_DRAIN flag to force drain on close
Sometimes we'd like to do our best to drain pending data before closing
in order to save the peer from risking to receive an RST on close.
This adds a new connection flag CO_FL_WANT_DRAIN that is used to
trigger a call to conn_ctrl_drain() from conn_ctrl_close(), and the
sock_drain() function ignores fd_recv_ready() if this flag is set,
in order to catch latest data. It's not used for now.
Willy Tarreau [Thu, 21 Oct 2021 15:30:06 +0000 (17:30 +0200)]
BUG/MINOR: mux-h2: do not prevent from sending a final GOAWAY frame
Some checks were added by commit 9a3d3fcb5 ("BUG/MAJOR: mux-h2: Don't try
to send data if we know it is no longer possible") to make sure we don't
loop forever trying to send data that cannot leave. But one of the
conditions there is not correct, the one relying on H2_CS_ERROR2. Indeed,
this state indicates that the error code was serialized into the mux
buffer, and since the test is placed before trying to send the data to
the socket, if the connection states only contains a GOAWAY frame, it
may refrain from sending and may close without sending anything. It's
not dramatic, as GOAWAY reports connection errors in situations where
delivery is not even certain, but it's cleaner to make sure the error
is properly sent, and it avoids upsetting h2spec, as seen in github
issue #1422.
Given that the patch above was backported as far as 1.8, this patch will
also have to be backported that far.
Willy Tarreau [Thu, 21 Oct 2021 14:17:29 +0000 (16:17 +0200)]
BUG/MINOR: task: do not set TASK_F_USR1 for no reason
This applicationn specific flag was added in 2.4-dev by commit 6fa8bcdc7
("MINOR: task: add an application specific flag to the state: TASK_F_USR1")
to help preserve a the idle connections status across wakeup calls. While
the code to do this was OK for tasklets, it was wrong for tasks, as in an
effort not to lose it when setting the RUNNING flag (that tasklets don't
have), it ended up being inconditionally set. It just happens that for now
no regular tasks use it, only tasklets.
This fix makes sure we always atomically perform (state & flags | running)
there, using a CAS. It also does it for tasklets because it was possible
to lose some such flags if set by another thread, even though this should
not happen with current code. In order to make the code more readable (and
avoid the previous mistake of repeated flags in the bit field), a new
TASK_PERSISTENT aggregate was declared in task.h for this.
In practice the CAS is cheap here because task states are stable or
convergent so the loop will almost never be taken.
Willy Tarreau [Thu, 21 Oct 2021 12:33:38 +0000 (14:33 +0200)]
OPTIM: dns: use an atomic check for the list membership
The crash that was fixed by commit 7045590d8 ("BUG/MAJOR: dns: attempt
to lock globaly for msg waiter list instead of use barrier") was now
completely analysed and confirmed to be partially a result of the
debugging code added to LIST_INLIST(), which was looking at both
pointers and their reciprocals, and that, if used in a concurrent
context, could perfectly return false if a neighbor was being added or
removed while the current one didn't change, allowing the LIST_APPEND
to fail.
As the LIST API was not designed to be used in a concurrent context,
we should not rely on LIST_INLIST() but on the newly introduced
LIST_INLIST_ATOMIC().
This patch simply reverts the commit above to switch to the new test,
saving a lock during potentially long operations. It was verified that
the check doesn't fail anymore.
It is unsure what the performance impact of the fix above could be in
some contexts. If any performance regression is observed, it could make
sense to backport this patch, along with the previous commit introducing
the LIST_INLIST_ATOMIC() macro.
Willy Tarreau [Thu, 21 Oct 2021 12:06:01 +0000 (14:06 +0200)]
MINOR: list: add new macro LIST_INLIST_ATOMIC()
This macro is similar to LIST_INLIST() except that it is guaranteed to
perform the test atomically, so that even if LIST_INLIST() is intrumented
with debugging code to perform extra consistency checks, it will not fail
when used in the context of barriers and atomic ops.
Willy Tarreau [Thu, 21 Oct 2021 12:45:28 +0000 (14:45 +0200)]
OPTIM: resolvers: move the eb32 node before the data in the answer_item
perf top shows that we spend a lot of time trying to read item->type in
the lookup loop, because the node is placed after the very long name,
so when the node is found, no data is in the cache yet. Let's simply
move the node upper in the struct. This results in the CPU usage of
resolv_validate_dns_response() to drop by 4 points.
Willy Tarreau [Thu, 21 Oct 2021 13:05:34 +0000 (15:05 +0200)]
BUG/MAJOR: buf: fix varint API post- vs pre- increment
A bogus test in b_get_varint(), b_put_varint(), b_peek_varint() shifts
the end of the buffer by one byte. Since the bug is the same in the read
and write functions, the buffer contents remain compatible, which explains
why this bug was not detected earlier. But if the buffer ends on an
aligned address or page, it can result in a one-byte overflow which will
typically cause a crash or an inconsistent behavior.
This API is only used by rings (e.g. for traces and boot messages) and
by DNS responses, so the probability to hit it is extremely low, but a
crash on boot was observed.
Willy Tarreau [Thu, 21 Oct 2021 06:18:01 +0000 (08:18 +0200)]
MEDIUM: resolvers: hash the records before inserting them into the tree
We're using an XXH32() on the record to insert it into or look it up from
the tree. This way we don't change the rest of the code, the comparisons
are still made on all fields and the next node is visited on mismatch. This
also allows to continue to use roundrobin between identical nodes.
Just doing this is sufficient to see the CPU usage go down from ~60-70% to
4% at ~2k DNS requests per second for farm with 300 servers. A larger
config with 12 backends of 2000 servers each shows ~8-9% CPU for 6-10000
DNS requests per second.
It would probably be possible to go further with multiple levels of indexing
but it's not worth it, and it's important to remember that tree nodes take
space (the struct answer_list went back from 576 to 600 bytes).
Willy Tarreau [Thu, 21 Oct 2021 05:39:57 +0000 (07:39 +0200)]
MEDIUM: resolvers: replace the answer_list with a (flat) tree
With SRV records, a huge amount of time is spent looking for records
by walking long lists. It is possible to reduce this by indexing values
in trees instead. However the whole code relies a lot on the list
ordering, and even implements some round-robin on it to distribute IP
addresses to servers.
This patch starts carefully by replacing the list with a an eb32 tree
that is still used like a list, with a constant key 0. Since ebtrees
preserve insertion order for duplicates, the tree walk visits the nodes
in the exact same order it did with the lists. This allows to implement
the required infrastructure without changing the behavior.
projects / thirdparty / haproxy.git / log
