On Wed, Apr 29, 2015 at 10:48 PM, <yla...@apache.org> wrote:
>
> + * mod_proxy_http: Don't establish or reuse a backend connection before
> pre-
> + fetching the request body, so to minimize the delay between it is
> supposed
> + to be alive and the first bytes sent: this is a best effort to prevent
> the
> + backend from closing because of idle or keepalive timeout in the
> meantime.
> + Also, handle a new "proxy-flushall" environment variable which allows to
> + flush any forwarded body data immediately. PR 56541+37920.
> + trunk patch: http://svn.apache.org/r1656259
> + http://svn.apache.org/r1656359 (CHANGES entry)
> + 2.4.x patch: trunk works (modulo CHANGES, docs/log-message-tags)
> + +1: ylavic
> + -0: jim: This seems to be a hit to normal performance, to handle an
> + error and/or non-normal condition. The pre-fetch is
> + expensive, and is always done, even before we know that
> + the backend is available to rec' it. I understand the
> + error described, but is the fix actually worth it (plus
> + it seems to allow for a DDoS vector).
> + ylavic: It seems to me that the problem is real since we reuse the
> + connection before prefetching 16K (either controlled by the
> + client, or by an input filter), we currently always prefetch
> + these bytes already. Regarding performance I don't see any
> + difference (more cycles) compared with the current code.
> + However I think I failed to rebuild the header_brigade when
> + the proxy loop is retried (ping), so I need to rework this.
> + Do you think we'd better remove the prefetch, or maybe just
> + make it nonblocking (by default)?
> + jim: Non-blocking seems the best way to handle...
The prefetch, AIUI, was (before r1656259) and is still is here to help
determine C-L vs T-E (which may not be accepted on the backend side)
or spool (which is costly) for the request, at least with bodies <
64K.
If we were to use non-blocking for it, I guess we would lose this
ability whenever the client's header and body are not sent/received at
the same time...
And I don't think this is necessary, at no cost (AFAICT).
Can you please elaborate on the performance hit and DDoS?
Regarding performance, the new code is just a reordering of the steps
compared to the old one.
Regarding DDoS, you seem to refer to the fact that we now prefetch
(and may spool) the request body before we know whether or not the
backend is available, but is that exploitable without also
controlling/downing the backend?
And in that case, was the previous code much better wrt
ap_discard_request_body() (which is finally called after the error
response)?
I'm not saying this is not a concern, it is surely, and I think the
new changes proposed below can address this issue too.
So I reworked the idea to prefetch before connecting/reusing the
backend/connection (in blocking mode still).
To address the issue regarding the retry (ping) loop and the
{header,input/prefetch}_brigade being consumed on the first try:
- the header brigade can be rebuilt when ping request is retried (by
calling ap_proxy_create_hdrbrgd() again, as before r1656259),
- this is not the case for the input/prefetch brigade (which was also
buggy before r1656259), since we don't want to preventively set it
aside (that was discussed in a previous thread IIRC).
Hence we need to the ability to send the header first, check for
success and continue with the body, or retry with another connection
(and keep the body for it).
To acheive that, we could split stream_reqbody_{cl,chunked}() in two,
the existing ones for the body only, and the new
send_reqhdr_{cl,chunked}() for the header (note that
spool_reqbody_cl() is called during prefetch, so we only need to make
it provide two separate header and input brigades which we can
ap_proxy_pass_brigade() at any time).
Then, once the body is prefetched, the backend is reused/connected, we
could first send the header and either stream/pass the body immediatly
(no ping configured), or go with ap_proxy_http_process_response()
which will stream/pass it once the 100-continue is received, or fail
(and be retried once).
Note that this also allows for end-to-end 100-continue, since we could
forward the 100-continue to the client before streaming its body (no
need to prefetch at all if the client expects 100-continue!).
I agree that this could be done either with prefetch done before or
after connecting/reusing the backend, but since before is (much) less
racy wrt backend and the above can possibly avoid the cons, I'd prefer
to keep it.
I'm almost done with these changes and will propose a patch soon,
thoughts/showstoppers?
