httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: Upgrade Summary
Date Fri, 11 Dec 2015 13:18:44 GMT
On Fri, Dec 11, 2015 at 10:20 AM, Stefan Eissing
<stefan.eissing@greenbytes.de> wrote:
>
>> Am 11.12.2015 um 02:22 schrieb Yann Ylavic <ylavic.dev@gmail.com>:
>>
>> On Thu, Dec 10, 2015 at 11:46 AM, Stefan Eissing
>> <stefan.eissing@greenbytes.de> wrote:
>>> Given all the input on this thread, I arrive at the following pseudo code:
>>
>> Thanks for _compiling_ this thread, quite exhaustive :)
>
> Code often says more than a thousand...tokens.

++1 (though I won't propose code this time, possibly soon :p )

>
> Currently, TLS and current core step on each others toes when it
> comes to the Upgrade response header. This we should fix.

Agreed.

>
> If it turns out that TLS, h2c and websocket all need the actual
> switching at different phases of request processing, then we need
> to accomodate for that.
>
> What I can see:
> - TLS and h2c should switch in post_read_req
> - websocket should switch just before the handler phase

Yes, they could use one of those phase, their choice to hook wherever
they need to.

In my proposal, they'd rely on the core's Upgrade output filter being
in the chain (which will be the case since the core's
post_read_request (really-)first hook would have run).
The Upgrade output filter would catch the first brigade sent to the
client and issue the 101-Upgrade response first before removing itself
from the chain and forwarding the brigade.

- TLS (post_read_request):
0. make a decision when a request body exists (either bail out,
discard or return 413),
1. enable SSL/TLS engine and filters,
2. call ap_pass_brigade(FLUSH) for the Upgrade to occur,
3. return DECLINED.

- h2c/ws (wherever, post_read_request/[pre_]handler):
0. make a decision when a request body exists (either bail out,
discard or use it!),
1. run their process_request(),
2. return DONE.

As far as they use ap_{get,pass}_brigade() for reading/writing data
this should work.
They wouldn't do anything (ie. return DECLINED early) if
r->selected_protocol != their protocol.
If some headers are needed in the 101 response, one can fill in
r->headers_out for the core Upgrade output filter to use that when
Upgrading.

>
> Protocol implementations should make up their minds in the "propose" phase, I think,
> because once a protocol gets selected, the upgrade *should* succeed unless the
> connection catches fire or something. Not upgrading is better than failing.

Yes, all could be based on (communication via) current/selected proto
(set by the core!) for decisions, each module can use that to
determine if it was elected or not.
Whether the core Upgrade output filter would do the Upgrade or not can
be driven by a r->flag too (or anything else, special bucket, ...),
the module really handling the Protocols would set it unless the offer
is finally DECLINED (for some reason).

>> +        && ap_find_token(r->pool, upgrade, "TLS")
>> +        && !ap_has_request_body(r)) {
>> []
>
> I think the first way, don't Upgrade if you know it will not work, is better.

Agreed, let's do that in ssl_hook_ReadReq(), along with:
+        && !ap_has_request_body(r)
+        && !strcasecmp(r->selected_protocol, "TLS")) {
...

>>> 1. Post Read Request Hook:
>>>
>>>        if (Upgrade: request header present) {
>>>                collect protocol proposals;
>>>                ps = protocol with highest preference from proposals;
>>>                if (ps && ps != current) {
>>
>> Here I would use ap_add_output_filter(switch_protocol_filter, r); with
>> switch_protocol_filter() which would flush out the 101 response (or
>> not) based on r->need_upgrade and r->current_protocol, before any
>> Protocols data.
>
> I am not convinced that a flushing is necessary. If a protocol needs
> this, it can just pass out a flush bucket, or?

Yes, this is what I meant, as maybe better explained above.

>
> h2c needs to receive request body separate, get an indication
> that EOS for that body has been reached,

ap_discard_request_body()?

> and then continue reading
> HTTP/2 protocol data on the connection. I am not sure what is the
> best way to do that and, when upgrading on post_read_req hook, if
> all necessary request filters are in place by then.

I think so with my proposal, unless I'm missing something (which is
ofter the case ;)

>
>> For the headers in the 101 response, another hook called from
>> switch_protocol_filter() could be used.
>
> That'd be the pre_switch hook Jacob and I talked about and where he
> posted his implementation yesterday.

We wouldn't need it actually, as said above, why not simply use
r->headers_out while we are still HTTP/1...


Thanks for the feedbacks, Stefan.

Mime
View raw message