[PATCH] REGTEST/MINOR: loadtest: add a test for connection counters
Hi List, Willy,
I've created a regtest that checks that when concurrent connections are
being handled that the connection counters are kept properly.
I think it could be committed as attached. It takes a few seconds to
run. It currently fails on 1.9-dev2 (also fails on 1.8.13 with kqueue on
FreeBSD, adding a 'nokqueue' on 1.8.13 makes it succeed though..).
I think it might be a good and reproducible test to run.?
Or does it need more tweaking? Thoughts appreciated :).
Regards,
PiBa-NL (Pieter)
From 4b1af997e796e1bb2098c5f66ac24690841c72e8 Mon Sep 17 00:00:00 2001
From: PiBa-NL
Date: Sat, 15 Sep 2018 01:51:54 +0200
Subject: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters
after running 3000 requests in a loop
---
reg-tests/loadtest/b0-loadtest.vtc | 94 ++
1 file changed, 94 insertions(+)
create mode 100644 reg-tests/loadtest/b0-loadtest.vtc
diff --git a/reg-tests/loadtest/b0-loadtest.vtc
b/reg-tests/loadtest/b0-loadtest.vtc
new file mode 100644
index ..f66df5ee
--- /dev/null
+++ b/reg-tests/loadtest/b0-loadtest.vtc
@@ -0,0 +1,94 @@
+# Checks that request and connection counters are properly kept
+
+varnishtest "Seamless reload issue with abns sockets"
+feature ignore_unknown_macro
+
+server s1 {
+rxreq
+expect req.http.TESTsize == 1000
+txresp
+} -repeat 3 -start
+
+syslog Slg_1 -level notice {
+recv
+} -repeat 15 -start
+
+haproxy h1 -W -D -conf {
+ global
+nbthread 3
+log ${Slg_1_addr}:${Slg_1_port} local0
+maxconn 50
+#nokqueue
+
+ defaults
+mode http
+option dontlog-normal
+log global
+option httplog
+timeout connect 3s
+timeout client 4s
+timeout server 15s
+
+ frontend fe1
+maxconn 20001
+bind "fd@${fe_1}"
+acl donelooping hdr(TEST) -m len 1000
+http-request set-header TEST "%[hdr(TEST)]x"
+use_backend b2 if donelooping
+default_backend b1
+
+ backend b1
+fullconn 2
+server srv1 ${h1_fe_1_addr}:${h1_fe_1_port} maxconn 2
+
+ frontend fe2
+bind "fd@${fe_2}"
+default_backend b2
+
+ backend b2
+# haproxy 1.8 does not have the ,length converter.
+acl OK hdr(TEST) -m len 1000
+http-request deny deny_status 200 if OK
+http-request deny deny_status 400
+
+# haproxy 1.9 does have a ,length converter.
+#http-request set-header TESTsize "%[hdr(TEST),length]"
+#http-request del-header TEST
+#server srv2 ${s1_addr}:${s1_port}
+
+} -start
+
+barrier b1 cond 3
+
+client c1 -connect ${h1_fe_1_sock} {
+ timeout 17
+ barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c2 -connect ${h1_fe_1_sock} {
+ timeout 17
+ barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c3 -connect ${h1_fe_1_sock} {
+ timeout 17
+ barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+
+client c1 -wait
+client c2 -wait
+client c3 -wait
+
+haproxy h1 -cli {
+send "show info"
+expect ~ "CumConns: 3001"
+send "show info"
+expect ~ "CumReq: 3002"
+}
--
2.18.0.windows.1
patch to avoid null pointer dereference
hi,
please find attached patch
cheers,
Ilya Shipitsin
From 7961bb27597cf529a88da475d3928d6223a88753 Mon Sep 17 00:00:00 2001
From: Ilya Shipitsin
Date: Sat, 15 Sep 2018 00:50:05 +0500
Subject: [PATCH] MINOR: src/connection.c: avoid null pointer dereference
found by coverity
---
src/connection.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/connection.c b/src/connection.c
index b970d418..1e139ec2 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -1191,7 +1191,7 @@ int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connec
ret += make_tlv([ret], (buf_len - ret), PP2_TYPE_CRC32C, sizeof(zero_crc32c), (const char *)_crc32c);
}
- if (conn_get_alpn(remote, , _len)) {
+ if (remote && conn_get_alpn(remote, , _len)) {
if ((buf_len - ret) < sizeof(struct tlv))
return 0;
ret += make_tlv([ret], (buf_len - ret), PP2_TYPE_ALPN, value_len, value);
--
2.17.1
Re: PATCH/doc: load-server-state-from-file (was: BUG: changed server IPs do not get restored from saved state)
Hi Willy Am Fr., 14. Sep. 2018 um 19:15 Uhr schrieb Willy Tarreau : > On Fri, Sep 14, 2018 at 05:43:14PM +0200, Peter Fröhlich wrote: > > For your consideration, the doc patch with context. > > Please give me feedback if I should incorporate some other aspects. > > Thank you, however as you can see below, your mailer mangled the patch > by wrapping some lines and making it unusable. Could you please send it > as an attachment instead ? Sorry, I'm not used to do this without pull requests. ^^;;; Here is the diff. Thanks Peter doc_load-server-state-from-file.diff Description: Binary data
Re: PATCH/doc: load-server-state-from-file (was: BUG: changed server IPs do not get restored from saved state)
Hi Peter,
On Fri, Sep 14, 2018 at 05:43:14PM +0200, Peter Fröhlich wrote:
> For your consideration, the doc patch with context.
> Please give me feedback if I should incorporate some other aspects.
Thank you, however as you can see below, your mailer mangled the patch
by wrapping some lines and making it unusable. Could you please send it
as an attachment instead ? Oh by the way, if you're doing your patches
by hand, it's generally better to perform the diff from one directory
above the development tree, or by prepending "./" in front of the file
names so that the patch always applies fine using "patch -p1". Example
in your case :
$ diff -u ./doc/configuration.txt{.edited,} > file.diff
thanks!
Willy
> --- configuration.txt 2018-09-14 17:35:35.0 +0200
> +++ configuration.txt.edited 2018-09-14 17:35:29.0 +0200
> @@ -4894,69 +4894,79 @@
>
> load-server-state-from-file { global | local | none }
>Allow seamless reload of HAProxy
>May be used in sections:defaults | frontend | listen | backend
>yes |no | yes | yes
>
>This directive points HAProxy to a file where server state from previous
>running process has been saved. That way, when starting up, before
> handling
>traffic, the new process can apply old states to servers exactly has if
> no
>reload occurred. The purpose of the "load-server-state-from-file"
> directive is
>to tell haproxy which file to use. For now, only 2 arguments to either
> prevent
>loading state or load states from a file containing all backends and
> servers.
>The state file can be generated by running the command "show servers
> state"
>over the stats socket and redirect output.
(...)
PATCH/doc: load-server-state-from-file (was: BUG: changed server IPs do not get restored from saved state)
Am Do., 13. Sep. 2018 um 15:23 Uhr schrieb Peter Fröhlich
:
>
> Am Do., 13. Sep. 2018 um 10:06 Uhr schrieb Willy Tarreau :
> > On Thu, Sep 13, 2018 at 09:29:42AM +0200, Peter Fröhlich wrote:
> > > Would our use case of setting the backend servers only through the
socket,
> > > and the necessary workaround of setting the server entries to
'localhost'
> > > be worth a 'Notes:' entry in the 'load-server-state-from-file'
section?
> >
> > Yes, very likely. Feel free to propose a patch for the doc. Generally
doc
> > written by users who were bitten by a problem is much clearer than the
one
> > written by the initial developer!
> >
For your consideration, the doc patch with context.
Please give me feedback if I should incorporate some other aspects.
Peter
--- configuration.txt 2018-09-14 17:35:35.0 +0200
+++ configuration.txt.edited 2018-09-14 17:35:29.0 +0200
@@ -4894,69 +4894,79 @@
load-server-state-from-file { global | local | none }
Allow seamless reload of HAProxy
May be used in sections:defaults | frontend | listen | backend
yes |no | yes | yes
This directive points HAProxy to a file where server state from previous
running process has been saved. That way, when starting up, before
handling
traffic, the new process can apply old states to servers exactly has if
no
reload occurred. The purpose of the "load-server-state-from-file"
directive is
to tell haproxy which file to use. For now, only 2 arguments to either
prevent
loading state or load states from a file containing all backends and
servers.
The state file can be generated by running the command "show servers
state"
over the stats socket and redirect output.
The format of the file is versioned and is very specific. To understand
it,
please read the documentation of the "show servers state" command
(chapter
9.3 of Management Guide).
Arguments:
global load the content of the file pointed by the global directive
named "server-state-file".
local load the content of the file pointed by the directive
"server-state-file-name" if set. If not set, then the
backend
name is used as a file name.
none don't load any stat for this backend
Notes:
-- server's IP address is preserved across reloads by default, but the
- order can be changed thanks to the server's "init-addr" setting. This
- means that an IP address change performed on the CLI at run time will
+- a servers IP address will only be applied from the state file if the
+ servers config file line refers to a DNS address. If the server has a
+ static IP configured in the config file the IP from the state file
gets
+ ignored silently (the other settings get restored).
+
+- the resolved IP address of a server is preserved across reloads by
default,
+ but the order can be changed thanks to the servers "init-addr"
setting.
+ This means that an IP address change performed on the CLI at run
time will
be preserved, and that any change to the local resolver (e.g.
/etc/hosts)
will possibly not have any effect if the state file is in use.
-- server's weight is applied from previous running process unless it
has
- has changed between previous and new configuration files.
+- a continuous reconfiguration of server IPs + ports over the state
socket
+ could require to always restore the IP from the state file at reload.
+ To enable this use a safe default DNS name (like "localhost") in the
+ servers config line, that gets resolved over the local resolver.
+- servers weight is applied from previous running process unless it has
+ has changed between previous and new configuration files.
+
Example: Minimal configuration
global
stats socket /tmp/socket
server-state-file /tmp/server_state
defaults
load-server-state-from-file global
backend bk
server s1 127.0.0.1:22 check weight 11
server s2 127.0.0.1:22 check weight 12
Then one can run :
socat /tmp/socket - <<< "show servers state" > /tmp/server_state
Content of the file /tmp/server_state would be like this:
1
#
1 bk 1 s1 127.0.0.1 2 0 11 11 4 6 3 4 6 0 0
1 bk 2 s2 127.0.0.1 2 0 12 12 4 6 3 4 6 0 0
Example: Minimal configuration
global
stats socket /tmp/socket
server-state-base /etc/haproxy/states
Re: [ANNOUNCE] haproxy-1.9-dev2
Hi, Quick test with 1.9-dev2, and i see latency (in seconds) to connect to haproxy with SSL (tcp mode). It’s ok in master with 9f9b0c6a. No time to investigate more for the moment. ++ Manu
Fix some warnings and a small bug in debug logic
Hi all, While working on the OpenSSL 1.1.1 and TLS 1.3 cipher support issue, I also saw a number of compiler warnings that led me to investigate a bit. It resulted in some small cleanups and also one bug I think in some of the debugging logic for the h2 mux. Hopefully these are useful! Cheers, Dirkjan 0001-Remove-unused-variable.patch Description: Binary data 0002-Fix-debug-warnings-for-h1-headers.patch Description: Binary data 0003-Remove-unneeded-double-around-conditional-clause.patch Description: Binary data
Re: TLS 1.3 options available with OpenSSL 1.1.1
Hi all, > On 14 Sep 2018, at 14:15, Emmanuel Hocdet wrote: > > It’s not necessary, BoringSSL and LibreSSL have, at best, > OPENSSL_VERSION_NUMBER set to 1.1.0 for API compatibilité. Looking at LibreSSL, it’s defining this (in their latest Git code): src/lib/libcrypto/opensslv.h:#define OPENSSL_VERSION_NUMBER 0x2000L I also see this conditional used in other places to explicitly exclude BoringSSL and LibreSSL, so that’s why I thought it would be needed here as well. -- Cheers, Dirkjan
Re: TLS 1.3 options available with OpenSSL 1.1.1
> Le 14 sept. 2018 à 14:01, Dirkjan Bussink a écrit : > > Hi all, > >> On 14 Sep 2018, at 12:18, Emmanuel Hocdet wrote: >> >> Same deal with boringssl, TLSv <= 1.2 ciphers configuration and TLSv1.3 >> ciphers are segregated. >> https://boringssl.googlesource.com/boringssl/+/abbbee10ad4739648bcbf36a5ac52f23263a67dd%5E!/ > > This reminded me to double check with BoringSSL and LibreSSL how they handle > this. Neither has this new method, so I have updated the conditional used in > the patch to exclude these two as well. > It’s not necessary, BoringSSL and LibreSSL have, at best, OPENSSL_VERSION_NUMBER set to 1.1.0 for API compatibilité. ++ Manu
Re: TLS 1.3 options available with OpenSSL 1.1.1
Hi all, > On 14 Sep 2018, at 12:18, Emmanuel Hocdet wrote: > > Same deal with boringssl, TLSv <= 1.2 ciphers configuration and TLSv1.3 > ciphers are segregated. > https://boringssl.googlesource.com/boringssl/+/abbbee10ad4739648bcbf36a5ac52f23263a67dd%5E!/ This reminded me to double check with BoringSSL and LibreSSL how they handle this. Neither has this new method, so I have updated the conditional used in the patch to exclude these two as well. Cheers, Dirkjan 0001-Add-support-for-ciphersuites-option-for-TLS-1.3.patch Description: Binary data
Re: TLS 1.3 options available with OpenSSL 1.1.1
Hi Emeric, Lukas, Dirkjan > Le 14 sept. 2018 à 11:12, Emeric Brun a écrit : > > Hi Lukas, Dirkjan, > > On 09/13/2018 10:17 PM, Lukas Tribus wrote: >> Hello Dirkjan, >> >> >> On Thu, 13 Sep 2018 at 16:44, Dirkjan Bussink wrote: >>> So with a new API call, does that mean adding for example a `ciphersuites` >>> option that works similar to `ciphers` today that it accepts a string and >>> then >>> calls `SSL_CTX_set_ciphersuites`? >> >> Yes, that's what I'd have in mind. Sufficiently #ifdef that everything >> still builds the same and documented of course so that people >> understand what is what. Let's wait for Manu and Emeric's feedback >> though, before investing your time. > > I think if TLSv <= 1.2 and TLSv1.3 ciphers are handled separately, this is > good reason > to add a new keyword to manage both at a same line on an haproxy > configuration file's line . > > I've just realized that it may be the openssl's response to an issue we faced > on earlier version of > openssl1.1.1 dev branch where forcing cipher suite on a SSL_CTX broke TLSv1.2 > handshakes if > no TLSv1.3 ciphers were specified in this list. > > Doing this, managing differently TLS <= v1.2 and 1.3 ciphers permits the user > to not face regression issues > upgrading to v1.1.1 when suites where forced in configuration because > openssl-1.1.1 kept default > TVSv1.3 ciphers. > Same deal with boringssl, TLSv <= 1.2 ciphers configuration and TLSv1.3 ciphers are segregated. https://boringssl.googlesource.com/boringssl/+/abbbee10ad4739648bcbf36a5ac52f23263a67dd%5E!/ > So i'm convinced we need to handle this new TLSv1.3 cipher suite with a new > config keyword, but I > don't know how we should name it. I think it will be a mistake to make appear > 1.3 in the new name because > there is no warranty that next TLS versions will specify specific cipher > lists. Openssl's > API make the choice of "ciphersuites" ... perhaps a the right choice. Following the API name should be the right choice. ++ Manu
Re: TLS 1.3 options available with OpenSSL 1.1.1
Hi all, I took the liberty of writing up a patch with what this could look like. I have named the option `ciphersuites` and also added the documentation for it as well. I have attached the patch to this email. > On 14 Sep 2018, at 11:12, Emeric Brun wrote: > > I think if TLSv <= 1.2 and TLSv1.3 ciphers are handled separately, this is > good reason > to add a new keyword to manage both at a same line on an haproxy > configuration file's line . This is what my patch does indeed. > I've just realized that it may be the openssl's response to an issue we faced > on earlier version of > openssl1.1.1 dev branch where forcing cipher suite on a SSL_CTX broke TLSv1.2 > handshakes if > no TLSv1.3 ciphers were specified in this list. > > Doing this, managing differently TLS <= v1.2 and 1.3 ciphers permits the user > to not face regression issues > upgrading to v1.1.1 when suites where forced in configuration because > openssl-1.1.1 kept default > TVSv1.3 ciphers. Yeah, without the configurations setting it uses the 1.3 defaults which are already good safe defaults. > So i'm convinced we need to handle this new TLSv1.3 cipher suite with a new > config keyword, but I > don't know how we should name it. I think it will be a mistake to make appear > 1.3 in the new name because > there is no warranty that next TLS versions will specify specific cipher > lists. Openssl's > API make the choice of "ciphersuites" ... perhaps a the right choice. That’s what I did :). Hopefully it looks somewhat sensible. > Did any of you check how this is endled on "openssl s_client" command line? From the help for this command: -cipher valSpecify TLSv1.2 and below cipher list to be used -ciphersuites val Specify TLSv1.3 ciphersuites to be used So it does the same thing as my patch does. Cheers, Dirkjan 0001-Add-support-for-ciphersuites-option-for-TLS-1.3.patch Description: Binary data
Re: TLS 1.3 options available with OpenSSL 1.1.1
Hi Lukas, Dirkjan, On 09/13/2018 10:17 PM, Lukas Tribus wrote: > Hello Dirkjan, > > > On Thu, 13 Sep 2018 at 16:44, Dirkjan Bussink wrote: >> So with a new API call, does that mean adding for example a `ciphersuites` >> option that works similar to `ciphers` today that it accepts a string and >> then >> calls `SSL_CTX_set_ciphersuites`? > > Yes, that's what I'd have in mind. Sufficiently #ifdef that everything > still builds the same and documented of course so that people > understand what is what. Let's wait for Manu and Emeric's feedback > though, before investing your time. I think if TLSv <= 1.2 and TLSv1.3 ciphers are handled separately, this is good reason to add a new keyword to manage both at a same line on an haproxy configuration file's line . I've just realized that it may be the openssl's response to an issue we faced on earlier version of openssl1.1.1 dev branch where forcing cipher suite on a SSL_CTX broke TLSv1.2 handshakes if no TLSv1.3 ciphers were specified in this list. Doing this, managing differently TLS <= v1.2 and 1.3 ciphers permits the user to not face regression issues upgrading to v1.1.1 when suites where forced in configuration because openssl-1.1.1 kept default TVSv1.3 ciphers. So i'm convinced we need to handle this new TLSv1.3 cipher suite with a new config keyword, but I don't know how we should name it. I think it will be a mistake to make appear 1.3 in the new name because there is no warranty that next TLS versions will specify specific cipher lists. Openssl's API make the choice of "ciphersuites" ... perhaps a the right choice. Did any of you check how this is endled on "openssl s_client" command line? > > >> The curve functions are synonyms for the equivalently named group functions >> and are identical in every respect. > > So there is no technical reason to implement the new API call, it's > just to go with the new naming convention. No need for any action > then, we don't have to blow up our code with additional #ifdef mess > for the sake of using better looking names in API calls. Perfectly agreed. > > Thanks for looking into this! > > cheers, > lukas > R, Emeric
Re: regtest lua/b00002.vtc fails with 1.9-dev2 / master
Hi Pieter, On Thu, Sep 13, 2018 at 05:13:43PM +0200, PiBa-NL wrote: > Just tried another run of regtests on FreeBSD, and found that lua/b2.vtc > fails (coredump, gdb bt below) with todays snapshot: HA-Proxy version > 1.9-dev2-253006d 2018/09/12 Yesterday we've looked at this with Olivier and it raised some questions between us regarding the nature of certain events that need to be reported upwards. Here the issue is related to the sequencing of the connect_server() call and the subscription to try to receive. If we do it the current way, we read from a non-existent mux and if we do it the other way around we'll lose connect events if there's no data to send. We have some plans for this that we're discussing today in front of the white board. Getting rid of ~15 years of accumulated hacks in the connection layers is amazingly difficult, and much more than what it looks like before trying to do it! I hope that today we'll have clear ideas about a very short-term workaround and how we want to address this situation from an architecture perspective. > p.s. should reg-tests maybe get run before the -dev releases get tagged? (or > is that already done, and this could be a FreeBSD specific issue and as such > not have been spotted?) It's my fault. Yes I'm supposed to run these tests. But after finally managed to collect all the long-awaited code a few minutes before I really had to leave, I was impatient to push it out so that we can all resync. And not having vtc anymore after I rebooted my PC (last time it was built in /dev/shm and I still didn't take the time to rebuild it) didn't give me enough motivation to add this extra task. The process will improve over time, we need it to become more natural and usual. You don't kill 18 years of bad habits in a few weeks! Just to be clear, spotting bugs at this moment will not prevent me from emitting a dev release, but at least I'll report them in the changelog so that nobody wastes their time ;-) Cheers, Willy
Re: [PATCH] DOC: Fix typos in lua documentation
On Thu, Sep 13, 2018 at 01:50:29PM +0100, Bertrand Jacquin wrote: > Hi, > > Please find attached a patch to fix some typos in the lua documentation. Applied, thanks Bertrand! Willy
