Hi,
On Mon, Jan 16, Willy Tarreau wrote:
> > 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));
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
--
Jarno Huuskonen
--- a/src/proto_http.c 2017-02-06 15:46:20.440984489 +0200
+++ b/src/proto_http.c 2017-02-06 16:06:13.228343991 +0200
@@ -366,6 +366,26 @@
}
}
+/* This function returns HTTP_ERR_<num> (enum) matching http status code.
+ * Returned value should match codes from http_err_codes.
+ */
+const int http_get_status_idx(unsigned int status)
+{
+ switch (status) {
+ case 200: return HTTP_ERR_200;
+ case 400: return HTTP_ERR_400;
+ case 403: return HTTP_ERR_403;
+ case 405: return HTTP_ERR_405;
+ case 408: return HTTP_ERR_408;
+ case 429: return HTTP_ERR_429;
+ case 500: return HTTP_ERR_500;
+ case 502: return HTTP_ERR_502;
+ case 503: return HTTP_ERR_503;
+ case 504: return HTTP_ERR_504;
+ default: return HTTP_ERR_500;
+ }
+}
+
void init_proto_http()
{
int i;
@@ -1031,10 +1051,10 @@
channel_erase(si_oc(si));
channel_auto_close(si_ic(si));
channel_auto_read(si_ic(si));
- if (status > 0 && msg) {
+ if (status > 0)
s->txn->status = status;
+ if (msg)
bo_inject(si_ic(si), msg->str, msg->len);
- }
if (!(s->flags & SF_ERR_MASK))
s->flags |= err;
if (!(s->flags & SF_FINST_MASK))
@@ -1045,8 +1065,10 @@
* and message.
*/
-struct chunk *http_error_message(struct stream *s, int msgnum)
+struct chunk *http_error_message(struct stream *s)
{
+ const int msgnum = http_get_status_idx(s->txn->status);
+
if (s->be->errmsg[msgnum].str)
return &s->be->errmsg[msgnum];
else if (strm_fe(s)->errmsg[msgnum].str)
@@ -1259,33 +1281,23 @@
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, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_CLICL, SF_FINST_Q, 503, NULL);
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, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_CLICL, SF_FINST_C, 503, NULL);
else if (err_type & SI_ET_QUEUE_TO)
- http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_Q,
- 503, http_error_message(s, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_Q, 503, NULL);
else if (err_type & SI_ET_QUEUE_ERR)
- http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_Q,
- 503, http_error_message(s, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_Q, 503, NULL);
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, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_SRVTO, SF_FINST_C, 503, NULL);
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, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_SRVCL, SF_FINST_C, 503, NULL);
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, HTTP_ERR_503));
+ http_server_error(s, si, SF_ERR_RESOURCE, SF_FINST_C, 503,
NULL);
else /* SI_ET_CONN_OTHER and others */
- http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C,
- 500, http_error_message(s, HTTP_ERR_500));
+ http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C, 500,
NULL);
+
+ bo_putchk(s->res.buf, http_error_message(s));
}
extern const char sess_term_cond[8];
@@ -2772,7 +2784,7 @@
txn->status = 408;
msg->err_state = msg->msg_state;
msg->msg_state = HTTP_MSG_ERROR;
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_408));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
req->analysers &= AN_REQ_FLT_END;
stream_inc_http_req_ctr(s);
@@ -2802,7 +2814,7 @@
txn->status = 400;
msg->err_state = msg->msg_state;
msg->msg_state = HTTP_MSG_ERROR;
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_400));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
req->analysers &= AN_REQ_FLT_END;
stream_inc_http_err_ctr(s);
stream_inc_http_req_ctr(s);
@@ -2931,7 +2943,7 @@
if (ret) {
/* we fail this request, let's return 503
service unavail */
txn->status = 503;
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_503));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_LOCAL; /* we don't
want a real error here */
goto return_prx_cond;
@@ -2940,7 +2952,7 @@
/* nothing to fail, let's reply normaly */
txn->status = 200;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_200));
+ http_reply_and_close(s, txn->status, http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_LOCAL; /* we don't want a real error
here */
goto return_prx_cond;
@@ -3175,7 +3187,7 @@
txn->req.err_state = txn->req.msg_state;
txn->req.msg_state = HTTP_MSG_ERROR;
txn->status = 400;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_400));
+ http_reply_and_close(s, txn->status, http_error_message(s));
sess->fe->fe_counters.failed_req++;
if (sess->listener->counters)
@@ -4346,7 +4358,7 @@
if (unlikely(!stream_int_register_handler(&s->si[1],
objt_applet(s->target)))) {
txn->status = 500;
s->logs.tv_request = now;
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_500));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_RESOURCE;
@@ -4373,9 +4385,11 @@
if (txn->flags & TX_CLDENY)
goto deny;
- if (txn->flags & TX_CLTARPIT)
+ if (txn->flags & TX_CLTARPIT) {
+ deny_status = HTTP_ERR_500;
goto tarpit;
}
+ }
/* add request headers from the rule sets in the same order */
list_for_each_entry(wl, &px->req_add, list) {
@@ -4463,6 +4477,8 @@
if (s->be->cookie_name || sess->fe->capture_name)
manage_client_side_cookies(s, req);
+ txn->status = http_err_codes[deny_status];
+
req->analysers &= AN_REQ_FLT_END; /* remove switching rules etc... */
req->analysers |= AN_REQ_HTTP_TARPIT;
req->analyse_exp = tick_add_ifset(now_ms, s->be->timeout.tarpit);
@@ -4486,7 +4502,7 @@
txn->flags |= TX_CLDENY;
txn->status = http_err_codes[deny_status];
s->logs.tv_request = now;
- http_reply_and_close(s, txn->status, http_error_message(s,
deny_status));
+ http_reply_and_close(s, txn->status, http_error_message(s));
stream_inc_http_err_ctr(s);
sess->fe->fe_counters.denied_req++;
if (sess->fe != s->be)
@@ -4507,7 +4523,7 @@
txn->req.err_state = txn->req.msg_state;
txn->req.msg_state = HTTP_MSG_ERROR;
txn->status = 400;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_400));
+ http_reply_and_close(s, txn->status, http_error_message(s));
sess->fe->fe_counters.failed_req++;
if (sess->listener->counters)
@@ -4577,7 +4593,7 @@
txn->req.msg_state = HTTP_MSG_ERROR;
txn->status = 500;
req->analysers &= AN_REQ_FLT_END;
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_500));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_RESOURCE;
@@ -4853,7 +4869,7 @@
txn->req.msg_state = HTTP_MSG_ERROR;
txn->status = 400;
req->analysers &= AN_REQ_FLT_END;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_400));
+ http_reply_and_close(s, txn->status, http_error_message(s));
sess->fe->fe_counters.failed_req++;
if (sess->listener->counters)
@@ -4892,9 +4908,8 @@
*/
s->logs.t_queue = tv_ms_elapsed(&s->logs.tv_accept, &now);
- txn->status = 500;
if (!(req->flags & CF_READ_ERROR))
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_500));
+ http_reply_and_close(s, txn->status, http_error_message(s));
req->analysers &= AN_REQ_FLT_END;
req->analyse_exp = TICK_ETERNITY;
@@ -5010,7 +5025,7 @@
if ((req->flags & CF_READ_TIMEOUT) || tick_is_expired(req->analyse_exp,
now_ms)) {
txn->status = 408;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_408));
+ http_reply_and_close(s, txn->status, http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_CLITO;
@@ -5044,7 +5059,7 @@
txn->req.err_state = txn->req.msg_state;
txn->req.msg_state = HTTP_MSG_ERROR;
txn->status = 400;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_400));
+ http_reply_and_close(s, txn->status, http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_PRXCOND;
@@ -5849,7 +5864,7 @@
http_reply_and_close(s, txn->status, NULL);
} else {
txn->status = 400;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_400));
+ http_reply_and_close(s, txn->status, http_error_message(s));
}
req->analysers &= AN_REQ_FLT_END;
s->res.analysers &= AN_RES_FLT_END; /* we're in data phase, we want to
abort both directions */
@@ -5872,7 +5887,7 @@
http_reply_and_close(s, txn->status, NULL);
} else {
txn->status = 502;
- http_reply_and_close(s, txn->status, http_error_message(s,
HTTP_ERR_502));
+ http_reply_and_close(s, txn->status, http_error_message(s));
}
req->analysers &= AN_REQ_FLT_END;
s->res.analysers &= AN_RES_FLT_END; /* we're in data phase, we want to
abort both directions */
@@ -6015,7 +6030,7 @@
txn->status = 502;
s->si[1].flags |= SI_FL_NOLINGER;
channel_truncate(rep);
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_502));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_PRXCOND;
@@ -6050,7 +6065,7 @@
txn->status = 502;
s->si[1].flags |= SI_FL_NOLINGER;
channel_truncate(rep);
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_502));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_SRVCL;
@@ -6075,7 +6090,7 @@
txn->status = 504;
s->si[1].flags |= SI_FL_NOLINGER;
channel_truncate(rep);
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_504));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_SRVTO;
@@ -6096,7 +6111,7 @@
txn->status = 400;
channel_truncate(rep);
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_400));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_CLICL;
@@ -6125,7 +6140,7 @@
txn->status = 502;
s->si[1].flags |= SI_FL_NOLINGER;
channel_truncate(rep);
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_502));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_SRVCL;
@@ -6607,7 +6622,7 @@
s->logs.t_data = -1; /* was not a valid
response */
s->si[1].flags |= SI_FL_NOLINGER;
channel_truncate(rep);
- http_reply_and_close(s, txn->status,
http_error_message(s, HTTP_ERR_502));
+ http_reply_and_close(s, txn->status,
http_error_message(s));
if (!(s->flags & SF_ERR_MASK))
s->flags |= SF_ERR_PRXCOND;
if (!(s->flags & SF_FINST_MASK))
--- a/src/filters.c 2017-02-06 15:46:08.325744813 +0200
+++ b/src/filters.c 2017-02-05 21:01:51.282058403 +0200
@@ -1069,7 +1069,7 @@
http_reply_and_close(s, s->txn->status, NULL);
else {
s->txn->status = 400;
- http_reply_and_close(s, 400, http_error_message(s,
HTTP_ERR_400));
+ http_reply_and_close(s, 400, http_error_message(s));
}
}
--- a/include/proto/proto_http.h 2017-02-06 15:46:08.324744793 +0200
+++ b/include/proto/proto_http.h 2017-02-05 21:01:51.279058349 +0200
@@ -136,7 +136,7 @@
void free_http_req_rules(struct list *r);
void free_http_res_rules(struct list *r);
void http_reply_and_close(struct stream *s, short status, struct chunk *msg);
-struct chunk *http_error_message(struct stream *s, int msgnum);
+struct chunk *http_error_message(struct stream *s);
struct redirect_rule *http_parse_redirect_rule(const char *file, int linenum,
struct proxy *curproxy,
const char **args, char
**errmsg, int use_fmt, int dir);
int smp_fetch_cookie(const struct arg *args, struct sample *smp, const char
*kw, void *private);
