On Mon, Nov 4, 2019 at 10:16 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
>
>
> On 11/04/2019 06:12 PM, Yann Ylavic wrote:
> > On Mon, Nov 4, 2019 at 4:16 PM Ruediger Pluem <rpl...@apache.org> wrote:
> >>
> >> On 11/03/2019 04:48 PM, yla...@apache.org wrote:
> >>>
> >>> + /* XXX: what's the point of "NONE"? We probably should _always_ check
> >>> + * that the client wants an Upgrade..
> >>> + */
> >>
> >> NONE was used to upgrade the client whether it requested it or no idea
> >> what the purpose was.
> >> Maybe Jean-Frederic who introduced it in r1792092 can explain.
> >
> > I asked Jean-Frédéric in another thread, let's wait.
> > I think it was not the correct way to address the issue reported to
> > JBoss, and wish we are not stuck with "NONE" compatibility (i.e. no
> > user..).
> >
> >>
> >>
> >>> + upgrade_method = *worker->s->upgrade ? worker->s->upgrade :
> >>> "WebSocket";
> >>> if (ap_cstr_casecmp(upgrade_method, "NONE") != 0) {
> >>> - const char *upgrade;
> >>> upgrade = apr_table_get(r->headers_in, "Upgrade");
> >>> if (!upgrade || (ap_cstr_casecmp(upgrade, upgrade_method) != 0 &&
> >>> - ap_cstr_casecmp(upgrade_method, "ANY") !=0)) {
> >>> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02900)
> >>> - "declining URL %s (not %s, Upgrade: header is
> >>> %s)",
> >>> - url, upgrade_method, upgrade ? upgrade :
> >>> "missing");
> >>> - return DECLINED;
> >>> + ap_cstr_casecmp(upgrade_method, "ANY") != 0)) {
> >>> + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02900)
> >>> + "require upgrade for URL %s "
> >>> + "(Upgrade header is %s, expecting %s)",
> >>> + url, upgrade ? upgrade : "missing",
> >>> upgrade_method);
> >>> + apr_table_setn(r->err_headers_out, "Connection", "Upgrade");
> >>> + apr_table_setn(r->err_headers_out, "Upgrade",
> >>> upgrade_method);
> >>> + return HTTP_UPGRADE_REQUIRED;
> >>
> >> Why don't we fallback any longer to normal HTTP handling if no upgrade
> >> handler was sent?
> >> This was itruduced by you in r1674632
> >
> > Actually we never fallback despite r1674632 or even later r1776290 by
> > Eric, because previous proxy_trans hook already assigned "ws(s):"
> > scheme to r->filename. So at this point HTTP_UPGRADE_REQUIRED is the
> > best we can do IMHO.
> >
> > I'm about to commit something like the attached patch to make this
> > work, but wouldn't want to handle "NONE" there nor in a future
> > mod_proxy_http change to handle Upgrade..
>
> Just that I get it correct:
>
> This would require something like
>
> ProxyPass / ws://hostname/
> ProxyPass / http://hostname/
>
> to work and would only work with ProxyPass,
Yes, that's the idea. I think the ProxyPass(es) order matters here
though, because proxy_trans() walks the aliases in configuration
order, so if it finds "http" first then "ws" will never be elected.
We could be more clever, not sure it's worth it...
My plan is to handle the "ws(s)" schemes in mod_proxy_http directly,
using the new proxy tunnel interface of this commit to start tunneling
if/when needed (according to the backend). Then mod_proxy_wstunnel
would be obsolete.
> not with RewriteRules and [P] flag, correct?
mod_rewrite wouldn't be concerned indeed, but today one can already:
RewriteCond %{HTTP:Upgrade} websocket [NC]
RewriteRule / ws://hostname/ [P]
to make this work.
Regards,
Yann.
