Re: TLV problem after updating to 2.1.14
Hello Willy, > Hativ, if I send you a patch to test next week, is it possible to > give > it a try on your side ? I'm interested in knowing if a clean "LOCAL" > connection works fine with Dovecot. If so then in parallel we can > file > a report on Dovecot to make their parser more robust but at least > we'd > have a longterm solution that doesn't affect deployed servers. Yes, sure. Am Samstag, den 04.04.2020, 14:17 +0200 schrieb Willy Tarreau: > Hativ, if I send you a patch to test next week, is it possible to > giveit a try on your side ? I'm interested in knowing if a clean > "LOCAL"connection works fine with Dovecot. If so then in parallel we > can filea report on Dovecot to make their parser more robust but at > least we'dhave a longterm solution that doesn't affect deployed > servers. -- Greetings Hativ
Re: TLV problem after updating to 2.1.14
On Sat, Apr 04, 2020 at 01:49:06PM +0200, Tim Düsterhus wrote: > > Do you think we ought to refrain from sending any address at all ? > > I preferred to avoid possibly visible changes and apparently it didn't > > go that well :-/ > > > > Based on a strict reading of the proxy protocol definition Dovecot is > wrong here: > > > The receiver must accept this connection as valid and must use the > > real connection endpoints and discard the protocol block including the > > family which is ignored. Sure but one component being wrong doesn't deserve being broken by a unilateral change :-/ > To my understanding the TLV are part of the protocol block, because they > were bolted onto the proxy protocol v2 after the fact in commit > afb768340c9d7e50d8e7372051c8a9c3b3f2151c > (https://github.com/haproxy/haproxy/commit/afb768340c9d7e50d8e7372051c8a9c3b3f2151c). > I also notice that this commit made an incompatible change to proxy > protocol v2 1.5 years after the initial specification (however both > during the 1.5 development cycle). Yes TLVs were added after the initial v2 spec. > However HAProxy violates a should: > > > When a sender presents a > > LOCAL connection, it should not present any address so it sets this field to > > zero. So that fuels the fact that we should definitely get rid of these addresses and we should possibly expect less breakage by doing so. > My conclusion is that the proxy protocol v2 definition is not super > clear with regards to TLV. I assume that is because they were not > initially part of the specification. Not sending an explicit length for > the address and instead only a length for address + TLV is non-ideal. > Another case in point: My bugfix for the TLV parsing that ignored the > length of the proxy header, because each TLV had its own length. So I think that I'll revert the fix from stable branches, because it was meant to address bug #511 which is not that important. And in master we'd instead send a real, naked LOCAL request that complies with the rule above. Hativ, if I send you a patch to test next week, is it possible to give it a try on your side ? I'm interested in knowing if a clean "LOCAL" connection works fine with Dovecot. If so then in parallel we can file a report on Dovecot to make their parser more robust but at least we'd have a longterm solution that doesn't affect deployed servers. Thanks! Willy
Re: TLV problem after updating to 2.1.14
Willy, Am 04.04.20 um 13:29 schrieb Willy Tarreau: >> Am 04.04.20 um 12:41 schrieb Tim Düsterhus: >>> The Dovecot source code is here: >>> https://github.com/dovecot/core/blob/de9968d623e331a18b43dfe8a00421f72f7f7962/src/lib-master/master-service-haproxy.c#L354 >>> >>> A quick glance at the Dovecot code looks like Dovecot parses the proxy >>> protocol correctly with regard to TLVs. >> >> I spoke too soon here. Dovecot ignores the addresses as it should for >> LOCAL (i.e. health check) connections [1] and then attempts to parse the >> TLV, interpreting the IP addresses as a TLV [2]. From my understanding >> HAProxy sends IP addresses for LOCAL connections for compatibility. > > Do you think we ought to refrain from sending any address at all ? > I preferred to avoid possibly visible changes and apparently it didn't > go that well :-/ > Based on a strict reading of the proxy protocol definition Dovecot is wrong here: > The receiver must accept this connection as valid and must use the > real connection endpoints and discard the protocol block including the > family which is ignored. To my understanding the TLV are part of the protocol block, because they were bolted onto the proxy protocol v2 after the fact in commit afb768340c9d7e50d8e7372051c8a9c3b3f2151c (https://github.com/haproxy/haproxy/commit/afb768340c9d7e50d8e7372051c8a9c3b3f2151c). I also notice that this commit made an incompatible change to proxy protocol v2 1.5 years after the initial specification (however both during the 1.5 development cycle). As the TLV are part of the protocol block they must be ignored for LOCAL connections to my understanding, but I'm not sure. I already mentioned / asked about that within my email [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections (Message-Id: <20200306121540.28050-3-...@bastelstu.be>): > I've also added a reg-test that verifies that pulling unique IDs works > properly. My first attempt was to use a LOCAL connection, because then > I would not need to send IP addresses in HEX encoding, however HAProxy > does not appear to read TLVs in LOCAL mode. I've attempted to find out > whether TLVs are supported for LOCAL mode or not in the proxy-protocol > specification, but it was not terribly clear. Supporting unique IDs in > LOCAL mode would definitely make sense to me. And supporting the CRC32 > checksum would also make sense I guess. However HAProxy violates a should: > When a sender presents a > LOCAL connection, it should not present any address so it sets this field to > zero. My conclusion is that the proxy protocol v2 definition is not super clear with regards to TLV. I assume that is because they were not initially part of the specification. Not sending an explicit length for the address and instead only a length for address + TLV is non-ideal. Another case in point: My bugfix for the TLV parsing that ignored the length of the proxy header, because each TLV had its own length. Best regards Tim Düsterhus
Re: TLV problem after updating to 2.1.14
On Sat, Apr 04, 2020 at 12:52:07PM +0200, Tim Düsterhus wrote: > Hativ, > Willy, > > Am 04.04.20 um 12:41 schrieb Tim Düsterhus: > > The Dovecot source code is here: > > https://github.com/dovecot/core/blob/de9968d623e331a18b43dfe8a00421f72f7f7962/src/lib-master/master-service-haproxy.c#L354 > > > > A quick glance at the Dovecot code looks like Dovecot parses the proxy > > protocol correctly with regard to TLVs. > > I spoke too soon here. Dovecot ignores the addresses as it should for > LOCAL (i.e. health check) connections [1] and then attempts to parse the > TLV, interpreting the IP addresses as a TLV [2]. From my understanding > HAProxy sends IP addresses for LOCAL connections for compatibility. Do you think we ought to refrain from sending any address at all ? I preferred to avoid possibly visible changes and apparently it didn't go that well :-/ Willy
Re: TLV problem after updating to 2.1.14
Hativ, Willy, Am 04.04.20 um 12:41 schrieb Tim Düsterhus: > The Dovecot source code is here: > https://github.com/dovecot/core/blob/de9968d623e331a18b43dfe8a00421f72f7f7962/src/lib-master/master-service-haproxy.c#L354 > > A quick glance at the Dovecot code looks like Dovecot parses the proxy > protocol correctly with regard to TLVs. I spoke too soon here. Dovecot ignores the addresses as it should for LOCAL (i.e. health check) connections [1] and then attempts to parse the TLV, interpreting the IP addresses as a TLV [2]. From my understanding HAProxy sends IP addresses for LOCAL connections for compatibility. Best regards Tim Düsterhus [1] https://github.com/dovecot/core/blob/de9968d623e331a18b43dfe8a00421f72f7f7962/src/lib-master/master-service-haproxy.c#L386-L390 [2] https://github.com/dovecot/core/blob/de9968d623e331a18b43dfe8a00421f72f7f7962/src/lib-master/master-service-haproxy.c#L456
Re: TLV problem after updating to 2.1.14
Hativ, Am 04.04.20 um 08:22 schrieb Hativ: > what I've found in the meantime: Dovecot's error messages even appear > permanently, regardless of the TCP check. > > Reverting that commit (7f26391bc51ad56c31480d03f56e1db604f1c617) back solves > the issue. No more error message in Dovecot and the Layer 7 check works again. > > What's the cause of this? Something wrong with the commit or is Dovecot wrong? > That's to be determined. The error message sounds as if Dovecot expects a TLV that is not being sent. Can you possibly provide a PCAP of a single (failing) health request to Dovecot with the commit NOT reverted? Also Ccing Willy as the author of that commit. The Dovecot source code is here: https://github.com/dovecot/core/blob/de9968d623e331a18b43dfe8a00421f72f7f7962/src/lib-master/master-service-haproxy.c#L354 A quick glance at the Dovecot code looks like Dovecot parses the proxy protocol correctly with regard to TLVs. Best regards Tim Düsterhus
[PATCH] fix function comment
Hello,
small fix attached.
Ilya Shipitcin
From 2cf4b1a3baab84e420dcbbdf084c8138b2f8bd25 Mon Sep 17 00:00:00 2001
From: Ilya Shipitsin
Date: Sat, 4 Apr 2020 12:59:53 +0500
Subject: [PATCH] CLEANUP: src/log.c: fix comment
"fmt" is passed to parse_logformat_string, adjust comment
accordingly
---
src/log.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/log.c b/src/log.c
index 2cac07486..89639e99f 100644
--- a/src/log.c
+++ b/src/log.c
@@ -550,7 +550,7 @@ int add_sample_to_logformat_list(char *text, char *arg, int arg_len, struct prox
* You can set arguments using { } : %{many arguments}varname.
* The curproxy->conf.args.ctx must be set by the caller.
*
- * str: the string to parse
+ * fmt: the string to parse
* curproxy: the proxy affected
* list_format: the destination list
* options: LOG_OPT_* to force on every node
--
2.25.1
Re: TLV problem after updating to 2.1.14
Hello Tim, what I've found in the meantime: Dovecot's error messages even appear permanently, regardless of the TCP check. Reverting that commit (7f26391bc51ad56c31480d03f56e1db604f1c617) back solves the issue. No more error message in Dovecot and the Layer 7 check works again. What's the cause of this? Something wrong with the commit or is Dovecot wrong? -- Greetings Hativ Am Freitag, den 03.04.2020, 11:27 +0200 schrieb Tim Düsterhus: > Hativ, > Am 03.04.20 um 00:38 schrieb Hativ: > > Any ideas what's wrong? > > I would assume that this patch changed the behavior there: > https://github.com/haproxy/haproxy/commit/7f26391bc51ad56c31480d03f56e1db604f1c617 > > Can you try reverting that to check whether it is the cause? > Best regardsTim Düsterhus
