Re: [PATCH] support CRC32c for proxy protocol v2 (send, accept)

[Willy Tarreau]

Hi Manu,

On Tue, Mar 20, 2018 at 02:40:41PM +0100, Emmanuel Hocdet wrote:
> Thank you for taking the time to review.

OK patch now applied, thanks. Since you added a new hash algo, it could
be nice to create a "crc32c" converter to expose it to the configuration
as well. Could you please take a look at crc32 and do the same ?

> About PROXYv2, it's now possible to extract all known TLV from varnish via
> http://varnish-cache.org/docs/6.0/reference/vmod_proxy.generated.html#vmod-proxy-3
>  
> 

Oh, pretty cool. Thanks for the link!

Willy

Re: [PATCH] DOC: log: more than 2 log servers are allowed

[Willy Tarreau]

On Tue, Mar 20, 2018 at 11:30:27PM +0100, Cyril Bonté wrote:
> Since commit 0f99e3497, loggers are not limited to 2 instances anymore.

Applied, thanks Cyril.
Willy

[PATCH] DOC: log: more than 2 log servers are allowed

[Cyril Bonté]

Since commit 0f99e3497, loggers are not limited to 2 instances anymore.
---
 doc/configuration.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index d6f8b8d38..c2e44354d 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -818,7 +818,7 @@ group 
   See also "gid" and "user".
 
 log  [len ] [format ]  [max level [min 
level]]
-  Adds a global syslog server. Up to two global servers can be defined. They
+  Adds a global syslog server. Several global servers can be defined. They
   will receive logs for starts and exits, as well as all logs from proxies
   configured with "log global".
 
-- 
2.11.0

Re: Logging check response

[Aaron West]

Just another idea, you could utilize the external check feature to
script something that does the check and logs the output:

https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#option%20external-check


Aaron West

Loadbalancer.org Ltd.

www.loadbalancer.org

+1 888 867 9504 / +44 (0)330 380 1064
aa...@loadbalancer.org

LEAVE A REVIEW | DEPLOYMENT GUIDES | BLOG

Re: Logging check response

[PiBa-NL]

Hi Andreas,
Op 20-3-2018 om 12:21 schreef Andreas Mock:

Hi all,

I don't get the http-check (ssl) up and running.

Is there a way to log the content returned by a check run
so that I can get a hint on the problem?

haproxy 1.8

Best regards
Andreas



I'm not sure if logging of the content is possible easily..

But if you look at the status code on the stats page, and then take also 
into account that http-check does by default not send SNI nor a Host 
header. You should be able to mimic the requested check url with a CURL 
request. And can likely guess what is going on..


Hope it helps..

Regards,

PiBa-NL (Pieter)

actconn issue

[J. Kendzorra]

All,

we've recently upgraded to 1.8.4-1ppa1~trusty from Vincent Bernat's repo 
(running with nbproc=1).
Running a small-ish service on this instance, we were quite surprised to 
see 4294966046 current connections in "show info":


CurrConns: 4294966046

The access log at the same time logs "actconn" as -1249 which somewhat 
resembles the maximum of uint32.
Since we're pretty sure to not have seen similar behaviour in 1.7.9, we 
suspect the changes from 
http://git.haproxy.org/?p=haproxy-1.8.git;a=commit;h=8d8aa0d681c001891839588c0d51fa3cc9f652c7 
may have introduced this. In contrast to a more high volume deployment 
running 1.8.4 as well, we're terminating SSL connections on the instance 
that shows this behavior.

Please let me know if you need further details.

Regards,
J.

Re: strange cppcheck finding

[Willy Tarreau]

On Tue, Mar 20, 2018 at 06:45:08PM +0500,  ??? wrote:
> "UB" stands for undefined behaviour. that's the reason why cppcheck is
> unhappy.
> how do that properly - that's the question :)

The thing is that I'm not aware of any other way to safely detect integer
overflows, it's always done like this. In fact this undefined behaviour
on unsigned ints is defined per-architecture. I think you can safely turn
this one off as we do use integer wrapping at other places on purpose, and
we even build with -fwrapv to make it defined :-)

Cheers,
Willy

Re: strange cppcheck finding

[Илья Шипицин]

"UB" stands for undefined behaviour. that's the reason why cppcheck is
unhappy.
how do that properly - that's the question :)

2018-03-20 10:48 GMT+05:00 Willy Tarreau :

> On Mon, Mar 19, 2018 at 06:55:46PM +0500,  ??? wrote:
> > (it's master)
> >
> > is it in purpose ?
> >
> > [src/ssl_sock.c:1553]: (warning) Invalid test for overflow
> > 'msg+rec_len and
> > overflow is UB.
>
> The code is :
>
> rec_len = (msg[0] << 8) + msg[1];
> msg += 2;
> if (msg + rec_len > end || msg + rec_len < msg)
> return;
>
> It's indeed an overflow check which was placed on purpose. What does
> your tool propose as a better way to check for an overflow ? rec_len
> being a size_t, it's unsigned so the overflow check is fine and
> necessary in my opinion.
>
> Regards,
> Willy
>

Re: [PATCH] support CRC32c for proxy protocol v2 (send, accept)

[Emmanuel Hocdet]

Hi Willy,Le 19 mars 2018 à 12:38, Willy Tarreau  a écrit :Hi Manu,On Mon, Feb 05, 2018 at 05:10:05PM +0100, Emmanuel Hocdet wrote:Hi,Series of patches to support CRC32c checksum to proxy protocol v2 header(as describe in "doc/proxy-protocol.txt »). add hash_crc32c function. add « crc32c » option to proxy-v2-options. check crc32c checksum when CRC32C tlv is received.Thanks. I'm having a few comments here :From a62ca59e175631a0adbb9b8adf1d58492f6bce5a Mon Sep 17 00:00:00 2001From: Emmanuel Hocdet Date: Mon, 5 Feb 2018 15:26:43 +0100Subject: [PATCH 2/3] MINOR: proxy-v2-options: add crc32cThis patch add option crc32c (PP2_TYPE_CRC32C) to proxy protocol v2.It compute the checksum of proxy protocol v2 header as describe in"doc/proxy-protocol.txt".(...)diff --git a/include/types/server.h b/include/types/server.hindex 6d0566be2..e68b2036b 100644--- a/include/types/server.h+++ b/include/types/server.h@@ -195,7 +196,7 @@ struct server { 	enum obj_type obj_type; /* object type == OBJ_TYPE_SERVER */ 	enum srv_state next_state, cur_state;   /* server state among SRV_ST_* */ 	enum srv_admin next_admin, cur_admin;   /* server maintenance status : SRV_ADMF_* */-	unsigned char pp_opts;  /* proxy protocol options (SRV_PP_*) */+	unsigned int pp_opts;   /* proxy protocol options (SRV_PP_*) */ 	struct server *next;This is going to punch a 8-bit hole before the struct member. While notdramatic for such a struct, it's a good practice to place a comment beforeindicating that 8 bits are available there.I see, enum are packed.I don’t like hole, optinal patch 4 added to reorg a little :).diff --git a/src/connection.c b/src/connection.cindex 412fe3f41..833763e4c 100644--- a/src/connection.c+++ b/src/connection.c@@ -990,6 +991,7 @@ static int make_tlv(char *dest, int dest_len, char type, uint16_t length, const int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connection *remote) { 	const char pp2_signature[] = PP2_SIGNATURE;+    uint32_t *tlv_crc32c_p = NULL; 	int ret = 0;Spaces instead of tab above, lazy copy-paste ? :-)Or bad auto-indent mode :-)@@ -1037,6 +1039,14 @@ int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connec 		ret = PP2_HDR_LEN_UNSPEC; 	}+	if (srv->pp_opts & SRV_PP_V2_CRC32C) {+		uint32_t zero_crc32c = 0;+		if ((buf_len - ret) < sizeof(struct tlv))+			return 0;+		tlv_crc32c_p = (uint32_t *)((struct tlv *)[ret])->value;+		ret += make_tlv([ret], (buf_len - ret), PP2_TYPE_CRC32C, sizeof(zero_crc32c), (const char *)_crc32c);+	}+ 	if (conn_get_alpn(remote, , _len)) { 		if ((buf_len - ret) < sizeof(struct tlv)) 			return 0;@@ -1115,6 +1125,10 @@ int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connec 	hdr->len = htons((uint16_t)(ret - PP2_HEADER_LEN));+	if (tlv_crc32c_p) {+		*tlv_crc32c_p = htonl(hash_crc32c(buf, ret));How can you be sure the pointer is 32-bit aligned there ? I don't thinkwe have anything guaranteeing it. Please take a look at write_u32() innet_helper.h, it's provided exactly for this, and does exactly the samething on architectures supporting unaligned accesses (x86, armv7/8). Andin order to avoid any accidental misuse you can then declare tlv_crc32c_pas a void*.Indeed, I asked myself the question and I forgot to return to it.I like net_helper.h :)From 4a22d7ca1d4f711b0d777095e5ce793e04a93c78 Mon Sep 17 00:00:00 2001From: Emmanuel Hocdet Date: Mon, 5 Feb 2018 16:23:23 +0100Subject: [PATCH 3/3] MINOR: accept-proxy: support proxy protocol v2 CRC32c checksumWhen proxy protocol v2 CRC32c tlv is received, check it before acceptconnection (as describe in "doc/proxy-protocol.txt").--- src/connection.c | 7 +++ 1 file changed, 7 insertions(+)diff --git a/src/connection.c b/src/connection.cindex 833763e4c..8fd3209d5 100644--- a/src/connection.c+++ b/src/connection.c@@ -612,6 +612,13 @@ int conn_recv_proxy(struct connection *conn, int flag) tlv_offset += tlv_len + TLV_HEADER_SIZE; switch (tlv_packet->type) {+case PP2_TYPE_CRC32C: {+	uint32_t n_crc32c = ntohl(*(uint32_t *)tlv_packet->value);+	*(uint32_t *)tlv_packet->value = 0;Same here, please see read_u32() and write_u32().Otherwise looks good.Thanks!WillyThank you for taking the time to review.About PROXYv2, it’s now possible to extract all known TLV from varnish viahttp://varnish-cache.org/docs/6.0/reference/vmod_proxy.generated.html#vmod-proxy-3++Manu

0001-MINOR-hash-add-new-function-hash_crc32c.patch
Description: Binary data


0002-MINOR-proxy-v2-options-add-crc32c.patch
Description: Binary data


0003-MINOR-accept-proxy-support-proxy-protocol-v2-CRC32c-.patch
Description: Binary data


0004-REORG-compact-struct-server.patch
Description: Binary data

RE: BUG/MINOR: limiting the value of "inter" parameter for Health check

[Nikhil Kapoor]

Hi Willy,

-Original Message-
From: Willy Tarreau [mailto:w...@1wt.eu] 
Sent: Thursday, March 8, 2018 10:18 PM
To: Jonathan Matthews 
Cc: Nikhil Kapoor ; haproxy@formilux.org
Subject: Re: BUG/MINOR: limiting the value of "inter" parameter for Health check

Hi Jonathan,

On Wed, Mar 07, 2018 at 09:38:00PM +, Jonathan Matthews wrote:
> On Wed, 7 Mar 2018 at 09:50, Nikhil Kapoor  
> wrote
> 
> > As currently, no parsing error is displayed when larger value is 
> > given to "inter" parameter in config file.
> >
> > After applying this patch the maximum value of "inter" is set to 24h (i.e.
> > 8640 ms).
> >
> 
> I regret to inform you, with no little embarrassment, that some years 
> ago I designed a system which relied upon this parameter being set 
> higher than 24 hours.
> 
> I was not proud of this system, and it served absolutely minimal 
> quantities of traffic ... but it was a valid setup.
> 
> What's the rationale for having *any* maximum value here - saving 
> folks from unintentional misconfigurations, or something else?

>I agree with you here. In fact what we should do is instead strengthen the 
>timeout parser to emit errors when the parsed number >overflows. The timer 
>wraps around 49.7 days with a sliding window of half of it allowing
>24.85 usable days to always be possible. That would be preferable and it 
>wouldn't set any arbitrary had limits.


After analyzing the code on the given problem, I found that the check is based 
on INT_MAX value and will only be executed when "stick-table" option is 
included in haproxy.cfg file.

However for a user, if "stick-table" option is not used in haproxy.cfg file 
then there is no other limit(24.85 days) in the haproxy code.
I believe the below check should not be based on "stick-table" option and 
should run every time when haproxy is started. 
Please let me know if there is any gap in understanding and we can perform the 
code change as mentioned.

File-> cfgparse.c 
-->  if (val > INT_MAX ) {
ha_alert("parsing [%s:%d] : Expire 
value [%u]ms exceeds maxmimum value of 24.85 days.\n",
 file, linenum, val);
err_code |= ERR_ALERT | ERR_FATAL;
goto out;

Regards 
Nikhil Kapoor 

Logging check response

[Andreas Mock]

Hi all,

I don't get the http-check (ssl) up and running.

Is there a way to log the content returned by a check run
so that I can get a hint on the problem?

haproxy 1.8

Best regards
Andreas

Re: Backporting of minor bug in haproxy 1.8/1.8.4 version

[Willy Tarreau]

Hi,

On Tue, Mar 20, 2018 at 07:27:01AM +, Nikhil Kapoor wrote:
> Hi
> 
> I was going through the known bugs of 1.7.9 version. I found a minor bug 
> "BUG/MINOR: lua: Fix default value for pattern in 
> Socket.receive"
>  which is not backported in 1.8/1.8.4 versions. So, are there any plans to 
> backport it or any other reason for not backporting it?
> 
> Link: http://git.haproxy.org/?p=haproxy-1.7.git;a=commitdiff;h=07fe2d9

Yes it is, look, it's the last "cherry-picked" line in the commit message,
the 1.7 commit is itself a backport of the 1.8 one, which is commit
0c9d9a9621 there, it was merged between 1.8.3 and 1.8.4.

Regards,
Willy

Backporting of minor bug in haproxy 1.8/1.8.4 version

[Nikhil Kapoor]

Hi

I was going through the known bugs of 1.7.9 version. I found a minor bug 
"BUG/MINOR: lua: Fix default value for pattern in 
Socket.receive"
 which is not backported in 1.8/1.8.4 versions. So, are there any plans to 
backport it or any other reason for not backporting it?

Link: http://git.haproxy.org/?p=haproxy-1.7.git;a=commitdiff;h=07fe2d9

Regards
Nikhil Kapoor