Hi,
On Fri, Feb 10, Willy Tarreau wrote:
> On Mon, Feb 06, 2017 at 04:33:01PM +0200, Jarno Huuskonen wrote:
> > 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 remember, I have to reread the code :-/
>
> > 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));
>
> Ah you're right apparently this one takes a buffer.
>
> > 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
--
Jarno Huuskonen
