You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2019/11/03 15:48:53 UTC
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
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/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1869338&r1=1869337&r2=1869338&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Sun Nov 3 15:48:53 2019
@@ -1,6 +1,9 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1
+ *) mod_proxy: Put mod_proxy_{connect,wstunnel} tunneling code in common in
+ proxy_util. [Yann Ylavic]
+
*) mod_proxy_http: Fix the forwarding of requests with content body when a
balancer member is unavailable; the retry on the next member was issued
with an empty body (regression introduced in 2.4.41). [Yann Ylavic]
Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1869338&r1=1869337&r2=1869338&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
+++ httpd/httpd/trunk/docs/log-message-tags/next-number Sun Nov 3 15:48:53 2019
@@ -1 +1 @@
-10208
+10224
Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1869338&r1=1869337&r2=1869338&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Sun Nov 3 15:48:53 2019
@@ -615,6 +615,8 @@
* 20190312.3 (2.5.1-dev) Add forward_100_continue{,_set} to proxy_dir_conf
* 20190312.4 (2.5.1-dev) Add add dns_pool to proxy_conn_pool and define
* AP_VOLATILIZE_T.
+ * 20190312.5 (2.5.1-dev) Add proxy_tunnel_rec, ap_proxy_tunnel_create()
+ * and ap_proxy_tunnel_run() to proxy_util.
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -622,7 +624,7 @@
#ifndef MODULE_MAGIC_NUMBER_MAJOR
#define MODULE_MAGIC_NUMBER_MAJOR 20190312
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 4 /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 5 /* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1869338&r1=1869337&r2=1869338&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Sun Nov 3 15:48:53 2019
@@ -1213,6 +1213,40 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade
conn_rec *origin, apr_bucket_brigade *bb,
int flush);
+typedef struct {
+ request_rec *r;
+ conn_rec *origin;
+ apr_pollset_t *pollset;
+ apr_array_header_t *pfds;
+ apr_interval_time_t timeout;
+ apr_bucket_brigade *bb_i;
+ apr_bucket_brigade *bb_o;
+ int replied;
+} proxy_tunnel_rec;
+
+/**
+ * Create a tunnel, to be activated by ap_proxy_tunnel_run().
+ * @param tunnel tunnel created
+ * @param r client request
+ * @param origin backend connection
+ * @return APR_SUCCESS or error status
+ */
+PROXY_DECLARE(apr_status_t) ap_proxy_tunnel_create(proxy_tunnel_rec **tunnel,
+ request_rec *r,
+ conn_rec *origin);
+
+/**
+ * Forward anything from either side of the tunnel to the other,
+ * until one end aborts or a polling timeout/error occurs.
+ * @param tunnel tunnel created
+ * @return OK: closed/aborted on one side,
+ * HTTP_GATEWAY_TIME_OUT: polling timeout,
+ * HTTP_INTERNAL_SERVER_ERROR: polling error,
+ * HTTP_BAD_GATEWAY: no response from backend, ever,
+ * so client may expect one still.
+ */
+PROXY_DECLARE(int) ap_proxy_tunnel_run(proxy_tunnel_rec *tunnel);
+
/**
* Clear the headers referenced by the Connection header from the given
* table, and remove the Connection header.
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c?rev=1869338&r1=1869337&r2=1869338&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c Sun Nov 3 15:48:53 2019
@@ -156,25 +156,19 @@ static int proxy_connect_handler(request
apr_socket_t *sock;
conn_rec *c = r->connection;
conn_rec *backconn;
- int done = 0;
- apr_bucket_brigade *bb_front;
- apr_bucket_brigade *bb_back;
apr_status_t rv;
apr_size_t nbytes;
char buffer[HUGE_STRING_LEN];
- apr_socket_t *client_socket = ap_get_conn_socket(c);
+
+ apr_bucket_brigade *bb;
+ proxy_tunnel_rec *tunnel;
int failed, rc;
- apr_pollset_t *pollset;
- apr_pollfd_t pollfd;
- const apr_pollfd_t *signalled;
- apr_int32_t pollcnt, pi;
- apr_int16_t pollevent;
- apr_sockaddr_t *nexthop;
apr_uri_t uri;
const char *connectname;
apr_port_t connectport = 0;
+ apr_sockaddr_t *nexthop;
/* is this for us? */
if (r->method_number != M_CONNECT) {
@@ -261,28 +255,6 @@ static int proxy_connect_handler(request
}
}
- /* setup polling for connection */
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "setting up poll()");
-
- if ((rv = apr_pollset_create(&pollset, 2, r->pool, 0)) != APR_SUCCESS) {
- apr_socket_close(sock);
- ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01020)
- "error apr_pollset_create()");
- return HTTP_INTERNAL_SERVER_ERROR;
- }
-
- /* Add client side to the poll */
- pollfd.p = r->pool;
- pollfd.desc_type = APR_POLL_SOCKET;
- pollfd.reqevents = APR_POLLIN | APR_POLLHUP;
- pollfd.desc.s = client_socket;
- pollfd.client_data = NULL;
- apr_pollset_add(pollset, &pollfd);
-
- /* Add the server side to the poll */
- pollfd.desc.s = sock;
- apr_pollset_add(pollset, &pollfd);
-
/*
* Step Three: Send the Request
*
@@ -315,9 +287,7 @@ static int proxy_connect_handler(request
apr_table_setn(r->notes, "proxy-source-port", apr_psprintf(r->pool, "%hu",
backconn->local_addr->port));
-
- bb_front = apr_brigade_create(p, c->bucket_alloc);
- bb_back = apr_brigade_create(p, backconn->bucket_alloc);
+ bb = apr_brigade_create(p, c->bucket_alloc);
/* If we are connecting through a remote proxy, we need to pass
* the CONNECT request on to it.
@@ -327,24 +297,24 @@ static int proxy_connect_handler(request
*/
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
"sending the CONNECT request to the remote proxy");
- ap_fprintf(backconn->output_filters, bb_back,
+ ap_fprintf(backconn->output_filters, bb,
"CONNECT %s HTTP/1.0" CRLF, r->uri);
- ap_fprintf(backconn->output_filters, bb_back,
+ ap_fprintf(backconn->output_filters, bb,
"Proxy-agent: %s" CRLF CRLF, ap_get_server_banner());
- ap_fflush(backconn->output_filters, bb_back);
+ ap_fflush(backconn->output_filters, bb);
}
else {
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "Returning 200 OK");
nbytes = apr_snprintf(buffer, sizeof(buffer),
"HTTP/1.0 200 Connection Established" CRLF);
ap_xlate_proto_to_ascii(buffer, nbytes);
- ap_fwrite(c->output_filters, bb_front, buffer, nbytes);
+ ap_fwrite(c->output_filters, bb, buffer, nbytes);
nbytes = apr_snprintf(buffer, sizeof(buffer),
"Proxy-agent: %s" CRLF CRLF,
ap_get_server_banner());
ap_xlate_proto_to_ascii(buffer, nbytes);
- ap_fwrite(c->output_filters, bb_front, buffer, nbytes);
- ap_fflush(c->output_filters, bb_front);
+ ap_fwrite(c->output_filters, bb, buffer, nbytes);
+ ap_fflush(c->output_filters, bb);
#if 0
/* This is safer code, but it doesn't work yet. I'm leaving it
* here so that I can fix it later.
@@ -355,8 +325,7 @@ static int proxy_connect_handler(request
ap_rflush(r);
#endif
}
-
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "setting up poll()");
+ apr_brigade_cleanup(bb);
/*
* Step Four: Handle Data Transfer
@@ -364,83 +333,25 @@ static int proxy_connect_handler(request
* Handle two way transfer of data over the socket (this is a tunnel).
*/
- /* we are now acting as a tunnel - the input/output filter stacks should
- * not contain any non-connection filters.
- */
- r->output_filters = c->output_filters;
- r->proto_output_filters = c->output_filters;
- r->input_filters = c->input_filters;
- r->proto_input_filters = c->input_filters;
-/* r->sent_bodyct = 1;*/
-
- do { /* Loop until done (one side closes the connection, or an error) */
- rv = apr_pollset_poll(pollset, -1, &pollcnt, &signalled);
- if (rv != APR_SUCCESS) {
- if (APR_STATUS_IS_EINTR(rv)) {
- continue;
- }
- apr_socket_close(sock);
- ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01023) "error apr_poll()");
- return HTTP_INTERNAL_SERVER_ERROR;
- }
-
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01024)
- "woke from poll(), i=%d", pollcnt);
+ /* r->sent_bodyct = 1; */
- for (pi = 0; pi < pollcnt; pi++) {
- const apr_pollfd_t *cur = &signalled[pi];
-
- if (cur->desc.s == sock) {
- pollevent = cur->rtnevents;
- if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01025)
- "backend was readable");
- done |= ap_proxy_transfer_between_connections(r, backconn,
- c, bb_back,
- bb_front,
- "backend", NULL,
- CONN_BLKSZ, 1)
- != APR_SUCCESS;
- }
- else if (pollevent & APR_POLLERR) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01026)
- "err on backend connection");
- backconn->aborted = 1;
- done = 1;
- }
- }
- else if (cur->desc.s == client_socket) {
- pollevent = cur->rtnevents;
- if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01027)
- "client was readable");
- done |= ap_proxy_transfer_between_connections(r, c,
- backconn,
- bb_front,
- bb_back,
- "client",
- NULL,
- CONN_BLKSZ, 1)
- != APR_SUCCESS;
- }
- else if (pollevent & APR_POLLERR) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02827)
- "err on client connection");
- c->aborted = 1;
- done = 1;
- }
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01028)
- "unknown socket in pollset");
- done = 1;
- }
+ rv = ap_proxy_tunnel_create(&tunnel, r, backconn);
+ if (rv != APR_SUCCESS) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10208)
+ "can't create tunnel for %pI (%s)",
+ nexthop, connectname);
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ rc = ap_proxy_tunnel_run(tunnel);
+ if (ap_is_HTTP_ERROR(rc)) {
+ /* Don't send an error page if we sent data already */
+ if (proxyport && !tunnel->replied) {
+ return rc;
}
- } while (!done);
-
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
- "finished with poll() - cleaning up");
+ /* Custom log may need this, still */
+ r->status = rc;
+ }
/*
* Step Five: Clean Up
@@ -453,8 +364,6 @@ static int proxy_connect_handler(request
else
ap_lingering_close(backconn);
- c->keepalive = AP_CONN_CLOSE;
-
return OK;
}
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
@@ -27,141 +27,30 @@ typedef struct {
typedef struct ws_baton_t {
request_rec *r;
- proxy_conn_rec *proxy_connrec;
- apr_socket_t *server_soc;
- apr_socket_t *client_soc;
- apr_pollset_t *pollset;
- apr_bucket_brigade *bb_i;
- apr_bucket_brigade *bb_o;
- apr_pool_t *subpool; /* cleared before each suspend, destroyed when request ends */
- char *scheme; /* required to release the proxy connection */
+ proxy_conn_rec *backend;
+ proxy_tunnel_rec *tunnel;
+ const char *scheme;
} ws_baton_t;
static void proxy_wstunnel_callback(void *b);
-static int proxy_wstunnel_pump(ws_baton_t *baton, apr_time_t timeout, int try_poll)
+static int proxy_wstunnel_pump(ws_baton_t *baton, int async)
{
- request_rec *r = baton->r;
- conn_rec *c = r->connection;
- proxy_conn_rec *conn = baton->proxy_connrec;
- apr_socket_t *sock = conn->sock;
- conn_rec *backconn = conn->connection;
- const apr_pollfd_t *signalled;
- apr_int32_t pollcnt, pi;
- apr_int16_t pollevent;
- apr_pollset_t *pollset = baton->pollset;
- apr_socket_t *client_socket = baton->client_soc;
- apr_status_t rv;
- apr_bucket_brigade *bb_i = baton->bb_i;
- apr_bucket_brigade *bb_o = baton->bb_o;
- int done = 0, replied = 0;
-
- do {
- rv = apr_pollset_poll(pollset, timeout, &pollcnt, &signalled);
- if (rv != APR_SUCCESS) {
- if (APR_STATUS_IS_EINTR(rv)) {
- continue;
- }
- else if (APR_STATUS_IS_TIMEUP(rv)) {
- if (try_poll) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02542) "Attempting to go async");
- return SUSPENDED;
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, r, APLOGNO(10031) "Closing idle tunnel");
- return HTTP_REQUEST_TIME_OUT;
- }
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02444) "error apr_poll()");
- return HTTP_INTERNAL_SERVER_ERROR;
- }
- }
-
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(02445)
- "woke from poll(), i=%d", pollcnt);
-
- for (pi = 0; pi < pollcnt; pi++) {
- const apr_pollfd_t *cur = &signalled[pi];
-
- if (cur->desc.s == sock) {
- pollevent = cur->rtnevents;
- if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(02446)
- "backend was readable");
- done |= ap_proxy_transfer_between_connections(r, backconn,
- c, bb_i, bb_o,
- "backend",
- &replied,
- AP_IOBUFSIZE,
- 0)
- != APR_SUCCESS;
- }
- else if (pollevent & APR_POLLERR) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02447)
- "error on backend connection");
- backconn->aborted = 1;
- done = 1;
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02605)
- "unknown event on backend connection %d", pollevent);
- done = 1;
- }
- }
- else if (cur->desc.s == client_socket) {
- pollevent = cur->rtnevents;
- if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(02448)
- "client was readable");
- done |= ap_proxy_transfer_between_connections(r, c, backconn,
- bb_o, bb_i,
- "client", NULL,
- AP_IOBUFSIZE,
- 0)
- != APR_SUCCESS;
- }
- else if (pollevent & APR_POLLERR) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02607)
- "error on client connection");
- c->aborted = 1;
- done = 1;
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02606)
- "unknown event on client conn %d", pollevent);
- done = 1;
- }
- }
- else {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02449)
- "unknown socket in pollset");
- done = 1;
- }
-
- }
- } while (!done);
-
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
- "finished with poll() - cleaning up");
-
- if (!replied) {
- return HTTP_BAD_GATEWAY;
- }
- else {
- return OK;
+ int status = ap_proxy_tunnel_run(baton->tunnel);
+ if (async && status == HTTP_GATEWAY_TIME_OUT) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, baton->r,
+ APLOGNO(02542) "Attempting to go async");
+ status = SUSPENDED;
}
+ return status;
}
static void proxy_wstunnel_finish(ws_baton_t *baton)
{
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, baton->r, "proxy_wstunnel_finish");
- baton->proxy_connrec->close = 1; /* new handshake expected on each back-conn */
- baton->r->connection->keepalive = AP_CONN_CLOSE;
- ap_proxy_release_connection(baton->scheme, baton->proxy_connrec, baton->r->server);
+ ap_proxy_release_connection(baton->scheme, baton->backend, baton->r->server);
ap_finalize_request_protocol(baton->r);
ap_lingering_close(baton->r->connection);
- apr_socket_close(baton->client_soc);
ap_mpm_resume_suspended(baton->r->connection);
ap_process_request_after_handler(baton->r); /* don't touch baton or r after here */
}
@@ -185,34 +74,18 @@ static void proxy_wstunnel_cancel_callba
*/
static void proxy_wstunnel_callback(void *b)
{
- int status;
ws_baton_t *baton = (ws_baton_t*)b;
- proxyws_dir_conf *dconf = ap_get_module_config(baton->r->per_dir_config, &proxy_wstunnel_module);
- apr_pool_clear(baton->subpool);
- status = proxy_wstunnel_pump(baton, dconf->async_delay, dconf->mpm_can_poll);
+ proxyws_dir_conf *dconf = ap_get_module_config(baton->r->per_dir_config,
+ &proxy_wstunnel_module);
+ int status = proxy_wstunnel_pump(baton, 1);
if (status == SUSPENDED) {
- apr_pollfd_t *pfd;
-
- apr_array_header_t *pfds = apr_array_make(baton->subpool, 2, sizeof(apr_pollfd_t));
-
- pfd = apr_array_push(pfds);
- pfd->desc_type = APR_POLL_SOCKET;
- pfd->reqevents = APR_POLLIN | APR_POLLERR | APR_POLLHUP;
- pfd->desc.s = baton->client_soc;
- pfd->p = baton->subpool;
-
- pfd = apr_array_push(pfds);
- pfd->desc_type = APR_POLL_SOCKET;
- pfd->reqevents = APR_POLLIN | APR_POLLERR | APR_POLLHUP;
- pfd->desc.s = baton->server_soc;
- pfd->p = baton->subpool;
-
- ap_mpm_register_poll_callback_timeout(pfds,
+ ap_mpm_register_poll_callback_timeout(baton->tunnel->pfds,
proxy_wstunnel_callback,
proxy_wstunnel_cancel_callback,
baton,
dconf->idle_timeout);
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, baton->r, "proxy_wstunnel_callback suspend");
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, baton->r,
+ "proxy_wstunnel_callback suspend");
}
else {
proxy_wstunnel_finish(baton);
@@ -302,30 +175,26 @@ static int proxy_wstunnel_request(apr_po
proxy_worker *worker,
proxy_server_conf *conf,
apr_uri_t *uri,
- char *url, char *server_portstr, char *scheme)
+ char *url, char *server_portstr, char *scheme,
+ const char *upgrade)
{
apr_status_t rv;
- apr_pollset_t *pollset;
- apr_pollfd_t pollfd;
conn_rec *c = r->connection;
- apr_socket_t *sock = conn->sock;
conn_rec *backconn = conn->connection;
+ proxyws_dir_conf *dconf = ap_get_module_config(r->per_dir_config,
+ &proxy_wstunnel_module);
+ proxy_tunnel_rec *tunnel = NULL;
char *buf;
apr_bucket_brigade *header_brigade;
apr_bucket *e;
char *old_cl_val = NULL;
char *old_te_val = NULL;
- apr_bucket_brigade *bb = apr_brigade_create(p, c->bucket_alloc);
- apr_socket_t *client_socket = ap_get_conn_socket(c);
- ws_baton_t *baton = apr_pcalloc(r->pool, sizeof(ws_baton_t));
+ ws_baton_t *baton;
int status;
- proxyws_dir_conf *dconf = ap_get_module_config(r->per_dir_config, &proxy_wstunnel_module);
- const char *upgrade_method = *worker->s->upgrade ? worker->s->upgrade : "WebSocket";
-
- header_brigade = apr_brigade_create(p, backconn->bucket_alloc);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending request");
+ header_brigade = apr_brigade_create(p, backconn->bucket_alloc);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
@@ -333,15 +202,9 @@ static int proxy_wstunnel_request(apr_po
return rv;
}
- if (ap_cstr_casecmp(upgrade_method, "NONE") == 0) {
- buf = apr_pstrdup(p, "Upgrade: WebSocket" CRLF "Connection: Upgrade" CRLF CRLF);
- } else if (ap_cstr_casecmp(upgrade_method, "ANY") == 0) {
- const char *upgrade;
- upgrade = apr_table_get(r->headers_in, "Upgrade");
- buf = apr_pstrcat(p, "Upgrade: ", upgrade, CRLF "Connection: Upgrade" CRLF CRLF, NULL);
- } else {
- buf = apr_pstrcat(p, "Upgrade: ", upgrade_method, CRLF "Connection: Upgrade" CRLF CRLF, NULL);
- }
+ buf = apr_pstrcat(p, "Upgrade: ", upgrade, CRLF
+ "Connection: Upgrade" CRLF
+ CRLF, NULL);
ap_xlate_proto_to_ascii(buf, strlen(buf));
e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(header_brigade, e);
@@ -352,76 +215,30 @@ static int proxy_wstunnel_request(apr_po
apr_brigade_cleanup(header_brigade);
- ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "setting up poll()");
+ ap_remove_input_filter_byhandle(c->input_filters, "reqtimeout");
- if ((rv = apr_pollset_create(&pollset, 2, p, 0)) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02443)
- "error apr_pollset_create()");
+ rv = ap_proxy_tunnel_create(&tunnel, r, conn->connection);
+ if (rv != APR_SUCCESS) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02543)
+ "error creating websocket tunnel");
return HTTP_INTERNAL_SERVER_ERROR;
}
-#if 0
- apr_socket_opt_set(sock, APR_SO_NONBLOCK, 1);
- apr_socket_opt_set(sock, APR_SO_KEEPALIVE, 1);
- apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1);
- apr_socket_opt_set(client_socket, APR_SO_KEEPALIVE, 1);
-#endif
-
- pollfd.p = p;
- pollfd.desc_type = APR_POLL_SOCKET;
- pollfd.reqevents = APR_POLLIN | APR_POLLHUP;
- pollfd.desc.s = sock;
- pollfd.client_data = NULL;
- apr_pollset_add(pollset, &pollfd);
-
- pollfd.desc.s = client_socket;
- apr_pollset_add(pollset, &pollfd);
-
- ap_remove_input_filter_byhandle(c->input_filters, "reqtimeout");
-
- r->output_filters = c->output_filters;
- r->proto_output_filters = c->output_filters;
- r->input_filters = c->input_filters;
- r->proto_input_filters = c->input_filters;
-
- /* This handler should take care of the entire connection; make it so that
- * nothing else is attempted on the connection after returning. */
- c->keepalive = AP_CONN_CLOSE;
-
+ baton = apr_pcalloc(r->pool, sizeof(*baton));
baton->r = r;
- baton->pollset = pollset;
- baton->client_soc = client_socket;
- baton->server_soc = sock;
- baton->proxy_connrec = conn;
- baton->bb_o = bb;
- baton->bb_i = header_brigade;
+ baton->backend = conn;
+ baton->tunnel = tunnel;
baton->scheme = scheme;
- apr_pool_create(&baton->subpool, r->pool);
if (!dconf->mpm_can_poll) {
- status = proxy_wstunnel_pump(baton, dconf->idle_timeout, dconf->mpm_can_poll);
+ tunnel->timeout = dconf->idle_timeout;
+ status = proxy_wstunnel_pump(baton, 0);
}
else {
- status = proxy_wstunnel_pump(baton, dconf->async_delay, dconf->mpm_can_poll);
- apr_pool_clear(baton->subpool);
+ tunnel->timeout = dconf->async_delay;
+ status = proxy_wstunnel_pump(baton, 1);
if (status == SUSPENDED) {
- apr_pollfd_t *pfd;
-
- apr_array_header_t *pfds = apr_array_make(baton->subpool, 2, sizeof(apr_pollfd_t));
-
- pfd = apr_array_push(pfds);
- pfd->desc_type = APR_POLL_SOCKET;
- pfd->reqevents = APR_POLLIN | APR_POLLERR | APR_POLLHUP;
- pfd->desc.s = baton->client_soc;
- pfd->p = baton->subpool;
-
- pfd = apr_array_push(pfds);
- pfd->desc_type = APR_POLL_SOCKET;
- pfd->reqevents = APR_POLLIN | APR_POLLERR | APR_POLLHUP;
- pfd->desc.s = baton->server_soc;
- pfd->p = baton->subpool;
-
- rv = ap_mpm_register_poll_callback_timeout(pfds,
+ rv = ap_mpm_register_poll_callback_timeout(tunnel->pfds,
proxy_wstunnel_callback,
proxy_wstunnel_cancel_callback,
baton,
@@ -431,26 +248,28 @@ static int proxy_wstunnel_request(apr_po
}
else if (APR_STATUS_IS_ENOTIMPL(rv)) {
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(02544) "No async support");
- status = proxy_wstunnel_pump(baton, dconf->idle_timeout, 0); /* force no async */
+ tunnel->timeout = dconf->idle_timeout;
+ status = proxy_wstunnel_pump(baton, 0); /* force no async */
}
else {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
- APLOGNO(02543) "error creating websockets tunnel");
- return HTTP_INTERNAL_SERVER_ERROR;
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10211)
+ "error registering websocket tunnel");
+ status = HTTP_INTERNAL_SERVER_ERROR;
}
}
}
- if (status != OK) {
- /* Avoid sending error pages down an upgraded connection */
- if (status != HTTP_REQUEST_TIME_OUT) {
- r->status = status;
+ if (ap_is_HTTP_ERROR(status)) {
+ /* Don't send an error page down an upgraded connection */
+ if (!tunnel->replied) {
+ return status;
}
- status = OK;
+ /* Custom log may need this, still */
+ r->status = status;
}
- return status;
+ return OK;
}
-
+
/*
*/
static int proxy_wstunnel_handler(request_rec *r, proxy_worker *worker,
@@ -461,12 +280,12 @@ static int proxy_wstunnel_handler(reques
int status;
char server_portstr[32];
proxy_conn_rec *backend = NULL;
+ const char *upgrade_method, *upgrade;
char *scheme;
apr_pool_t *p = r->pool;
char *locurl = url;
apr_uri_t *uri;
int is_ssl = 0;
- const char *upgrade_method = *worker->s->upgrade ? worker->s->upgrade : "WebSocket";
if (ap_cstr_casecmpn(url, "wss:", 4) == 0) {
scheme = "WSS";
@@ -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..
+ */
+ 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;
}
}
+ else {
+ upgrade = "WebSocket";
+ }
uri = apr_palloc(p, sizeof(*uri));
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02451) "serving URL %s", url);
@@ -502,7 +330,6 @@ static int proxy_wstunnel_handler(reques
}
backend->is_ssl = is_ssl;
- backend->close = 0;
/* Step One: Determine Who To Connect To */
status = ap_proxy_determine_connection(p, r, conf, worker, backend,
@@ -530,13 +357,15 @@ static int proxy_wstunnel_handler(reques
/* Step Four: Process the Request */
status = proxy_wstunnel_request(p, r, backend, worker, conf, uri, locurl,
- server_portstr, scheme);
+ server_portstr, scheme, upgrade);
cleanup:
/* Do not close the socket */
- if (backend && status != SUSPENDED) {
+ if (backend) {
backend->close = 1;
- ap_proxy_release_connection(scheme, backend, r->server);
+ if (status != SUSPENDED) {
+ ap_proxy_release_connection(scheme, backend, r->server);
+ }
}
return status;
}
Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1869338&r1=1869337&r2=1869338&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Sun Nov 3 15:48:53 2019
@@ -4064,6 +4064,7 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tra
APR_NONBLOCK_READ, bsize);
if (rv == APR_SUCCESS) {
if (c_o->aborted) {
+ apr_brigade_cleanup(bb_i);
return APR_EPIPE;
}
if (APR_BRIGADE_EMPTY(bb_i)) {
@@ -4104,7 +4105,9 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tra
"error on %s - ap_pass_brigade",
name);
}
- } else if (!APR_STATUS_IS_EAGAIN(rv) && !APR_STATUS_IS_EOF(rv)) {
+ apr_brigade_cleanup(bb_o);
+ }
+ else if (!APR_STATUS_IS_EAGAIN(rv) && !APR_STATUS_IS_EOF(rv)) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, APLOGNO(03308)
"ap_proxy_transfer_between_connections: "
"error on %s - ap_get_brigade",
@@ -4114,7 +4117,9 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tra
if (after) {
ap_fflush(c_o->output_filters, bb_o);
+ apr_brigade_cleanup(bb_o);
}
+ apr_brigade_cleanup(bb_i);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, rv, r,
"ap_proxy_transfer_between_connections complete");
@@ -4126,6 +4131,193 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tra
return rv;
}
+PROXY_DECLARE(apr_status_t) ap_proxy_tunnel_create(proxy_tunnel_rec **ptunnel,
+ request_rec *r,
+ conn_rec *origin)
+{
+ apr_status_t rv;
+ apr_pollfd_t *pfds;
+ conn_rec *c = r->connection;
+ proxy_tunnel_rec *tunnel;
+
+ *ptunnel = NULL;
+
+ tunnel = apr_pcalloc(r->pool, sizeof(*tunnel));
+
+ tunnel->r = r;
+ tunnel->origin = origin;
+ tunnel->bb_i = apr_brigade_create(r->pool,
+ c->bucket_alloc);
+ tunnel->bb_o = apr_brigade_create(origin->pool,
+ origin->bucket_alloc);
+
+ tunnel->timeout = -1;
+ rv = apr_pollset_create(&tunnel->pollset, 2, r->pool,
+ APR_POLLSET_NOCOPY);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ tunnel->pfds = apr_array_make(r->pool, 2, sizeof(apr_pollfd_t));
+ apr_array_push(tunnel->pfds); /* pfds[0] */
+ apr_array_push(tunnel->pfds); /* pfds[1] */
+
+ pfds = &APR_ARRAY_IDX(tunnel->pfds, 0, apr_pollfd_t);
+ pfds[0].desc.s = ap_get_conn_socket(c);
+ pfds[1].desc.s = ap_get_conn_socket(origin);
+ pfds[0].desc_type = pfds[1].desc_type = APR_POLL_SOCKET;
+ pfds[0].reqevents = pfds[1].reqevents = APR_POLLIN | APR_POLLHUP;
+ pfds[0].p = pfds[1].p = r->pool;
+
+ /* The input/output filter stacks should contain connection filters only */
+ r->output_filters = c->output_filters;
+ r->proto_output_filters = c->output_filters;
+ r->input_filters = c->input_filters;
+ r->proto_input_filters = c->input_filters;
+
+ c->keepalive = AP_CONN_CLOSE;
+ origin->keepalive = AP_CONN_CLOSE;
+
+ *ptunnel = tunnel;
+ return APR_SUCCESS;
+}
+
+PROXY_DECLARE(apr_status_t) ap_proxy_tunnel_run(proxy_tunnel_rec *tunnel)
+{
+ apr_status_t rv;
+ request_rec *r = tunnel->r;
+ conn_rec *c_i = r->connection;
+ conn_rec *c_o = tunnel->origin;
+ apr_socket_t *sock_i = ap_get_conn_socket(c_i);
+ apr_socket_t *sock_o = ap_get_conn_socket(c_o);
+ apr_interval_time_t timeout = tunnel->timeout >= 0 ? tunnel->timeout : -1;
+ apr_pollfd_t *pfds = &APR_ARRAY_IDX(tunnel->pfds, 0, apr_pollfd_t);
+ apr_pollset_t *pollset = tunnel->pollset;
+ const apr_pollfd_t *signalled;
+ apr_int32_t pollcnt, pi;
+ int done = 0;
+
+ AP_DEBUG_ASSERT(tunnel->pfds->nelts == 2);
+ AP_DEBUG_ASSERT(pfds[0].desc.s == sock_i);
+ AP_DEBUG_ASSERT(pfds[1].desc.s == sock_o);
+
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(10212)
+ "proxy: tunnel: running (timeout %" APR_TIME_T_FMT "."
+ "%" APR_TIME_T_FMT ")",
+ timeout > 0 ? apr_time_sec(timeout) : timeout,
+ timeout > 0 ? timeout % APR_USEC_PER_SEC : 0);
+
+#if 0
+ apr_socket_opt_set(sock_i, APR_SO_NONBLOCK, 1);
+ apr_socket_opt_set(sock_i, APR_SO_NONBLOCK, 1);
+ apr_socket_opt_set(sock_o, APR_SO_KEEPALIVE, 1);
+ apr_socket_opt_set(sock_o, APR_SO_KEEPALIVE, 1);
+#endif
+
+ apr_pollset_add(pollset, &pfds[0]);
+ apr_pollset_add(pollset, &pfds[1]);
+
+ do { /* Loop until done (one side closes the connection, or an error) */
+ rv = apr_pollset_poll(tunnel->pollset, timeout, &pollcnt, &signalled);
+ if (rv != APR_SUCCESS) {
+ if (APR_STATUS_IS_EINTR(rv)) {
+ continue;
+ }
+
+ apr_pollset_remove(pollset, &pfds[1]);
+ apr_pollset_remove(pollset, &pfds[0]);
+
+ if (APR_STATUS_IS_TIMEUP(rv)) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(10213)
+ "proxy: tunnel: woken up, i=%d", (int)pollcnt);
+
+ return HTTP_GATEWAY_TIME_OUT;
+ }
+
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10214)
+ "proxy: tunnel: polling failed");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(10215)
+ "proxy: tunnel: woken up, i=%d", (int)pollcnt);
+
+ for (pi = 0; pi < pollcnt; pi++) {
+ const apr_pollfd_t *cur = &signalled[pi];
+ apr_int16_t pollevent = cur->rtnevents;
+
+ if (cur->desc.s == sock_o) {
+ if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(10216)
+ "proxy: tunnel: backend was readable");
+ rv = ap_proxy_transfer_between_connections(r, c_o, c_i,
+ tunnel->bb_o,
+ tunnel->bb_i,
+ "backend",
+ &tunnel->replied,
+ AP_IOBUFSIZE,
+ 0);
+ done |= (rv != APR_SUCCESS);
+ }
+ else if (pollevent & APR_POLLERR) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10217)
+ "proxy: tunnel: error on backend connection");
+ c_o->aborted = 1;
+ done = 1;
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10218)
+ "proxy: tunnel: unknown event %d on backend connection",
+ (int)pollevent);
+ done = 1;
+ }
+ }
+ else if (cur->desc.s == sock_i) {
+ if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(10219)
+ "proxy: tunnel: client was readable");
+ rv = ap_proxy_transfer_between_connections(r, c_i, c_o,
+ tunnel->bb_i,
+ tunnel->bb_o,
+ "client", NULL,
+ AP_IOBUFSIZE,
+ 0);
+ done |= (rv != APR_SUCCESS);
+ }
+ else if (pollevent & APR_POLLERR) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10220)
+ "proxy: tunnel: error on client connection");
+ c_i->aborted = 1;
+ done = 1;
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10221)
+ "proxy: tunnel: unknown event %d on client connection",
+ (int)pollevent);
+ done = 1;
+ }
+ }
+ else {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10222)
+ "proxy: tunnel: unknown socket in pollset");
+ done = 1;
+ }
+ }
+ } while (!done);
+
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO(10223)
+ "proxy: tunnel: finished");
+
+ apr_pollset_remove(pollset, &pfds[1]);
+ apr_pollset_remove(pollset, &pfds[0]);
+
+ if (!tunnel->replied) {
+ return HTTP_BAD_GATEWAY;
+ }
+
+ return OK;
+}
+
PROXY_DECLARE (const char *) ap_proxy_show_hcmethod(hcmethod_t method)
{
proxy_hcmethods_t *m = proxy_hcmethods;
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 12/8/21 7:30 PM, Yann Ylavic wrote:
> On Wed, Dec 8, 2021 at 5:19 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> Should I take care of adjusting mod_proxy_connect and mod_proxy_http to align this again with standard flows and in a second step
>> we take care of that r->async_status idea?
>
> It's fine by me to restore the previous behavior and add
> r->async_status if/when really needed.
r1895715
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 Wed, Dec 8, 2021 at 5:19 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> Should I take care of adjusting mod_proxy_connect and mod_proxy_http to align this again with standard flows and in a second step
> we take care of that r->async_status idea?
It's fine by me to restore the previous behavior and add
r->async_status if/when really needed.
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 12/8/21 2:49 PM, Yann Ylavic wrote:
> On Wed, Dec 8, 2021 at 2:42 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>> On 12/8/21 2:26 PM, Ruediger Pluem wrote:
>>>
>>>
>>> On 11/3/19 4:48 PM, ylavic@apache.org wrote:
>>>> Author: ylavic
>>
>>>>
>>>> + rc = ap_proxy_tunnel_run(tunnel);
>>>> + if (ap_is_HTTP_ERROR(rc)) {
>>>> + /* Don't send an error page if we sent data already */
>>>> + if (proxyport && !tunnel->replied) {
>>>> + return rc;
>>>> }
>>>> - } while (!done);
>>>> -
>>>> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
>>>> - "finished with poll() - cleaning up");
>>>> + /* Custom log may need this, still */
>>>> + r->status = rc;
>>>
>>> I have some lively discussions outside this forum on the consequences the above line has.
>>> In fact this means that even though we replied to the client with a 200 on its CONNECT request we might log a different status
>>> code if something goes wrong during the tunneling. In contrast to this we don't do this for other other methods e.g. GET or POST:
>>> Once we sent the status code to the client we do not log a different one should something go wrong while delivering the response.
>>> Removing r->status = rc is IMHO enough to align the behavior again.
>>>
>>> Thoughts?
>>>
>>
>> BTW: We do a similar thing in mod_proxy_http.c in the backend replied with Switching Protocols:
>>
>> status = ap_proxy_tunnel_run(tunnel);
>> if (ap_is_HTTP_ERROR(status)) {
>> /* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
>> * but we can differentiate between client and backend here.
>> */
>> if (status == HTTP_GATEWAY_TIME_OUT
>> && tunnel->timeout == client_timeout) {
>> status = HTTP_REQUEST_TIME_OUT;
>> }
>> }
>> else {
>> /* Update r->status for custom log */
>> status = HTTP_SWITCHING_PROTOCOLS;
>> }
>> r->status = status;
>>
>> Hence we have the same sort of different behavior compared to "ordinary" requests here as well. Hence should we align here as well
>> and always log HTTP_SWITCHING_PROTOCOLS no matter if there was an error while tunneling the protocol?
>
> I think you are right, the goal was to show in the access_log how
> tunneling went (primary for the Upgrade case, didn't think too much of
> CONNECT actually :/), but if worthy it looks better to have a new
> r->async_status for that because we are missing the initial status
> now..
Should I take care of adjusting mod_proxy_connect and mod_proxy_http to align this again with standard flows and in a second step
we take care of that r->async_status idea?
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 Wed, Dec 8, 2021 at 2:42 PM Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 12/8/21 2:26 PM, Ruediger Pluem wrote:
> >
> >
> > On 11/3/19 4:48 PM, ylavic@apache.org wrote:
> >> Author: ylavic
>
> >>
> >> + rc = ap_proxy_tunnel_run(tunnel);
> >> + if (ap_is_HTTP_ERROR(rc)) {
> >> + /* Don't send an error page if we sent data already */
> >> + if (proxyport && !tunnel->replied) {
> >> + return rc;
> >> }
> >> - } while (!done);
> >> -
> >> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> >> - "finished with poll() - cleaning up");
> >> + /* Custom log may need this, still */
> >> + r->status = rc;
> >
> > I have some lively discussions outside this forum on the consequences the above line has.
> > In fact this means that even though we replied to the client with a 200 on its CONNECT request we might log a different status
> > code if something goes wrong during the tunneling. In contrast to this we don't do this for other other methods e.g. GET or POST:
> > Once we sent the status code to the client we do not log a different one should something go wrong while delivering the response.
> > Removing r->status = rc is IMHO enough to align the behavior again.
> >
> > Thoughts?
> >
>
> BTW: We do a similar thing in mod_proxy_http.c in the backend replied with Switching Protocols:
>
> status = ap_proxy_tunnel_run(tunnel);
> if (ap_is_HTTP_ERROR(status)) {
> /* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
> * but we can differentiate between client and backend here.
> */
> if (status == HTTP_GATEWAY_TIME_OUT
> && tunnel->timeout == client_timeout) {
> status = HTTP_REQUEST_TIME_OUT;
> }
> }
> else {
> /* Update r->status for custom log */
> status = HTTP_SWITCHING_PROTOCOLS;
> }
> r->status = status;
>
> Hence we have the same sort of different behavior compared to "ordinary" requests here as well. Hence should we align here as well
> and always log HTTP_SWITCHING_PROTOCOLS no matter if there was an error while tunneling the protocol?
I think you are right, the goal was to show in the access_log how
tunneling went (primary for the Upgrade case, didn't think too much of
CONNECT actually :/), but if worthy it looks better to have a new
r->async_status for that because we are missing the initial status
now..
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 12/8/21 2:26 PM, Ruediger Pluem wrote:
>
>
> On 11/3/19 4:48 PM, ylavic@apache.org wrote:
>> Author: ylavic
>>
>> + rc = ap_proxy_tunnel_run(tunnel);
>> + if (ap_is_HTTP_ERROR(rc)) {
>> + /* Don't send an error page if we sent data already */
>> + if (proxyport && !tunnel->replied) {
>> + return rc;
>> }
>> - } while (!done);
>> -
>> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
>> - "finished with poll() - cleaning up");
>> + /* Custom log may need this, still */
>> + r->status = rc;
>
> I have some lively discussions outside this forum on the consequences the above line has.
> In fact this means that even though we replied to the client with a 200 on its CONNECT request we might log a different status
> code if something goes wrong during the tunneling. In contrast to this we don't do this for other other methods e.g. GET or POST:
> Once we sent the status code to the client we do not log a different one should something go wrong while delivering the response.
> Removing r->status = rc is IMHO enough to align the behavior again.
>
> Thoughts?
>
BTW: We do a similar thing in mod_proxy_http.c in the backend replied with Switching Protocols:
status = ap_proxy_tunnel_run(tunnel);
if (ap_is_HTTP_ERROR(status)) {
/* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
* but we can differentiate between client and backend here.
*/
if (status == HTTP_GATEWAY_TIME_OUT
&& tunnel->timeout == client_timeout) {
status = HTTP_REQUEST_TIME_OUT;
}
}
else {
/* Update r->status for custom log */
status = HTTP_SWITCHING_PROTOCOLS;
}
r->status = status;
Hence we have the same sort of different behavior compared to "ordinary" requests here as well. Hence should we align here as well
and always log HTTP_SWITCHING_PROTOCOLS no matter if there was an error while tunneling the protocol?
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 Ruediger Pluem <rp...@apache.org>.
On 11/3/19 4: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_connect.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c?rev=1869338&r1=1869337&r2=1869338&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c Sun Nov 3 15:48:53 2019
> @@ -364,83 +333,25 @@ static int proxy_connect_handler(request
> * Handle two way transfer of data over the socket (this is a tunnel).
> */
>
> - /* we are now acting as a tunnel - the input/output filter stacks should
> - * not contain any non-connection filters.
> - */
> - r->output_filters = c->output_filters;
> - r->proto_output_filters = c->output_filters;
> - r->input_filters = c->input_filters;
> - r->proto_input_filters = c->input_filters;
> -/* r->sent_bodyct = 1;*/
> -
> - do { /* Loop until done (one side closes the connection, or an error) */
> - rv = apr_pollset_poll(pollset, -1, &pollcnt, &signalled);
> - if (rv != APR_SUCCESS) {
> - if (APR_STATUS_IS_EINTR(rv)) {
> - continue;
> - }
> - apr_socket_close(sock);
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01023) "error apr_poll()");
> - return HTTP_INTERNAL_SERVER_ERROR;
> - }
> -
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01024)
> - "woke from poll(), i=%d", pollcnt);
> + /* r->sent_bodyct = 1; */
>
> - for (pi = 0; pi < pollcnt; pi++) {
> - const apr_pollfd_t *cur = &signalled[pi];
> -
> - if (cur->desc.s == sock) {
> - pollevent = cur->rtnevents;
> - if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01025)
> - "backend was readable");
> - done |= ap_proxy_transfer_between_connections(r, backconn,
> - c, bb_back,
> - bb_front,
> - "backend", NULL,
> - CONN_BLKSZ, 1)
> - != APR_SUCCESS;
> - }
> - else if (pollevent & APR_POLLERR) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01026)
> - "err on backend connection");
> - backconn->aborted = 1;
> - done = 1;
> - }
> - }
> - else if (cur->desc.s == client_socket) {
> - pollevent = cur->rtnevents;
> - if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01027)
> - "client was readable");
> - done |= ap_proxy_transfer_between_connections(r, c,
> - backconn,
> - bb_front,
> - bb_back,
> - "client",
> - NULL,
> - CONN_BLKSZ, 1)
> - != APR_SUCCESS;
> - }
> - else if (pollevent & APR_POLLERR) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02827)
> - "err on client connection");
> - c->aborted = 1;
> - done = 1;
> - }
> - }
> - else {
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01028)
> - "unknown socket in pollset");
> - done = 1;
> - }
> + rv = ap_proxy_tunnel_create(&tunnel, r, backconn);
> + if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10208)
> + "can't create tunnel for %pI (%s)",
> + nexthop, connectname);
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
>
> + rc = ap_proxy_tunnel_run(tunnel);
> + if (ap_is_HTTP_ERROR(rc)) {
> + /* Don't send an error page if we sent data already */
> + if (proxyport && !tunnel->replied) {
> + return rc;
> }
> - } while (!done);
> -
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> - "finished with poll() - cleaning up");
> + /* Custom log may need this, still */
> + r->status = rc;
I have some lively discussions outside this forum on the consequences the above line has.
In fact this means that even though we replied to the client with a 200 on its CONNECT request we might log a different status
code if something goes wrong during the tunneling. In contrast to this we don't do this for other other methods e.g. GET or POST:
Once we sent the status code to the client we do not log a different one should something go wrong while delivering the response.
Removing r->status = rc is IMHO enough to align the behavior again.
Thoughts?
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.
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/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