Re: 1.7.6 redirect regression (commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32)

[Willy Tarreau]

Hi Vincent,

On Fri, Jun 23, 2017 at 07:24:05AM +0200, Vincent Bernat wrote:
> Is the patch important enough to warrant a 1.7.7 soon? Notably, should
> downstreams continue to push 1.7.6 to users?

So I just checked and in fact it was introduced in version 1.7.6 by this
commit :

  73d071e ("BUG/MINOR: http: Fix conditions to clean up a txn and to handle the 
next request")

Thus in fact the initial commit message was right, 1.7 was not impacted by
then, and it was unfortunately revealed by another fix. This is a regression
of 1.7.6 compared to 1.7.5. Not a big one but nevertheless a regression. So
yes, I think we should issue 1.7.7 to fix it, that would be better.

Thanks,
Willy

Re: 1.7.6 redirect regression (commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32)

[Vincent Bernat]

 ❦ 21 juin 2017 11:48 +0200, William Lallemand  :

>> > This bug was fixed in 1.8 (see commit
>> > 9f724edbd8d1cf595d4177c3612607f395b4380e "BUG/MEDIUM: http: Drop the
>> > connection establishment when a redirect is performed"). I attached
>> > the patch. Could you quickly check if it fixes your bug (it should
>> > do so) ?
>> > 
>> > It was not backported in 1.7 because we thought it only affected the
>> > 1.8. I will check with Willy.
>> 
>> Thanks, patch fixes the problem (with test config (I'll try to
>> test with prod. config later)).
>> 
>> -Jarno
>> 
>
> Thanks for tests, I will backport it in the 1.7 branch.

Is the patch important enough to warrant a 1.7.7 soon? Notably, should
downstreams continue to push 1.7.6 to users?
-- 
Parenthesise to avoid ambiguity.
- The Elements of Programming Style (Kernighan & Plauger)

Re: 1.7.6 redirect regression (commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32)

[William Lallemand]

On Wed, Jun 21, 2017 at 12:30:47PM +0300, Jarno Huuskonen wrote:
> Hi Christopher,
> 
> On Wed, Jun 21, Christopher Faulet wrote:
> > This bug was fixed in 1.8 (see commit
> > 9f724edbd8d1cf595d4177c3612607f395b4380e "BUG/MEDIUM: http: Drop the
> > connection establishment when a redirect is performed"). I attached
> > the patch. Could you quickly check if it fixes your bug (it should
> > do so) ?
> > 
> > It was not backported in 1.7 because we thought it only affected the
> > 1.8. I will check with Willy.
> 
> Thanks, patch fixes the problem (with test config (I'll try to
> test with prod. config later)).
> 
> -Jarno
> 

Thanks for tests, I will backport it in the 1.7 branch.

-- 
William Lallemand

Re: 1.7.6 redirect regression (commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32)

[Jarno Huuskonen]

Hi Christopher,

On Wed, Jun 21, Christopher Faulet wrote:
> This bug was fixed in 1.8 (see commit
> 9f724edbd8d1cf595d4177c3612607f395b4380e "BUG/MEDIUM: http: Drop the
> connection establishment when a redirect is performed"). I attached
> the patch. Could you quickly check if it fixes your bug (it should
> do so) ?
> 
> It was not backported in 1.7 because we thought it only affected the
> 1.8. I will check with Willy.

Thanks, patch fixes the problem (with test config (I'll try to
test with prod. config later)).

-Jarno

-- 
Jarno Huuskonen

Re: 1.7.6 redirect regression (commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32)

[Christopher Faulet]

Le 21/06/2017 à 07:27, Jarno Huuskonen a écrit :

Hi,

1.7.6 gives me errors (in log) with redirect rules. Example config that
produces 503 errors in logs and curl -v complains:
< HTTP/1.1 301 Moved Permanently
< Content-length: 0
< Location: https://127.0.0.1:8080/
<
* Excess found in a non pipelined read: excess = 212 url = /
* (zero-length body)
* Connection #0 to host 127.0.0.1 left intact

Example config:
frontend test
 bind ipv4@127.0.0.1:8080

 redirect scheme https code 301 if { dst_port 8080 }
 # this also gives 503
 #http-request redirect scheme https code 301 if { dst_port 8080 }
 default_backend test_be2

backend test_be2
 server wp1 ip.add.re.ss:80 id 1

(I have quite a few redirect rules in prod. config and all seem to
produce 503 in logs).

git bisect gives this commit as "bad":

commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32
BUG/MINOR: http: Fix conditions to clean up a txn and to handle the next req

If I revert this commit then the example config gives 301 in log and
curl doesn't complain about read.

-Jarno



Hi Jarno,

This bug was fixed in 1.8 (see commit 
9f724edbd8d1cf595d4177c3612607f395b4380e "BUG/MEDIUM: http: Drop the 
connection establishment when a redirect is performed"). I attached the 
patch. Could you quickly check if it fixes your bug (it should do so) ?


It was not backported in 1.7 because we thought it only affected the 
1.8. I will check with Willy.


Thanks.
--
Christopher Faulet
commit 9f724edbd8d1cf595d4177c3612607f395b4380e
Author: Christopher Faulet 
Date:   Thu Apr 20 14:16:13 2017 +0200

BUG/MEDIUM: http: Drop the connection establishment when a redirect is performed

This bug occurs when a redirect rule is applied during the request analysis on a
persistent connection, on a proxy without any server. This means, in a frontend
section or in a listen/backend section with no "server" line.

Because the transaction processing is shortened, no server can be selected to
perform the connection. So if we try to establish it, this fails and a 503 error
is returned, while a 3XX was already sent. So, in this case, HAProxy generates 2
replies and only the first one is expected.

Here is the configuration snippet to easily reproduce the problem:

listen www
bind :8080
mode http
timeout connect 5s
timeout client 3s
timeout server 6s
redirect location /

A simple HTTP/1.1 request without body will trigger the bug:

$ telnet 0 8080
Trying 0.0.0.0...
Connected to 0.
Escape character is '^]'.
GET / HTTP/1.1

HTTP/1.1 302 Found
Cache-Control: no-cache
Content-length: 0
Location: /

HTTP/1.0 503 Service Unavailable
Cache-Control: no-cache
Connection: close
Content-Type: text/html

503 Service Unavailable
No server is available to handle this request.

Connection closed by foreign host.

[wt: only 1.8-dev is impacted though the bug is present in older ones]

diff --git a/src/proto_http.c b/src/proto_http.c
index 24d034a..6c940f1 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -4261,6 +4261,8 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s
 		/* Trim any possible response */
 		res->chn->buf->i = 0;
 		res->next = res->sov = 0;
+		/* If not already done, don't perform any connection establishment */
+		channel_dont_connect(req->chn);
 	} else {
 		/* keep-alive not possible */
 		if (unlikely(txn->flags & TX_USE_PX_CONN)) {

1.7.6 redirect regression (commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32)

[Jarno Huuskonen]

Hi,

1.7.6 gives me errors (in log) with redirect rules. Example config that
produces 503 errors in logs and curl -v complains:
< HTTP/1.1 301 Moved Permanently
< Content-length: 0
< Location: https://127.0.0.1:8080/
< 
* Excess found in a non pipelined read: excess = 212 url = /
* (zero-length body)
* Connection #0 to host 127.0.0.1 left intact

Example config:
frontend test
bind ipv4@127.0.0.1:8080

redirect scheme https code 301 if { dst_port 8080 }
# this also gives 503
#http-request redirect scheme https code 301 if { dst_port 8080 }
default_backend test_be2

backend test_be2
server wp1 ip.add.re.ss:80 id 1 

(I have quite a few redirect rules in prod. config and all seem to
produce 503 in logs).

git bisect gives this commit as "bad":

commit 73d071ecc84e0f26ebe1b9576fffc1ed0357ef32
BUG/MINOR: http: Fix conditions to clean up a txn and to handle the next req

If I revert this commit then the example config gives 301 in log and
curl doesn't complain about read.

-Jarno

-- 
Jarno Huuskonen