Currently a connection is required on the remote side to emit a proxy
protocol header line. Let's support NULL addresses to emit an UNKNOWN
tag as well.
These two fetch methods predate the samples and used to store the
destination address into the server-facing connection's address field
because we had no other place at this time.
This will become problematic with the current connection changes, so
let's fix this.
We make the peers code use applet->ptr instead of conn->xprt_ctx to
store the pointer to the current peer. That way it does not depend
on a connection anymore.
This field was used by dumpstats to retrieve a pointer to the current
session, which may already be found from ->owner. With this change,
the stats code doesn't need the connection at all anymore.
We're trying to move the applets out of the struct connection. So
let's remove the dependence on xprt_st and introduce si->applet.st2
to store the missing contextual data instead.
The free() function must free the "struct pat_idx_elt".
This bug was introduced by commit ed66c29 (REORG: acl/pattern: extract
pattern matching from the acl file and create pattern.c), no backport
is needed.
The commit 37e340c (BUG/MEDIUM: stick: completely remove the unused flag
from the store entries) was incomplete. We also need to ensure that only
the first store-response for a table is applied and that it may coexist
with a possible store-request that was already done on this table.
This patch with the previous one should be backported to 1.4.
The store[] array in the session holds a flag which probably aimed to
differenciate store entries learned from the request from those learned
from the response, and allowing responses to overwrite only the request
ones (eg: have a server set a response cookie which overwrites the request
one).
But this flag is set when a response data is stored, and is never cleared.
So in practice, haproxy always runs with this flag set, meaning that
responses prevent themselves from overriding the request data.
It is desirable anyway to keep the ability not to override data, because
the override is performed only based on the table and not on the key, so
that would mean that it would be impossible to retrieve two different
keys to store into a same table. For example, if a client sets a cookie
and a server another one, both need to be updated in the table in the
proper order. This is especially true when multiple keys may be tracked
on each side into the same table (eg: list of IP addresses in a header).
So the correct fix which also maintains the current behaviour consists in
simply removing this flag and never try to optimize for the overwrite case.
This fix also has the benefit of significantly reducing the session size,
by 64 bytes due to alignment issues caused by this flag!
The bug has been there forever (since 1.4-dev7), so a backport to 1.4
would be appropriate.
Now instead of seeing many send() calls from multiple "tcp-check send"
rules, we fill the output buffer and try to send all only when we're
not in a send state or when the output buffer is too small for sending
the next message.
This results in a lot less syscalls and avoids filling the network with
many small packets. It will also improve the behaviour of some bogus
servers which expect a complete request in the first packet.
In recent commit 5ecb77f (MEDIUM: checks: add send/expect tcp based check),
bitfields were mistakenly used at some places for the actions. Fortunately,
the only two actions right now are 1 and 2 so they don't share any bit in
common and the bug has no impact.
No backport is needed.
ACL parse errors are not easy to understand since recent commit 348971e
(MEDIUM: acl: use the fetch syntax 'fetch(args),conv(),conv()' into the
ACL keyword) :
[ALERT] 339/154717 (26437) : parsing [check-bug.cfg:10] : error detected while parsing a 'stats admin' rule : unknown ACL or sample keyword 'env(a,b,c)': invalid arg 2 in fetch method 'env' : end of arguments expected at position 2, but got ',b,c'..
This error is only relevant to sample fetch keywords, so the new form is
a bit easier to understand :
[ALERT] 339/160011 (26626) : parsing [check-bug.cfg:12] : error detected while parsing a 'stats admin' rule : invalid arg 2 in fetch method 'env' : end of arguments expected at position 2, but got ',b,c' in sample expression 'env(a,b,c),upper'.
No backport is needed.
William Lallemand reported a double free on the args parser used in fetches
and ACLs. The cause is that the arg expression is not fully initialized nor
deinitialized when killed and that one of the pointers was already freed once
in certain error conditions.
Simply set it to NULL after the first call to free().
The bug was apparently introduced in 1.5-dev9 with commit 2ac5718
(MEDIUM: add a new typed argument list parsing framework).
If a "tcp-check send" experiences an EAGAIN on a send() call, it will
nevertheless go to next rule, and will not try to send again if the next
rule is an expect.
Change this so that we always try to send whatever remains in the buffer
before doing anything else.
A config with just a "tcp-check expect string XXX" loops at 100% CPU
because the connect() wakes the function and there's nothing to send,
but it does not disable the polling.
Rearrange the polling setup to fix this. This was just caused by latest
commit, no backport is needed.
This is a generic health check which can be used to match a
banner or send a request and analyse a server response.
It works in a send/expect ways and many exchange can be done between
HAProxy and a server to decide the server status, making HAProxy able to
speak the server's protocol.
It can send arbitrary regular or binary strings and match content as a
regular or binary string or a regex.
Signed-off-by: Baptiste Assmann <bedis9@gmail.com>
We currently use such an hex parser in pat_parse_bin() to parse hex
string patterns. We'll need another generic one so let's move it to
standard.c and have pat_parse_bin() make use of it.
This patch permits to use the same struct pattern for two indentical maps.
This permits to preserve memory, and permits to update only one
"struct pattern" when the dynamic map update is supported.
Commit 348971e (MEDIUM: acl: use the fetch syntax 'fetch(args),conv(),conv()'
into the ACL keyword) introduced a regression in the ACL parser. The second
argument of an ACL keyword is now mistakenly confused with a converter.
This bug is post-dev19 and does not require any backport.
We happened to preform this call twice on some checks, once in the
recv event handler, and another one in the main function. Remove
the one from the event handler which does not make any more sense
there.
When pure TCP checks are used, we see a useless call to recvfrom()
in strace resulting from an inconditional poll on recv after the
connect() succeeds. Let's remove this one and properly report
connection success in the write events.
Error reporting in health checks is unreliable as the number of recent
patch shows. The main reason is that the code required to detect the
exact situation where the error occurred is not simple, and the errors
have to be handled closer to where they occur in order to be accurate
(rely on getsockopt(SO_ERROR) and errno).
To solve this, we introduce chk_report_conn_err(). It does its best to
consider a possible errno passed in argument, a possible timeout passed
as well, then it completes this with getsockopt() if needed, and takes
into account the current status of the connection. The result is that
by simply calling this function with errno when it's known, we can emit
accurate log messages from every location. We can now see a messages
like "Connection error during SSL handshake (No route to host)" which
were not previously possible.
The only case where errno is supposed to be valid is when the connection
has just got the CO_FL_ERROR flag and errno is not zero, because it will
have been set by the same function that has set the flag. For all other
situations, we need to check the socket using getsockopt(), but only do
it once, since it clears the pending error code. For this reason, we
assign the error code to errno in order not to lose it. The same call
is made at the entry of event_srv_chk_r(), event_srv_chk_w(), and
wake_srv_chk() so that we get a chance to collect errors reported by
the poller or by failed syscalls.
Note that this fix relies on the 4 previous patches, so backporters
must be very careful.
At some places, we report an error by just detecting FD_POLL_ERR.
The problem is that the caller never knows if it must use errno or
call getsockopt(SO_ERROR). And since this last one clears the
pending error from the queue, it cannot be used inconditionally.
An elegant solution consists in clearing errno prior to inspecting
FD_POLL_ERR. The caller then knows that if it gets CO_FL_ERROR and
errno == 0, it must call getsockopt().
Since commit 348971e (MEDIUM: acl: use the fetch syntax
'fetch(args),conv(),conv()' into the ACL keyword), ACLs wait on input
that may change. This is visible in the configuration below :
tcp-request inspect-delay 3s
tcp-request content accept if REQ_CONTENT
Nothing will pass before the end of the timer. This is because
historically, sample_process() was dedicated to stick tables where
it was absolutely necessary to wait for a stable sample. Now samples
are used by many other things and we can't afford this. So let's move
this check to the stick tables after the call to sample_process()
instead.
This is post-1.5-dev19 work, no backport is required.
When we get a hard error from a syscall indicating the socket is dead,
it makes sense to set the CO_FL_SOCK_WR_SH and CO_FL_SOCK_RD_SH flags
to indicate that the socket may not be used anymore. It will ease the
error processing in health checks where the state of socket is very
important. We'll also be able to avoid some setsockopt(nolinger) after
an error.
For now, the rest of the code is not impacted because CO_FL_ERROR is
always tested prior to these flags.
PROXY protocol header was not tolerant to signals, so it might cause a
connection to report an error if a signal comes in at the exact same
moment the send is done.
This is 1.5-specific and does not need any backport.
The tcp_connect_probe() function may be called upon I/O activity when
no recv/send callbacks were called (eg: recv not possible, nothing to
send). It only relies on connect() to observe the connection establishment
progress but that does not work when some network errors are pending on
the socket (eg: a delayed connection refused).
For this reason we need to run a getsockopt() in the case where the
poller reports FD_POLL_ERR on the socket. We use this opportunity to
update errno so that the conn->data->wake() function has all relevant
info when it sees CO_FL_ERROR.
At the moment no code is impacted by this bug because recv polling is
always enabled during a connect, so recvfrom() always sees the error
first. But this may change with the health check cleanup.
No backport is needed.
The connection flag CO_FL_ERROR will be tested in the functions both
si_conn_recv_cb() and si_conn_send_cb(). If CO_FL_ERROR has been set, out_error
branch will be executed. But the only job of out_error branch is to set
CO_FL_ERROR on connection flag. So it's better return directly than goto
out_error branch under such conditions. As a result, out_error branch becomes
needless and can be removed.
In addition, the return type of si_conn_send_loop() is also changed to void.
The caller should check conn->flags for errors just like stream_int_chk_snd_conn()
does as below:
static void stream_int_chk_snd_conn(struct stream_interface *si)
{
...
conn_refresh_polling_flags(si->conn);
- if (si_conn_send(si->conn) < 0) {
+ si_conn_send(si->conn);
+ if (si->conn->flags & CO_FL_ERROR) {
...
}
Signed-off-by: Godbach <nylzhaowei@gmail.com>
We now have to report 2 conflicting information on the stats page :
- NOLB = server which returns 404 and stops load balancing ;
- DRAIN = server with a weight forced to zero
The DRAIN state was previously detected from eweight==0 and represented in
blue so that a temporarily disabled server was noticed. This was done by
commit cc8bb92 (MINOR: stats: show soft-stopped servers in different color).
This choice suffered from a small defect however, which is that a server
with a zero weight was reported in this color whatever its state (even down
or switching).
Also, one of the motivations for the color above was because the NOLB state
is barely detectable as it's very close to the UP state.
Since commit 8c3d0be (MEDIUM: Add DRAIN state and report it on the stats page),
we have the new DRAIN state to show servers with a zero weight. The colors are
unfortunately very close to those of the MAINT state, and some users were
confused by the disappearance of the blue bars.
Additionally, the NOLB state had precedence over DRAIN, which could be an
issue since DRAIN is the only thing the admin can act on, so once NOLB was
shown, there was nothing to indicate that the weight was forced to zero.
By switching the two priorities we can report DRAIN (forced mode) before
NOLB (detected mode).
The best solution to fix all this is to reuse the previous blue color for
all cases where weight == 0, whether it's set by config / agent / cli (DRAIN)
or detected by a 404 response (NOLB). However we only use this color when the
server is 100% UP. If it's going down we switch to the usual yellow color
showing failed checks, and when it's down it keeps its usual red color.
That way, a blue bar on the display indicates a server not taking new
sessions but perfectly up. And other colors keep their usual meaning.
In commit 8c3d0be (MEDIUM: Add DRAIN state and report it on the stats page),
the drain state was updated on every weight change except those that can be
sent via the web interface. This caused inconsistent state combinations to
be reported in the stats depending on the sequence (web then cli vs cli
then web).
It would seem that a call to set_server_drain_state() from within
server_recalc_eweight() would simplify things but that's not completely
certain yet.
The last fix on checks (02b0f58: BUG/MEDIUM: checks: fix a long-standing
issue with reporting connection errors) tried to isolate error codes
retrieved from the socket in order to report appropriate messages. The
only thing is that we must not pre-initialize err to errno since we're
not in I/O context anymore and errno will be the one of the last syscall
(whatever it was). However we can complete the message with more info
from the transport layer (eg: SSL can inform us we were in a handshake).
Also add a catch-all case for CO_FL_ERROR when the connection was
established. No check currently seem to leave this case open, but better
catch it because it's hard to find all possible cases.
Error handling in checks is complex because some stuff must be done in
the central task (mandatory at least for timeouts) and other stuff is
done closer to the data.
Since checks have their own buffers now, we could move everything to
the main task and only keep the low-level I/O for sending/retrieving
data to/from this buffer. It would also avoid sending logs from the
I/O context!
In 1.5-dev14 we fixed a bug induced by the new connection system which caused
handshake failures not to be reported in health checks. It was done with
commit 6c560da (BUG/MEDIUM: checks: report handshake failures). This fix
caused another issue which is that every check getting a TCP RST after a
valid response was flagged as error. This was fixed using commit c5c61fc
(BUG/MEDIUM: checks: ignore late resets after valid responses).
But because of this, we completely miss the status report. These two fixes
only set the check result as failed and did not call set_server_check_status()
to pass the information to upper layers.
The impact is that some failed checks are reported as INI or are simply not
updated if they happen fast enough (eg: TCP RST in response to connect()
without data in a pure TCP check). So the server appears down but the check
status says "L4OK".
After commit 6c560da, the handshake failures have been correctly dealt with
and every error causes process_chk() to be called with the appropriate
information still present on the socket. So let's get the error code in
process_chk() instead and stop mangling it in wake_srv_chk().
Now both L4 and L6 checks are correctly reported.
This bug was first introduced in 1.5-dev12 so no backport is needed.
Since commit 4a74143 (MEDIUM: Paramatise functions over the check of a
server), the check type is inherited from the current proxy's check type
at the moment where the server is declared instead of when reviewing
server configs. This causes an issue where a health check is disabled
when the server is declared before the checks. In fact the server will
inherit the last known check type declared before the "server" line :
backend foo
# this server is not checked at all
server s1 1.1.1.1:80 check
option tcpchk
# this server is tcp-checked :
server s2 1.1.1.2:80 check
option httpchk
# this server is http-checked :
server s3 1.1.1.3:80 check
The fix consists in assigning the check type during the config review
phase where the config is stable. No backport is nedeed.
Recent commit 06d97f9 (MEDIUM: log-format: relax parsing of '%' followed
by unsupported characters) caused the following warning on some compilers
since isblank is not always present :
src/log.c: In function 'parse_logformat_string':
src/log.c:453: warning: implicit declaration of function 'isblank'
As usual, replace it with the two values (space and tab).
We need to initialize the rdr_fmt list inconditionally. Using only
a redirect rule without an http-redirect may cause a crash during
deinit because of the list iterating from null.
We handle "http-request redirect" with a log-format string now, but we
leave "redirect" unaffected.
Note that the control of the special "/" case is move from the runtime
execution to the configuration parsing. If the format rule list is
empty, the build_logline() function does nothing.
At the moment when a '%' character is followed by any unhandled character,
it is considered as a variable name, and if it cannot be resolved, a warning
is emitted and the configuration goes on.
When we start using log-format for redirect rules, it may happen that some
people accidently use '%' instead of '%%' without understanding the cause
of the issue. Thus we do two things here :
- if a single '%' is followed by a blank or a digit, we fix it and emit a
warning explaining how this should be done ; this ensures that existing
configs continue to work ;
- if a single '%' is followed by an unknown variable name, we report it
and explain how to emit a verbatim '%' in case this is what the user
desired.
Add a new converter with the following prototype :
map(<map_file>[,<default_value>])
map_<match_type>(<map_file>[,<default_value>])
map_<match_type>_<output_type>(<map_file>[,<default_value>])
It searches the for input value from <map_file> using the <match_type>
matching method, and return the associated value converted to the type
<output_type>. If the input value cannot be found in the <map_file>,
the converter returns the <default_value>. If the <default_value> is
not set, the converter fails and acts as if no input value could be
fetched. If the <match_type> is not set, it defaults to "str".
Likewise, if the <output_type> is not set, it defaults to "str". For
convenience, the "map" keyword is an alias for "map_str" and maps a
string to another string. The following array contains contains the
list of all the map* converters.
+----+----------+---------+-------------+------------+
| `-_ out | | | |
| input `-_ | str | int | ip |
| / match `-_ | | | |
+---------------+---------+-------------+------------+
| str / str | map_str | map_str_int | map_str_ip |
| str / sub | map_sub | map_sub_int | map_sub_ip |
| str / dir | map_dir | map_dir_int | map_dir_ip |
| str / dom | map_dom | map_dom_int | map_dom_ip |
| str / end | map_end | map_end_int | map_end_ip |
| str / reg | map_reg | map_reg_int | map_reg_ip |
| int / int | map_int | map_int_int | map_int_ip |
| ip / ip | map_ip | map_ip_int | map_ip_ip |
+---------------+---------+-------------+------------+
The names are intentionally chosen to reflect the same match methods
as ACLs use.
This patch allows each sample cast function to specify the sample
output type. The goal is to be able to emit an output type IPv4 or
IPv6 depending on what is found in the input if the next converter
is able to process them both.
The patch also adds a new pseudo type called "ADDR". This type is an
alias for IPV4 and IPV6 which is only used as an input type by converters
who want to express their compatibility with both address formats. It may
not be emitted.
The goal is to unify as much as possible the processing of IPv4 and IPv6
in order not to add extra keywords for the maps which act as converters,
but will match samples like ACLs do with their patterns.
Make the stick-table key converter automatically adapt to the address
family of the input sample. Samples such as "src" will return an address
with a sample type depending on the input family. We'll have to support
such combinations when we add support for maps because the output type
will not necessarily be fixed.
We now have the following enums and all related functions return them and
consume them :
enum pat_match_res {
PAT_NOMATCH = 0, /* sample didn't match any pattern */
PAT_MATCH = 3, /* sample matched at least one pattern */
};
enum acl_test_res {
ACL_TEST_FAIL = 0, /* test failed */
ACL_TEST_MISS = 1, /* test may pass with more info */
ACL_TEST_PASS = 3, /* test passed */
};
enum acl_cond_pol {
ACL_COND_NONE, /* no polarity set yet */
ACL_COND_IF, /* positive condition (after 'if') */
ACL_COND_UNLESS, /* negative condition (after 'unless') */
};
It's just in order to avoid doubts when reading some code.
This patch just renames functions, types and enums. No code was changed.
A significant number of files were touched, especially the ACL arrays,
so it is likely that some external patches will not apply anymore.
One important thing is that we had to split ACL_PAT_* into two groups :
- ACL_TEST_{PASS|MISS|FAIL}
- PAT_{MATCH|UNMATCH}
A future patch will enforce enums on all these places to avoid confusion.
This patch just moves code without any change.
The ACL are just the association between sample and pattern. The pattern
contains the match method and the parse method. These two things are
different. This patch cleans the code by splitting it.
This will be used later with maps. Each map will associate an entry with
a sample_storage value.
This patch changes the "parse" prototype and all the parsing methods.
The goal is to associate "struct sample_storage" to each entry of
"struct acl_pattern". Only the "parse" function can add the sample value
into the "struct acl_pattern".
