HTTP 429 Too Many Requests

[Daniel Schneller]

Hello!

We use haproxy as an L7 rate limiter based on tracking certain header
fields and URLs. A more detailed description of what we do can be found
in a blog post I wrote about this some time ago: 

https://blog.codecentric.de/en/2014/12/haproxy-http-header-rate-limiting

Our exact setup has changed a bit since then, but the gist remains the
same:

* Calculate the rate of requests by tracking those with identical
authorization header values
* If they exceed a threshold, slow the client down (tarpit) and ask
them to come back after a certain period by sending them HTTP 429:

HTTP/1.1 429 Too Many Requests
Cache-Control: no-cache
Connection: close
Content-Type: text/plain
Retry-After: 60

Too Many Requests (HAP429).

I am currently refactoring our haproxy config to make it more readable
and maintainable; while doing so, I would like to get rid of the
somewhat crude pseudo backend in which I specify the errorfile for
status code 500, replacing 500 with 429 when sending it out to the
client. This, of course, leads to the status code being 500 the logs
and other inconveniences.

My suggestion about how to handle this would be an extension to the
"http-request deny" directive. Currently it will always respond with
HTTP status code 403. If there were a configuration setting allowing me
to specify different code (like 429 in my case) as the reason for the
rejection, that would be an elegant solution. Using an "http-request
set-header" would even allow me to specify different values for the
"Retry-After:" header to inform well-written clients after which time
they should come back and try again.

Does that sound like a sensible addition?

Cheers,
Daniel

[James Brown]

+1 I am also using a fake backend with no servers and a 503 errorfile, and
it confuses everybody who looks at the config or the metrics. Being able to
directly emit a 429 would be fantastic.

On Fri, Jun 24, 2016 at 10:30 AM, Daniel Schneller <

Post by Daniel Schneller
Hello!
We use haproxy as an L7 rate limiter based on tracking certain header
fields and URLs. A more detailed description of what we do can be found
https://blog.codecentric.de/en/2014/12/haproxy-http-header-rate-limiting
Our exact setup has changed a bit since then, but the gist remains the
* Calculate the rate of requests by tracking those with identical
authorization header values
* If they exceed a threshold, slow the client down (tarpit) and ask
HTTP/1.1 429 Too Many Requests
Cache-Control: no-cache
Connection: close
Content-Type: text/plain
Retry-After: 60
Too Many Requests (HAP429).
I am currently refactoring our haproxy config to make it more readable
and maintainable; while doing so, I would like to get rid of the
somewhat crude pseudo backend in which I specify the errorfile for
status code 500, replacing 500 with 429 when sending it out to the
client. This, of course, leads to the status code being 500 the logs
and other inconveniences.
My suggestion about how to handle this would be an extension to the
"http-request deny" directive. Currently it will always respond with
HTTP status code 403. If there were a configuration setting allowing me
to specify different code (like 429 in my case) as the reason for the
rejection, that would be an elegant solution. Using an "http-request
set-header" would even allow me to specify different values for the
"Retry-After:" header to inform well-written clients after which time
they should come back and try again.
Does that sound like a sensible addition?
Cheers,
Daniel
--
Daniel Schneller
Principal Cloud Engineer
CenterDevice GmbH
https://www.centerdevice.de

[Cyril Bonté]

Hi all,

Post by James Brown
+1 I am also using a fake backend with no servers and a 503 errorfile,
and it confuses everybody who looks at the config or the metrics. Being
able to directly emit a 429 would be fantastic.

Interestingly, it already exists since 1.6-dev2 [1] for "http-request
deny" but the documentation is absolutely missing. And it has recently
been fixed by Willy [2].

Another point is that everything in the code seems to be ready to use
the same option with tarpit... except the configuration parser.

The syntax is :
http-request deny [deny_status <code>]

Example :
http-request deny deny_status 429

[1]
http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=108b1dd69d4e26312af465237487bdb855b0de60
[2]
http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=60f01f8c89e4fb2723d5a9f2046286e699567e0b

Post by James Brown
On Fri, Jun 24, 2016 at 10:30 AM, Daniel Schneller
Hello!
We use haproxy as an L7 rate limiter based on tracking certain header
fields and URLs. A more detailed description of what we do can be found
https://blog.codecentric.de/en/2014/12/haproxy-http-header-rate-limiting
Our exact setup has changed a bit since then, but the gist remains the
* Calculate the rate of requests by tracking those with identical
authorization header values
* If they exceed a threshold, slow the client down (tarpit) and ask
HTTP/1.1 429 Too Many Requests
Cache-Control: no-cache
Connection: close
Content-Type: text/plain
Retry-After: 60
Too Many Requests (HAP429).
I am currently refactoring our haproxy config to make it more readable
and maintainable; while doing so, I would like to get rid of the
somewhat crude pseudo backend in which I specify the errorfile for
status code 500, replacing 500 with 429 when sending it out to the
client. This, of course, leads to the status code being 500 the logs
and other inconveniences.
My suggestion about how to handle this would be an extension to the
"http-request deny" directive. Currently it will always respond with
HTTP status code 403. If there were a configuration setting allowing me
to specify different code (like 429 in my case) as the reason for the
rejection, that would be an elegant solution. Using an "http-request
set-header" would even allow me to specify different values for the
"Retry-After:" header to inform well-written clients after which time
they should come back and try again.
Does that sound like a sensible addition?
Cheers,
Daniel
--
Daniel Schneller
Principal Cloud Engineer
CenterDevice GmbH
https://www.centerdevice.de
--
James Brown
Engineer

[Daniel Schneller]

That is indeed pretty cool :-)
Would the addition of a header work the way I originally suggested, though?

Post by Cyril Bonté
Hi all,

Post by James Brown
+1 I am also using a fake backend with no servers and a 503 errorfile,
and it confuses everybody who looks at the config or the metrics. Being
able to directly emit a 429 would be fantastic.

Interestingly, it already exists since 1.6-dev2 [1] for "http-request deny" but the documentation is absolutely missing. And it has recently been fixed by Willy [2].
Another point is that everything in the code seems to be ready to use the same option with tarpit... except the configuration parser.
http-request deny [deny_status <code>]
http-request deny deny_status 429
[1] http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=108b1dd69d4e26312af465237487bdb855b0de60
[2] http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=60f01f8c89e4fb2723d5a9f2046286e699567e0b

Post by James Brown
On Fri, Jun 24, 2016 at 10:30 AM, Daniel Schneller
Hello!
We use haproxy as an L7 rate limiter based on tracking certain header
fields and URLs. A more detailed description of what we do can be found
https://blog.codecentric.de/en/2014/12/haproxy-http-header-rate-limiting
Our exact setup has changed a bit since then, but the gist remains the
* Calculate the rate of requests by tracking those with identical
authorization header values
* If they exceed a threshold, slow the client down (tarpit) and ask
HTTP/1.1 429 Too Many Requests
Cache-Control: no-cache
Connection: close
Content-Type: text/plain
Retry-After: 60
Too Many Requests (HAP429).
I am currently refactoring our haproxy config to make it more readable
and maintainable; while doing so, I would like to get rid of the
somewhat crude pseudo backend in which I specify the errorfile for
status code 500, replacing 500 with 429 when sending it out to the
client. This, of course, leads to the status code being 500 the logs
and other inconveniences.
My suggestion about how to handle this would be an extension to the
"http-request deny" directive. Currently it will always respond with
HTTP status code 403. If there were a configuration setting allowing me
to specify different code (like 429 in my case) as the reason for the
rejection, that would be an elegant solution. Using an "http-request
set-header" would even allow me to specify different values for the
"Retry-After:" header to inform well-written clients after which time
they should come back and try again.
Does that sound like a sensible addition?
Cheers,
Daniel
--
Daniel Schneller
Principal Cloud Engineer
CenterDevice GmbH
https://www.centerdevice.de
--
James Brown
Engineer

--
Cyril Bonté

[Cyril Bonté]

Post by Daniel Schneller
That is indeed pretty cool :-)
Would the addition of a header work the way I originally suggested, though?

Only by adding an errorfile for 429 status.
Or you can play with lua !
For example :
http-request use-service lua.shaping if <any condition you want>

and the lua service :
core.register_service("shaping", "http", function(applet)
applet:set_status(429)
applet:add_header("Content-Type", "text/plain")
applet:add_header("Retry-After", "60")
applet:start_response()
applet:send("Please come back later")
end )

Post by Daniel Schneller

Post by Cyril Bonté
Hi all,

Post by James Brown
+1 I am also using a fake backend with no servers and a 503 errorfile,
and it confuses everybody who looks at the config or the metrics. Being
able to directly emit a 429 would be fantastic.

Interestingly, it already exists since 1.6-dev2 [1] for "http-request deny" but the documentation is absolutely missing. And it has recently been fixed by Willy [2].
Another point is that everything in the code seems to be ready to use the same option with tarpit... except the configuration parser.
http-request deny [deny_status <code>]
http-request deny deny_status 429
[1] http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=108b1dd69d4e26312af465237487bdb855b0de60
[2] http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=60f01f8c89e4fb2723d5a9f2046286e699567e0b

Post by James Brown
On Fri, Jun 24, 2016 at 10:30 AM, Daniel Schneller
Hello!
We use haproxy as an L7 rate limiter based on tracking certain header
fields and URLs. A more detailed description of what we do can be found
https://blog.codecentric.de/en/2014/12/haproxy-http-header-rate-limiting
Our exact setup has changed a bit since then, but the gist remains the
* Calculate the rate of requests by tracking those with identical
authorization header values
* If they exceed a threshold, slow the client down (tarpit) and ask
HTTP/1.1 429 Too Many Requests
Cache-Control: no-cache
Connection: close
Content-Type: text/plain
Retry-After: 60
Too Many Requests (HAP429).
I am currently refactoring our haproxy config to make it more readable
and maintainable; while doing so, I would like to get rid of the
somewhat crude pseudo backend in which I specify the errorfile for
status code 500, replacing 500 with 429 when sending it out to the
client. This, of course, leads to the status code being 500 the logs
and other inconveniences.
My suggestion about how to handle this would be an extension to the
"http-request deny" directive. Currently it will always respond with
HTTP status code 403. If there were a configuration setting allowing me
to specify different code (like 429 in my case) as the reason for the
rejection, that would be an elegant solution. Using an "http-request
set-header" would even allow me to specify different values for the
"Retry-After:" header to inform well-written clients after which time
they should come back and try again.
Does that sound like a sensible addition?
Cheers,
Daniel
--
Daniel Schneller
Principal Cloud Engineer
CenterDevice GmbH
https://www.centerdevice.de
--
James Brown
Engineer

--
Cyril Bonté

[Daniel Schneller]

Thank you very much. That will be a good opportunity to work with the Lua functionality. As the value for the retry-after header should be variable for different situations, the error file would not help; but for simple scenarios it will be perfectly fine, leaving the right information in the logs and being nice and readable :-)

Post by Cyril Bonté

Post by Daniel Schneller
That is indeed pretty cool :-)
Would the addition of a header work the way I originally suggested, though?

Only by adding an errorfile for 429 status.
Or you can play with lua !
http-request use-service lua.shaping if <any condition you want>
core.register_service("shaping", "http", function(applet)
applet:set_status(429)
applet:add_header("Content-Type", "text/plain")
applet:add_header("Retry-After", "60")
applet:start_response()
applet:send("Please come back later")
end )

Post by Daniel Schneller

Post by Cyril Bonté
Hi all,

Post by James Brown
+1 I am also using a fake backend with no servers and a 503 errorfile,
and it confuses everybody who looks at the config or the metrics. Being
able to directly emit a 429 would be fantastic.

Interestingly, it already exists since 1.6-dev2 [1] for "http-request deny" but the documentation is absolutely missing. And it has recently been fixed by Willy [2].
Another point is that everything in the code seems to be ready to use the same option with tarpit... except the configuration parser.
http-request deny [deny_status <code>]
http-request deny deny_status 429
[1] http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=108b1dd69d4e26312af465237487bdb855b0de60
[2] http://www.haproxy.org/git?p=haproxy-1.6.git;a=commit;h=60f01f8c89e4fb2723d5a9f2046286e699567e0b

Post by James Brown
On Fri, Jun 24, 2016 at 10:30 AM, Daniel Schneller
Hello!
We use haproxy as an L7 rate limiter based on tracking certain header
fields and URLs. A more detailed description of what we do can be found
https://blog.codecentric.de/en/2014/12/haproxy-http-header-rate-limiting
Our exact setup has changed a bit since then, but the gist remains the
* Calculate the rate of requests by tracking those with identical
authorization header values
* If they exceed a threshold, slow the client down (tarpit) and ask
HTTP/1.1 429 Too Many Requests
Cache-Control: no-cache
Connection: close
Content-Type: text/plain
Retry-After: 60
Too Many Requests (HAP429).
I am currently refactoring our haproxy config to make it more readable
and maintainable; while doing so, I would like to get rid of the
somewhat crude pseudo backend in which I specify the errorfile for
status code 500, replacing 500 with 429 when sending it out to the
client. This, of course, leads to the status code being 500 the logs
and other inconveniences.
My suggestion about how to handle this would be an extension to the
"http-request deny" directive. Currently it will always respond with
HTTP status code 403. If there were a configuration setting allowing me
to specify different code (like 429 in my case) as the reason for the
rejection, that would be an elegant solution. Using an "http-request
set-header" would even allow me to specify different values for the
"Retry-After:" header to inform well-written clients after which time
they should come back and try again.
Does that sound like a sensible addition?
Cheers,
Daniel
--
Daniel Schneller
Principal Cloud Engineer
CenterDevice GmbH
https://www.centerdevice.de
--
James Brown
Engineer

--
Cyril Bonté

--
Cyril Bonté

[Willy Tarreau]

Hi Cyril,

Post by Cyril Bonté
Hi all,

Post by James Brown
+1 I am also using a fake backend with no servers and a 503 errorfile,
and it confuses everybody who looks at the config or the metrics. Being
able to directly emit a 429 would be fantastic.

Interestingly, it already exists since 1.6-dev2 [1] for "http-request deny"
but the documentation is absolutely missing. And it has recently been fixed
by Willy [2].

Yes indeed. I'm a bit upset not to have noticed the doc provided with this
patch was incomplete : it only changed the list of possible return codes.
It's not the first time we introduce new features without the respective
doc, and I really think that in the future we'll simply revert the patch
if we discover the doc is missing. A feature with no doc is only a selfish
way for someone to have their own features in mainline in order to avoid
having to maintain patches. But for features to be useful they need to be
documented. Without doc they don't exist so they can be reverted. I
encourage everyone to hunt for undocumented features. We may even discover
that a part of the roadmap is already implemented.

Post by Cyril Bonté
Another point is that everything in the code seems to be ready to use the
same option with tarpit... except the configuration parser.

Yep indeed. And it does not even support http-response either. That clearly
looks like a quick hack that was rushed into mainline for a very specific
use case, one more tempting reason to revert it :-(

I've added the doc now, but no support for configuring it for tarpit.

Thanks,
Willy

[Jarno Huuskonen]

Hello,

Post by James Brown
+1 I am also using a fake backend with no servers and a 503 errorfile, and
it confuses everybody who looks at the config or the metrics. Being able to
directly emit a 429 would be fantastic.

This is my first attempt in adding deny_status to:
http-request tarpit [deny_status <status>]

First patch updates parse_http_req_cond so config parser accepts
[deny_status <status>] for http-request tarpit (and sets
rule->deny_status).

The second patch updates http_process_req_common/http_process_tarpit to
use deny_status. http_process_tarpit has a switch statement for mapping
txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?

Do tarpitted (http-request tarpit / req(i)tarpit) requests always
go thru http_process_req_common (so txn->status is always set to
txn->status = http_err_codes[deny_status]) ?
Or is there a possibility that int deny_status could be overwritten
in http_process_req_common (with multiple deny/block/tarpit rules) ?

Both patches are against 1.7.1, but if this looks otherwise ok then
I can modify the patches against 1.8dev and add missing documentation.

I've used this minimal config for testing:
(missing defaults/global):
frontend test
bind ***@127.0.0.1:8080
http-request tarpit deny_status 429 if TRUE
default_backend test_be

frontend test2
bind ***@127.0.0.1:8081
http-request tarpit if TRUE
default_backend test_be

frontend test3
bind ***@127.0.0.1:8082
reqtarpit . if TRUE
default_backend test_be

backend test_be
server dummy 127.0.0.1:80 id 1

curl -v http://127.0.0.1:8080 should be tarpitted with status 429
and curl -v http://127.0.0.1:8081 (and :8082) with 500.

-Jarno

[Willy Tarreau]

Hi Jarno,

Post by Jarno Huuskonen
http-request tarpit [deny_status <status>]
First patch updates parse_http_req_cond so config parser accepts
[deny_status <status>] for http-request tarpit (and sets
rule->deny_status).
The second patch updates http_process_req_common/http_process_tarpit to
use deny_status. http_process_tarpit has a switch statement for mapping
txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?

Yes it's reasonable however it would be a good idea to instead write a
short function in charge of returning this index based on the status code
and then to use it everywhere we need this index, including in your own
code. When you look at http_error_message(), everywhere it's called,
txn->status already contains the status code, except in one place :
http_return_srv_error() where we set the status code. Thus I'd proceed
like this :

1) change http_server_error() to set the status code separately from the
message :

if (status > 0)
s->txn->status = status;
if (msg)
bo_inject(si_ic(si), msg->str, msg->len);

2) implement your switch/case as a function (eg: http_get_status_idx())

3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
to retrieve the message index, and drop its second argument ; modify
http_return_srv_error() to pass NULL to all calls to http_server_error()
instead of http_error_message(), and add a final call in this function
to do this :

bo_putchk(&s->res, http_error_message(s));

This way adding new status codes will be more straight forward and will
not require to touch multiple places all the time.

Post by Jarno Huuskonen
Do tarpitted (http-request tarpit / req(i)tarpit) requests always
go thru http_process_req_common (so txn->status is always set to
txn->status = http_err_codes[deny_status]) ?

I think so, yes.

Post by Jarno Huuskonen
Or is there a possibility that int deny_status could be overwritten
in http_process_req_common (with multiple deny/block/tarpit rules) ?

Rules evaluation is stopped when you have a deny/block/tarpit so it
will not be overwritten.

However I'm seeing the config parser become really ugly around the
deny_status part (it already was but now it's worse).

What would you think about removing the rule->deny_status = HTTP_ERR_403
entry that's hidden before the rules are parsed, and to place this and
the action this way under the deny/block/tarpit "if" block :

+ if (!strcmp(args[0], "tarpit")) {
+ rule->action = ACT_HTTP_REQ_TARPIT;
+ rule->deny_status = HTTP_ERR_500;
+ }
+ else {
+ rule->action = ACT_ACTION_DENY;
+ rule->deny_status = HTTP_ERR_403;
+ }

Post by Jarno Huuskonen
@@ -9031,13 +9035,10 @@ struct act_rule *parse_http_req_cond(const char **args, const char *file, int li
}
if (hc >= HTTP_ERR_SIZE) {
- Warning("parsing [%s:%d] : status code %d not handled, using default code 403.\n",
- file, linenum, code);
+ Warning("parsing [%s:%d] : status code %d not handled, using default code %d.\n",
+ file, linenum, code, rule->action == ACT_ACTION_DENY ? 403: 500);
}

Simply pass "rule->deny_status" instead of the condition, you'll always
report the status that is programmed to be returned, it will always be
more accurate and makes the code simpler.

Otherwise it looks good.

Thanks!
Willy

[Jarno Huuskonen]

Hi Willy,

Post by Willy Tarreau

Post by Jarno Huuskonen
http-request tarpit [deny_status <status>]
First patch updates parse_http_req_cond so config parser accepts
[deny_status <status>] for http-request tarpit (and sets
rule->deny_status).
The second patch updates http_process_req_common/http_process_tarpit to
use deny_status. http_process_tarpit has a switch statement for mapping
txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?

Yes it's reasonable however it would be a good idea to instead write a
short function in charge of returning this index based on the status code
and then to use it everywhere we need this index, including in your own
code. When you look at http_error_message(), everywhere it's called,
http_return_srv_error() where we set the status code. Thus I'd proceed
1) change http_server_error() to set the status code separately from the
if (status > 0)
s->txn->status = status;
if (msg)
bo_inject(si_ic(si), msg->str, msg->len);
2) implement your switch/case as a function (eg: http_get_status_idx())

Does it make sense to try to optimize the switch statement
and have most common status codes at the top ? Something like
switch(txn->status) {
case 500:
http_err_code_idx = HTTP_ERR_500;
break;
case 403:
http_err_code_idx = HTTP_ERR_403;
break;
...

Post by Willy Tarreau
3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
to retrieve the message index, and drop its second argument ; modify
http_return_srv_error() to pass NULL to all calls to http_server_error()
instead of http_error_message(), and add a final call in this function
bo_putchk(&s->res, http_error_message(s));
This way adding new status codes will be more straight forward and will
not require to touch multiple places all the time.

Sounds good, I'll (try to) work on this over the weekend.

Post by Willy Tarreau
However I'm seeing the config parser become really ugly around the
deny_status part (it already was but now it's worse).
What would you think about removing the rule->deny_status = HTTP_ERR_403
entry that's hidden before the rules are parsed, and to place this and
+ if (!strcmp(args[0], "tarpit")) {
+ rule->action = ACT_HTTP_REQ_TARPIT;
+ rule->deny_status = HTTP_ERR_500;
+ }
+ else {
+ rule->action = ACT_ACTION_DENY;
+ rule->deny_status = HTTP_ERR_403;
+ }

Post by Jarno Huuskonen
@@ -9031,13 +9035,10 @@ struct act_rule *parse_http_req_cond(const char **args, const char *file, int li
}
if (hc >= HTTP_ERR_SIZE) {
- Warning("parsing [%s:%d] : status code %d not handled, using default code 403.\n",
- file, linenum, code);
+ Warning("parsing [%s:%d] : status code %d not handled, using default code %d.\n",
+ file, linenum, code, rule->action == ACT_ACTION_DENY ? 403: 500);
}

Simply pass "rule->deny_status" instead of the condition, you'll always
report the status that is programmed to be returned, it will always be
more accurate and makes the code simpler.

I'm including a new patch for the parser changes. Is this what you had
in mind for the parser ?

-Jarno

[Willy Tarreau]

Hi Jarno,

Post by Jarno Huuskonen
Does it make sense to try to optimize the switch statement
and have most common status codes at the top ? Something like
switch(txn->status) {
http_err_code_idx = HTTP_ERR_500;
break;
http_err_code_idx = HTTP_ERR_403;
break;
...

No it does not because anyway they are not evaluated sequentially, the
compiler will often sort them in a tree-like method in order to limit
the number of evaluations. You will notice that the code is exactly the
same whatever the order you use.

Post by Jarno Huuskonen

Post by Willy Tarreau
3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
to retrieve the message index, and drop its second argument ; modify
http_return_srv_error() to pass NULL to all calls to http_server_error()
instead of http_error_message(), and add a final call in this function
bo_putchk(&s->res, http_error_message(s));
This way adding new status codes will be more straight forward and will
not require to touch multiple places all the time.

Sounds good, I'll (try to) work on this over the weekend.

Great!

Post by Jarno Huuskonen

Post by Willy Tarreau
However I'm seeing the config parser become really ugly around the
deny_status part (it already was but now it's worse).
What would you think about removing the rule->deny_status = HTTP_ERR_403
entry that's hidden before the rules are parsed, and to place this and
+ if (!strcmp(args[0], "tarpit")) {
+ rule->action = ACT_HTTP_REQ_TARPIT;
+ rule->deny_status = HTTP_ERR_500;
+ }
+ else {
+ rule->action = ACT_ACTION_DENY;
+ rule->deny_status = HTTP_ERR_403;
+ }

Post by Jarno Huuskonen
@@ -9031,13 +9035,10 @@ struct act_rule *parse_http_req_cond(const char **args, const char *file, int li
}
if (hc >= HTTP_ERR_SIZE) {
- Warning("parsing [%s:%d] : status code %d not handled, using default code 403.\n",
- file, linenum, code);
+ Warning("parsing [%s:%d] : status code %d not handled, using default code %d.\n",
+ file, linenum, code, rule->action == ACT_ACTION_DENY ? 403: 500);
}

Simply pass "rule->deny_status" instead of the condition, you'll always
report the status that is programmed to be returned, it will always be
more accurate and makes the code simpler.

I'm including a new patch for the parser changes. Is this what you had
in mind for the parser ?

Yes that's it. Sorry, I believed that the deny_status contained the status
code itself, but what you did is right.

Thanks,
Willy

[Jarno Huuskonen]

Hi,

Post by Willy Tarreau

Post by Jarno Huuskonen
The second patch updates http_process_req_common/http_process_tarpit to
use deny_status. http_process_tarpit has a switch statement for mapping
txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?

Yes it's reasonable however it would be a good idea to instead write a
short function in charge of returning this index based on the status code
and then to use it everywhere we need this index, including in your own
code. When you look at http_error_message(), everywhere it's called,
http_return_srv_error() where we set the status code. Thus I'd proceed
1) change http_server_error() to set the status code separately from the
if (status > 0)
s->txn->status = status;
if (msg)
bo_inject(si_ic(si), msg->str, msg->len);
2) implement your switch/case as a function (eg: http_get_status_idx())
3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
to retrieve the message index, and drop its second argument ; modify
http_return_srv_error() to pass NULL to all calls to http_server_error()
instead of http_error_message(), and add a final call in this function
bo_putchk(&s->res, http_error_message(s));

Do you mean that bo_putchk should go to end of http_return_srv_error ?

I don't think &s->res is correct ? It produces a compiler warning:
struct buffer * vs struct channel *
and coredumps if backend server is down and haproxy should return 503.

What seems to work (very minimal testing) is:
bo_putchk(s->res.buf, http_error_message(s));

Does the attached patch look otherwise ok ?
(Do I need to add http_get_status_idx prototype to
include/proto/proto_http.h ?)

How should I send the patches ? One commit for
http_server_error/http_get_status_idx changes and tarpit deny_status
parser / doc in another commit ?

-Jarno

[Willy Tarreau]

Hi Jarno,

Post by Jarno Huuskonen
Hi,

Post by Willy Tarreau

Post by Jarno Huuskonen
The second patch updates http_process_req_common/http_process_tarpit to
use deny_status. http_process_tarpit has a switch statement for mapping
txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?

Yes it's reasonable however it would be a good idea to instead write a
short function in charge of returning this index based on the status code
and then to use it everywhere we need this index, including in your own
code. When you look at http_error_message(), everywhere it's called,
http_return_srv_error() where we set the status code. Thus I'd proceed
1) change http_server_error() to set the status code separately from the
if (status > 0)
s->txn->status = status;
if (msg)
bo_inject(si_ic(si), msg->str, msg->len);
2) implement your switch/case as a function (eg: http_get_status_idx())
3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
to retrieve the message index, and drop its second argument ; modify
http_return_srv_error() to pass NULL to all calls to http_server_error()
instead of http_error_message(), and add a final call in this function
bo_putchk(&s->res, http_error_message(s));

Do you mean that bo_putchk should go to end of http_return_srv_error ?

I don't remember, I have to reread the code :-/

Post by Jarno Huuskonen
struct buffer * vs struct channel *
and coredumps if backend server is down and haproxy should return 503.
bo_putchk(s->res.buf, http_error_message(s));

Ah you're right apparently this one takes a buffer.

Post by Jarno Huuskonen
Does the attached patch look otherwise ok ?

I think there's a problem in http_error_message(), there were some
conditions to detect certain error cases on reused connections for
which we had to remain silent and these conditions have disappeared
so we'll return an error instead of silently closing.

Post by Jarno Huuskonen
(Do I need to add http_get_status_idx prototype to
include/proto/proto_http.h ?)

It depends if it's used somewhere else or if its scope really makes no
sense out of this file. A good rule of thumb is to say that if your
function is not used outside of the C file, mark it static. That way
we don't even expect to find it outside. If someone later needs it,
they'll simply have to remove the static and export the function but
at least there will be no confusion around it.

Post by Jarno Huuskonen
How should I send the patches ? One commit for
http_server_error/http_get_status_idx changes and tarpit deny_status
parser / doc in another commit ?

Yes that's the prefered way to do it, one commit per architecture or
functional change to ease review and bug tracking later.

Thanks!
Willy

[Jarno Huuskonen]

Hi,

Post by Willy Tarreau

Post by Jarno Huuskonen
Hi,

Post by Willy Tarreau

Post by Jarno Huuskonen
The second patch updates http_process_req_common/http_process_tarpit to
use deny_status. http_process_tarpit has a switch statement for mapping
txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ?

Yes it's reasonable however it would be a good idea to instead write a
short function in charge of returning this index based on the status code
and then to use it everywhere we need this index, including in your own
code. When you look at http_error_message(), everywhere it's called,
http_return_srv_error() where we set the status code. Thus I'd proceed
1) change http_server_error() to set the status code separately from the
if (status > 0)
s->txn->status = status;
if (msg)
bo_inject(si_ic(si), msg->str, msg->len);
2) implement your switch/case as a function (eg: http_get_status_idx())
3) make http_error_message() pass s->txn->status to htpt_get_status_idx()
to retrieve the message index, and drop its second argument ; modify
http_return_srv_error() to pass NULL to all calls to http_server_error()
instead of http_error_message(), and add a final call in this function
bo_putchk(&s->res, http_error_message(s));

Do you mean that bo_putchk should go to end of http_return_srv_error ?

I don't remember, I have to reread the code :-/

Post by Jarno Huuskonen
struct buffer * vs struct channel *
and coredumps if backend server is down and haproxy should return 503.
bo_putchk(s->res.buf, http_error_message(s));

Ah you're right apparently this one takes a buffer.

Post by Jarno Huuskonen
Does the attached patch look otherwise ok ?

I think there's a problem in http_error_message(), there were some
conditions to detect certain error cases on reused connections for
which we had to remain silent and these conditions have disappeared
so we'll return an error instead of silently closing.

I think the problem is with http_return_srv_error, I removed too many
arguments (and s->txn->flags checks) to http_server_error.
So we need to silently close if s->txn->flags & TX_NOT_FIRST or
s->flags & SF_SRV_REUSED ?

So it might work with (end of http_return_srv_error):
if (! ((s->txn->flags & TX_NOT_FIRST) || (s->flags & SF_SRV_REUSED)))
bo_putchk(s->res.buf, http_error_message(s));

or (http_return_srv_error) just remove second argument from
http_error_message call (and no bo_putchk). So http_return_srv_error
would be something like:

void http_return_srv_error(struct stream *s, struct stream_interface *si)
{
int err_type = si->err_type;

if (err_type & SI_ET_QUEUE_ABRT)
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_Q,
503, http_error_message(s));
else if (err_type & SI_ET_CONN_ABRT)
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_C,
503, (s->txn->flags & TX_NOT_FIRST) ? NULL :
http_error_message(s));
else if (err_type & SI_ET_QUEUE_TO)
http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_Q,
503, http_error_message(s));
else if (err_type & SI_ET_QUEUE_ERR)
http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_Q,
503, http_error_message(s));
else if (err_type & SI_ET_CONN_TO)
http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_C,
503, (s->txn->flags & TX_NOT_FIRST) ? NULL :
http_error_message(s));
else if (err_type & SI_ET_CONN_ERR)
http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_C,
503, (s->flags & SF_SRV_REUSED) ? NULL :
http_error_message(s));
else if (err_type & SI_ET_CONN_RES)
http_server_error(s, si, SF_ERR_RESOURCE, SF_FINST_C,
503, (s->txn->flags & TX_NOT_FIRST) ? NULL :
http_error_message(s));
else /* SI_ET_CONN_OTHER and others */
http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C,
500, http_error_message(s));
}

Does it work with this version of http_return_srv_error ?

-Jarno

[Willy Tarreau]

Hi Jarno,

Post by Jarno Huuskonen

Post by Willy Tarreau
I think there's a problem in http_error_message(), there were some
conditions to detect certain error cases on reused connections for
which we had to remain silent and these conditions have disappeared
so we'll return an error instead of silently closing.

I think the problem is with http_return_srv_error, I removed too many
arguments (and s->txn->flags checks) to http_server_error.
So we need to silently close if s->txn->flags & TX_NOT_FIRST or
s->flags & SF_SRV_REUSED ?

The purpose was to avoid returning a 5xx to the client whenever a
connection error was met while sending a request on an already used
connection, so that the client can decide to resend. So I don't remember
all the conditions involved (and am not in front of the code at the
moment) but you get the idea.

Post by Jarno Huuskonen
if (! ((s->txn->flags & TX_NOT_FIRST) || (s->flags & SF_SRV_REUSED)))
bo_putchk(s->res.buf, http_error_message(s));

From what I remember, the function also deals with other errors so you
don't want to hide all of them.
X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4

Post by Jarno Huuskonen
or (http_return_srv_error) just remove second argument from
http_error_message call (and no bo_putchk). So http_return_srv_error
void http_return_srv_error(struct stream *s, struct stream_interface *si)
{
int err_type = si->err_type;
if (err_type & SI_ET_QUEUE_ABRT)
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_Q,
503, http_error_message(s));
else if (err_type & SI_ET_CONN_ABRT)
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_C,
http_error_message(s));
else if (err_type & SI_ET_QUEUE_TO)
http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_Q,
503, http_error_message(s));
else if (err_type & SI_ET_QUEUE_ERR)
http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_Q,
503, http_error_message(s));
else if (err_type & SI_ET_CONN_TO)
http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_C,
http_error_message(s));
else if (err_type & SI_ET_CONN_ERR)
http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_C,
http_error_message(s));
else if (err_type & SI_ET_CONN_RES)
http_server_error(s, si, SF_ERR_RESOURCE, SF_FINST_C,
http_error_message(s));
else /* SI_ET_CONN_OTHER and others */
http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C,
500, http_error_message(s));
}
Does it work with this version of http_return_srv_error ?

At first glance I think it should work.

Thanks,
Willy

[Jarno Huuskonen]

Hi Willy,

Post by Willy Tarreau

Post by Jarno Huuskonen
How should I send the patches ? One commit for
http_server_error/http_get_status_idx changes and tarpit deny_status
parser / doc in another commit ?

Yes that's the prefered way to do it, one commit per architecture or
functional change to ease review and bug tracking later.

I'm including two commits for this feature.
- 0001-MEDIUM-http_error_message-txn-status-http_get_status.patch
Removes second argument from http_error_message and uses
txn->status / http_get_status_idx to map the 200..504 status code
to HTTP_ERR_200..504 enum.
(http_get_status_idx has default return value of HTTP_ERR_500, is
this ok ?)

- 0002-MINOR-http-request-tarpit-deny_status.patch
Adds http-request tarpit deny_status functionality.
(depends on the 0001-... commit).

Alternative implementation (0001-....) for http_return_srv_error
could be something like this:
void http_return_srv_error(struct stream *s, struct stream_interface *si)
{
int err_type = si->err_type;
int send_msg = 1;

if (err_type & SI_ET_QUEUE_ABRT)
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_Q, 503, NULL);
else if (err_type & SI_ET_CONN_ABRT) {
if (s->txn->flags & TX_NOT_FIRST)
send_msg = 0;
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_C, 503, NULL);
}
...
...
else /* SI_ET_CONN_OTHER and others */
http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C, 500, NULL);

if (send_msg)
bo_putchk(s->res.buf, http_error_message(s));
}

-Jarno

[Willy Tarreau]

Hi Jarno,

Post by Jarno Huuskonen
Hi Willy,

Post by Willy Tarreau

Post by Jarno Huuskonen
How should I send the patches ? One commit for
http_server_error/http_get_status_idx changes and tarpit deny_status
parser / doc in another commit ?

Yes that's the prefered way to do it, one commit per architecture or
functional change to ease review and bug tracking later.

I'm including two commits for this feature.
- 0001-MEDIUM-http_error_message-txn-status-http_get_status.patch
Removes second argument from http_error_message and uses
txn->status / http_get_status_idx to map the 200..504 status code
to HTTP_ERR_200..504 enum.
(http_get_status_idx has default return value of HTTP_ERR_500, is
this ok ?)

Yep that sounds OK. Anyway any unhandled error is an haproxy bug so it
indeed should return a 500 :-)

Post by Jarno Huuskonen
- 0002-MINOR-http-request-tarpit-deny_status.patch
Adds http-request tarpit deny_status functionality.
(depends on the 0001-... commit).

Thank you, this all looks good, I've now merged them. I'm even seeing an
opportunity for slightly simplifying things further : http_server_error()
sets the status in txn->status from what it received in argument, but in
practice you had to prepare txn->status before calling it due to
http_error_message(). So I'll now get rid of this assignment and make
http_server_error() retrieve the status from the txn instead, that will
be more consistent. There's a single call place where it was not yet done,
its the server redirect.

Post by Jarno Huuskonen
Alternative implementation (0001-....) for http_return_srv_error
void http_return_srv_error(struct stream *s, struct stream_interface *si)
{
int err_type = si->err_type;
int send_msg = 1;
if (err_type & SI_ET_QUEUE_ABRT)
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_Q, 503, NULL);
else if (err_type & SI_ET_CONN_ABRT) {
if (s->txn->flags & TX_NOT_FIRST)
send_msg = 0;
http_server_error(s, si, SF_ERR_CLICL, SF_FINST_C, 503, NULL);
}
...
...
else /* SI_ET_CONN_OTHER and others */
http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C, 500, NULL);
if (send_msg)
bo_putchk(s->res.buf, http_error_message(s));
}

I'm not at all convinced that it provides any benefit over your current
implementation, so let's stick with what you already have.

Thanks!
Willy