Re: Tests timeout on my ARM64 test VM
once in a while I saw "reg-tests/compression/lua_validation.vtc" failed. say 1 times out of 20. it is slow and racy by nature. also, seems 3 weeks of Linaro cloud are passed, 1 week is left. I'll do more tries ср, 11 мар. 2020 г. в 19:14, Martin Grigorov : > > > On Wed, Mar 11, 2020 at 3:06 PM Илья Шипицин wrote: > >> I will a look during next weekend >> > > Thank you, Илья! > > >> >> BTW, I've managed to get Linaro VM :) >> > > Congrats! :-) > > >> >> On Wed, Mar 11, 2020, 5:40 PM Martin Grigorov >> wrote: >> >>> Hi, >>> >>> On Mon, Mar 9, 2020 at 10:22 PM Martin Grigorov < >>> martin.grigo...@gmail.com> wrote: >>> Hi, I am not sure what have changed on my test ARM64 VM but the reg tests started timing out. Everything is fine on my dev machine (x86_64) and at Travis ( https://travis-ci.com/haproxy/haproxy). I don't think it is ARM64 related. Most probably some OS setting or something. I've rebooted the system just to make sure it is not some busy port or opened file descriptor but it still fails the same way. Does someone see in the attached logs what could be the problem? >>> >>> Anyone can help me here ? >>> >>> Martin >>> >>> Thank you! Martin >>>
Re: [PATCH[ special purpose weekly CI (spellcheck)
сб, 14 мар. 2020 г. в 01:06, Willy Tarreau : > On Fri, Mar 13, 2020 at 03:24:15PM +0100, William Lallemand wrote: > > If you grep on BUILD: in the git log, this keyword does not mean > > anything anymore. And this is confusing in my opinion. We could > > introduce "CI: " instead. > > Good idea. I can adjust it. > > Ilya, just a question, what will be the impact of this ? Will we > all be spammed or anything or are you the one volunteering to stand > in front of the cannon ? :-) > yes, I will take care of it. > > Willy >
Re: s390x and HAProxy?
On Fri, Mar 13, 2020 at 04:50:12PM +0500, ??? wrote: > ??, 13 ???. 2020 ?. ? 16:48, Aleksandar Lazic : > > > Mar 13, 2020 12:11:16 PM ??? : > > > > > initial motivation was that article > > > > > > https://docs.travis-ci.com/user/multi-cpu-architectures > > > > Thanks for the link. > > > > > travis allows to run builds on various archs, why not to test ? > > > > Full Agreeing. ;-) > > > > Would be interesting if anyone use it on Host or power. > > > > there were issues related to haproxy on openwrt. > I am not surprised if somebody runs on s390x It's very possible. You often find annoyingly rigit applications in such environments, for various legacy reasons (i.e. old application ported to linux on the same machine but rewritten in Java for example). And given that a lot of haproxy users use it as a Swiss Army Knife, it's quite understandable that some place it there to adapt the application to its legacy environment. Regarding other archs like PPC64, I've been aware of some clouds running on Power8 and using it several years ago. There was even a live deployment demo of Ubuntu on Power8 run by Mark Shuttleworth during an IBM conference showing how fast they could deploy an application on Ubuntu on this setup and they installed haproxy with the application. So it might not be as rare as one can think. Anyway the main reason for testing other archs is to stress unusual code parts (e.g. missing double-CAS but having threads etc), and detecting if we did crap on an alignment issue for example. Willy
Re: "warning" on reg-tests
On Fri, Mar 13, 2020 at 11:18:42PM +0500, ??? wrote: > Hello, > > there's always noise like that > > > *** h1 debug|[WARNING] 072/163523 (6090) : config : log format > ignored for proxy 'testme' since it has no log address. > *** h1 debug|[WARNING] 072/163523 (6090) : config : log format > ignored for frontend 'test_abns' since it has no log address. > > > any special meaning to have logs misconfigured for reg-tests ? or we > can reduce that noise ? I observed a few of these that quite bothered me as well, but it's difficult to spot the ones emitting errors. I remember we noted for the todo list to add an option to turn warnings into errors, and we still hadn't done it. It could be an excellent reason for doing it now, we'd always start the tests with the option set and we'd instantly know if there are warnings in the config. Probably that a temporary solution could be to run a test with --keep-logs and grep for WARNING in each of them :-/ Willy
Re: [PATCH[ special purpose weekly CI (spellcheck)
On Fri, Mar 13, 2020 at 03:24:15PM +0100, William Lallemand wrote: > If you grep on BUILD: in the git log, this keyword does not mean > anything anymore. And this is confusing in my opinion. We could > introduce "CI: " instead. Good idea. I can adjust it. Ilya, just a question, what will be the impact of this ? Will we all be spammed or anything or are you the one volunteering to stand in front of the cannon ? :-) Willy
Re: Tests timeout on my ARM64 test VM
Hi Martin, On Fri, Mar 13, 2020 at 12:35:12PM +0200, Martin Grigorov wrote: > Hi , > > Suddenly today the build is again green here! > I didn't make any changes to my testing setup. > It must be something on the OS level but I wasn't able to figure out what > makes the HAProxy tests timeout in the last several days. We've had issues with the abns test on other platforms in the past, namely s390x and ppc64le. It used to occasionally break on x86_64 as well but far less frequently. It was affected by two bugs that were solved yesterday after a few days of investigation and testing. We've seen yet another failure again on ppc64 while it was expected not to fail, so I'd be careful before claiming victory. However the abns test is extremely time sensitive and uses short delays around 15ms to try to trigger the issue, and in a VM it is possible to see this happen from time to time due to noisy neighbors. That's why I'm staying extremely prudent on the verdict. The PPC64 machine I tested on is provided by Minicloud and is a VM running on a real CPU, so it's much less affected by timing issues. I've run the test several hundreds of times in a row and couldn't make it fail anymore. So don't worry too much if it appeared and disappeared. The change that emphasized it was the increase in default maxconn (304e17eb8), apparently just due to a slightly longer startup time! And the ones expected to have fixed it are between bdb00c5d and 4b3f27b included. Note that I didn't manage to make it fail on arm64 (real machine, SolidRun's Macchiatobin). Hoping this clarifies the situation. Regards, Willy
"warning" on reg-tests
Hello, there's always noise like that *** h1 debug|[WARNING] 072/163523 (6090) : config : log format ignored for proxy 'testme' since it has no log address. *** h1 debug|[WARNING] 072/163523 (6090) : config : log format ignored for frontend 'test_abns' since it has no log address. any special meaning to have logs misconfigured for reg-tests ? or we can reduce that noise ? Cheers, Ilya Shipitcin
Re: [PATCH] BUILD: Avoid warning about ignoring write()'s return value in BUG_ON()
On Fri, Mar 13, 2020 at 05:41:14PM +0100, Tim Düsterhus wrote: > Willy, > > Am 13.03.20 um 17:34 schrieb Willy Tarreau: > > Indeed, just found it in my queue. However we usually use it > > differently, with the function instead of the variable. Do you > > mind if I adapt it ? > > > > I attempted to use the function, but it didn't compile. I guess because > of a circular dependency. If it's a small obvious change feel free to > adapt the patch. If it requires larger changes please fix it yourself > and ignore my patch. OK, I'll check when I have a few minutes. I think that since we now have this ALREADY_CHECKED() macro that's used to prevent gcc from seeing null-derefs where they can't happen, we can use it as well to pretend we've consumed the result from such occasional syscalls and even remove the shut_* macro. Cheers, Willy
Re: [PATCH] BUILD: Avoid warning about ignoring write()'s return value in BUG_ON()
Willy, Am 13.03.20 um 17:34 schrieb Willy Tarreau: > Indeed, just found it in my queue. However we usually use it > differently, with the function instead of the variable. Do you > mind if I adapt it ? > I attempted to use the function, but it didn't compile. I guess because of a circular dependency. If it's a small obvious change feel free to adapt the patch. If it requires larger changes please fix it yourself and ignore my patch. Best regards Tim Düsterhus
Re: [PATCH] BUILD: Avoid warning about ignoring write()'s return value in BUG_ON()
On Fri, Mar 13, 2020 at 12:23:26PM +0100, Tim Düsterhus wrote: > Willy, > > Am 09.03.20 um 17:05 schrieb Tim Duesterhus: > > gcc complains (non-rightfully): > > > >> include/common/buf.h: In function 'br_head_pick': > >> include/common/debug.h:62:4: warning: ignoring return value of 'write', > >> declared with attribute warn_unused_result [-Wunused-result] > >> (void)write(2, msg, strlen(msg)); \ > >> ^ > >> include/common/debug.h:57:35: note: in expansion of macro '__BUG_ON' > >> #define _BUG_ON(cond, file, line) __BUG_ON(cond, file, line) > >>^ > >> include/common/debug.h:56:22: note: in expansion of macro '_BUG_ON' > >> #define BUG_ON(cond) _BUG_ON(cond, __FILE__, __LINE__) > >> ^ > >> include/common/buf.h:1011:2: note: in expansion of macro 'BUG_ON' > >> BUG_ON(r->area != BUF_RING.area); > >> ^ > > While checking my list of outgoing patches I noticed that this one > wasn't acknowledged yet. It will become important with: > https://github.com/haproxy/haproxy/issues/546 Indeed, just found it in my queue. However we usually use it differently, with the function instead of the variable. Do you mind if I adapt it ? Willy
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Willy, Am 13.03.20 um 14:18 schrieb Willy Tarreau: > I was willing to take it as is but since I had a comment for the last > one and I know that many times when reworking a patch we spot things > we wish has been done differently in the previous ones, I let you > decide what suits you best :-) If you want me to take it now I can. In fact I added a blank line in that patch. The v2-series is good from my side. I don't plan any more changes. If you are happy as well then please take it. Best regards Tim Düsterhus
Re: [PATCH[ special purpose weekly CI (spellcheck)
On Fri, Mar 13, 2020 at 07:05:15PM +0500, Илья Шипицин wrote: > Willy, can we apply this? > > I scheduled it on tuesday every week. > > вт, 10 мар. 2020 г. в 12:12, Илья Шипицин : > > > Hello, > > > > I implemented spell check CI. > > it is a bit noisy, however, I hope we will polish it soon. > > > > Cheers, > > Ilya Shipitcin > > Can we introduce a new prefix in the commit subject? Because I keep seeing "BUILD:" for things that are specific to the CI, sometimes its BUILD: : sometimes it's only "BUILD:". If you grep on BUILD: in the git log, this keyword does not mean anything anymore. And this is confusing in my opinion. We could introduce "CI: " instead. -- William Lallemand
Re: [PATCH[ special purpose weekly CI (spellcheck)
Willy, can we apply this? I scheduled it on tuesday every week. вт, 10 мар. 2020 г. в 12:12, Илья Шипицин : > Hello, > > I implemented spell check CI. > it is a bit noisy, however, I hope we will polish it soon. > > Cheers, > Ilya Shipitcin >
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
On Fri, Mar 13, 2020 at 11:57:14AM +0100, Tim Düsterhus wrote: > Willy, > > Am 13.03.20 um 08:44 schrieb Willy Tarreau: > >> I've added the new `struct connection` member at the end. Please check > >> whether you think that is the appropriate place for it or if it should > >> be moved somewhere else because of holes or caches. > > > > I'm seeing that the struct connection has become huge (160 bytes now) > > I suspected something like that, that's why I specifically mentioned > that piece. > > > But for now I'm OK with taking your patch above. > > > > I'm not seeing anything in git, yet. > > Is the patch okay as-is and you are just looking into the `struct > connection` refactoring before actually taking it or would you like me > to make any changes to it? I was willing to take it as is but since I had a comment for the last one and I know that many times when reworking a patch we spot things we wish has been done differently in the previous ones, I let you decide what suits you best :-) If you want me to take it now I can. Willy
Re: s390x and HAProxy?
пт, 13 мар. 2020 г. в 16:48, Aleksandar Lazic : > Mar 13, 2020 12:11:16 PM Илья Шипицин : > > > initial motivation was that article > > > > https://docs.travis-ci.com/user/multi-cpu-architectures > > Thanks for the link. > > > travis allows to run builds on various archs, why not to test ? > > Full Agreeing. ;-) > > Would be interesting if anyone use it on Host or power. > there were issues related to haproxy on openwrt. I am not surprised if somebody runs on s390x > > > пт, 13 мар. 2020 г. в 16:07, Aleksandar Lazic < al-hapr...@none.at >: > > > > > > > Hi. > > > > > > I'm wondering that this target is tested. > > > > http://git.haproxy.org/?p=haproxy.git;a=commitdiff;h=d726386421dcd184ca2518d17332f82e9cd79f2d > > > > > > Are there really user which runs HAProxy on Host? 8-O > > > How perform HAProxy on that platform? > > > > > > Regards > > > Aleks > > >
Re: s390x and HAProxy?
Mar 13, 2020 12:11:16 PM Илья Шипицин : > initial motivation was that article > > https://docs.travis-ci.com/user/multi-cpu-architectures Thanks for the link. > travis allows to run builds on various archs, why not to test ? Full Agreeing. ;-) Would be interesting if anyone use it on Host or power. > пт, 13 мар. 2020 г. в 16:07, Aleksandar Lazic < al-hapr...@none.at >: > > > > Hi. > > > > I'm wondering that this target is tested. > > http://git.haproxy.org/?p=haproxy.git;a=commitdiff;h=d726386421dcd184ca2518d17332f82e9cd79f2d > > > > Are there really user which runs HAProxy on Host? 8-O > > How perform HAProxy on that platform? > > > > Regards > > Aleks
[PATCH v2 4/4] CLEANUP: connection: Add blank line after declarations in PP handling
Willy,
squash this one if you like.
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
This adds the missing blank lines in `make_proxy_line_v2` and
`conn_recv_proxy`. It also adjusts the type of the temporary variable
used for the return value of `recv` to be `ssize_t` instead of `int`.
---
src/connection.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/connection.c b/src/connection.c
index ba0e0fcd1..dd20cdcfd 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -802,7 +802,8 @@ int conn_recv_proxy(struct connection *conn, int flag)
* fail.
*/
while (1) {
- int len2 = recv(conn->handle.fd, trash.area, trash.data, 0);
+ ssize_t len2 = recv(conn->handle.fd, trash.area, trash.data, 0);
+
if (len2 < 0 && errno == EINTR)
continue;
if (len2 != trash.data)
@@ -1437,6 +1438,7 @@ int make_proxy_line_v2(char *buf, int buf_len, struct
server *srv, struct connec
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 = (void *)((struct tlv *)[ret])->value;
@@ -1486,6 +1488,7 @@ int make_proxy_line_v2(char *buf, int buf_len, struct
server *srv, struct connec
if (srv->pp_opts & SRV_PP_V2_SSL) {
struct tlv_ssl *tlv;
int ssl_tlv_len = 0;
+
if ((buf_len - ret) < sizeof(struct tlv_ssl))
return 0;
tlv = (struct tlv_ssl *)[ret];
--
2.25.1
[PATCH v2 3/4] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
Willy, here I removed the BUG_ON() and added the blank lines you requested. > > + BUG_ON(sess != conn->owner); /* XXX is this correct? */ > > I'm pretty sure this one will break from time to time, maybe during > retries or when reusing an idle connection or something. Better drop > it. You don't really care who's the owner of the connection you're > using anyway. In case of multiplexing it could be anyone because > the connection will plugged onto by several streams before it's > finally connected and once it's OK and the sending code starts, you > have no way to tell whether the selected stream is the one that asked > for the connection first. > Then it served the purpose well, you've taken a look at my assumptions to check whether `strm_sess()` is the correct thing to use :-) > So modulo the very few minor stuff above I'm overall OK without your > patch, just let me know if you have any question. No, the review was crystal clear. Best regards Tim Düsterhus Apply with `git am --scissors` to automatically cut the commit message. -- >8 -- This patch adds the `unique-id` option to `proxy-v2-options`. If this option is set a unique ID will be generated based on the `unique-id-format` while sending the proxy protocol v2 header and stored as the unique id for the first stream of the connection. This feature is meant to be used in `tcp` mode. It works on HTTP mode, but might result in inconsistent unique IDs for the first request on a keep-alive connection, because the unique ID for the first stream is generated earlier than the others. Now that we can send unique IDs in `tcp` mode the `%ID` log variable is made available in TCP mode. --- doc/configuration.txt | 24 +++ include/proto/connection.h| 4 +- include/types/server.h| 1 + .../proxy_protocol_send_unique_id.vtc | 42 +++ src/connection.c | 20 +++-- src/log.c | 2 +- src/server.c | 2 + src/stream_interface.c| 11 +++-- 8 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 reg-tests/connection/proxy_protocol_send_unique_id.vtc diff --git a/doc/configuration.txt b/doc/configuration.txt index 57f777a78..cc5fbf2d0 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -12657,13 +12657,23 @@ send-proxy-v2 this section and send-proxy" option of the "bind" keyword. proxy-v2-options [,]* - The "proxy-v2-options" parameter add option to send in PROXY protocol version - 2 when "send-proxy-v2" is used. Options available are "ssl" (see also - send-proxy-v2-ssl), "cert-cn" (see also "send-proxy-v2-ssl-cn"), "ssl-cipher": - name of the used cipher, "cert-sig": signature algorithm of the used - certificate, "cert-key": key algorithm of the used certificate), "authority": - host name value passed by the client (only sni from a tls connection is - supported), "crc32c": checksum of the proxy protocol v2 header. + The "proxy-v2-options" parameter add options to send in PROXY protocol + version 2 when "send-proxy-v2" is used. Options available are: + + - ssl : See also "send-proxy-v2-ssl". + - cert-cn : See also "send-proxy-v2-ssl-cn". + - ssl-cipher: Name of the used cipher. + - cert-sig : Signature algorithm of the used certificate. + - cert-key : Key algorithm of the used certificate + - authority : Host name value passed by the client (only SNI from a TLS +connection is supported). + - crc32c: Checksum of the PROXYv2 header. + - unique-id : Send a unique ID generated using the frontend's +"unique-id-format" within the PROXYv2 header. +This unique-id is primarily meant for "mode tcp". It can +lead to unexpected results in "mode http", because the +generated unique ID is also used for the first HTTP request +within a Keep-Alive connection. send-proxy-v2-ssl The "send-proxy-v2-ssl" parameter enforces use of the PROXY protocol version diff --git a/include/proto/connection.h b/include/proto/connection.h index 9b8eb8ad3..ecc03de8a 100644 --- a/include/proto/connection.h +++ b/include/proto/connection.h @@ -47,9 +47,9 @@ int conn_fd_check(struct connection *conn); /* receive a PROXY protocol header over a connection */ int conn_recv_proxy(struct connection *conn, int flag); -int make_proxy_line(char *buf, int buf_len, struct server *srv, struct connection *remote); +int make_proxy_line(char *buf, int buf_len, struct server *srv, struct connection *remote, struct stream *strm); int make_proxy_line_v1(char *buf, int buf_len, struct sockaddr_storage *src, struct sockaddr_storage *dst); -int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connection *remote); +int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct
[PATCH v2 1/4] DOC: proxy_protocol: Reserve TLV type 0x05 as PP2_TYPE_UNIQUE_ID
Willy,
this one has not been modified compared to v1.
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
This reserves and defines TLV type 0x05.
---
doc/proxy-protocol.txt | 20
include/types/connection.h | 1 +
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/doc/proxy-protocol.txt b/doc/proxy-protocol.txt
index 26f86a345..fc1ca4a04 100644
--- a/doc/proxy-protocol.txt
+++ b/doc/proxy-protocol.txt
@@ -1,4 +1,4 @@
-2017/03/10Willy Tarreau
+2020/03/05Willy Tarreau
HAProxy Technologies
The PROXY protocol
Versions 1 & 2
@@ -27,6 +27,7 @@ Revision history
reserved TLV type ranges, added TLV documentation, clarified
string encoding. With contributions from Andriy Palamarchuk
(Amazon.com).
+ 2020/03/05 - added the unique ID TLV type (Tim Düsterhus)
1. Background
@@ -538,6 +539,7 @@ The following types have already been registered for the
field :
#define PP2_TYPE_AUTHORITY 0x02
#define PP2_TYPE_CRC32C 0x03
#define PP2_TYPE_NOOP 0x04
+#define PP2_TYPE_UNIQUE_ID 0x05
#define PP2_TYPE_SSL0x20
#define PP2_SUBTYPE_SSL_VERSION 0x21
#define PP2_SUBTYPE_SSL_CN 0x22
@@ -602,7 +604,17 @@ bytes. Can be used for data padding or alignment. Note
that it can be used
to align only by 3 or more bytes because a TLV can not be smaller than that.
-2.2.5. The PP2_TYPE_SSL type and subtypes
+2.2.5. PP2_TYPE_UNIQUE_ID
+
+The value of the type PP2_TYPE_UNIQUE_ID is an opaque byte sequence of up to
+128 bytes generated by the upstream proxy that uniquely identifies the
+connection.
+
+The unique ID can be used to easily correlate connections across multiple
+layers of proxies, without needing to look up IP addresses and port numbers.
+
+
+2.2.6. The PP2_TYPE_SSL type and subtypes
For the type PP2_TYPE_SSL, the value is itself a defined like this :
@@ -654,13 +666,13 @@ In all cases, the string representation (in UTF8) of the
Common Name field
using the TLV format and the type PP2_SUBTYPE_SSL_CN. E.g. "example.com".
-2.2.6. The PP2_TYPE_NETNS type
+2.2.7. The PP2_TYPE_NETNS type
The type PP2_TYPE_NETNS defines the value as the US-ASCII string representation
of the namespace's name.
-2.2.7. Reserved type ranges
+2.2.8. Reserved type ranges
The following range of 16 type values is reserved for application-specific
data and will be never used by the PROXY Protocol. If you need more values
diff --git a/include/types/connection.h b/include/types/connection.h
index bfd6547ee..0c2d960b9 100644
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -558,6 +558,7 @@ struct proxy_hdr_v2 {
#define PP2_TYPE_AUTHORITY 0x02
#define PP2_TYPE_CRC32C 0x03
#define PP2_TYPE_NOOP 0x04
+#define PP2_TYPE_UNIQUE_ID 0x05
#define PP2_TYPE_SSL0x20
#define PP2_SUBTYPE_SSL_VERSION 0x21
#define PP2_SUBTYPE_SSL_CN 0x22
--
2.25.1
[PATCH v2 2/4] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Willy,
In this one I added a blank line after declaration that you missed during
review.
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
This patch reads a proxy protocol v2 provided unique ID and makes it
available using the `fc_pp_unique_id` fetch.
---
doc/configuration.txt | 4 +++
include/proto/connection.h| 5 +++
include/types/connection.h| 1 +
reg-tests/stream/unique-id-from-proxy.vtc | 38
src/connection.c | 42 +++
5 files changed, 90 insertions(+)
create mode 100644 reg-tests/stream/unique-id-from-proxy.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 33425a6c6..57f777a78 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -15131,6 +15131,10 @@ fc_pp_authority : string
Returns the authority TLV sent by the client in the PROXY protocol header,
if any.
+fc_pp_unique_id : string
+ Returns the unique ID TLV sent by the client in the PROXY protocol header,
+ if any.
+
fc_rcvd_proxy : boolean
Returns true if the client initiated the connection with a PROXY protocol
header.
diff --git a/include/proto/connection.h b/include/proto/connection.h
index fb264d2b5..9b8eb8ad3 100644
--- a/include/proto/connection.h
+++ b/include/proto/connection.h
@@ -325,6 +325,7 @@ static inline void conn_init(struct connection *conn)
conn->src = NULL;
conn->dst = NULL;
conn->proxy_authority = NULL;
+ conn->proxy_unique_id = IST_NULL;
}
/* sets as the connection's owner */
@@ -458,6 +459,10 @@ static inline void conn_free(struct connection *conn)
pool_free(pool_head_authority, conn->proxy_authority);
conn->proxy_authority = NULL;
}
+ if (isttest(conn->proxy_unique_id)) {
+ pool_free(pool_head_uniqueid, conn->proxy_unique_id.ptr);
+ conn->proxy_unique_id = IST_NULL;
+ }
/* By convention we always place a NULL where the ctx points to if the
* mux is null. It may have been used to store the connection as a
diff --git a/include/types/connection.h b/include/types/connection.h
index 0c2d960b9..30cb895ff 100644
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -469,6 +469,7 @@ struct connection {
char *proxy_authority;/* Value of authority TLV received via
PROXYv2 */
unsigned int idle_time; /* Time the connection was
added to the idle list, or 0 if not in the idle list */
uint8_t proxy_authority_len; /* Length of authority TLV received via
PROXYv2 */
+ struct ist proxy_unique_id; /* Value of the unique ID TLV received via
PROXYv2 */
};
/* PROTO token registration */
diff --git a/reg-tests/stream/unique-id-from-proxy.vtc
b/reg-tests/stream/unique-id-from-proxy.vtc
new file mode 100644
index 0..81ee3dea9
--- /dev/null
+++ b/reg-tests/stream/unique-id-from-proxy.vtc
@@ -0,0 +1,38 @@
+varnishtest "Check that we are able to read a unique-id from PROXYv2"
+
+#REQUIRE_VERSION=2.2
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+defaults
+mode http
+timeout connect 1s
+timeout client 1s
+timeout server 1s
+
+frontend echo
+bind "fd@${fe1}" accept-proxy
+http-after-response set-header echo %[fc_pp_unique_id,hex]
+http-request return status 200
+} -start
+
+client c1 -connect ${h1_fe1_sock} {
+# PROXY v2 signature
+sendhex "0d 0a 0d 0a 00 0d 0a 51 55 49 54 0a"
+# version + PROXY
+sendhex "21"
+# TCP4
+sendhex "11"
+# length of the address (12) + length of the TLV (8)
+sendhex "00 14"
+# 127.0.0.1 42 127.0.0.1 1337
+sendhex "7F 00 00 01 7F 00 00 01 00 2A 05 39"
+# PP2_TYPE_UNIQUE_ID + length of the value + "12345"
+sendhex "05 00 05 31 32 33 34 35"
+
+txreq -url "/"
+rxresp
+expect resp.status == 200
+expect resp.http.echo == "3132333435"
+} -run
diff --git a/src/connection.c b/src/connection.c
index 4e3a92f0c..728c0ec39 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -755,6 +755,22 @@ int conn_recv_proxy(struct connection *conn, int flag)
conn->proxy_authority_len = tlv_len;
break;
}
+ case PP2_TYPE_UNIQUE_ID: {
+ const struct ist tlv = ist2((const char
*)tlv_packet->value, tlv_len);
+
+ if (tlv.len > UNIQUEID_LEN)
+ goto bad_header;
+ conn->proxy_unique_id.ptr =
pool_alloc(pool_head_uniqueid);
+ if (!isttest(conn->proxy_unique_id))
+ goto fail;
+ if (istcpy(>proxy_unique_id, tlv,
UNIQUEID_LEN) < 0) {
+
Re: [PATCH] BUILD: Avoid warning about ignoring write()'s return value in BUG_ON()
Willy, Am 09.03.20 um 17:05 schrieb Tim Duesterhus: > gcc complains (non-rightfully): > >> include/common/buf.h: In function ‘br_head_pick’: >> include/common/debug.h:62:4: warning: ignoring return value of ‘write’, >> declared with attribute warn_unused_result [-Wunused-result] >> (void)write(2, msg, strlen(msg)); \ >> ^ >> include/common/debug.h:57:35: note: in expansion of macro ‘__BUG_ON’ >> #define _BUG_ON(cond, file, line) __BUG_ON(cond, file, line) >>^ >> include/common/debug.h:56:22: note: in expansion of macro ‘_BUG_ON’ >> #define BUG_ON(cond) _BUG_ON(cond, __FILE__, __LINE__) >> ^ >> include/common/buf.h:1011:2: note: in expansion of macro ‘BUG_ON’ >> BUG_ON(r->area != BUF_RING.area); >> ^ While checking my list of outgoing patches I noticed that this one wasn't acknowledged yet. It will become important with: https://github.com/haproxy/haproxy/issues/546 Best regards Tim Düsterhus
Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
Willy, Ilya, Am 13.03.20 um 12:06 schrieb Willy Tarreau: >> I just realized that BUG_ON is not active by default (should we add >> DEBUG_STRICT=1 to CI?). The check in there does not even compile (facepalm). > > Yes, definitely a good idea. I've filed an issue for the CI expert Ilya to look into that: https://github.com/haproxy/haproxy/issues/546 Best regards Tim Düsterhus
Re: s390x and HAProxy?
initial motivation was that article https://docs.travis-ci.com/user/multi-cpu-architectures travis allows to run builds on various archs, why not to test ? пт, 13 мар. 2020 г. в 16:07, Aleksandar Lazic : > Hi. > > I'm wondering that this target is tested. > > http://git.haproxy.org/?p=haproxy.git;a=commitdiff;h=d726386421dcd184ca2518d17332f82e9cd79f2d > > Are there really user which runs HAProxy on Host? 8-O > How perform HAProxy on that platform? > > Regards > Aleks > > > >
Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
On Fri, Mar 06, 2020 at 01:15:40PM +0100, Tim Duesterhus wrote:
> @@ -1466,6 +1466,20 @@ int make_proxy_line_v2(char *buf, int buf_len, struct
> server *srv, struct connec
> ret += make_tlv([ret], (buf_len - ret),
> PP2_TYPE_AUTHORITY, value_len, value);
> }
> }
> +
> + if (srv->pp_opts & SRV_PP_V2_UNIQUE_ID) {
> + struct session* sess = strm_sess(strm);
> + BUG_ON(sess != conn->owner); /* XXX is this correct? */
I'm pretty sure this one will break from time to time, maybe during
retries or when reusing an idle connection or something. Better drop
it. You don't really care who's the owner of the connection you're
using anyway. In case of multiplexing it could be anyone because
the connection will plugged onto by several streams before it's
finally connected and once it's OK and the sending code starts, you
have no way to tell whether the selected stream is the one that asked
for the connection first.
> + struct ist unique_id = stream_generate_unique_id(strm,
> >fe->format_unique_id);
> + value = unique_id.ptr;
> + value_len = unique_id.len;
Also above, please do two things:
- always leave a blank line between declarations and statements, that
allow to quickly spot where variables are declared
- and as such please don't place BUG_ON() in the declarations since
it's code :-)
> + if (value_len >= 0) {
> + if ((buf_len - ret) < sizeof(struct tlv))
> + return 0;
> + ret += make_tlv([ret], (buf_len - ret),
> PP2_TYPE_UNIQUE_ID, value_len, value);
> + }
> + }
>
> #ifdef USE_OPENSSL
> if (srv->pp_opts & SRV_PP_V2_SSL) {
> diff --git a/src/log.c b/src/log.c
> index 8f502ac7e..2cac07486 100644
> --- a/src/log.c
> +++ b/src/log.c
> @@ -137,7 +137,7 @@ static const struct logformat_type logformat_keywords[] =
> {
> { "CC", LOG_FMT_CCLIENT, PR_MODE_HTTP, LW_REQHDR, NULL }, /* client
> cookie */
> { "CS", LOG_FMT_CSERVER, PR_MODE_HTTP, LW_RSPHDR, NULL }, /* server
> cookie */
> { "H", LOG_FMT_HOSTNAME, PR_MODE_TCP, LW_INIT, NULL }, /* Hostname */
> - { "ID", LOG_FMT_UNIQUEID, PR_MODE_HTTP, LW_BYTES, NULL }, /* Unique ID
> */
> + { "ID", LOG_FMT_UNIQUEID, PR_MODE_TCP, LW_BYTES, NULL }, /* Unique ID */
> { "ST", LOG_FMT_STATUS, PR_MODE_TCP, LW_RESP, NULL }, /* status code
> */
> { "T", LOG_FMT_DATEGMT, PR_MODE_TCP, LW_INIT, NULL }, /* date GMT */
> { "Ta", LOG_FMT_Ta, PR_MODE_HTTP, LW_BYTES, NULL }, /* Time active
> (tr to end) */
> diff --git a/src/server.c b/src/server.c
> index 965570222..908439d00 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -656,6 +656,8 @@ static int srv_parse_proxy_v2_options(char **args, int
> *cur_arg,
> newsrv->pp_opts |= SRV_PP_V2_AUTHORITY;
> } else if (!strcmp(p, "crc32c")) {
> newsrv->pp_opts |= SRV_PP_V2_CRC32C;
> + } else if (!strcmp(p, "unique-id")) {
> + newsrv->pp_opts |= SRV_PP_V2_UNIQUE_ID;
> } else
> goto fail;
> }
> diff --git a/src/stream_interface.c b/src/stream_interface.c
> index 47cbd9693..d4acea8bc 100644
> --- a/src/stream_interface.c
> +++ b/src/stream_interface.c
> @@ -354,9 +354,11 @@ int conn_si_send_proxy(struct connection *conn, unsigned
> int flag)
> if (cs && cs->data_cb == _conn_cb) {
> struct stream_interface *si = cs->data;
> struct conn_stream *remote_cs =
> objt_cs(si_opposite(si)->end);
> + struct stream *strm = si_strm(si);
Same here, a blank line please, even though I know you were not the first
one to miss it.
> ret = make_proxy_line(trash.area, trash.size,
> objt_server(conn->target),
> - remote_cs ? remote_cs->conn :
> NULL);
> + remote_cs ? remote_cs->conn :
> NULL,
> + strm);
> /* We may not have a conn_stream yet, if we don't
>* know which mux to use, because it will be decided
>* during the SSL handshake. In this case, there should
(...)
So modulo the very few minor stuff above I'm overall OK without your
patch, just let me know if you have any question.
Thanks!
Willy
Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
On Mon, Mar 09, 2020 at 05:07:38PM +0100, Tim Düsterhus wrote: > Willy, > > Am 06.03.20 um 13:15 schrieb Tim Duesterhus: > > is acceptable. There's also a `BUG_ON` left in the patch, because I wasn't > > sure if I should grab the session from the stream or from the connection. > > I just realized that BUG_ON is not active by default (should we add > DEBUG_STRICT=1 to CI?). The check in there does not even compile (facepalm). Yes, definitely a good idea. Willy
s390x and HAProxy?
Hi. I'm wondering that this target is tested. http://git.haproxy.org/?p=haproxy.git;a=commitdiff;h=d726386421dcd184ca2518d17332f82e9cd79f2d Are there really user which runs HAProxy on Host? 8-O How perform HAProxy on that platform? Regards Aleks
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Willy, Am 13.03.20 um 08:44 schrieb Willy Tarreau: >> I've added the new `struct connection` member at the end. Please check >> whether you think that is the appropriate place for it or if it should >> be moved somewhere else because of holes or caches. > > I'm seeing that the struct connection has become huge (160 bytes now) I suspected something like that, that's why I specifically mentioned that piece. > But for now I'm OK with taking your patch above. > I'm not seeing anything in git, yet. Is the patch okay as-is and you are just looking into the `struct connection` refactoring before actually taking it or would you like me to make any changes to it? Best regards Tim Düsterhus
Re: Tests timeout on my ARM64 test VM
Hi Илья, Suddenly today the build is again green here! I didn't make any changes to my testing setup. It must be something on the OS level but I wasn't able to figure out what makes the HAProxy tests timeout in the last several days. Regards, Martin On Wed, Mar 11, 2020 at 4:13 PM Martin Grigorov wrote: > > > On Wed, Mar 11, 2020 at 3:06 PM Илья Шипицин wrote: > >> I will a look during next weekend >> > > Thank you, Илья! > > >> >> BTW, I've managed to get Linaro VM :) >> > > Congrats! :-) > > >> >> On Wed, Mar 11, 2020, 5:40 PM Martin Grigorov >> wrote: >> >>> Hi, >>> >>> On Mon, Mar 9, 2020 at 10:22 PM Martin Grigorov < >>> martin.grigo...@gmail.com> wrote: >>> Hi, I am not sure what have changed on my test ARM64 VM but the reg tests started timing out. Everything is fine on my dev machine (x86_64) and at Travis ( https://travis-ci.com/haproxy/haproxy). I don't think it is ARM64 related. Most probably some OS setting or something. I've rebooted the system just to make sure it is not some busy port or opened file descriptor but it still fails the same way. Does someone see in the attached logs what could be the problem? >>> >>> Anyone can help me here ? >>> >>> Martin >>> >>> Thank you! Martin >>>
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
On Fri, Mar 13, 2020 at 08:44:59AM +0100, Willy Tarreau wrote: > I'm seeing that the struct connection has become huge (160 bytes now) By the way, regarding this, I'm seeing this order of criticity in terms of structures sizes: 1) struct fdtab : 64 bytes, cache-line-aligned, shared between threads, no room for negociation, the only way to add something is to replace something else with an added benefit. 2) struct tasklet : 48 bytes, shares the first 32 or so with struct task (must alias), we don't want it to go beyond a cache line 3) struct task: 160 bytes already :-( Must be very carefully arranged to group fields used together as close as possible to benefit from the appear in the cache line when accessed 4) struct connection: 160 bytes, many of which are never used. I'd love to shrink it down to 128 by moving PPv2 away from it 5) struct session: 184 bytes, nor optimized, 3 cache lines already, could get a bit more love. 6) struct conn_stream: 40 bytes, OK to grow up to a cache line 7) struct stream (1 kB) and all others at the same level. Willy
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Hi Tim, On Fri, Mar 06, 2020 at 01:15:39PM +0100, Tim Duesterhus wrote: > For the whole unique ID processing I've used the new `ist` helpers, it > might make sense to update the authority processing to make use of ist > in a future CLEANUP patch. > > I've added the new `struct connection` member at the end. Please check > whether you think that is the appropriate place for it or if it should > be moved somewhere else because of holes or caches. I'm seeing that the struct connection has become huge (160 bytes now) and is going to be inflated by 10% with this change. The authority used to save a few bytes by keeping the length on a uint8_t but that's still a detail. I think that the real problem is the proxy protocol processing here. It's very rarely used by those where the connection size matters, and when we look at it, we can see : proxy_netns (8) proxy_authority (8) proxy_authority_len (1) proxy_unique_id (16) That's a total of 33 bytes (well, 40-48 in practice due to holes around) that are there only for the proxy proto. What could possibly be done to improve the situation would be to create a pool of proxy-proto entries which would contain room for netns, auth and unique_id and which would be allocated on the fly when receiving such TLV fields. We'd only inflate it when adding new fields and that would only eat memory in such a case. Thinking that the change above alone is enough to go back to 128 bytes hence two cache lines makes me really tempted to have a look at it. But for now I'm OK with taking your patch above. Willy
[PATCH] BUG/MEDIUM: spoe: Use unique engine_id for all agents in all scopes
Hi again
In version 2.1.3:
When config spoe engine proxys > 1, the function "srandom" will run more
than one times, it will make some engine_id duplicated, and the SPOA
is group SPOP connections by engine_id when option async is on, the
ack will reply to the wrong SPOE.
This patch should backport to 2.0 and 1.9.
My pleasure
Kevin
From 512e4aca8e3ffd57fb3f12581ede6d8e8d624319 Mon Sep 17 00:00:00 2001
From: Kevin Zhu
Date: Fri, 13 Mar 2020 14:40:46 +0800
Subject: [PATCH] BUG/MEDIUM: spoe: Use unique engine_id for all agents in all
scopes
When config spoe engine proxys > 1, the function "srandom" will run more
than one times, it will make some engine_id duplicated, and the SPOA
is group SPOP connections by engine_id when option async is on, the
ack will reply to the wrong SPOE.
---
src/flt_spoe.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index 930ac8d..2181138 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -3118,14 +3118,17 @@ spoe_init_per_thread(struct proxy *p, struct flt_conf *fconf)
{
struct spoe_config *conf = fconf->conf;
struct spoe_agent *agent = conf->agent;
+ static int init = 0;
- /* Use a != seed per process */
- if (relative_pid > 1 && tid == 0)
+ if (HA_ATOMIC_LOAD() == 0) {
+ HA_ATOMIC_STORE(, 1);
srandom(now_ms * pid);
+ }
agent->rt[tid].engine_id = generate_pseudo_uuid();
if (agent->rt[tid].engine_id == NULL)
return -1;
+
return 0;
}
--
2.7.4
[PATCH] BUG/MEDIUM: spoe: dup agent's engine_id string from trash.area
Hi
The agent's engine_id forgot to dup from trash, all engine_ids point to the
same address "", the engine_id changed at run time and will
double-free when release agents and trash.
Kevin
From 674ba1e318cb561a1650db98030e12939e604171 Mon Sep 17 00:00:00 2001
From: Kevin Zhu
Date: Fri, 13 Mar 2020 10:39:51 +0800
Subject: [PATCH] BUG/MEDIUM: spoe: dup agent's engine_id string from
trash.area
The agent's engine_id forgot to dup from trash, all engine_ids point to
the same address "", the engine_id changed at run time and will
double free when release agents and trash.
---
src/flt_spoe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index df080d8..57c2246 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -258,7 +258,7 @@ static char *
generate_pseudo_uuid()
{
ha_generate_uuid();
- return trash.area;
+ return my_strndup(trash.area, trash.data);
}
--
2.7.4
