When a message is sent, an extra check is performed when the parser is
switch to MSG_DONE state to be sure the EOM flag is really set. This flag is
quite new and replaces the EOM block. Thus, this test is a safeguard waiting
for a proper refactoring of the outgoing side.
In the H2 mux, when a empty DATA frame is used to finish a message, just to
set the ES flag, we now only set the EOM flag on the HTX message. However,
if the HTX message is empty, this event will not be properly handled on the
other side because there is no effective data to handle. Thus, it is
interpreted as an abort by the H1 mux.
It is in part caused by the current H1 mux design but also because there is
no way to emit empty HTX block (NOOP HTX block) or to wakeup a mux for send
when there is no data to finish some internal processing.
Thus, for now, to work around this limitation, an EOT HTX block is added by
the H2 mux if a EOM flag is added on an empty HTX message. This case is only
possible when an empty DATA frame with the ES flag is received.
This fix is specific for 2.4. No backport needed.
In HTTP/2, we may have trailers for messages with a Content-length
header. Thus, when the H2 mux receives a HEADERS frame at the end of a
message, it always emits TLR and EOT HTX blocks. On the H1 mux, if this
happens, these blocks are just skipped because we cannot emit trailers for a
non-chunked message. But the EOT HTX block must not be blindly
ignored. Indeed, there is no longer EOM HTX block to mark the end of the
message. Thus the EOT block, when found, is the end of the message. So we
must handle it to swith in MSG_DONE state.
This fix is specific for 2.4. No backport needed.
When payload is received for a bodyless response, for instance a response to
a HEAD request, it is silently skipped. Unfortunately, when this happens,
the end of the message is not properly handled. The response remains in the
MSG_DATA state (or MSG_TRAILERS if the message is chunked). In addition,
when a zero-copy is possible, the data are not removed from the channel
buffer and the H1 connection is killed because an error is then triggered.
To fix the bug, the zero-copy is disabled for bodyless responses. It is not
a problem because there is no copy at all. And the last block (DATA or EOT)
is now properly handled.
This bug was introduced by the commit e5596bf53 ("MEDIUM: mux-h1: Don't emit
any payload for bodyless responses").
This fix is specific for 2.4. No backport needed.
During the message parsing, if in MSG_DONE state, the CS_FL_EOI flag must
always be set on the conn-stream if following conditions are met :
* It is a response or
* It is a request but not a protocol upgrade nor a CONNECT.
For now, there is no test on the message type (request or response). Thus
the CS_FL_EOI flag is not set for a response with a "Connection: upgrade"
header but not a 101 response.
This bug was introduced by the commit 3e1748bbf ("BUG/MINOR: mux-h1: Don't
set CS_FL_EOI too early for protocol upgrade requests"). It was backported
as far as 2.0. Thus, this patch must also be backported as far as 2.0.
If internal error is reported by the mux during HTTP request parsing, the
HTTP error counter should not be incremented. It should only be incremented
on parsing error to reflect errors caused by clients.
This patch must be backported as far as 2.0. During the backport, the same
must be performed for 408-request-time-out errors.
The HTTP error counter reflects the number of errors caused by
clients. Thus, In the H1 mux, it should only be increment on parsing errors.
This fix is specific for 2.4. No backport needed.
Amaury reported that the commit 3ce6eed ("MEDIUM: ssl: add a rwlock for
SSL server session cache") introduced some warning during compilation:
include/haproxy/thread.h|411 col 2| warning: enumeration value 'SSL_SERVER_LOCK' not handled in switch [-Wswitch]
This patch fix the issue by adding the right entry in the switch block.
Must be backported where 3ce6eed is backported. (2.4 only for now)
Historically we've been counting lots of client-triggered events in stick
tables to help detect misbehaving ones, but we've been missing the same on
the server side, and there's been repeated requests for being able to count
the server errors per URL in order to precisely monitor the quality of
service or even to avoid routing requests to certain dead services, which
is also called "circuit breaking" nowadays.
This commit introduces http_fail_cnt and http_fail_rate, which work like
http_err_cnt and http_err_rate in that they respectively count events and
their frequency, but they only consider server-side issues such as network
errors, unparsable and truncated responses, and 5xx status codes other
than 501 and 505 (since these ones are usually triggered by the client).
Note that retryable errors are purposely not accounted for, so that only
what the client really sees is considered.
With this it becomes very simple to put some protective measures in place
to perform a redirect or return an excuse page when the error rate goes
beyond a certain threshold for a given URL, and give more chances to the
server to recover from this condition. Typically it could look like this
to bypass a URL causing more than 10 requests per second:
stick-table type string len 80 size 4k expire 1m store http_fail_rate(1m)
http-request track-sc0 base # track host+path, ignore query string
http-request return status 503 content-type text/html \
lf-file excuse.html if { sc0_http_fail_rate gt 10 }
A more advanced mechanism using gpt0 could even implement high/low rates
to disable/enable the service.
Reg-test converteers_ref_cnt_never_dec.vtc was updated to test it.
The sleep time calculation in next_event_delay() was wrong because it
was dividing 999 by the number of pending events, and was directly
responsible for an observation made a long time ago that listeners
would eat all the CPU when hammered while globally rate-limited,
because the more the queued events, the least it would wait, and would
ignore the configured frequency to compute the delay.
This was addressed in various ways in listeners through the switch to
the FULL state and the wakeup of manage_global_listener_queue() that
avoids this fast loop, but the calculation made there remained wrong
nevertheless. It's even visible with this patch that the accept
frequency is much more accurate at low values now; for example,
configuring a maxconrate of 10 would give between 8.99 and 11.0 cps
before this patch and between 9.99 and 10.0 with it.
Better fix it now in case it's reused anywhere else and causes confusion
again. It maybe be backported but is probably not worth it.
mul32hi() multiples a constant a with a variable b from 0 to 0xffffffff
and shifts the result by 32 bits. It's visible that it's always impossible
to reach the constant a this way because the product always misses exactly
one unit of a to be preserved. And this cannot be corrected by the caller
either as adding one to the output will only shift the output range, and
it's not possible to pass 2^32 on the ratio <b>. The right approach is to
add "a" after the multiplication so that the input range is always
preserved for all ratio values from 0 to 0xffffffff:
(a=0x00000000 * b=0x00000000 + a=0x00000000) >> 32 = 0x00000000
(a=0x00000000 * b=0x00000001 + a=0x00000000) >> 32 = 0x00000000
(a=0x00000000 * b=0xffffffff + a=0x00000000) >> 32 = 0x00000000
(a=0x00000001 * b=0x00000000 + a=0x00000001) >> 32 = 0x00000000
(a=0x00000001 * b=0x00000001 + a=0x00000001) >> 32 = 0x00000000
(a=0x00000001 * b=0xffffffff + a=0x00000001) >> 32 = 0x00000001
(a=0xffffffff * b=0x00000000 + a=0xffffffff) >> 32 = 0x00000000
(a=0xffffffff * b=0x00000001 + a=0xffffffff) >> 32 = 0x00000001
(a=0xffffffff * b=0xffffffff + a=0xffffffff) >> 32 = 0xffffffff
This is only used in freq_ctr calculations and the slightly lower value
is unlikely to have ever been noticed by anyone. This may be backported
though it is not important.
When adding the server side support for certificate update over the CLI
we encountered a design problem with the SSL session cache which was not
locked.
Indeed, once a certificate is updated we need to flush the cache, but we
also need to ensure that the cache is not used during the update.
To prevent the use of the cache during an update, this patch introduce a
rwlock for the SSL server session cache.
In the SSL session part this patch only lock in read, even if it writes.
The reason behind this, is that in the session part, there is one cache
storage per thread so it is not a problem to write in the cache from
several threads. The problem is only when trying to write in the cache
from the CLI (which could be on any thread) when a session is trying to
access the cache. So there is a write lock in the CLI part to prevent
simultaneous access by a session and the CLI.
This patch also remove the thread_isolate attempt which is eating too
much CPU time and was not protecting from the use of a free ptr in the
session.
both SSL_CTX_set_msg_callback and SSL_CTRL_SET_MSG_CALLBACK defined since
ea262260469e49149cb10b25a87dfd6ad3fbb4ba, we can safely switch to that guard
instead of OpenSSL version
I saw some people falling back to unix socket to collect some data they
could not find in prometheus exporter. One of them is base info from
stick tables (used/size).
I do not plan to extend it more for now; keys are quite a mess to
handle.
This should resolve github issue #1008.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
Remove remaining descrition which are common to stats.c.
This patch is a followup of commit
82b2ce2f96 ("MINOR:
contrib/prometheus-exporter: use stats desc when possible"). I probably
messed up with one of my rebase because I'm pretty sure I removed them
at some point, but who knows what happened.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
Because of a buggy tests when processing the EOH HTX block, an extra CRLF is
added for empty chunked messages. This bug was introduced by the commit
d1ac2b90c ("MAJOR: htx: Remove the EOM block type and use HTX_FL_EOM
instead").
This fix is specific for 2.4. No backport needed.
special guard macros HAVE_SSL_CTX_ADD_SERVER_CUSTOM_EXT was defined earlier
exactly for guarding SSL_CTX_add_server_custom_ext, let us use it wherever
appropriate
HAVE_SSL_CTX_ADD_SERVER_CUSTOM_EXT was introduced in ec60909871
however it was defined as HAVE_SL_CTX_ADD_SERVER_CUSTOM_EXT (missing "S")
let us fix typo
Released version 2.4-dev7 with the following main changes :
- BUG/MINOR: stats: Continue to fill frontend stats on unimplemented metric
- BUILD: ssl: guard Client Hello callbacks with HAVE_SSL_CLIENT_HELLO_CB macro instead of openssl version
- BUG/MINOR: stats: Init the metric variable when frontend stats are filled
- MINOR: contrib/prometheus-exporter: better output of Not-a-Number
- CLEANUP: stats: improve field selection for frontend http fields
- CLEANUP: assorted typo fixes in the code and comments
- DOC: Improve documentation of the various hdr() fetches
- MEDIUM: stats: allow to select one field in `stats_fill_be_stats`
- MINOR: contrib/prometheus-exporter: use fill_be_stats for backend dump
- MEDIUM: stats: allow to select one field in `stats_fill_sv_stats`
- MINOR: contrib/prometheus-exporter: use fill_sv_stats for server dump
- MINOR: abort() on my_unreachable() when DEBUG_USE_ABORT is set.
- BUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown
- BUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name
- MINOR: reg-tests: add http-reuse test
- CLEANUP: srv: fix comment for pool-max-conn
- CLEANUP: backend: remove an obsolete comment on conn_backend_get
- REORG: backend: simplify conn_backend_get
- MINOR: ssl: Server ssl context prepare function refactoring
- MINOR: ssl: Certificate chain loading refactorization
- MEDIUM: ssl: Load client certificates in a ckch for backend servers
- MEDIUM: ssl: Enable backend certificate hot update
- MINOR: ssl: Remove client_crt member of the server's ssl context
- CLEANUP: ssl/cli: rework free in cli_io_handler_commit_cert()
- CLEANUP: ssl: remove SSL_CTX function parameter
- CLEANUP: ssl: make load_srv_{ckchs,cert} match their bind counterpart
- BUILD: Include stdlib.h in compiler.h if DEBUG_USE_ABORT is set
- CI: Fix DEBUG_STRICT definition for Coverity
- BUG/MINOR: stats: Remove a break preventing ST_F_QCUR to be set for servers
- BUG/MINOR: stats: Add a break after filling ST_F_MODE field for servers
- CLEANUP: ssl: remove dead code in ckch_inst_new_load_srv_store()
- BUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()
- BUG/MEDIUM: session: only retrieve ready idle conn from session
- BUG/MEDIUM: backend: never reuse a connection for tcp mode
- REGTESTS: set_ssl_server_cert.vtc: remove the abort command
- REGTESTS: set_ssl_server_cert.vtc: check the Sha1 Fingerprint
- REGTESTS: set_ssl_server_cert.vtc: check the sha1 from the server
- MEDIUM: stream-int: Take care of EOS if the SI wake callback function
- MINOR: mux-h1: Try to wake up data layer first before calling its wake callback
- MINOR: mux-h1: Wake up H1C after its creation if input buffer is not empty
- MEDIUM: mux-h1: Add ST_READY state for the H1 connections
- MINOR: stream: Add a function to validate TCP to H1 upgrades
- MEDIUM: http-ana: Do nothing in wait-for-request analyzer if not htx
- BUG/MEDIUM: stream: Don't immediatly ack the TCP to H1 upgrades
- BUG/MAJOR: mux-h1: Properly handle TCP to H1 upgrades
- MINOR: htx/http-ana: Save info about Upgrade option in the Connection header
- MEDIUM: http-ana: Refuse invalid 101-switching-protocols responses
- BUG/MINOR: h2/mux-h2: Reject 101 responses with a PROTOCOL_ERROR h2s error
- MINOR: mux-h1/mux-fcgi: Don't set TUNNEL mode if payload length is unknown
- MINOR: mux-h1: Split H1C_F_WAIT_OPPOSITE flag to separate input/output sides
- MINOR: mux-h2: Add 2 flags to help to properly handle tunnel mode
- MEDIUM: mux-h2: Block client data on server side waiting tunnel establishment
- MEDIUM: mux-h2: Close streams when processing data for an aborted tunnel
- MEDIUM: mux-h1: Properly handle tunnel establishments and aborts
- BUG/MAJOR: mux-h1/mux-h2/htx: Fix HTTP tunnel management at the mux level
- MINOR: htx: Rename HTX_FL_EOI flag into HTX_FL_EOM
- REGTESTS: Don't run http_msg_full_on_eom script on the 2.4 anymore
- MINOR: htx: Add a function to know if a block is the only one in a message
- MAJOR: htx: Remove the EOM block type and use HTX_FL_EOM instead
- MINOR: mux-h1: Add a flag on H1 streams with a response known to be bodyless
- MEDIUM: mux-h1: Don't emit any payload for bodyless responses
- MINOR: mux-h1: Don't emit C-L and T-E headers for 204 and 1xx responses
- MINOR: mux-h1: Don't add Connection close/keep-alive header for 1xx messages
- MINOR: h2/mux-h2: Add flags to notify the response is known to have no body
- MEDIUM: mux-h2: Don't emit DATA frame for bodyless responses
- MEDIUM: http-ana: Deal with L7 retries in HTTP analysers
- MINOR: h1: reject websocket handshake if missing key
- MEDIUM: h1: generate WebSocket key on response if needed
- MINOR: mux_h2: define H2_SF_EXT_CONNECT_SENT stream flag
- MEDIUM: h2: parse Extended CONNECT reponse to htx
- MEDIUM: mux_h2: generate Extended CONNECT from htx upgrade
- MEDIUM: h1: add a WebSocket key on handshake if needed
- MEDIUM: mux_h2: generate Extended CONNECT response
- MEDIUM: h2: parse Extended CONNECT request to htx
- MEDIUM: h2: send connect protocol h2 settings
- MINOR: vtc: add test for h1/h2 protocol upgrade translation
- MINOR: vtc: add websocket test
- REGTESTS: Fix required versions for several scripts
- REGTEST: Don't use the websocket to validate http-check
- MINOR: mux-h1/trace: add traces at level ERROR for all kind of errors
- MINOR: mux-fcgi/trace: add traces at level ERROR for all kind of errors
- MINOR: h1: Raise the chunk size limit up to (2^52 - 1)
- BUG/MEDIUM: listener: do not accept connections faster than we can process them
- REGTESTS: set_ssl_server_cert.vtc: set as broken
- Revert "BUG/MEDIUM: listener: do not accept connections faster than we can process them"
- BUG/MINOR: backend: check available list allocation for reuse
- CI: Fix the coverity builds
- DOC: management: fix "show resolvers" alphabetical ordering
- MINOR: tools: add print_time_short() to print a condensed duration value
- MINOR: activity: make profiling more manageable
- MINOR: activity: declare a new structure to collect per-function activity
- MEDIUM: tasks/activity: collect per-task statistics when profiling is enabled
- MINOR: activity: also report collected tasks stats in "show profiling"
- MINOR: activity: flush scheduler stats on "set profiling tasks on"
- MINOR: activity: add a new "show tasks" command to list currently active tasks
- MINOR: listener: export accept_queue_process
- MINOR: session: export session_expire_embryonic()
- MINOR: muxes: export the timeout and shutr task handlers
- MINOR: checks: export a few functions that appear often in trace dumps
- MINOR: peers: export process_peer_sync() to improve traces
- MINOR: stick-tables: export process_table_expire()
- MINOR: mux-h1: Remove first useless test on count in h1_process_output()
- BUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list
- MINOR: http-fetch: Don't check if argument list is set in sample fetches
- MINOR: http-conv: Don't check if argument list is set in sample converters
- MINOR: sample: Don't check if argument list is set in sample fetches
- MINOR: ssl-sample: Don't check if argument list is set in sample fetches
- MINOR: mux-h2: Don't tests the start-line when sending HEADERS frame
- MINOR: mux-h2: Slightly improve request HEADERS frames sending
- MINOR: contrib/prometheus-exporter: declare states for objects
- MAJOR: contrib/prometheus-exporter: move ftd/bkd/srv states to labels
- MEDIUM: contrib/prometheus-exporter: Use dynamic labels instead of static ones
- MINOR: listener: export manage_global_listener_queue()
- BUG/MINOR: activity: take care of late wakeups in "show tasks"
- REGTESTS: set_ssl_server_cert.vtc: remove SSL caching and set as working
- REGTESTS: set_ssl_server_cert: cleanup the SSL caching option
- MINOR: checks: Add function to get the result code corresponding to a status
- MAJOR: contrib/prometheus-exporter: move health check status to labels
- MINOR: contrib/prometheus-exporter: improve service status description field
- MINOR: stats: improve pending connections description
- MINOR: stats: improve max stats descriptions
- MINOR: contrib/prometheus-exporter: use stats desc when possible
- MINOR: contrib/prometheus-exporter: add uweight field
- MINOR: contrib/prometheus-exporter: add recv logs_logs_total field
- CLEANUP: contrib/prometheus-exporter: remove unused includes
- CLEANUP: contrib/prometheus-exporter: align and reorder fields
- CLEANUP: contrib/prometheus-exporter: remove description in README
- DOC: contrib/prometheus-exporter: Add missing metrics in README
- BUG/MINOR: contrib/prometheus-exporter: Add missing label for ST_F_HRSP_1XX
- BUG/MINOR: contrib/prometheus-exporter: Restart labels dump at the right pos
- BUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store
- BUG/MEDIUM: ssl: check a connection's status before computing a handshake
- BUG/MINOR: mux_h2: fix incorrect stat titles
- MINOR: ssl/cli: flush the server session cache upon 'commit ssl cert'
- BUG/MINOR: cli: fix set server addr/port coherency with health checks
- MINOR: server: Don't set the check port during the update from a state file
- MINOR: dns: Don't set the check port during a server dns resolution
- MEDIUM: check: remove checkport checkaddr flag
- MEDIUM: server: adding support for check_port in server state
- BUG/MINOR: check: consitent way to set agentaddr
- MEDIUM: check: align agentaddr and agentport behaviour
- DOC: server: Add missing params in comment of the server state line parsing
- BUG/MINOR: xxhash: make sure armv6 uses memcpy()
- REGTESTS: mark http-check-send.vtc as 2.4-only
- REGTESTS: mark sample_fetches/hashes.vtc as 2.4-only
- BUG/MINOR: ssl: do not try to use early data if not configured
- REGTESTS: unbreak http-check-send.vtc
- MINOR: cli/show_fd: report local and report ports when known
- BUILD: Makefile: move REGTESTST_TYPE default setting
- BUG/MEDIUM: mux-h2: handle remaining read0 cases
- CLEANUP: http-htx: Set buffer area to NULL instead of malloc(0)
- BUG/MINOR: sock: Unclosed fd in case of connection allocation failure
- BUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED
The demux loop could quit on missing data but the H2_CF_END_REACHED flag
would not be set in this case. This fixes a remaining situation where
previous commit f09612289 ("BUG/MEDIUM: mux-h2: handle remaining read0
cases") could not be sufficient and still leave CLOSE_WAIT. It's harder
to reproduce but was still observed in prod.
Now we quit via the end of the loop which already takes care of shutr.
This should be backported along with the patch above as far as 2.0.
If allocating a connection object failed right after a successful accept
on a listener, the new file descriptor was not properly closed.
This fixes GitHub issue #905.
It can be backported to 2.3.
During error files conversion to HTX message, in http_str_to_htx(), if a
file is empty, the corresponding buffer's area is initialized with a
malloc(0) and its size is set to 0. There is no problem here. The behaviour
is totally defined. But it is not really intuitive. Instead, we can simply
set the area to NULL.
This patch should fix the issue #1022.
Commit 3d4631fec ("BUG/MEDIUM: mux-h2: fix read0 handling on partial
frames") tried to address an issue introduced in commit aade4edc1 where
read0 wasn't properly handled in the middle of a frame. But the fix was
incomplete for two reasons:
- first, it would set H2_CF_RCVD_SHUT in h2_recv() after detecting
a read0 but the condition was guarded by h2_recv_allowed() which
explicitly excludes read0 ;
- second, h2_process would only call h2_process_demux() when there
were still data in the buffer, but closing after a short pause to
leave a buffer empty wouldn't be caught in this case.
This patch fixes this by properly taking care of the received shutdown
and by also waking up h2_process_demux() on an empty buffer if the demux
is not blocked.
Given the patches above were tagged for backporting to 2.0, this one
should be as well.
In patch 3bad3d5 ("BUILD: Makefile: exclude broken tests by default"),
the default setting of the REGTESTST_TYPE variable was set in the
Makefile instead of the run-regtests.sh script.
Doing it in the Makefile was breaking the use of this environment
varible with make ( REGTESTS_TYPES=slow,default make reg-tests )
This patch move the default setting from the Makefile to
run-regtests.sh. It also change the documentation in `make
reg-tests-help` about the default value.
This patch should be backported where 3bad3d5 is backported.
FD dumps are not always easy to match against netstat dumps, and often
require an lsof as a third dump. Let's emit the socket family, and the
local and remore ports when the FD is an IPv4/IPv6 socket, this will
significantly ease the matching.
As noticed by Christopher, I messed up the version fix in commit
cb4ed02ef ("REGTESTS: mark http-check-send.vtc as 2.4-only"), as while
looking up the commit introducing the change I accidently reverted it.
Let's reinsert the contents of the file prior to that fix, except the
version, of course.
The CO_FL_EARLY_SSL_HS flag was inconditionally set on the connection,
resulting in SSL_read_early_data() always being used first in handshake
calculations. While this seems to work well (probably that there are
fallback paths inside openssl), it's particularly confusing and makes
the debugging quite complicated. It possibly is not optimal by the way.
This flag ought to be set only when early_data is configured on the bind
line. Apparently there used to be a good reason for doing it this way in
1.8 times, but it really does not make sense anymore. It may be OK to
backport this to 2.3 if this helps with troubleshooting, but better not
go too far as it's unlikely to fix any real issue while it could introduce
some in old versions.
Commit 9eea56009 ("REGTESTS: add tests for the xxh3 converter") introduced
the xxh3 to the tests thus made it incompatible with 2.3 and older, let's
upgrade the version requirement.
There was a special case made to allow ARMv6 to use unaligned accesses
via a cast in xxHash when __ARM_FEATURE_UNALIGNED is defined. But while
ARMv6 (and v7) does support unaligned accesses, it's only for 32-bit
pointers, not 64-bit ones, leading to bus errors when the compiler emits
an ldrd instruction and the input (e.g. a pattern) is not aligned, as in
issue #1035.
Note that v7 was properly using the packed approach here and was safe,
however haproxy versions 2.3 and older use the old r39 xxhash code which
has the same issue for armv7. A slightly different fix is required there,
by using a different definition of packed for 32 and 64 bits.
The problem is really visible when running v7 code on a v8 kernel because
such kernels do not implement alignment trap emulation, and the process
dies when this happens. This is why in the issue above it was only detected
under lxc. The emulation could have been disabled on v7 as well by writing
zero to /proc/cpu/alignment though.
This commit is a backport of xxhash commit a470f2ef ("update default memory
access for armv6").
Thanks to @srkunze for the report and tests, @stgraber for his help on
setting up an easy reproducer outside of lxc, and @Cyan4973 for the
discussion around the best way to fix this. Details and alternate patches
available on https://github.com/Cyan4973/xxHash/issues/490.
in the same manner of agentaddr, we now:
- permit to set agentport through `port` keyword, like it is the case
for agentaddr through `addr`
- set the priority on `agent-port` keyword when used
- add a flag to be able to test when the value is set like for agentaddr
it makes the behaviour between `addr` and `port` more consistent.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
small consistency problem with `addr` and `agent-addr` options:
for the both options, the last one parsed is always used to set the
agent-check addr. Thus these two lines don't have the same behavior:
server ... addr <addr1> agent-addr <addr2>
server ... agent-addr <addr2> addr <addr1>
After this patch `agent-addr` will always be the priority option over
`addr`. It means we test the flag before setting agentaddr.
We also fix all the places where we did not set the flag to be coherent
everywhere.
I was not really able to determine where this issue is coming from. So
it is probable we may backport it to all stable version where the agent
is supported.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
We can currently change the check-port using the cli command `set server
check-port` but there is a consistency issue when using server state.
This patch aims to fix this problem but will be also a good preparation
work to get rid of checkport flag, so we are able to know when checkport
was set by config.
I am fully aware this is not making github #953 moving forward, I
however think this might be acceptable while waiting for a proper
solution and resolve consistency problem faced with port settings.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
While trying to fix some consistency problem with the config file/cli
(e.g. check-port cli command does not set the flag), we realised
checkport flag was not necessarily needed. Indeed tcpcheck uses service
port as the last choice if check.port is zero. So we can assume if
check.port is zero, it means it was never set by the user, regardless if
it is by the cli or config file. In the longterm this will avoid to
introduce a new consistency issue if we forget to set the flag.
in the same manner of checkport flag, we don't really need checkaddr
flag. We can assume if checkaddr is not set, it means it was never set
by the user or config.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
When a server dns resolution is performed, there is no reason to set an
unconfigured check port with the server port. Because by default, if the
check port is not set, the server's one is used. Thus we can remove this
useless assignment. It is mandatory for next improvements.
When the server state is loaded from a server-state file, there is no reason
to set an unconfigured check port with the server port. Because by default,
if the check port is not set, the server's one is used. Thus we can remove
this useless assignment. It is mandatory for next improvements.
while reading `update_server_addr_port` I found out some things which
can be seen as incoherency. I hope I did not overlooked anything:
- one comment is stating check's address should be updated if it uses
the server one; however the condition checks if `SRV_F_CHECKADDR` is
set; this flag is set when a check address is set; result is that we
override the check address where I was not expecting it. In fact we
don't need to update anything here as server addr is used when check
addr is not set.
- same goes for check agent addr
- for port, it is a bit different, we update the check port if it is
unset. This is harmless because we also use server port if check port
is unset. However it creates some incoherency before/after using this
command, as check port should stay unset througout the life of the
process unless it is is set by `set server check-port` command.
quite hard to locate the origin of this this issue but the function was
introduced in commit d458adcc52 ("MINOR:
new update_server_addr_port() function to change both server's ADDR and
service PORT"). I was however not able to determine whether this is due
to a change of behavior along the years. So this patch can potentially
be backported up to v1.8 but we must be careful while doing so, as the
code has changed a lot. That being said, the bug being not very
impacting I would be fine keeping it for 2.4 only.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
Flush the SSL session cache when updating a certificate which is used on a
server line. This prevent connections to be established with a cached
session which was using the previous SSL_CTX.
This patch also replace the ha_barrier with a thread_isolate() since there
are more operations to do. The reg-test was also updated to remove the
'no-ssl-reuse' keyword which is now uneeded.
Duplicate titles for the stats H2_ST_{OPEN,TOTAL}_{CONN,STREAM}. These
entries are used on csv for the heading.
This must be backported up to 2.3.
This fixes the github issue #1102.
As spotted in issue #822, we're having a problem with error detection in
the SSL layer. The problem is that on an overwhelmed machine, accepted
connections can start to pile up, each of them requiring a slow handshake,
and during all this time if the client aborts, the handshake will still be
calculated.
The error controls are properly placed, it's just that the SSL layer
reads records exactly of the advertised size, without having the ability
to encounter a pending connection error. As such if injecting many TLS
connections to a listener with a huge backlog, it's fairly possible to
meet this situation:
12:50:48.236056 accept4(8, {sa_family=AF_INET, sin_port=htons(62794), sin_addr=inet_addr("127.0.0.1")}, [128->16], SOCK_NONBLOCK) = 1109
12:50:48.236071 setsockopt(1109, SOL_TCP, TCP_NODELAY, [1], 4) = 0
(process other connections' handshakes)
12:50:48.257270 getsockopt(1109, SOL_SOCKET, SO_ERROR, [ECONNRESET], [4]) = 0
(proof that error was detectable there but this code was added for the PoC)
12:50:48.257297 recvfrom(1109, "\26\3\1\2\0", 5, 0, NULL, NULL) = 5
12:50:48.257310 recvfrom(1109, "\1\0\1\3"..., 512, 0, NULL, NULL) = 512
(handshake calculation taking 700us)
12:50:48.258004 sendto(1109, "\26\3\3\0z"..., 1421, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = -1 EPIPE (Broken pipe)
12:50:48.258036 close(1109) = 0
The situation was amplified by the multi-queue accept code, as it resulted
in many incoming connections to be accepted long before they could be
handled. Prior to this they would have been accepted and the handshake
immediately started, which would have resulted in most of the connections
waiting in the the system's accept queue, and dying there when the client
aborted, thus the error would have been detected before even trying to
pass them to the handshake code.
As a result, with a listener running on a very large backlog, it's possible
to quickly accept tens of thousands of connections and waste time slowly
running their handshakes while they get replaced by other ones.
This patch adds an SO_ERROR check on the connection's FD before starting
the handshake. This is not pretty as it requires to access the FD, but it
does the job.
Some improvements should be made over the long term so that the transport
layers can report extra information with their ->rcv_buf() call, or at the
very least, implement a ->get_conn_status() function to report various
flags such as shutr, shutw, error at various stages, allowing an upper
layer to inquire for the relevance of engaging into a long operation if
it's known the connection is not usable anymore. An even simpler step
could probably consist in implementing this in the control layer.
This patch is simple enough to be backported as far as 2.0.
Many thanks to @ngaugler for his numerous tests with detailed feedback.
The "abort ssl cert" command is buggy and removes the current ckch store,
and instances, leading to SNI removal. It must only removes the new one.
This patch also adds a check in set_ssl_cert.vtc and
set_ssl_server_cert.vtc.
Must be backported as far as 2.2.
For some metrics, several lines are produced per entity, one per label
value. For instance, the health-check status (ST_F_CHECK_STATUS) or the
entity status (ST_F_STATUS). The dump may be stopped in the middle of the
labels processing if the output buffer is full. This means the next time, we
must take care to restart on the right label value.
For now, this part is buggy and we always restart to dump all the label
values again from the beginning. To be sure to restart at the right
position, the field <ctx.stats.st_code> in the applet context is used to
save the last position. Of course, we take care to reset this value when
necessary.
This fix is specific for 2.4. No backport needed.
Since the labels are dynamically created for each metric, the "code" label
of the ST_F_HRSP_1XX field is missing. To fix the bug, this metric is
handled in the same way the other ST_F_HRSP_* field are. We only take care
to dump the metric header only once.
This bug was introduced by the commit 5a2f93873 ("MEDIUM:
contrib/prometheus-exporter: Use dynamic labels instead of static ones"). No
backport needed.
Some metrics were missing (haproxy_process_uptime_seconds and
haproxy_process_build_info). To ease the review against the service output,
the same order is used in the README.
Now that we got ride of description in prometheus code, let's assume we
no longer need to maintain it in README, and diret user to the output of
prometheus to get more info.
Signed-off-by: William Dauchy <wdauchy@gmail.com>
