You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2019/11/04 15:16:53 UTC
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunnel.c
modules/proxy/proxy_util.c
On 11/03/2019 04:48 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Sun Nov 3 15:48:53 2019
> New Revision: 1869338
>
> URL: http://svn.apache.org/viewvc?rev=1869338&view=rev
> Log:
> mod_proxy: factorize mod_proxy_{connect,wstunnel} tunneling code in proxy_util.
>
> This commit adds struct proxy_tunnel_rec that contains the fields needed for a
> poll() loop through the filters chains, plus functions ap_proxy_tunnel_create()
> and ap_proxy_tunnel_run() to respectively initialize a tunnel and (re)start it.
>
> Proxy connect and wstunnel modules now make use of this new API to avoid
> duplicating logic and code.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c?rev=1869338&r1=1869337&r2=1869338&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c Sun Nov 3 15:48:53 2019
> @@ -480,17 +299,26 @@ static int proxy_wstunnel_handler(reques
> return DECLINED;
> }
>
> + /* 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.
> + 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
> }
> }
> + else {
> + upgrade = "WebSocket";
> + }
>
> uri = apr_palloc(p, sizeof(*uri));
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02451) "serving URL %s", url);
Regards
Rüdiger
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunnel.c
modules/proxy/proxy_util.c
Posted by Rüdiger Plüm <r....@gmx.de>.
On 11/04/2019 10:55 PM, Yann Ylavic wrote:
> On Mon, Nov 4, 2019 at 10:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>>
>> On 11/04/2019 06:12 PM, Yann Ylavic wrote:
>>> On Mon, Nov 4, 2019 at 4:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>>>>
>>
>> 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.
Exactly order is important here.
>
> 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.
Could make it easier, but not sure if it's worth it.
>
>> 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.
In parallel to
RewriteRule ^/(.*) http://hostname/$1 [P]
Good point. In this case order only matters from performance point of view in order to limit the amount of RewriteRules
executed for the common case.
Regards
Rüdiger
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunnel.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Nov 4, 2019 at 10:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 11/04/2019 06:12 PM, Yann Ylavic wrote:
> > On Mon, Nov 4, 2019 at 4:16 PM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >> On 11/03/2019 04:48 PM, ylavic@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.
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunnel.c
modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/04/2019 06:12 PM, Yann Ylavic wrote:
> On Mon, Nov 4, 2019 at 4:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 11/03/2019 04:48 PM, ylavic@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, not with RewriteRules and [P] flag, correct?
Regards
Rüdiger
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunnel.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Nov 4, 2019 at 4:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 11/03/2019 04:48 PM, ylavic@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..
Regards,
Yann.