On Tue, Feb 25, 2014 at 6:18 PM, Yann Ylavic <[email protected]> wrote:
> On Tue, Feb 25, 2014 at 4:21 PM, Jim Jagielski <[email protected]> wrote:
>> Of course, this doesn't mean that Yann should wait for
>> me... you seem to have a good grasp.
>
I'm coming back here...
After spending some time on this and several patches to implement It
in different ways, it seems in fact that one can already reuse UDS
connections with a mod_rewrite [P] rule, simply by not specifying the
UDS part of the worker's URL...
Eg.
RewriteEngine on
RewriteRule "^/(.*)$" "http://localhost/$1"; [P]
<Proxy "unix:/path/to/backend.sock|http://localhost"; disablereuse=off>
</Proxy>
is enough.
So probably Jim's commits are all that is necessary, and got my +1 in STATUS.
However, as shown in my second comment on this thread [1], whenever an
UDS part is used in mod_rewrite [P] rule, both
mod_rewrite::fully_qualify_uri() (which does not handle the "unix:"
scheme) and ap_proxy_pre_request() (which uses the UDS-unaware
ap_proxy_get_worker()) don't do the right thing, and it's working only
because ap_strcasestr() is used to find the "unix:" scheme in the
default "reverse".
If we were to allow UDS part of the URL for defined workers in
mod_rewrite (for completness or simply to fix the above), maybe the
attached patch could be applied (an improved version of the previous
one).
The idea is still to provide 2 mod_proxy's optional functions to
split/unsplit the URL into remote and UDS parts, and use them in
mod_rewrite [P] before/after fully_qualify_uri(), so that the UDS part
does not interfere.
I find this solution not so intrusive because it also allows to warn
on and ignore (?) the [P] rule if mod_proxy is not loaded (there is no
dependency on this currently).
The patch also introduces the new ap_proxy_get_worker_ex() function,
which does the same as the non _ex() one but can also split the URL
(if asked too) before matching the URL against the name, providing the
splits if the corresponding args are not NULL.
It allows to get rid of all the calls to ap_proxy_get_worker() with
de_socketfy(url) since it is now done internally.
For the mod_rewrite [P] case, ap_proxy_pre_request() now uses it to
find the existing worker (if any), and also uses its splitted URLs to
rework r->filename in the default "reverse" case.
The legacy ap_proxy_get_worker()'s behaviour is unchanged (UDS
unaware), although it calls ap_proxy_get_worker_ex().
[1]
http://mail-archives.apache.org/mod_mbox/httpd-dev/201402.mbox/%3CCAKQ1sVNjbUyv0cT-Ln1NYgyWxj6DoGNNLWL77jsfp4KVy6Zi=a...@mail.gmail.com%3E
Index: modules/proxy/mod_proxy.h
===================================================================
--- modules/proxy/mod_proxy.h (revision 1588466)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -605,23 +605,60 @@ typedef __declspec(dllimport) const char *
/* Connection pool API */
+
/**
+ * Return an UDS un-aware URL from the given @a url.
+ * @param p memory pool used for the returned URL
+ * @param url the URL to split
+ * @param uds output for the UDS part of the URL (if not NULL)
+ * @return the non-UDS part of the URL (if applicable), or the
+ * given @a url otherwise
+ */
+APR_DECLARE_OPTIONAL_FN(char *, ap_proxy_split_url,
+ (apr_pool_t *p, char *url, char **uds));
+
+/**
+ * Return an UDS aware URL from the given @a url and @a uds.
+ * @param p memory pool used for the returned URL
+ * @param url the non-UDS part of the URL
+ * @param uds the UDS part of the URL (or NULL)
+ * @return the full URL
+ */
+APR_DECLARE_OPTIONAL_FN(char *, ap_proxy_unsplit_url,
+ (apr_pool_t *p, char *url, const char *uds));
+
+/**
* Return the user-land, UDS aware worker name
* @param p memory pool used for displaying worker name
* @param worker the worker
* @return name
*/
-
PROXY_DECLARE(char *) ap_proxy_worker_name(apr_pool_t *p,
proxy_worker *worker);
/**
+ * Get the worker from proxy configuration, splitting the @a url into
+ * @a to (remote) and @a by (local, eg. UDS) parts (if not NULL).
+ * @param p memory pool used for finding worker
+ * @param balancer the balancer that the worker belongs to
+ * @param conf current proxy server configuration
+ * @param url url to find the worker from
+ * @return proxy_worker or NULL if not found
+ */
+PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker_ex(apr_pool_t *p,
+ proxy_balancer *balancer,
+ proxy_server_conf *conf,
+ const char *url, int uds,
+ char **to, char **by);
+/**
* Get the worker from proxy configuration
* @param p memory pool used for finding worker
* @param balancer the balancer that the worker belongs to
* @param conf current proxy server configuration
* @param url url to find the worker from
* @return proxy_worker or NULL if not found
+ * @remark This is equivalent to ap_proxy_get_worker(..., NULL, NULL),
+ * so the url won't be split into remote/local parts
*/
PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
proxy_balancer *balancer,
Index: modules/proxy/proxy_util.h
===================================================================
--- modules/proxy/proxy_util.h (revision 1588466)
+++ modules/proxy/proxy_util.h (working copy)
@@ -40,6 +40,9 @@ PROXY_DECLARE_DATA extern const apr_strmatch_patte
*/
void proxy_util_register_hooks(apr_pool_t *p);
+char *ap_proxy_split_url(apr_pool_t *p, char *url, char **uds);
+char *ap_proxy_unsplit_url(apr_pool_t *p, char *url, const char *uds);
+
/** @} */
#endif /* PROXY_UTIL_H_ */
Index: modules/proxy/mod_proxy_balancer.c
===================================================================
--- modules/proxy/mod_proxy_balancer.c (revision 1588466)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -1173,7 +1173,8 @@ static int balancer_handler(request_rec *r)
(val = apr_table_get(params, "b_nwrkr"))) {
char *ret;
proxy_worker *nworker;
- nworker = ap_proxy_get_worker(r->pool, bsel, conf, val);
+ nworker = ap_proxy_get_worker_ex(r->pool, bsel, conf, val,
+ 1, NULL, NULL);
if (!nworker && storage->num_free_slots(bsel->wslot)) {
if ((rv = PROXY_GLOBAL_LOCK(bsel)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01194)
Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c (revision 1588466)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -1461,36 +1461,6 @@ static const char *
return add_proxy(cmd, dummy, f1, r1, 1);
}
-static char *de_socketfy(apr_pool_t *p, char *url)
-{
- char *ptr;
- /*
- * We could be passed a URL during the config stage that contains
- * the UDS path... ignore it
- */
- if (!strncasecmp(url, "unix:", 5) &&
- ((ptr = ap_strchr(url, '|')) != NULL)) {
- /* move past the 'unix:...|' UDS path info */
- char *ret, *c;
-
- ret = ptr + 1;
- /* special case: "unix:....|scheme:" is OK, expand
- * to "unix:....|scheme://localhost"
- * */
- c = ap_strchr(ret, ':');
- if (c == NULL) {
- return NULL;
- }
- if (c[1] == '\0') {
- return apr_pstrcat(p, ret, "//localhost", NULL);
- }
- else {
- return ret;
- }
- }
- return url;
-}
-
static const char *
add_pass(cmd_parms *cmd, void *dummy, const char *arg, int is_regex)
{
@@ -1582,7 +1552,7 @@ static const char *
}
new->fake = apr_pstrdup(cmd->pool, f);
- new->real = apr_pstrdup(cmd->pool, de_socketfy(cmd->pool, r));
+ new->real = ap_proxy_split_url(cmd->pool, apr_pstrdup(cmd->pool, r), NULL);
new->flags = flags;
if (use_regex) {
new->regex = ap_pregcomp(cmd->pool, f, AP_REG_EXTENDED);
@@ -1631,7 +1601,8 @@ static const char *
new->balancer = balancer;
}
else {
- proxy_worker *worker = ap_proxy_get_worker(cmd->temp_pool, NULL, conf, de_socketfy(cmd->pool, r));
+ proxy_worker *worker = ap_proxy_get_worker_ex(cmd->temp_pool, NULL,
+ conf, r, 1, NULL, NULL);
int reuse = 0;
if (!worker) {
const char *err = ap_proxy_define_worker(cmd->pool, &worker, NULL, conf, r, 0);
@@ -1643,14 +1614,15 @@ static const char *
reuse = 1;
ap_log_error(APLOG_MARK, APLOG_INFO, 0, cmd->server, APLOGNO(01145)
"Sharing worker '%s' instead of creating new worker '%s'",
- ap_proxy_worker_name(cmd->pool, worker), new->real);
+ ap_proxy_worker_name(cmd->temp_pool, worker), new->real);
}
for (i = 0; i < arr->nelts; i++) {
if (reuse) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, cmd->server, APLOGNO(01146)
"Ignoring parameter '%s=%s' for worker '%s' because of worker sharing",
- elts[i].key, elts[i].val, ap_proxy_worker_name(cmd->pool, worker));
+ elts[i].key, elts[i].val, ap_proxy_worker_name(cmd->temp_pool,
+ worker));
} else {
const char *err = set_worker_param(cmd->pool, worker, elts[i].key,
elts[i].val);
@@ -2110,7 +2082,8 @@ static const char *add_member(cmd_parms *cmd, void
}
/* Try to find existing worker */
- worker = ap_proxy_get_worker(cmd->temp_pool, balancer, conf, de_socketfy(cmd->temp_pool, name));
+ worker = ap_proxy_get_worker_ex(cmd->temp_pool, balancer, conf, name,
+ 1, NULL, NULL);
if (!worker) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, cmd->server, APLOGNO(01147)
"Defining worker '%s' for balancer '%s'",
@@ -2196,7 +2169,8 @@ static const char *
}
}
else {
- worker = ap_proxy_get_worker(cmd->temp_pool, NULL, conf, de_socketfy(cmd->temp_pool, name));
+ worker = ap_proxy_get_worker_ex(cmd->temp_pool, NULL, conf, name,
+ 1, NULL, NULL);
if (!worker) {
if (in_proxy_section) {
err = ap_proxy_define_worker(cmd->pool, &worker, NULL,
@@ -2335,8 +2309,8 @@ static const char *proxysection(cmd_parms *cmd, vo
}
}
else {
- worker = ap_proxy_get_worker(cmd->temp_pool, NULL, sconf,
- de_socketfy(cmd->temp_pool, (char*)conf->p));
+ worker = ap_proxy_get_worker_ex(cmd->temp_pool, NULL, sconf,
+ (char*)conf->p, 1, NULL, NULL);
if (!worker) {
err = ap_proxy_define_worker(cmd->pool, &worker, NULL,
sconf, conf->p, 0);
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1588466)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1508,32 +1508,111 @@ PROXY_DECLARE(char *) ap_proxy_worker_name(apr_poo
return apr_pstrcat(p, "unix:", worker->s->uds_path, "|", worker->s->name, NULL);
}
-PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
- proxy_balancer *balancer,
- proxy_server_conf *conf,
- const char *url)
+char *ap_proxy_split_url(apr_pool_t *p, char *url, char **uds)
{
+ char *ptr;
+ /*
+ * We could be passed a URL that contains the UDS path...
+ * If any, save it in *uds, and strip it.
+ */
+ if (uds) {
+ *uds = NULL;
+ }
+ if (!strncasecmp(url, "unix:", 5) &&
+ ((ptr = ap_strchr(url, '|')) != NULL)) {
+ /* move past the 'unix:...|' UDS path info */
+ char *ret, *c;
+
+ ret = ptr + 1;
+ /* special case: "unix:....|scheme:" is OK, expand
+ * to "unix:....|scheme://localhost"
+ * */
+ c = ap_strchr(ret, ':');
+ if (c == NULL) {
+ return NULL;
+ }
+ if (uds) {
+ *uds = url;
+ *ptr = '\0';
+ }
+ if (c[1] == '\0') {
+ return apr_pstrcat(p, ret, "//localhost", NULL);
+ }
+ else {
+ return ret;
+ }
+ }
+ return url;
+}
+
+char *ap_proxy_unsplit_url(apr_pool_t *p, char *url, const char *uds)
+{
+ if (uds) {
+ return apr_pstrcat(p, uds, "|", url, NULL);
+ }
+ else {
+ return url;
+ }
+}
+
+PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker_ex(apr_pool_t *p,
+ proxy_balancer *balancer,
+ proxy_server_conf *conf,
+ const char *url, int uds,
+ char **to, char **by)
+{
proxy_worker *worker;
proxy_worker *max_worker = NULL;
- int max_match = 0;
- int url_length;
- int min_match;
- int worker_name_length;
+ apr_size_t max_match = 0;
+ apr_size_t url_length;
+ apr_size_t min_match;
+ apr_size_t worker_name_length;
const char *c;
char *url_copy;
int i;
if (!url) {
+ if (to) {
+ *to = NULL;
+ }
+ if (by) {
+ *by = NULL;
+ }
return NULL;
}
+ if (!uds) {
+ /* No real interest in *to or *by being used without uds,
+ * don't leave dangling pointers anyway.
+ */
+ if (to) {
+ *to = NULL;
+ }
+ if (by) {
+ *by = NULL;
+ }
+ url_length = 0;
+ url_copy = NULL;
+ }
+ else {
+ url_length = strlen(url);
+ url_copy = apr_pstrmemdup(p, url, url_length);
+ url_copy = ap_proxy_split_url(p, url_copy, by);
+ if (to) {
+ *to = url_copy;
+ }
+ url = url_copy;
+ }
+
c = ap_strchr_c(url, ':');
if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0') {
return NULL;
}
- url_length = strlen(url);
- url_copy = apr_pstrmemdup(p, url, url_length);
+ if (!url_copy) {
+ url_length = strlen(url);
+ url_copy = apr_pstrmemdup(p, url, url_length);
+ }
/*
* We need to find the start of the path and
@@ -1554,13 +1633,13 @@ PROXY_DECLARE(char *) ap_proxy_worker_name(apr_poo
ap_str_tolower(url_copy);
min_match = strlen(url_copy);
}
+
/*
* Do a "longest match" on the worker name to find the worker that
* fits best to the URL, but keep in mind that we must have at least
* a minimum matching of length min_match such that
* scheme://hostname[:port] matches between worker and url.
*/
-
if (balancer) {
proxy_worker **workers = (proxy_worker **)balancer->workers->elts;
for (i = 0; i < balancer->workers->nelts; i++, workers++) {
@@ -1590,7 +1669,15 @@ PROXY_DECLARE(char *) ap_proxy_worker_name(apr_poo
return max_worker;
}
+PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
+ proxy_balancer *balancer,
+ proxy_server_conf *conf,
+ const char *url)
+{
+ return ap_proxy_get_worker_ex(p, balancer, conf, url, 0, NULL, NULL);
+}
+
/*
* To create a worker from scratch first we define the
* specifics of the worker; this is all local data.
@@ -1606,29 +1693,25 @@ PROXY_DECLARE(char *) ap_proxy_define_worker(apr_p
int do_malloc)
{
int rv;
- apr_uri_t uri, urisock;
+ apr_uri_t uri;
proxy_worker_shared *wshared;
- char *ptr, *sockpath = NULL;
+ char *ptr, *uds, *sockpath = NULL;
/*
* Look to see if we are using UDS:
* require format: unix:/path/foo/bar.sock|http://ignored/path2/
* This results in talking http to the socket at /path/foo/bar.sock
*/
- ptr = ap_strchr((char *)url, '|');
- if (ptr) {
- *ptr = '\0';
- rv = apr_uri_parse(p, url, &urisock);
- if (rv == APR_SUCCESS && !strcasecmp(urisock.scheme, "unix")) {
- sockpath = ap_runtime_dir_relative(p, urisock.path);;
- url = ptr+1; /* so we get the scheme for the uds */
+ ptr = apr_pstrdup(p, url);
+ ptr = ap_proxy_split_url(p, ptr, &uds);
+ if (uds) {
+ rv = apr_uri_parse(p, uds, &uri);
+ if (rv == APR_SUCCESS && !strcasecmp(uri.scheme, "unix")) {
+ sockpath = ap_runtime_dir_relative(p, uri.path);;
+ url = ptr;
}
- else {
- *ptr = '|';
- }
}
rv = apr_uri_parse(p, url, &uri);
-
if (rv != APR_SUCCESS) {
return apr_pstrcat(p, "Unable to parse URL: ", url, NULL);
}
@@ -1906,8 +1989,23 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
access_status = proxy_run_pre_request(worker, balancer, r, conf, url);
if (access_status == DECLINED && *balancer == NULL) {
- *worker = ap_proxy_get_worker(r->pool, NULL, conf, *url);
+ char *real_url = NULL, *uds_url = NULL;
+ if (apr_table_get(r->notes, "rewrite-proxy")) {
+ *worker = ap_proxy_get_worker_ex(r->pool, NULL, conf, *url,
+ 1, &real_url, &uds_url);
+ }
+ else {
+ *worker = ap_proxy_get_worker(r->pool, NULL, conf, *url);
+ }
if (*worker) {
+ if (uds_url) {
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
+ "%s: found worker %s for %s as %s",
+ (*worker)->s->scheme, (*worker)->s->name,
+ *url, real_url);
+ *url = real_url;
+ }
+ else
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
"%s: found worker %s for %s",
(*worker)->s->scheme, (*worker)->s->name, *url);
@@ -1932,8 +2030,6 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
}
else if (r->proxyreq == PROXYREQ_REVERSE) {
if (conf->reverse) {
- char *ptr;
- char *ptr2;
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
"*: found reverse proxy worker for %s", *url);
*balancer = NULL;
@@ -1953,27 +2049,21 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
* NOTE: Here we use a quick note lookup, but we could also
* check to see if r->filename starts with 'proxy:'
*/
- if (apr_table_get(r->notes, "rewrite-proxy") &&
- (ptr2 = ap_strcasestr(r->filename, "unix:")) &&
- (ptr = ap_strchr(ptr2, '|'))) {
+ if (uds_url) {
apr_uri_t urisock;
apr_status_t rv;
- *ptr = '\0';
- rv = apr_uri_parse(r->pool, ptr2, &urisock);
+ rv = apr_uri_parse(r->pool, uds_url, &urisock);
if (rv == APR_SUCCESS) {
- char *rurl = ptr+1;
- char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);
+ char *sockpath = ap_runtime_dir_relative(r->pool,
+ urisock.path);
apr_table_setn(r->notes, "uds_path", sockpath);
- *url = apr_pstrdup(r->pool, rurl); /* so we get the scheme for the uds */
+ *url = real_url; /* so we get the scheme for the uds */
/* r->filename starts w/ "proxy:", so add after that */
- memmove(r->filename+6, rurl, strlen(rurl)+1);
+ r->filename = apr_pstrcat(r->pool, "proxy:", *url, NULL);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
"*: rewrite of url due to UDS(%s): %s (%s)",
sockpath, *url, r->filename);
}
- else {
- *ptr = '|';
- }
}
}
}
@@ -3517,6 +3607,8 @@ PROXY_DECLARE(apr_port_t) ap_proxy_port_of_scheme(
void proxy_util_register_hooks(apr_pool_t *p)
{
+ APR_REGISTER_OPTIONAL_FN(ap_proxy_split_url);
+ APR_REGISTER_OPTIONAL_FN(ap_proxy_unsplit_url);
APR_REGISTER_OPTIONAL_FN(ap_proxy_retry_worker);
APR_REGISTER_OPTIONAL_FN(ap_proxy_clear_connection);
}
Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c (revision 1588466)
+++ modules/mappers/mod_rewrite.c (working copy)
@@ -97,6 +97,7 @@
#include "util_mutex.h"
#include "mod_ssl.h"
+#include "mod_proxy.h"
#include "mod_rewrite.h"
#include "ap_expr.h"
@@ -109,6 +110,11 @@ static ap_dbd_t *(*dbd_acquire)(request_rec*) = NU
static void (*dbd_prepare)(server_rec*, const char*, const char*) = NULL;
static const char* really_last_key = "rewrite_really_last";
+static char *(*proxy_split_url)(apr_pool_t *p, char *url,
+ char **split) = NULL;
+static char *(*proxy_unsplit_url)(apr_pool_t *p, char *url,
+ const char *split) = NULL;
+
/*
* in order to improve performance on running production systems, you
* may strip all rewritelog code entirely from mod_rewrite by using the
@@ -4173,6 +4179,17 @@ static int apply_rewrite_rule(rewriterule_entry *p
* ourself).
*/
if (p->flags & RULEFLAG_PROXY) {
+ char *split = NULL;
+
+ if (!proxy_split_url || !proxy_unsplit_url) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO()
+ "mod_rewrite: can't apply [P] rule without "
+ "mod_proxy loaded: ignoring rule");
+ return 0;
+ }
+
+ r->filename = proxy_split_url(r->pool, r->filename, &split);
+
/* For rules evaluated in server context, the mod_proxy fixup
* hook can be relied upon to escape the URI as and when
* necessary, since it occurs later. If in directory context,
@@ -4192,6 +4209,9 @@ static int apply_rewrite_rule(rewriterule_entry *p
rewritelog((r, 2, ctx->perdir, "forcing proxy-throughput with %s",
r->filename));
+ if (split) {
+ r->filename = proxy_unsplit_url(r->pool, r->filename, split);
+ }
r->filename = apr_pstrcat(r->pool, "proxy:", r->filename, NULL);
apr_table_setn(r->notes, "rewrite-proxy", "1");
return 1;
@@ -4447,6 +4467,9 @@ static int post_config(apr_pool_t *p,
rewrite_ssl_lookup = APR_RETRIEVE_OPTIONAL_FN(ssl_var_lookup);
rewrite_is_https = APR_RETRIEVE_OPTIONAL_FN(ssl_is_https);
+ proxy_split_url = APR_RETRIEVE_OPTIONAL_FN(ap_proxy_split_url);
+ proxy_unsplit_url = APR_RETRIEVE_OPTIONAL_FN(ap_proxy_unsplit_url);
+
return OK;
}
