Fix small bug in srv_parse_agent_check
Hi all, I was building HAProxy using scan-build to see if there were any issues and it called out an unused variable write. I think this was due to a bug that the err_code was not used in srv_parse_agent_check. I’ve attached a patch to fix this issue. Cheers, Dirkjan Bussink 0001-BUG-MINOR-checks-return-correct-error-code-for-srv_p.patch Description: Binary data
Re: Bad backend selected
On Fri, Jun 18, 2021 at 05:08:56PM +0200, Tim Düsterhus wrote: > > So I had some thoughts about that discussion that started off-list. And > > now I think that the right thing to do is to always drop the port part > > of the authority when we have a scheme for which it's the default. I.e. > > if the scheme is "http" we drop ":80", and if the scheme is "https" we > > drop ":443". This will always be consistent with the standards, and by > > doing it early (i.e. during conversion to HTX) we're certain to address > > both the conversion of CONNECT to GET+Upgrade, and the hdr(host) match. > > Is this still on your radar? Should I file an issue for that? Amaury started something in this direction but it was only in H2 and I'd like that we explore the ability to do it the most generic way possible so as not to have different behaviours depending on where the requests enter and where they leave. I know it's tricky, which is one more reason for exploring this possibility. At least regardless of the final choice, we'll be certain not to have left unknown cases incorrectly handled. Thus no need to create a new issue for now, hopefully by next week we'll all agree on a durable solution. Thanks, Willy
Re: Bad backend selected
Willy, On 6/8/21 8:37 AM, Willy Tarreau wrote: However the only difference is the 443 port explicitly specified in the later request. I am not sure it's something specific to 2.4.0, but I've never seen it before. Is it an expected behaviour ? If so, how can I change my acls to correct it ? I encountered the same issue (incidentally also with socket.io). It's happening for WebSockets via HTTP/2. These are newly supported starting with HAProxy 2.4. The "broken" requests are most likely Firefox, while the working ones are not Firefox. I already have a private email thread with a few developers regarding this behavior. So I had some thoughts about that discussion that started off-list. And now I think that the right thing to do is to always drop the port part of the authority when we have a scheme for which it's the default. I.e. if the scheme is "http" we drop ":80", and if the scheme is "https" we drop ":443". This will always be consistent with the standards, and by doing it early (i.e. during conversion to HTX) we're certain to address both the conversion of CONNECT to GET+Upgrade, and the hdr(host) match. Is this still on your radar? Should I file an issue for that? Best regards Tim Düsterhus
Re: [PATCH] BUG/MINOR: cache: Correctly handle existing-but-empty, 'accept-encoding' header
On Fri, Jun 18, 2021 at 04:07:10PM +0200, Tim Düsterhus, WoltLab GmbH wrote: > Remi, > > On 6/18/21 3:46 PM, Remi Tricot-Le Breton wrote: > > > please find a small fix for the 'Vary' support of the cache to > > > correctly handle the difference between a missing 'accept-encoding' > > > and an empty 'accept-encoding'. > > > > > Nice catch, I completely forgot about this case. > > > > Frankly it's a bit dumb that there even is a difference between a missing > header and an empty header. And also that content-encoding is opt-out > instead of opt-in. It's the case for Host and for Accept-Encoding. There was even a lengthy discussion on this topic during the definition of the structured-headers (now RFC8941) which do not permit to express this difference. I don't think there's any other in this situation. While some of us preferred to maintain that ability for compatibility purposes, the general consensus was rather that these are design mistakes to be avoided in the future. > I found the issue, because HAProxy was serving me a gzip encoded response > from the cache when I was not providing an 'accept-encoding' header and I > initially wanted to report *that* as a bug report. Then I noticed that this > was intentional and that it is the expected behavior based on the HTTP > specification. Yep, accept-encoding should be seen as a filter on what you're willing to accept. Not specifying it means you're OK with anything. > I would not be surprised if this case is not correctly handled by the > majority of HTTP clients out there, because most (all?) servers tend to only > serve compressed responses if the client explicitly indicates support. This within the server's ability or willingness to compress, as this may be permanently or temporarily disabled based on path, extension, CPU or RAM usage. > After sending the patch to HAProxy I also patched the HTTP client without > compression support in our software to specify 'accept-encoding: identity' > to be on the safe side :-/ That's the nice thing when you control multiple components in the chain, you can adjust to whatever remains standard but suits you better :-) Willy
Re: [PATCH] BUG/MINOR: cache: Correctly handle existing-but-empty, 'accept-encoding' header
Remi, On 6/18/21 3:46 PM, Remi Tricot-Le Breton wrote: please find a small fix for the 'Vary' support of the cache to correctly handle the difference between a missing 'accept-encoding' and an empty 'accept-encoding'. Nice catch, I completely forgot about this case. Frankly it's a bit dumb that there even is a difference between a missing header and an empty header. And also that content-encoding is opt-out instead of opt-in. I found the issue, because HAProxy was serving me a gzip encoded response from the cache when I was not providing an 'accept-encoding' header and I initially wanted to report *that* as a bug report. Then I noticed that this was intentional and that it is the expected behavior based on the HTTP specification. I would not be surprised if this case is not correctly handled by the majority of HTTP clients out there, because most (all?) servers tend to only serve compressed responses if the client explicitly indicates support. After sending the patch to HAProxy I also patched the HTTP client without compression support in our software to specify 'accept-encoding: identity' to be on the safe side :-/ Best regards Tim Düsterhus Developer WoltLab GmbH -- WoltLab GmbH Nedlitzer Str. 27B 14469 Potsdam Tel.: +49 331 96784338 duester...@woltlab.com www.woltlab.com Managing director: Marcel Werk AG Potsdam HRB 26795 P
Re: [PATCH] BUG/MINOR: cache: Correctly handle existing-but-empty, 'accept-encoding' header
On Fri, Jun 18, 2021 at 03:46:26PM +0200, Remi Tricot-Le Breton wrote: > Hello Tim, > > On 18/06/2021 15:26, Tim Düsterhus, WoltLab GmbH wrote: > > Remi, > > > > please find a small fix for the 'Vary' support of the cache to correctly > > handle the difference between a missing 'accept-encoding' and an empty > > 'accept-encoding'. > > > > Best regards > > Tim Düsterhus > > Developer WoltLab GmbH > > > Nice catch, I completely forgot about this case. > > @Willy, the patch looks good to me. Thanks guys, now applied. Willy
Re: [PATCH] BUG/MINOR: cache: Correctly handle existing-but-empty, 'accept-encoding' header
Hello Tim, On 18/06/2021 15:26, Tim Düsterhus, WoltLab GmbH wrote: Remi, please find a small fix for the 'Vary' support of the cache to correctly handle the difference between a missing 'accept-encoding' and an empty 'accept-encoding'. Best regards Tim Düsterhus Developer WoltLab GmbH Nice catch, I completely forgot about this case. @Willy, the patch looks good to me. Thanks Rémi
[PATCH] BUG/MINOR: cache: Correctly handle existing-but-empty, 'accept-encoding' header
Remi,
please find a small fix for the 'Vary' support of the cache to correctly
handle the difference between a missing 'accept-encoding' and an empty
'accept-encoding'.
Best regards
Tim Düsterhus
Developer WoltLab GmbH
--
WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam
Tel.: +49 331 96784338
duester...@woltlab.com
www.woltlab.com
Managing director:
Marcel Werk
AG Potsdam HRB 26795 P
>From e33b3332a138319476d690acbf0aa20395df5598 Mon Sep 17 00:00:00 2001
From: Tim Duesterhus
Date: Fri, 18 Jun 2021 15:09:28 +0200
Subject: [PATCH] BUG/MINOR: cache: Correctly handle existing-but-empty
'accept-encoding' header
To: haproxy@formilux.org
Cc: rlebre...@haproxy.com,
w...@1wt.eu
RFC 7231#5.3.4 makes a difference between a completely missing
'accept-encoding' header and an 'accept-encoding' header without any values.
This case was already correctly handled by accident, because an empty accept
encoding does not match any known encoding. However this resulted in the
'other' encoding being added to the bitmap. Usually this also succeeds in
serving cached responses, because the cached response likely has no
'content-encoding', thus matching the identity case instead of not serving the
response, due to the 'other' encoding. But it's technically not 100% correct.
Fix this by special-casing 'accept-encoding' values with a length of zero and
extend the test to check that an empty accept-encoding is correctly handled.
Due to the reasons given above the test also passes without the change in
cache.c.
Vary support was added in HAProxy 2.4. This fix should be backported to 2.4+.
---
reg-tests/cache/vary.vtc | 47
src/cache.c | 15 +++--
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/reg-tests/cache/vary.vtc b/reg-tests/cache/vary.vtc
index 61d7af671..197f822b3 100644
--- a/reg-tests/cache/vary.vtc
+++ b/reg-tests/cache/vary.vtc
@@ -108,7 +108,18 @@ server s1 {
-hdr "Content-Encoding: gzip" \
-bodylen 188
+ rxreq
+ expect req.url == "/empty-vs-missing"
+ txresp -hdr "Content-Encoding: gzip" \
+ -hdr "Vary: accept-encoding" \
+ -hdr "Cache-Control: max-age=5" \
+ -bodylen 234
+ rxreq
+ expect req.url == "/empty-vs-missing"
+ txresp -hdr "Vary: accept-encoding" \
+ -hdr "Cache-Control: max-age=5" \
+ -bodylen 256
} -start
server s2 {
@@ -344,6 +355,42 @@ client c1 -connect ${h1_fe_sock} {
expect resp.bodylen == 188
expect resp.http.X-Cache-Hit == 0
+ # A missing 'Accept-Encoding' implies that anything is acceptable,
+ # while an empty 'Accept-Encoding' implies nothing is acceptable.
+
+ # Start by caching a gzip response.
+ txreq -url "/empty-vs-missing" -hdr "Accept-Encoding: gzip"
+ rxresp
+ expect resp.status == 200
+ expect resp.bodylen == 234
+ expect resp.http.content-encoding == "gzip"
+ expect resp.http.X-Cache-Hit == 0
+
+ # Check that it is cached.
+ txreq -url "/empty-vs-missing" -hdr "Accept-Encoding: gzip"
+ rxresp
+ expect resp.status == 200
+ expect resp.bodylen == 234
+ expect resp.http.content-encoding == "gzip"
+ expect resp.http.X-Cache-Hit == 1
+
+ # Check that the cached response is returned when no accept-encoding is
+ # specified.
+ txreq -url "/empty-vs-missing"
+ rxresp
+ expect resp.status == 200
+ expect resp.bodylen == 234
+ expect resp.http.content-encoding == "gzip"
+ expect resp.http.X-Cache-Hit == 1
+
+ # Check that the cached response is not returned when an empty
+ # accept-encoding is specified.
+ txreq -url "/empty-vs-missing" -hdr "Accept-Encoding:"
+ rxresp
+ expect resp.status == 200
+ expect resp.bodylen == 256
+ expect resp.http.content-encoding == ""
+ expect resp.http.X-Cache-Hit == 0
# The following requests are treated by a backend that does not cache
# responses containing a Vary header
diff --git a/src/cache.c b/src/cache.c
index bdb82583f..feab63f07 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -2280,6 +2280,19 @@ static int accept_encoding_normalizer(struct htx *htx, struct ist hdr_name,
/* Iterate over all the ACCEPT_ENCODING_MAX_ENTRIES first accept-encoding
* values that might span acrosse multiple accept-encoding headers. */
while (http_find_header(htx, hdr_name, , 0) && count < ACCEPT_ENCODING_MAX_ENTRIES) {
+ count++;
+
+ /* As per RFC7231#5.3.4, "An Accept-Encoding header field with a
+ * combined field-value that is empty implies that the user agent
+ * does not want any content-coding in response."
+ *
+ * We must (and did) count the existence of this empty header to not
+ * hit the `count == 0` case below, but must ignore the value to not
+ * include VARY_ENCODING_OTHER into the final
