]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoMEDIUM: stats: integrate static proxies stats in new stats
Amaury Denoyelle [Mon, 5 Oct 2020 09:49:42 +0000 (11:49 +0200)] 
MEDIUM: stats: integrate static proxies stats in new stats

This is executed on startup with the registered statistics module. The
existing statistics have been merged in a list containing all
statistics for each domain. This is useful to print all available
statistics in a generic way.

Allocate extra counters for all proxies/servers/listeners instances.
These counters are allocated with the counters from the stats modules
registered on startup.

4 years agoMEDIUM: stats: add abstract type to store counters
Amaury Denoyelle [Mon, 5 Oct 2020 09:49:41 +0000 (11:49 +0200)] 
MEDIUM: stats: add abstract type to store counters

Implement a small API to easily add extra counters inside a structure
instance. This will be used to implement dynamic statistics linked on
every type of object as needed.

The counters are stored in a dynamic array inside the relevant objects.

4 years agoMEDIUM: stats: define an API to register stat modules
Amaury Denoyelle [Mon, 5 Oct 2020 09:49:40 +0000 (11:49 +0200)] 
MEDIUM: stats: define an API to register stat modules

A stat module can be registered to quickly add new statistics on
haproxy. It must be attached to one of the available stats domain. The
register must be done using INITCALL on STG_REGISTER.

The stat module has a name which should be unique for each new module in
a domain. It also contains a statistics list with their name/desc and a
pointer to a function used to fill the stats from the module counters.

The module also provides the initial counters values used on
automatically allocated counters. The offset for these counters
are stored in the module structure.

4 years agoMEDIUM: stats: add delimiter for static proxy stats on csv
Amaury Denoyelle [Mon, 5 Oct 2020 09:49:39 +0000 (11:49 +0200)] 
MEDIUM: stats: add delimiter for static proxy stats on csv

Use the character '-' to mark the end of static statistics on proxy
domain. After this marker, the order of the fields is not guaranteed and
should be parsed with care.

4 years agoMINOR: stats: define additional flag px cap on domain
Amaury Denoyelle [Mon, 5 Oct 2020 09:49:38 +0000 (11:49 +0200)] 
MINOR: stats: define additional flag px cap on domain

This flag can be used to determine on what type of proxy object the
statistics should be relevant. It will be useful when adding dynamic
statistics. Currently, this flag is not used.

4 years agoMINOR: stats: define the concept of domain for statistics
Amaury Denoyelle [Mon, 5 Oct 2020 09:49:37 +0000 (11:49 +0200)] 
MINOR: stats: define the concept of domain for statistics

The domain option will be used to have statistics attached to other
objects than proxies/listeners/servers. At the moment, only the PROXY
domain is available.

Add an argument 'domain' on the 'show stats' cli command to specify the
domain. Only 'domain proxy' is available now. If not specified, proxy
will be considered the default domain.

For HTML output, only proxy statistics will be displayed.

4 years agoMINOR: hlua: Display debug messages on stderr only in debug mode
Christopher Faulet [Fri, 2 Oct 2020 16:13:52 +0000 (18:13 +0200)] 
MINOR: hlua: Display debug messages on stderr only in debug mode

Debug Messages emitted in lua using core.Debug() or core.log() are now only
displayed on stderr if HAProxy is started in debug mode (-d parameter on the
command line). There is no change for other message levels.

This patch should fix the issue #879. It may be backported to all stable
versions.

4 years agoREORG: stats: extract proxies dump loop in a function
Amaury Denoyelle [Fri, 2 Oct 2020 16:32:04 +0000 (18:32 +0200)] 
REORG: stats: extract proxies dump loop in a function

Create a dedicated function to loop on proxies and dump them. This will
be clearer when other object will be dump as well.

This patch is needed to extend stat support to components other than
proxies objects.

4 years agoREORG: stats: extract proxy json dump
Amaury Denoyelle [Fri, 2 Oct 2020 16:32:03 +0000 (18:32 +0200)] 
REORG: stats: extract proxy json dump

Create a dedicated function to dump a proxy as a json content. This
patch will be needed when other types of objects will be available for
json dump.

This patch is needed to extend stat support to components other than
proxies objects.

4 years agoMINOR: stats: hide px/sv/li fields in applet struct
Amaury Denoyelle [Fri, 2 Oct 2020 16:32:02 +0000 (18:32 +0200)] 
MINOR: stats: hide px/sv/li fields in applet struct

Use an opaque pointer to store proxy instance. Regroup server/listener
as a single opaque pointer. This has the benefit to render the structure
more evolutive to support statistics on other types of objects in the
future.

This patch is needed to extend stat support for components other than
proxies objects.

The prometheus module has been adapted for these changes.

4 years agoMINOR: stats: add stats size as a parameter for csv/json dump
Amaury Denoyelle [Fri, 2 Oct 2020 16:32:01 +0000 (18:32 +0200)] 
MINOR: stats: add stats size as a parameter for csv/json dump

Render the stats size parametric in csv/json dump functions. This is
needed for the future patch which provides dynamic stats. For now the
static value ST_F_TOTAL_FIELDS is provided.

Remove unused parameter px on stats_dump_one_line.

This patch is needed to extend stat support to components other than
proxies objects.

4 years agoREORG: stats: export some functions
Amaury Denoyelle [Fri, 2 Oct 2020 16:32:00 +0000 (18:32 +0200)] 
REORG: stats: export some functions

Un-mark stats_dump_one_line and stats_putchk as static and export them
in the header file. These functions will be reusable by other components to
print their statistics.

This patch is needed to extend stat support to components other than
proxies objects.

4 years agoBUG/MINOR: stats: fix validity of the json schema
Amaury Denoyelle [Fri, 2 Oct 2020 16:31:59 +0000 (18:31 +0200)] 
BUG/MINOR: stats: fix validity of the json schema

The json schema seems to be invalid when checking using the validator
from https://www.jsonschemavalidator.net/. Correct it using the
following specification :
http://json-schema.org/draft/2019-09/json-schema-validation.html#rfc.section.9.1

The impact of the bug it not well known as I am not sure of how useful
the json schema is for users. It is probably not used at all or else
this bug would have been reported.

This should be backported up to 1.8.

4 years agoMINOR: counters: fix a typo in comment
Amaury Denoyelle [Fri, 2 Oct 2020 16:31:58 +0000 (18:31 +0200)] 
MINOR: counters: fix a typo in comment

Wrong copy/paste comment, replace listeners/frontends by
servers/backends

This may be backported up to 1.7.

4 years agoCLEANUP: ssl: "bundle" is not an OpenSSL wording
William Lallemand [Fri, 2 Oct 2020 16:08:18 +0000 (18:08 +0200)] 
CLEANUP: ssl: "bundle" is not an OpenSSL wording

There is a confusion between the HAProxy bundle and OpenSSL. OpenSSL
does not have "bundles" but multiple certificates in the same store.

Fix a commentary in the crt-list code.

4 years agoBUG/MEDIUM: queue: make pendconn_cond_unlink() really thread-safe
Willy Tarreau [Fri, 2 Oct 2020 15:52:49 +0000 (17:52 +0200)] 
BUG/MEDIUM: queue: make pendconn_cond_unlink() really thread-safe

A crash reported in github issue #880 looks impossible unless
pendconn_cond_unlink() occasionally sees a null leaf_p when attempting
to remove an entry, which seems to be confirmed by the reporter. What
seems to be happening is that depending on compiler optimizations,
this pointer can appear as null while pointers are moved if one of
the node's parents is removed from or inserted into the tree. There's
no explicit null of the pointer during these operations but those
pointers are rewritten in multiple steps and nothing prevents this
situation from happening, and there are no particular barrier nor
atomic ops around this.

This test was used to avoid unnecessary locking, for already deleted
entries, but looking at the code it appears that pendconn_free() already
resets s->pend_pos that's used as <p> there, and that the other call
reasons are after an error where the connection will be dropped as
well. So we don't save anything by doing this test, and make it
unsafe. The older code used to check for list emptiness there and
not inside pendconn_unlink(), which explains why the code has stayed
there. Let's just remove this now.

Thanks to @jaroslawr for reporting this issue in great details and for
testing the proposed fix.

This should be backpored to 1.8, where the test on LIST_ISEMPTY should
be moved to pendconn_unlink() instead (inside the lock, just like 2.0+).

4 years agoDOC: ssl: new "cert bundle" behavior
William Lallemand [Fri, 2 Oct 2020 15:57:44 +0000 (17:57 +0200)] 
DOC: ssl: new "cert bundle" behavior

Update the documentation with the new bundle behavior which does not use
the same OpenSSL certificate store anymore but loads the PEM separately
as multiple "crt" were specified.

It should fix issue #872.

4 years agoBUG/MINOR: tcpcheck: Set socks4 and send-proxy flags before the connect call
Christopher Faulet [Fri, 2 Oct 2020 11:41:55 +0000 (13:41 +0200)] 
BUG/MINOR: tcpcheck: Set socks4 and send-proxy flags before the connect call

Since the health-check refactoring in the 2.2, the checks through a socks4 proxy
are broken. To fix this bug, CO_FL_SOCKS4 flag must be set on the connection
before calling the connect() callback function because this flags is checked to
use the right destination address. The same is done for the CO_FL_SEND_PROXY
flag for a consistency purpose.

A reg-test has been added to test the "check-via-socks4" directive.

This patch must be backported to 2.2.

4 years agoMEDIUM: tcp-rules: Warn if a track-sc* content rule doesn't depend on content
Christopher Faulet [Fri, 2 Oct 2020 09:48:57 +0000 (11:48 +0200)] 
MEDIUM: tcp-rules: Warn if a track-sc* content rule doesn't depend on content

The warning is only emitted for HTTP frontend. Idea is to encourage the usage of
"tcp-request session" rules to track counters that does not depend on the
request content. The documentation has been updated accordingly.

The warning is important because since the multiplexers were added in the
processing chain, the HTTP parsing is performed at a lower level. Thus parsing
errors are detected in the multiplexers, before the stream creation. In HTTP/2,
the error is reported by the multiplexer itself and the stream is never
created. This difference has a certain number of consequences, one of which is
that HTTP request counting in stick tables only works for valid H2 request, and
HTTP error tracking in stick tables never considers invalid H2 requests but only
invalid H1 ones. And the aim is to do the same with the mux-h1. This change will
not be done for the 2.3, but the 2.4. At the end, H1 and H2 parsing errors will
be caught by the multiplexers, at the session level. Thus, tracking counters at
the content level should be reserved for rules using a key based on the request
content or those using ACLs based on the request content.

To be clear, a warning will be emitted for the following rules :

  tcp-request content track-sc0 src
  tcp-request content track-sc0 src if ! { src 10.0.0.0/24 }
  tcp-request content track-sc0 src if { ssl_fc }

But not for the following ones :

  tcp-request content track-sc0 req.hdr(host)
  tcp-request content track-sc0 src if { req.hdr(host) -m found }

4 years agoDOC: tcp-rules: Refresh details about L7 matching for tcp-request content rules
Christopher Faulet [Fri, 2 Oct 2020 09:38:46 +0000 (11:38 +0200)] 
DOC: tcp-rules: Refresh details about L7 matching for tcp-request content rules

Because the parsing of HTTP message is now performed in the HTTP multiplexers,
the content is immediatly available when "tcp-request content" rules are
evaluated for an HTTP frontend. So, it is a good idea to make the documentation
explicit on this point. In addition, because in all cases, the parsing is
already performed, there is no reason to still use "tcp-request content" rules
based on L7 matching, although it is still valid. The recommended way is to use
"http-request" rules instead. Again, it is a good idea to update the
documentation on this point.

4 years agoBUG/MINOR: Fix several leaks of 'log_tag' in init().
Eric Salama [Fri, 2 Oct 2020 09:58:19 +0000 (11:58 +0200)] 
BUG/MINOR: Fix several leaks of 'log_tag' in init().

We use chunk_initstr() to store the program name as the default log-tag.

If we use the log-tag directive in the config file, this chunk will be
destroyed and replaced. chunk_initstr() sets the chunk size to 0 so we
will free the chunk itself, but not its content.

This happens for a global section and also for a proxy.

We fix this by using chunk_initlen() instead of chunk_initstr().
We also check that the memory allocation was successfull, otherwise we quit.

This fixes github issue #850.
It can be backported as far as 1.9, with minor adjustments to includes.

4 years agoMINOR: ssl: remove uneeded check in crtlist_parse_file
William Dauchy [Fri, 2 Oct 2020 13:27:20 +0000 (15:27 +0200)] 
MINOR: ssl: remove uneeded check in crtlist_parse_file

this condition is never true as we either break or goto error, so those
two lines could be removed in the current state of the code.

this is fixing github issue #862

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoMINOR: ssl: Add error if a crt-list might be truncated
Tim Duesterhus [Tue, 29 Sep 2020 16:00:28 +0000 (18:00 +0200)] 
MINOR: ssl: Add error if a crt-list might be truncated

Similar to warning during the parsing of the regular configuration file
that was added in 2fd5bdb439da29f15381aeb57c51327ba57674fc this patch adds
a warning to the parsing of a crt-list if the file does not end in a
newline (and thus might have been truncated).

The logic essentially just was copied over. It might be good to refactor
this in the future, allowing easy re-use within all line-based config
parsers.

see https://github.com/haproxy/haproxy/issues/860#issuecomment-693422936
see 0354b658f061d00d5ab4b728d7deeff2c8f1503a

This should be backported as a warning to 2.2.

4 years agoCLEANUP: ssl: Use structured format for error line report during crt-list parsing
Tim Duesterhus [Tue, 29 Sep 2020 16:00:27 +0000 (18:00 +0200)] 
CLEANUP: ssl: Use structured format for error line report during crt-list parsing

This reuses the known `parsing [%s:%d]:` from regular config file error
reporting.

4 years agoBUILD: makefile: Enable closefrom() support on Solaris
Brad Smith [Wed, 30 Sep 2020 19:46:16 +0000 (15:46 -0400)] 
BUILD: makefile: Enable closefrom() support on Solaris

Solaris 9 (released 2002) added support for closefrom().

I bumped the version in the comment to 10 as the default feature
flags already has event ports enabled which were introduced in
Solaris 10.

4 years agoBUILD: tools: fix minor build issue on isspace()
Willy Tarreau [Thu, 1 Oct 2020 16:04:40 +0000 (18:04 +0200)] 
BUILD: tools: fix minor build issue on isspace()

Previous commit fa41cb679 ("MINOR: tools: support for word expansion
of environment in parse_line") introduced two new isspace() on a char
and broke the build on systems using an array disguised in a macro
instead of a function (like cygwin). Just use the usual cast.

4 years agoMINOR: tools: support for word expansion of environment in parse_line
Amaury Denoyelle [Thu, 1 Oct 2020 12:32:35 +0000 (14:32 +0200)] 
MINOR: tools: support for word expansion of environment in parse_line

Allow the syntax "${...[*]}" to expand an environment variable
containing several values separated by spaces as individual arguments. A
new flag PARSE_OPT_WORD_EXPAND has been added to toggle this feature on
parse_line invocation. In case of an invalid syntax, a new error
PARSE_ERR_WRONG_EXPAND will be triggered.

This feature has been asked on the github issue #165.

4 years agoBUILD: makefile: add an EXTRAVERSION variable to ease local naming
Willy Tarreau [Thu, 1 Oct 2020 02:05:39 +0000 (04:05 +0200)] 
BUILD: makefile: add an EXTRAVERSION variable to ease local naming

Sometimes it's desirable to append local version naming to packages,
and currently it can only be done using SUBVERS which is already set
by default to the git commit ID and patch count since last known tag,
making the addition a bit complicated.

Let's just add a new EXTRAVERSION field that is empty by default, and
systematically appended verbatim to the version string everywhere. This
way it becomes trivial to append some local strings, such as:

   make TARGET=foo EXTRAVERSION=+$(quilt applied|wc -l)
   -> 2.3-dev5-5018aa-15+1

or :

   make TARGET=foo EXTRAVERSION=-$(date +%F)
   -> 2.3-dev5-5018aa-15-20200110

Let's be careful not to add double quotes (used as the string delimiter)
nor spaces (which can confuse version parsers on the output). The extra
version is also used to name a tarball. It's always pre-initialized to an
empty string so that it's not accidently inherited from the environment.
It's not reported in "make version" to avoid fooling tools (it would be
pointless anyway).

As a side effect it also becomes possible to force VERSION and SUBVERS
to an empty string and use EXTRAVERSION alone to force a specific version
(could possibly be useful when bisecting from patch queues outside of Git
for example).

4 years agoBUILD: makefile: Fix building with closefrom() support enabled
Brad Smith [Wed, 30 Sep 2020 05:04:48 +0000 (01:04 -0400)] 
BUILD: makefile: Fix building with closefrom() support enabled

I noticed the USE_CLOSEFROM define was not being passed along like the rest
during the build.

Looking around I see this was broken with the following two commits and related
series..

BUILD: Makefile: also report disabled options in the BUILD_OPTIONS variable
http://git.haproxy.org/?p=haproxy.git;a=commit;h=05fd82da76d1bbc8d65d63ab246bda7cbcf8481a

BUILD: pass all "USE_*" variables as -DUSE_* to the compiler
http://git.haproxy.org/?p=haproxy.git;a=commit;h=824cd00d3bda8f7f6d4c30baf77ba6c19ab47811

Looks like this should be back ported to 2.0, 2.1 and 2.2.

4 years agoOPTIM: backend: skip LB when we know the backend is full
Willy Tarreau [Tue, 29 Sep 2020 15:07:21 +0000 (17:07 +0200)] 
OPTIM: backend: skip LB when we know the backend is full

For some algos (roundrobin, static-rr, leastconn, first) we know that
if there is any request queued in the backend, it's because a previous
attempt failed at finding a suitable server after trying all of them.
This alone is sufficient to decide that the next request will skip the
LB algo and directly reach the backend's queue. Doing this alone avoids
an O(N) lookup when load-balancing on a saturated farm of N servers,
which starts to be very expensive for hundreds of servers, especially
under the lbprm lock. This change alone has increased the request rate
from 110k to 148k RPS for 200 saturated servers on 8 threads, and
fwlc_reposition_srv() doesn't show up anymore in perf top. See github
issue #880 for more context.

It could have been the same for random, except that random is performed
using a consistent hash and it only considers a small set of servers (2
by default), so it may result in queueing at the backend despite having
some free slots on unknown servers. It's no big deal though since random()
only performs two attempts by default.

For hashing algorithms this is pointless since we don't queue at the
backend, except when there's no hash key found, which is the least of
our concerns here.

4 years agoOPTIM: backend/random: never queue on the server, always on the backend
Willy Tarreau [Tue, 29 Sep 2020 14:58:30 +0000 (16:58 +0200)] 
OPTIM: backend/random: never queue on the server, always on the backend

If random() returns a server whose maxconn is reached or the queue is
used, instead of adding the request to the server's queue, better add
it to the backend queue so that it can be served by any server (hence
the fastest one).

4 years agoBUILD: makefile: Update feature flags for FreeBSD
Brad Smith [Tue, 15 Sep 2020 07:10:04 +0000 (03:10 -0400)] 
BUILD: makefile: Update feature flags for FreeBSD

This updates the feature flags for FreeBSD.

FreeBSD 10 adds support for accept4().

Enable getaddrinfo().

From the FreeBSD port / package.

4 years agoREGTEST: make map_regm_with_backref require 1.7
Willy Tarreau [Tue, 29 Sep 2020 09:00:51 +0000 (11:00 +0200)] 
REGTEST: make map_regm_with_backref require 1.7

map_regm was only introduced in 1.7, I don't know why the require field
was set to 1.6, probably tha the test evolved and didn't start with
map_regm.

4 years agoREGTEST: make abns_socket.vtc require 1.8
Willy Tarreau [Tue, 29 Sep 2020 08:58:44 +0000 (10:58 +0200)] 
REGTEST: make abns_socket.vtc require 1.8

It's not as much an abns test as it is a seamless reload test, which
appeared in 1.8 due to "expose-fd listeners".

4 years agoREGTEST: make agent-check.vtc require 1.8
Willy Tarreau [Tue, 29 Sep 2020 08:58:04 +0000 (10:58 +0200)] 
REGTEST: make agent-check.vtc require 1.8

This one looks at the output of "show server state", which change slightly
in 1.8 (shows DNS fields).

4 years agoREGTEST: the iif converter test requires 2.3
Willy Tarreau [Tue, 29 Sep 2020 08:51:16 +0000 (10:51 +0200)] 
REGTEST: the iif converter test requires 2.3

This one was recently added in 2.3.

4 years agoREGTEST: make ssl_client_samples and ssl_server_samples requiret to 2.3
Willy Tarreau [Tue, 29 Sep 2020 08:50:23 +0000 (10:50 +0200)] 
REGTEST: make ssl_client_samples and ssl_server_samples requiret to 2.3

These ones added new sample fetches that are only available in 2.3 and
which fail on older versions.

4 years agoREGTEST: fix host part in balance-uri-path-only.vtc
Willy Tarreau [Tue, 29 Sep 2020 07:58:23 +0000 (09:58 +0200)] 
REGTEST: fix host part in balance-uri-path-only.vtc

We used to set it to ${h1_px_addr} but it randomly fails on certain
hosts (FreeBSD and OSX) where the address is surprisingly set to "::1"
while the Host field contains 127.0.0.1 (hence two different address
families). While this is likely a minor issue in vtest, we don't need
to depend on this and can easily hard-code 127.0.0.1 which is already
used in other tests.

4 years agoBUG/MINOR: ssl/crt-list: exit on warning out of crtlist_parse_line()
William Lallemand [Mon, 28 Sep 2020 13:45:16 +0000 (15:45 +0200)] 
BUG/MINOR: ssl/crt-list: exit on warning out of crtlist_parse_line()

We should not exits on error out of the crtlist_parse_line() function.
The cfgerr error must be checked with the ERR_CODE mask.

Must be backported in 2.2.

4 years agoDOC: crt: advise to move away from cert bundle
William Dauchy [Sat, 26 Sep 2020 11:35:52 +0000 (13:35 +0200)] 
DOC: crt: advise to move away from cert bundle

especially when starting to use `new ssl cert` runtime API, it might
become a bit confusing for users to mix bundle and single cert,
especially when it comes to use the commit command:
e.g.:
- start the process with `crt` loading a bundle
- use `set ssl cert my_cert.pem.ecdsa`: API detects it as a replacement
  of a bundle.
- `commit` has to be done on the bundle: `commit ssl cert my_cert.pem`

however:
- add a new cert: `new ssl cert my_cert.pem.rsa`: added as a single
  certificate
- `commit` has to be done on the certificate: `commit ssl cert
  my_cert.pem.rsa`

this should resolve github issue #872

this should probably be backported in >= v2.2 in order to encourage
people to move away from bundle certificates loading.

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoDOC: agent-check: fix typo in "fail" word expected reply
William Dauchy [Sat, 26 Sep 2020 11:35:51 +0000 (13:35 +0200)] 
DOC: agent-check: fix typo in "fail" word expected reply

`tcpcheck_agent_expect_reply` expects "fail" not "failed"

This should fix github issue #876

This can be backported to all maintained versions (i.e >= 1.6) as of
today.

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoBUILD: makefile: Update feature flags for OpenBSD
Brad Smith [Sun, 27 Sep 2020 03:05:25 +0000 (23:05 -0400)] 
BUILD: makefile: Update feature flags for OpenBSD

Update the OpenBSD target features being enabled.

I updated the list of features after noticing
"BUILD: makefile: disable threads by default on OpenBSD".

The Makefile utilizing gcc(1) by default resulted in utilizing
our legacy and obsolete compiler (GCC 4.2.1) instead of the
proper system compiler (Clang), which does support TLS. With
"BUILD: makefile: change default value of CC from gcc to cc"
that is resolved.

4 years agoREGTESTS: use "command" instead of "which" for better POSIX compatibility
Ilya Shipitsin [Sat, 26 Sep 2020 06:54:27 +0000 (11:54 +0500)] 
REGTESTS: use "command" instead of "which" for better POSIX compatibility

for example, "which" is not installed by default in Fedora docker image.

4 years ago[RELEASE] Released version 2.3-dev5 v2.3-dev5
Christopher Faulet [Fri, 25 Sep 2020 16:40:47 +0000 (18:40 +0200)] 
[RELEASE] Released version 2.3-dev5

Released version 2.3-dev5 with the following main changes :
    - DOC: Fix typo in iif() example
    - CLEANUP: Update .gitignore
    - BUILD: introduce possibility to define ABORT_NOW() conditionally
    - CI: travis-ci: help Coverity to recognize abort()
    - BUG/MINOR: Fix type passed of sizeof() for calloc()
    - CLEANUP: Do not use a fixed type for 'sizeof' in 'calloc'
    - CLEANUP: tree-wide: use VAR_ARRAY instead of [0] in various definitions
    - BUILD: connection: fix build on clang after the VAR_ARRAY cleanup
    - BUG/MINOR: ssl: verifyhost is case sensitive
    - BUILD: makefile: change default value of CC from gcc to cc
    - CI: travis-ci: split asan step out of running tests
    - BUG/MINOR: server: report correct error message for invalid port on "socks4"
    - BUG/MEDIUM: ssl: Don't call ssl_sock_io_cb() directly.
    - BUG/MINOR: ssl/crt-list: crt-list could end without a \n
    - BUG/MINOR: log-forward: fail on unknown keywords
    - MEDIUM: log-forward: use "dgram-bind" instead of "bind" for the listener
    - BUG/MEDIUM: log-forward: always quit on parsing errors
    - MEDIUM: ssl: remove bundle support in crt-list and directories
    - MEDIUM: ssl/cli: remove support for multi certificates bundle
    - MINOR: ssl: crtlist_dup_ssl_conf() duplicates a ssl_bind_conf
    - MINOR: ssl: crtlist_entry_dup() duplicates a crtlist_entry
    - MEDIUM: ssl: emulates the multi-cert bundles in the crtlist
    - MEDIUM: ssl: emulate multi-cert bundles loading in standard loading
    - CLEANUP: ssl: remove test on "multi" variable in ckch functions
    - CLEANUP: ssl/cli: remove test on 'multi' variable in CLI functions
    - CLEANUP: ssl: remove utility functions for bundle
    - DOC: explain bundle emulation in configuration.txt
    - BUILD: fix build with openssl < 1.0.2 since bundle removal
    - BUG/MINOR: log: gracefully handle the "udp@" address format for log servers
    - BUG/MINOR: dns: gracefully handle the "udp@" address format for nameservers
    - MINOR: listener: create a new struct "settings" in bind_conf
    - MINOR: listener: move bind_proc and bind_thread to struct settings
    - MINOR: listener: move the interface to the struct settings
    - MINOR: listener: move the network namespace to the struct settings
    - REORG: listener: create a new struct receiver
    - REORG: listener: move the listening address to a struct receiver
    - REORG: listener: move the receiving FD to struct receiver
    - REORG: listener: move the listener's proto to the receiver
    - MINOR: listener: make sock_find_compatible_fd() check the socket type
    - REORG: listener: move the receiver part to a new file
    - MINOR: receiver: link the receiver to its settings
    - MINOR: receiver: link the receiver to its owner
    - MINOR: listener: prefer to retrieve the socket's settings via the receiver
    - MINOR: receiver: add a receiver-specific flag to indicate the socket is bound
    - MINOR: listener: move the INHERITED flag down to the receiver
    - MINOR: receiver: move the FOREIGN and V6ONLY options from listener to settings
    - MINOR: sock: make sock_find_compatible_fd() only take a receiver
    - MINOR: protocol: rename the ->bind field to ->listen
    - MINOR: protocol: add a new ->bind() entry to bind the receiver
    - MEDIUM: sock_inet: implement sock_inet_bind_receiver()
    - MEDIUM: tcp: make use of sock_inet_bind_receiver()
    - MEDIUM: udp: make use of sock_inet_bind_receiver()
    - MEDIUM: sock_unix: implement sock_unix_bind_receiver()
    - MEDIUM: uxst: make use of sock_unix_bind_receiver()
    - MEDIUM: sockpair: implement sockpair_bind_receiver()
    - MEDIUM: proto_sockpair: make use of sockpair_bind_receiver()
    - MEDIUM: protocol: explicitly start the receiver before the listener
    - MEDIUM: protocol: do not call proto->bind() anymore from bind_listener()
    - MINOR: protocol: add a new proto_fam structure for protocol families
    - MINOR: protocol: retrieve the family-specific fields from the family
    - CLEANUP: protocol: remove family-specific fields from struct protocol
    - MINOR: protocol: add a real family for existing FDs
    - CLEANUP: tools: make str2sa_range() less awful for fd@ and sockpair@
    - MINOR: tools: make str2sa_range() take more options than just resolve
    - MINOR: tools: add several PA_O_PORT_* flags in str2sa_range() callers
    - MEDIUM: tools: make str2sa_range() validate callers' port specifications
    - MEDIUM: config: remove all checks for missing/invalid ports/ranges
    - MINOR: tools: add several PA_O_* flags in str2sa_range() callers
    - MINOR: listener: remove the inherited arg to create_listener()
    - MINOR: tools: make str2sa_range() optionally return the fd
    - MINOR: log: detect LOG_TARGET_FD from the fd and not from the syntax
    - MEDIUM: tools: make str2sa_range() resolve pre-bound listeners
    - MINOR: config: do not test an inherited socket again
    - MEDIUM: tools: make str2sa_range() check for the sockpair's FD usability
    - MINOR: tools: start to distinguish stream and dgram in str2sa_range()
    - MEDIUM: tools: make str2sa_range() only report AF_CUST_UDP on listeners
    - MINOR: tools: remove the central test for "udp" in str2sa_range()
    - MINOR: cfgparse: add str2receiver() to parse dgram receivers
    - MINOR: log-forward: use str2receiver() to parse the dgram-bind address
    - MEDIUM: config: make str2listener() not accept datagram sockets anymore
    - MINOR: listener: pass the chosen protocol to create_listeners()
    - MINOR: tools: make str2sa_range() directly return the protocol
    - MEDIUM: tools: make str2sa_range() check that the protocol has ->connect()
    - MINOR: protocol: add the control layer type in the protocol struct
    - MEDIUM: protocol: store the socket and control type in the protocol array
    - MEDIUM: tools: make str2sa_range() use protocol_lookup()
    - MEDIUM: proto_udp: replace last AF_CUST_UDP* with AF_INET*
    - MINOR: tools: drop listener detection hack from str2sa_range()
    - BUILD: sock_unix: add missing errno.h
    - MINOR: sock_inet: report the errno string in binding errors
    - MINOR: sock_unix: report the errno string in binding errors
    - BUILD: sock_inet: include errno.h
    - MINOR: h2/trace: also display the remaining frame length in traces
    - BUG/MINOR: h2/trace: do not display "stream error" after a frame ACK
    - BUG/MEDIUM: h2: report frame bits only for handled types
    - BUG/MINOR: http-fetch: Don't set the sample type during the htx prefetch
    - BUG/MINOR: Fix memory leaks cfg_parse_peers
    - BUG/MINOR: config: Fix memory leak on config parse listen
    - MINOR: backend: make the "whole" option of balance uri take only one bit
    - MINOR: backend: add a new "path-only" option to "balance uri"
    - REGTESTS: add a few load balancing tests
    - BUG/MEDIUM: listeners: do not pause foreign listeners
    - BUG/MINOR: listeners: properly close listener FDs
    - BUILD: trace: include tools.h

4 years agoBUILD: trace: include tools.h
Miroslav Zagorac [Thu, 24 Sep 2020 07:15:46 +0000 (09:15 +0200)] 
BUILD: trace: include tools.h

If the TRACE option is used when compiling the haproxy source,
the following error occurs on debian 9.13:

src/calltrace.o: In function `make_line':
.../src/calltrace.c:204: undefined reference to `rdtsc'
src/calltrace.o: In function `calltrace':
.../src/calltrace.c:277: undefined reference to `rdtsc'
collect2: error: ld returned 1 exit status
Makefile:866: recipe for target 'haproxy' failed

4 years agoBUG/MINOR: listeners: properly close listener FDs
Willy Tarreau [Wed, 23 Sep 2020 14:33:00 +0000 (16:33 +0200)] 
BUG/MINOR: listeners: properly close listener FDs

The code dealing with zombie proxies in soft_stop() is bogus, it uses
close() instead of fd_delete(), leaving a live entry in the fdtab with
a dangling pointer to a free memory location. The FD might be reassigned
for an outgoing connection for the time it takes the proxy to completely
stop, or could be dumped on the CLI's "show fd" command. In addition,
the listener's FD was not even reset, leaving doubts about whether or
not it will happen again in deinit().

And in deinit(), the loop in charge of closing zombie FDs is particularly
unsafe because it closes the fd then calls unbind_listener() then
delete_listener() hoping none of them will touch it again. Since it
requires some mental efforts to figure what's done there, let's correctly
reset the fd here as well and close it using fd_delete() to eliminate any
remaining doubts.

It's uncertain whether this should be backported. Zombie proxies are rare
and the situations capable of triggering such issues are not trivial to
setup. However it's easy to imagine how things could go wrong if backported
too far. Better wait for any matching report if at all (this code has been
there since 1.8 without anobody noticing).

4 years agoBUG/MEDIUM: listeners: do not pause foreign listeners
Willy Tarreau [Wed, 23 Sep 2020 15:17:22 +0000 (17:17 +0200)] 
BUG/MEDIUM: listeners: do not pause foreign listeners

There's a nasty case with listeners that belong to foreign processes.
If a proxy is defined this way:

     global
         nbproc 2

     frontend f
         bind :1111 process 1
         bind :2222 process 2

and if stats expose-fd listeners is set, the listeners' FDs will not
be closed on the processes that don't use them. At this point it's not
a big deal, except that they're shared between processes and that a
"disable frontend f" issued on one process will pause all of them and
cause the other process to see accept() fail, turning its own listener
to state LI_LIMITED to try to leave it some time to recover. But it
will never recover, even after an enable.

The root cause of the issue is that the ZOMBIE state doesn't cover
this situation since it's only for a proxy being entirely bound to a
process.

What we do here to address this is that we refrain from pausing a
file descriptor that belongs to a foreign process in pause_listener().
This definitely solves the problem. A similar test is present in
resume_listener() and is the reason why the FD doesn't recover upon the
"enable" action by the way.

This ought to be backported to 1.8 where seamless reload was integrated.

The config above should be sufficient to validate that the fix works;
after a pair of "disable/enable frontend" no process will handle the
traffic to one of the ports anymore.

4 years agoREGTESTS: add a few load balancing tests
Willy Tarreau [Wed, 23 Sep 2020 07:02:13 +0000 (09:02 +0200)] 
REGTESTS: add a few load balancing tests

This adds "balance-rr" to test round robin, "balance-uri" to test the
default balance-uri method, and "balance-uri-path-only" which mixes H1
and H2 through "balance uri path-only" and verifies that they reach
the same server.

Note that for the latter, "proto h2" explicitly had to be placed on
the listening socket otherwise it would timeout. This may indicate an
issue in the H1->H2 upgrade depending how the H2 preface is sent maybe.

4 years agoMINOR: backend: add a new "path-only" option to "balance uri"
Willy Tarreau [Wed, 23 Sep 2020 06:56:29 +0000 (08:56 +0200)] 
MINOR: backend: add a new "path-only" option to "balance uri"

Since we've fixed the way URIs are handled in 2.1, some users have started
to experience inconsistencies in "balance uri" between requests received
over H1 and the same ones received over H2. This is caused by the fact
that H1 rarely uses absolute URIs while H2 always uses them. Similar
issues were reported already around replace-uri etc, leading to "pathq"
recently being introduced, so this isn't new.

Here what this patch does is add a new option to "balance uri" to indicate
that the hashing should only start at the path and not cover the authority.
This makes H1 relative URIs and H2 absolute URI hashes equally again.

Some extra options could be added to normalize URIs by always hashing the
authority (or host) in front of them, which would make sure that both
absolute and relative requests provide the same hash. This is left for
later if needed.

4 years agoMINOR: backend: make the "whole" option of balance uri take only one bit
Willy Tarreau [Wed, 23 Sep 2020 06:05:47 +0000 (08:05 +0200)] 
MINOR: backend: make the "whole" option of balance uri take only one bit

We'll want to add other boolean options on "balance uri", so let's make
some room aside "whole" and make it take only one bit and not one int.

4 years agoBUG/MINOR: config: Fix memory leak on config parse listen
Amaury Denoyelle [Fri, 18 Sep 2020 13:59:39 +0000 (15:59 +0200)] 
BUG/MINOR: config: Fix memory leak on config parse listen

This memory leak happens if there is two or more defaults section. When
the default proxy is reinitialized, the structure member containing the
config filename must be freed.

Fix github issue #851.
Should be backported as far as 1.6.

4 years agoBUG/MINOR: Fix memory leaks cfg_parse_peers
Eric Salama [Fri, 18 Sep 2020 09:55:17 +0000 (11:55 +0200)] 
BUG/MINOR: Fix memory leaks cfg_parse_peers

When memory allocation fails in cfg_parse_peers or when an error occurs
while parsing a stick-table, the temporary table and its id must be freed.

This fixes github issue #854. It should be backported as far as 2.0.

4 years agoBUG/MINOR: http-fetch: Don't set the sample type during the htx prefetch
Christopher Faulet [Fri, 18 Sep 2020 08:19:02 +0000 (10:19 +0200)] 
BUG/MINOR: http-fetch: Don't set the sample type during the htx prefetch

A subtle bug was introduced by the commit a6d9879e6 ("BUG/MEDIUM: htx:
smp_prefetch_htx() must always validate the direction"), for the "method"
sample fetch only. The sample data type and the method id are always
overwritten because smp_prefetch_htx() function is called later in the
sample fetch evaluation. The bug is in the smp_prefetch_htx() function but
it is only visible for the "method" sample fetch, for an unknown method.

In fact, when smp_prefetch_htx() is called, the sample object is
altered. The data type is set to SMP_T_BOOL and, on success, the data value
is set to 1.  Thus, if the caller has already set some infos into the sample
object, they may be lost. AFAIK, there is no reason to do so. It is
inherited from the legacy HTTP code and I honestely don't known why it was
done this way. So, instead of fixing the "method" sample fetch to set useful
info after the call to smp_prefetch_htx() function, I prefer to not alter
the sample object in smp_prefetch_htx().

This patch must be backported as far as 2.0. On the 2.0, only the HTX part
must be fixed.

4 years agoBUG/MEDIUM: h2: report frame bits only for handled types
Willy Tarreau [Fri, 18 Sep 2020 05:43:38 +0000 (07:43 +0200)] 
BUG/MEDIUM: h2: report frame bits only for handled types

As part of his GREASE experiments on Chromium, Bence Béky reported in
https://lists.w3.org/Archives/Public/ietf-http-wg/2020JulSep/0202.html
and https://bugs.chromium.org/p/chromium/issues/detail?id=1127060 that
a certain combination of frame type and frame flags was causing an error
on app.slack.com. It turns out that it's haproxy that is causing this
issue because the frame type is wrongly assumed to support padding, the
frame flags indicate padding is present, and the frame is too short for
this, resulting in an error.

The reason why only some frame types are affected is due to the frame
type being used in a bit shift to match against a mask, and where the
5 lower bits of the frame type only are used to compute the frame bit.
If the resulting frame bit matches a DATA, HEADERS or PUSH_PROMISE frame
bit, then padding support is assumed and the test is enforced, resulting
in a PROTOCOL_ERROR or FRAME_SIZE_ERROR depending on the payload size.

We must never match any such bit for unsupported frame types so let's
add a check for this. This must be backported as far as 1.8.

Thanks to Cooper Bethea for providing enough context to help narrow the
issue down and to Bence Béky for creating a simple reproducer.

4 years agoBUG/MINOR: h2/trace: do not display "stream error" after a frame ACK
Willy Tarreau [Fri, 18 Sep 2020 05:41:28 +0000 (07:41 +0200)] 
BUG/MINOR: h2/trace: do not display "stream error" after a frame ACK

When sending a frame ACK, the parser state is not equal to H2_CS_FRAME_H
and we used to report it as an error, which is not true. In fact we should
only indicate when we skip remaining data.

This may be backported as far as 2.1.

4 years agoMINOR: h2/trace: also display the remaining frame length in traces
Willy Tarreau [Fri, 18 Sep 2020 05:39:29 +0000 (07:39 +0200)] 
MINOR: h2/trace: also display the remaining frame length in traces

It's often missing when debugging, even though it's often zero for
control frames or after data are consumed.

4 years agoBUILD: sock_inet: include errno.h
Willy Tarreau [Thu, 17 Sep 2020 12:02:01 +0000 (14:02 +0200)] 
BUILD: sock_inet: include errno.h

I was careful to have it for sock_unix.c but missed it for sock_inet
which broke with commit 36722d227 ("MINOR: sock_inet: report the errno
string in binding errors") depending on the build options. No backport
is needed.

4 years agoMINOR: sock_unix: report the errno string in binding errors
Willy Tarreau [Thu, 17 Sep 2020 06:35:38 +0000 (08:35 +0200)] 
MINOR: sock_unix: report the errno string in binding errors

Just like with previous patch, let's report UNIX socket binding errors
in plain text. we can now see for example:

  [ALERT] 260/083531 (13365) : Starting frontend f: cannot switch final and temporary UNIX sockets (Operation not permitted) [/tmp/root.sock]
  [ALERT] 260/083640 (13375) : Starting frontend f: cannot change UNIX socket ownership (Operation not permitted) [/tmp/root.sock]

4 years agoMINOR: sock_inet: report the errno string in binding errors
Willy Tarreau [Thu, 17 Sep 2020 06:32:17 +0000 (08:32 +0200)] 
MINOR: sock_inet: report the errno string in binding errors

With the socket binding code cleanup it becomes easy to add more info to
error messages. One missing thing used to be the error string, which is
now added after the generic one, for example:

  [ALERT] 260/082852 (12974) : Starting frontend f: cannot bind socket (Permission denied) [0.0.0.0:4]
  [ALERT] 260/083053 (13292) : Starting frontend f: cannot bind socket (Address already in use) [0.0.0.0:4444]
  [ALERT] 260/083104 (13298) : Starting frontend f: cannot bind socket (Cannot assign requested address) [1.1.1.1:4444]

4 years agoBUILD: sock_unix: add missing errno.h
Willy Tarreau [Wed, 16 Sep 2020 20:15:40 +0000 (22:15 +0200)] 
BUILD: sock_unix: add missing errno.h

It builds fine when openssl is enabled, but fails otherwise. No backport
is needed.

4 years agoMINOR: tools: drop listener detection hack from str2sa_range()
Willy Tarreau [Wed, 16 Sep 2020 20:04:46 +0000 (22:04 +0200)] 
MINOR: tools: drop listener detection hack from str2sa_range()

We used to resort to a trick to detect whether the caller was a listener
or an outgoing socket in order never to present an AF_CUST_UDP* socket
to a log server nor a nameserver. This is no longer necessary, the socket
type alone will be enough.

4 years agoMEDIUM: proto_udp: replace last AF_CUST_UDP* with AF_INET*
Willy Tarreau [Wed, 16 Sep 2020 19:58:52 +0000 (21:58 +0200)] 
MEDIUM: proto_udp: replace last AF_CUST_UDP* with AF_INET*

We don't need to cheat with the sock_domain anymore, we now always have
the SOCK_DGRAM sock_type as a complementary selector. This patch restores
the sock_domain to AF_INET* in the udp* protocols and removes all traces
of the now unused AF_CUST_*.

4 years agoMEDIUM: tools: make str2sa_range() use protocol_lookup()
Willy Tarreau [Wed, 16 Sep 2020 19:37:31 +0000 (21:37 +0200)] 
MEDIUM: tools: make str2sa_range() use protocol_lookup()

By doing so we can remove the hard-coded mapping from AF_INET to AF_CUST_UDP
but we still need to keep the test on the listeners as long as these dummy
families remain present in the code.

4 years agoMEDIUM: protocol: store the socket and control type in the protocol array
Willy Tarreau [Wed, 16 Sep 2020 19:28:15 +0000 (21:28 +0200)] 
MEDIUM: protocol: store the socket and control type in the protocol array

The protocol array used to be only indexed by socket family, which is very
problematic with UDP (requiring an extra family) and with the forthcoming
QUIC (also requiring an extra family), especially since that binds them to
certain families, prevents them from supporting dgram UNIX sockets etc.

In order to address this, we now start to register the protocols with more
info, namely the socket type and the control type (either stream or dgram).
This is sufficient for the protocols we have to deal with, but could also
be extended further if multiple protocol variants were needed. But as is,
it still fits nicely in an array, which is convenient for lookups that are
instant.

4 years agoMINOR: protocol: add the control layer type in the protocol struct
Willy Tarreau [Wed, 16 Sep 2020 15:50:45 +0000 (17:50 +0200)] 
MINOR: protocol: add the control layer type in the protocol struct

This one will be needed to more accurately select a protocol. It may
differ from the socket type for QUIC, which uses dgram at the socket
layer and provides stream at the control layer. The upper level requests
a control layer only so we need this field.

4 years agoMEDIUM: tools: make str2sa_range() check that the protocol has ->connect()
Willy Tarreau [Wed, 16 Sep 2020 17:17:08 +0000 (19:17 +0200)] 
MEDIUM: tools: make str2sa_range() check that the protocol has ->connect()

Most callers of str2sa_range() need the protocol only to check that it
provides a ->connect() method. It used to be used to verify that it's a
stream protocol, but it might be a bit early to get rid of it. Let's keep
the test for now but move it to str2sa_range() when the new flag PA_O_CONNECT
is present. This way almost all call places could be cleaned from this.

There's a strange test in the server address parsing code that rechecks
the family from the socket which seems to be a duplicate of the previously
removed tests. It will have to be rechecked.

4 years agoMINOR: tools: make str2sa_range() directly return the protocol
Willy Tarreau [Wed, 16 Sep 2020 16:25:03 +0000 (18:25 +0200)] 
MINOR: tools: make str2sa_range() directly return the protocol

We'll need this so that it can return pointers to stacked protocol in
the future (for QUIC). In addition this removes a lot of tests for
protocol validity in the callers.

Some of them were checked further apart, or after a call to
str2listener() and they were simplified as well.

There's still a trick, we can fail to return a protocol in case the caller
accepts an fqdn for use later. This is what servers do and in this case it
is valid to return no protocol. A typical example is:

   server foo localhost:1111

4 years agoMINOR: listener: pass the chosen protocol to create_listeners()
Willy Tarreau [Wed, 16 Sep 2020 15:58:55 +0000 (17:58 +0200)] 
MINOR: listener: pass the chosen protocol to create_listeners()

The function will need to use more than just a family, let's pass it
the selected protocol. The caller will then be able to do all the fancy
stuff required to pick the best protocol.

4 years agoMEDIUM: config: make str2listener() not accept datagram sockets anymore
Willy Tarreau [Wed, 16 Sep 2020 14:28:08 +0000 (16:28 +0200)] 
MEDIUM: config: make str2listener() not accept datagram sockets anymore

str2listener() was temporarily hacked to support datagram sockets for
the log-forward listeners. This has has an undesirable side effect that
"bind udp@1.2.3.4:5555" was silently accepted as TCP for a bind line.

We don't need this hack anymore since the only user (log-forward) now
relies on str2receiver(). Now such an address will properly be rejected.

4 years agoMINOR: log-forward: use str2receiver() to parse the dgram-bind address
Willy Tarreau [Wed, 16 Sep 2020 13:22:19 +0000 (15:22 +0200)] 
MINOR: log-forward: use str2receiver() to parse the dgram-bind address

Thanks to this we don't need to specify "udp@" as it's implicitly a
datagram type listener that is expected, so any AF_INET/AF_INET4 address
will work.

4 years agoMINOR: cfgparse: add str2receiver() to parse dgram receivers
Willy Tarreau [Wed, 16 Sep 2020 13:13:04 +0000 (15:13 +0200)] 
MINOR: cfgparse: add str2receiver() to parse dgram receivers

This is at least temporary, as the migration at once is way too difficuly.
For now it still creates listeners but only allows DGRAM sockets. This
aims at easing the split between listeners and receivers.

4 years agoMINOR: tools: remove the central test for "udp" in str2sa_range()
Willy Tarreau [Wed, 16 Sep 2020 13:20:59 +0000 (15:20 +0200)] 
MINOR: tools: remove the central test for "udp" in str2sa_range()

Now we only rely on dgram type associated with AF_INET/AF_INET6 to infer
UDP4/UDP6. We still keep the hint based on PA_O_SOCKET_FD to detect that
the caller is a listener though. It's still far from optimal but UDP
remains rooted into the protocols and needs to be taken out first.

4 years agoMEDIUM: tools: make str2sa_range() only report AF_CUST_UDP on listeners
Willy Tarreau [Wed, 16 Sep 2020 18:35:12 +0000 (20:35 +0200)] 
MEDIUM: tools: make str2sa_range() only report AF_CUST_UDP on listeners

For now only listeners can make use of AF_CUST_UDP and it requires hacks
in the DNS and logsrv code to remap it to AF_INET. Make str2sa_range()
smarter by detecting that it's called for a listener and only set these
protocol families for listeners. This way we can get rid of the hacks.

4 years agoMINOR: tools: start to distinguish stream and dgram in str2sa_range()
Willy Tarreau [Wed, 16 Sep 2020 09:35:47 +0000 (11:35 +0200)] 
MINOR: tools: start to distinguish stream and dgram in str2sa_range()

The parser now supports a socket type for the control layer and a possible
other one for the transport layer. Usually they are the same except for
protocols like QUIC which will provide a stream transport layer based on
a datagram control layer. The default types are preset based on the caller's
expectations, and may be refined using "stream+" and "dgram+" prefixes.

For now they were not added to the docuemntation because other changes
will probably happen around UDP as well. It is conceivable that "tcpv4@"
or "udpv6@" will appear later as aliases for "stream+ipv4" or "dgram+ipv6".

4 years agoMEDIUM: tools: make str2sa_range() check for the sockpair's FD usability
Willy Tarreau [Wed, 16 Sep 2020 08:14:16 +0000 (10:14 +0200)] 
MEDIUM: tools: make str2sa_range() check for the sockpair's FD usability

Just like for inherited sockets, we want to make sure that FDs that are
mentioned in "sockpair@" are actually usable. Right now this test is
performed by the callers, but not everywhere. Typically, the following
config will fail if fd #5 is not bound:

  frontend
      bind sockpair@5

But this one will pass if fd #6 is not bound:

  backend
      server s1 sockpair@6

Now both will return an error in such a case:
   - 'bind' : cannot use file descriptor '5' : Bad file descriptor.
   - 'server s1' : cannot use file descriptor '6' : Bad file descriptor.

As such the test in str2listener() is not needed anymore (and it was
wrong by the way, as it used to test for the socket by overwriting the
local address with a new address that's made of the FD encoded on 16
bits and happens to still be at the same place, but that strictly
depends on whatever the kernel wants to put there).

4 years agoMINOR: config: do not test an inherited socket again
Willy Tarreau [Wed, 16 Sep 2020 08:43:38 +0000 (10:43 +0200)] 
MINOR: config: do not test an inherited socket again

Since previous patch we know that a successfully bound fd@XXX socket
is returned as its own protocol family from str2sa_range() and not as
AF_CUST_EXISTING_FD anymore o we don't need to check for that case
in str2listener().

4 years agoMEDIUM: tools: make str2sa_range() resolve pre-bound listeners
Willy Tarreau [Tue, 15 Sep 2020 15:41:56 +0000 (17:41 +0200)] 
MEDIUM: tools: make str2sa_range() resolve pre-bound listeners

When str2sa_range() is invoked for a bind or log line, and it gets a file
descriptor number, it will immediately resolve the socket's address (when
it's a socket) so that the address family, address and port are correctly
set. This will later allow to resolve some transport protocols that are
attached to existing FDs. For raw FDs (e.g. logs) and for socket pairs,
the FD number is still returned in the address, because we need the
underlying address management to complete the bind/listen/connect/whatever
needed. One immediate benefit is that passing a bad FD will now result in
one of these errors:

  'bind' : cannot use file descriptor '3' : Socket operation on non-socket.
  'bind' : socket on file descriptor '3' is of the wrong type.

Note that as of now, we never return a listening socket with a family of
AF_CUST_EXISTING_FD. The only case where this family is seen is for a raw
FD (e.g. logs).

4 years agoMINOR: log: detect LOG_TARGET_FD from the fd and not from the syntax
Willy Tarreau [Tue, 15 Sep 2020 12:03:26 +0000 (14:03 +0200)] 
MINOR: log: detect LOG_TARGET_FD from the fd and not from the syntax

Now that we have the FD value reported we don't need to cheat and detect
"fd@" in the address, we can safely rely on the FD value.

4 years agoMINOR: tools: make str2sa_range() optionally return the fd
Willy Tarreau [Tue, 15 Sep 2020 12:01:16 +0000 (14:01 +0200)] 
MINOR: tools: make str2sa_range() optionally return the fd

If a file descriptor was passed, we can optionally return it. This will
be useful for listening sockets which are both a pre-bound FD and a ready
socket.

4 years agoMINOR: listener: remove the inherited arg to create_listener()
Willy Tarreau [Tue, 15 Sep 2020 11:50:58 +0000 (13:50 +0200)] 
MINOR: listener: remove the inherited arg to create_listener()

This argument can now safely be determined from fd != -1, let's just
drop it.

4 years agoMINOR: tools: add several PA_O_* flags in str2sa_range() callers
Willy Tarreau [Fri, 4 Sep 2020 13:53:16 +0000 (15:53 +0200)] 
MINOR: tools: add several PA_O_* flags in str2sa_range() callers

These flags indicate whether the call is made to fill a bind or a server
line, or even just send/recv calls (like logs or dns). Some special cases
are made for outgoing FDs (e.g. pipes for logs) or socket FDs (e.g external
listeners), and there's a distinction between stream or dgram usage that's
expected to significantly help str2sa_range() proceed appropriately with
the input information. For now they are not used yet.

4 years agoMEDIUM: config: remove all checks for missing/invalid ports/ranges
Willy Tarreau [Tue, 15 Sep 2020 09:52:23 +0000 (11:52 +0200)] 
MEDIUM: config: remove all checks for missing/invalid ports/ranges

Now that str2sa_range() checks for appropriate port specification, we
don't need to implement adhoc test cases in every call place, if the
result is valid, the conditions are met otherwise the error message is
appropriately filled.

4 years agoMEDIUM: tools: make str2sa_range() validate callers' port specifications
Willy Tarreau [Tue, 15 Sep 2020 09:12:44 +0000 (11:12 +0200)] 
MEDIUM: tools: make str2sa_range() validate callers' port specifications

Now str2sa_range() will enforce the caller's port specification passed
using the PA_O_PORT_* flags, and will return an error on failure. For
optional ports, values 0-65535 will be enforced. For mandatory ports,
values 1-65535 are enforced. In case of ranges, it is also verified that
the upper bound is not lower than the lower bound, as this used to result
in empty listeners.

I couldn't find an easy way to test this using VTC since the purpose is
to trigger parse errors, so instead a test file is provided as
tests/ports.cfg with comments about what errors are expected for each
line.

4 years agoMINOR: tools: add several PA_O_PORT_* flags in str2sa_range() callers
Willy Tarreau [Tue, 15 Sep 2020 08:30:39 +0000 (10:30 +0200)] 
MINOR: tools: add several PA_O_PORT_* flags in str2sa_range() callers

These flags indicate what is expected regarding port specifications. Some
callers accept none, some need fixed ports, some have it mandatory, some
support ranges, and some take an offset. Each possibilty is reflected by
an option. For now they are not exploited, but the goal is to instrument
str2sa_range() to properly parse that.

4 years agoMINOR: tools: make str2sa_range() take more options than just resolve
Willy Tarreau [Fri, 4 Sep 2020 13:30:46 +0000 (15:30 +0200)] 
MINOR: tools: make str2sa_range() take more options than just resolve

We currently have an argument to require that the address is resolved
but we'll soon add more, so let's turn it into a bit field. The old
"resolve" boolean is now PA_O_RESOLVE.

4 years agoCLEANUP: tools: make str2sa_range() less awful for fd@ and sockpair@
Willy Tarreau [Fri, 4 Sep 2020 14:54:05 +0000 (16:54 +0200)] 
CLEANUP: tools: make str2sa_range() less awful for fd@ and sockpair@

The code is built to match prefixes at one place and to parse the address
as a second step, except for fd@ and sockpair@ where the test first passes
via AF_UNSPEC that is changed again. This is ugly and confusing, so let's
proceed like for the other ones.

4 years agoMINOR: protocol: add a real family for existing FDs
Willy Tarreau [Fri, 4 Sep 2020 14:44:20 +0000 (16:44 +0200)] 
MINOR: protocol: add a real family for existing FDs

At some places (log fd@XXX, bind fd@XXX) we support using an explicit
file descriptor number, that is placed into the sockaddr for later use.
The problem is that till now it was done with an AF_UNSPEC family, which
is also used for other situations like missing info or rings (for logs).

Let's create an "official" family AF_CUST_EXISTING_FD for this case so
that we are certain the FD can be found in the address when it is set.

4 years agoCLEANUP: protocol: remove family-specific fields from struct protocol
Willy Tarreau [Fri, 4 Sep 2020 06:23:14 +0000 (08:23 +0200)] 
CLEANUP: protocol: remove family-specific fields from struct protocol

This removes the following fields from struct protocol that are now
retrieved from the protocol family instead: .sock_family, .sock_addrlen,
.l3_addrlen, .addrcmp, .bind, .get_src, .get_dst.

This also removes the UDP-specific udp{,6}_get_{src,dst}() functions
which were referenced but not used yet. Their goal was only to remap
the original AF_INET* addresses to AF_CUST_UDP*.

Note that .sock_domain is still there as it's used as a selector for
the protocol struct to be used.

4 years agoMINOR: protocol: retrieve the family-specific fields from the family
Willy Tarreau [Fri, 4 Sep 2020 06:15:31 +0000 (08:15 +0200)] 
MINOR: protocol: retrieve the family-specific fields from the family

We now take care of retrieving sock_family, l3_addrlen, bind(),
addrcmp(), get_src() and get_dst() from the protocol family and
not just the protocol itself. There are very few places, this was
only seldom used. Interestingly in sock_inet.c used to rely on
->sock_family instead of ->sock_domain, and sock_unix.c used to
hard-code PF_UNIX instead of using ->sock_domain.

Also it appears obvious we have something wrong it the protocol
selection algorithm because sock_domain is the one set to the custom
protocols while it ought to be sock_family instead, which would avoid
having to hard-code some conversions for UDP namely.

4 years agoMINOR: protocol: add a new proto_fam structure for protocol families
Willy Tarreau [Fri, 4 Sep 2020 06:07:11 +0000 (08:07 +0200)] 
MINOR: protocol: add a new proto_fam structure for protocol families

We need to specially handle protocol families which regroup common
functions used for a given address family. These functions include
bind(), addrcmp(), get_src() and get_dst() for now. Some fields are
also added about the address family, socket domain (protocol family
passed to the socket() syscall), and address length.

These protocol families are referenced from the protocols but not yet
used.

4 years agoMEDIUM: protocol: do not call proto->bind() anymore from bind_listener()
Willy Tarreau [Wed, 2 Sep 2020 16:40:02 +0000 (18:40 +0200)] 
MEDIUM: protocol: do not call proto->bind() anymore from bind_listener()

All protocol's listeners now only take care of themselves and not of
the receiver anymore since that's already being done in proto_bind_all().
Now it finally becomes obvious that UDP doesn't need a listener, as the
only thing it does is to set the listener's state to LI_LISTEN!

4 years agoMEDIUM: protocol: explicitly start the receiver before the listener
Willy Tarreau [Wed, 2 Sep 2020 16:22:11 +0000 (18:22 +0200)] 
MEDIUM: protocol: explicitly start the receiver before the listener

Now protocol_bind_all() starts the receivers before their respective
listeners so that ultimately we won't need the listeners for non-
connected protocols.

We still have to resort to an ugly trick to set the I/O handler in
case of syslog over UDP because for now it's still not set in the
receiver, so we hard-code it.

4 years agoMEDIUM: proto_sockpair: make use of sockpair_bind_receiver()
Willy Tarreau [Wed, 2 Sep 2020 16:02:00 +0000 (18:02 +0200)] 
MEDIUM: proto_sockpair: make use of sockpair_bind_receiver()

Now we rely on the address family's receiver instead of binding everything
ourselves.

4 years agoMEDIUM: sockpair: implement sockpair_bind_receiver()
Willy Tarreau [Wed, 2 Sep 2020 15:52:23 +0000 (17:52 +0200)] 
MEDIUM: sockpair: implement sockpair_bind_receiver()

Note that for now we don't have a sockpair.c file to host that unusual
family, so the new function was placed directly into proto_sockpair.c.
It's no big deal given that this family is currently not shared with
multiple protocols.

The function does almost nothing but setting up the receiver. This is
normal as the socket the FDs are passed onto are supposed to have been
already created somewhere else, and the only usable identifier for such
a socket pair is the receiving FD itself.

The function was assigned to sockpair's ->bind() and is not used yet.

4 years agoMEDIUM: uxst: make use of sock_unix_bind_receiver()
Willy Tarreau [Wed, 2 Sep 2020 15:21:02 +0000 (17:21 +0200)] 
MEDIUM: uxst: make use of sock_unix_bind_receiver()

This removes all the AF_UNIX-specific code from uxst_bind_listener()
and now simply relies on sock_unix_bind_listener() to do the same
job. As mentionned in previous commit, the only difference is that
now an unlikely failure on listen() will not result in a roll back
of the temporary socket names since they will have been renamed
during the bind() operation (as expected). But such failures do not
correspond to any normal case and mostly denote operating system
issues so there's no functionality loss here.

4 years agoMEDIUM: sock_unix: implement sock_unix_bind_receiver()
Willy Tarreau [Wed, 2 Sep 2020 15:14:29 +0000 (17:14 +0200)] 
MEDIUM: sock_unix: implement sock_unix_bind_receiver()

This function performs all the bind-related stuff for UNIX sockets that
was previously done in uxst_bind_listener(). There is a very tiny
difference however, which is that previously, in the unlikely event
where listen() would fail, it was still possible to roll back the binding
and rename the backup to the original socket. Now we have to rename it
before calling returning, hence it will be done before calling listen().
However, this doesn't cover any particular use case since listen() has no
reason to fail there (and the rollback is not done for inherited sockets),
that was just done that way as a generic error processing path.

The code is not used yet and is referenced in the uxst proto's ->bind().

4 years agoMEDIUM: udp: make use of sock_inet_bind_receiver()
Willy Tarreau [Tue, 1 Sep 2020 14:23:29 +0000 (16:23 +0200)] 
MEDIUM: udp: make use of sock_inet_bind_receiver()

This removes all the AF_INET-specific code from udp_bind_listener()
and now simply relies on sock_inet_bind_listener() to do the same
job. The function is now basically just a wrapper around
sock_inet_bind_receiver().

4 years agoMEDIUM: tcp: make use of sock_inet_bind_receiver()
Willy Tarreau [Tue, 1 Sep 2020 14:12:50 +0000 (16:12 +0200)] 
MEDIUM: tcp: make use of sock_inet_bind_receiver()

This removes all the AF_INET-specific code from tcp_bind_listener()
and now simply relies on sock_inet_bind_listener() to do the same
job. The function was now roughly cut in half and its error path
significantly simplified.

4 years agoMEDIUM: sock_inet: implement sock_inet_bind_receiver()
Willy Tarreau [Tue, 1 Sep 2020 12:18:04 +0000 (14:18 +0200)] 
MEDIUM: sock_inet: implement sock_inet_bind_receiver()

This function collects all the receiver-specific code from both
tcp_bind_listener() and udp_bind_listener() in order to provide a more
generic AF_INET/AF_INET6 socket binding function. For now the API is
not very elegant because some info are still missing from the receiver
while there's no ideal place to fill them except when calling ->listen()
at the protocol level. It looks like some polishing code is needed in
check_config_validity() or somewhere around this in order to finalize
the receivers' setup. The main issue is that listeners and receivers
are created *before* bind_conf options are parsed and that there's no
finishing step to resolve some of them.

The function currently sets up a receiver and subscribes it to the
poller. In an ideal world we wouldn't subscribe it but let the caller
do it after having finished to configure the L4 stuff. The problem is
that the caller would then need to perform an fd_insert() call and to
possibly set the exported flag on the FD while it's not its job. Maybe
an improvement could be to have a separate sock_start_receiver() call
in sock.c.

For now the function is not used but it will soon be. It's already
referenced as tcp and udp's ->bind().

4 years agoMINOR: protocol: add a new ->bind() entry to bind the receiver
Willy Tarreau [Wed, 2 Sep 2020 11:48:45 +0000 (13:48 +0200)] 
MINOR: protocol: add a new ->bind() entry to bind the receiver

This will be the function that must be used to bind the receiver. It
solely depends on the address family but for now it's simpler to have
it per protocol.