On Tue, May 01, 2018 at 05:10:19PM +0200, Tim Düsterhus wrote:
> Willy,
> Am 01.05.2018 um 06:28 schrieb Willy Tarreau:
> >> It might make sense to enlarge the rewrite buffer reservation by
> >> default.
> > 
> > We used to have this a long time ago, the maxrewrite value used to
> > default to half the buffer size. But it caused too many requests to
> > be rejected and became absurd when users configure large buffers.
> > 
> >> I can absolutely imagine that people put in a ton of
> >> information in the times of Content-Security-Policy, Expect-{CT,Staple}
> >> and whatnot.
> > 
> > Yes but even once you put this, you hardly reach 1 kB. Most requests are
> > way below 1 kB headers, at least for performance reasons.
> Just to make sure that we are on the same page: My failure case is not
> the requests, but rather the responses.

Yes I understood this well, sorry for using the wrong term. When I said
"request" I meant "messages" (in either direction).

> As another data point: GitHub adds a 812 Byte Content-Security-Policy
> header to all their responses. In total they send more than 2kB of
> response headers for a logged-out user:

Nice one, though I'd argue that sites which do this know that they
are manipulating large contents (it's visible in the config file and
sometimes they are the ones asking to relax the config parsing rules).
So they're also aware of the need to increase maxrewrite.

> >> Clearly the body must be able to span multiple buffers already,
> >> otherwise I would not be able to send bodies greater than 16kB.
> >> Will it need to allocate more buffers to do the same work, because every
> >> single one is smaller?
> > 
> > No, the body is simply streamed, not stored. If however you need to analyse
> > it (eg for a WAF) then you need to configure a larger buffer so that more
> > of the body can fit.
> > 
> > To give you an idea about how things currently work, when we perform an
> > recvfrom() call, we decide whether we read up to bufsize or up to
> > (bufsize-maxrewrite) depending on whether we are forwarding data or
> > still analysing the message. Thus when we receive a POST, we're not yet
> > forwarding, so up to 15 kB of headers+body are placed into the buffer,
> > leaving 1 kB available to add headers.
> To make sure I understand that correctly (for the response case):
> tune.bufsize:
> Maximum size of response headers.
> tune.bufsize - tune.maxrewrite:
> Maximum supported size of backend generated response headers.
> tune.maxrewrite:
> Maximum size of response headers you are guaranteed to be able to add to
> a response. You might or might not be able to add more depending on the
> speed of the backend sending the body.
> Is that correct?

Absolutely (though it's true both for request and response).

> > After the new native internal HTTP representation is implemented, I think
> > that the need for the maxrewrite will disappear, at the expense of using
> > one buffer for the headers and another one for the body, but we'll see.
> Is this planned for 1.9? I'm aware that plans can change :-)

Yes, but it has ramifications everywhere in the lower layers. You just
made me realize that I wanted to announce the plans a few months ago and
that we were so much busy dealing with complex bugs that I completely
forgot to communicate on this :-(

> > Anyway we need to address the lack of error checking, and I really predict
> > some breakage here :-/
> > 
> I'd start of with *logging* when the call fails for the short term.

Adding extra logs not related to traffic actually makes debugging much
worse because these logs don't end up in the correct file/storage/whatever.
It's much easier to filter on 400/502 and find all the request elements to
understand what happens than seeing a random error out of any context.
Part of the difficulty here is to properly indicate what was attempted
and failed, in complex configs where it's hard to even enumerate rulesets.

Maybe one solution could be to tag transactions that experienced such a
failure and to put a warning on them, that would be reported in the logs
with a "W" flag in the termination codes. It would not indicate what
failed, obviously, but would very quickly raise awareness of the fact
that there is a problem. Usually thanks to the second flag indicating
the transaction state and to the rest of the log, it's easier to follow
the processing and to figure what went wrong. Later once we add the
ability to reject again such failures, we could add an "F" (for "failure"
to process).

What do you think ?


Reply via email to