Yes, mod_rewrite is patched so that it calls fully_qualify_url() on the
non-UDS part of the URL only.
Either mod_rewrite has to split the URL (and needs mod_proxy's helpers), or
it does not fully qualify on the "unix:" scheme and let mod_proxy fully
qualitfy later (which then needs mod_rewrite's helper).
I choose the mod_proxy helpers with optional functions to hide the
internals.
Maybe we could also require fully qualified URLs for RewriteRule(s) with
UDS, and avoid the helpers (mod_rewrite would still need to fully qualify
the "unix:" scheme)...
On Tue, Feb 25, 2014 at 7:26 PM, Jim Jagielski <j...@jagunet.com> wrote:
> Thx...
>
> I haven't looked over it yet, but I do see that mod_rewrite
> is patched... why can't we set that note var and do
> all the required magic in mod_proxy? Does mod_rewrite
> itself really need to know about "unix:..."??
>
> Why are they optional functions?
>
> On Feb 25, 2014, at 12:18 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
>
> > On Tue, Feb 25, 2014 at 4:21 PM, Jim Jagielski <j...@jagunet.com> wrote:
> > > Of course, this doesn't mean that Yann should wait for
> > > me... you seem to have a good grasp.
> >
> > The following (attached) patch does the job, but I'm not it is "elegant".
> > It introduces the new optional ap_proxy_worker_real_url() and
> proxy_worker_full_url() functions that can be used by both mod_rewrite and
> mod_proxy to split/unsplit the URL wherever it needs to.
> >
> > I propose it here but you probably have a better idea...
> >
> > Index: modules/mappers/mod_rewrite.c
> > ===================================================================
> > --- modules/mappers/mod_rewrite.c (revision 1570613)
> > +++ 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_worker_real_url)(apr_pool_t *p, char *url,
> > + char **split) = NULL;
> > +static char *(*proxy_worker_full_url)(apr_pool_t *p, char *url,
> > + 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
> > @@ -4145,6 +4151,13 @@ static int apply_rewrite_rule(rewriterule_entry *p
> > * ourself).
> > */
> > if (p->flags & RULEFLAG_PROXY) {
> > + char *real = NULL, *split = NULL;
> > +
> > + if (proxy_worker_real_url &&
> > + (real = proxy_worker_real_url(r->pool, r->filename,
> &split))) {
> > + r->filename = real;
> > + }
> > +
> > /* 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,
> > @@ -4164,6 +4177,9 @@ static int apply_rewrite_rule(rewriterule_entry *p
> > rewritelog((r, 2, ctx->perdir, "forcing proxy-throughput with
> %s",
> > r->filename));
> >
> > + if (proxy_worker_full_url && real) {
> > + r->filename = proxy_worker_full_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;
> > @@ -4384,6 +4400,8 @@ static int pre_config(apr_pool_t *pconf,
> > }
> > dbd_acquire = APR_RETRIEVE_OPTIONAL_FN(ap_dbd_acquire);
> > dbd_prepare = APR_RETRIEVE_OPTIONAL_FN(ap_dbd_prepare);
> > + proxy_worker_real_url =
> APR_RETRIEVE_OPTIONAL_FN(ap_proxy_worker_real_url);
> > + proxy_worker_full_url =
> APR_RETRIEVE_OPTIONAL_FN(ap_proxy_worker_full_url);
> > return OK;
> > }
> >
> > Index: modules/proxy/mod_proxy.h
> > ===================================================================
> > --- modules/proxy/mod_proxy.h (revision 1570613)
> > +++ modules/proxy/mod_proxy.h (working copy)
> > @@ -595,13 +595,34 @@ 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 split 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_worker_real_url,
> > + (apr_pool_t *p, char *url, char **split));
> > +
> > +/**
> > + * Return an UDS aware URL from the given @a url and @a split.
> > + * @param p memory pool used for the returned URL
> > + * @param url the non-UDS part of the URL
> > + * @param split the UDS part of the URL (or NULL)
> > + * @return the full URL
> > + */
> > +APR_DECLARE_OPTIONAL_FN(char *, ap_proxy_worker_full_url,
> > + (apr_pool_t *p, char *url, char *split));
> > +
> > +/**
> > * 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);
> >
> > Index: modules/proxy/proxy_util.c
> > ===================================================================
> > --- modules/proxy/proxy_util.c (revision 1570613)
> > +++ modules/proxy/proxy_util.c (working copy)
> > @@ -1507,6 +1507,52 @@ PROXY_DECLARE(char *) ap_proxy_worker_name(apr_poo
> > return apr_pstrcat(p, "unix:", worker->s->uds_path, "|",
> worker->s->name, NULL);
> > }
> >
> > +static char *ap_proxy_worker_real_url(apr_pool_t *p, char *url, char
> **split)
> > +{
> > + char *ptr;
> > + /*
> > + * We could be passed a URL that contains the UDS path...
> > + * If any, save it in *split, and strip it.
> > + */
> > + if (split) {
> > + *split = 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 (split) {
> > + *split = apr_pstrmemdup(p, url, ptr - url);
> > + }
> > + if (c[1] == '\0') {
> > + return apr_pstrcat(p, ret, "//localhost", NULL);
> > + }
> > + else {
> > + return ret;
> > + }
> > + }
> > + return url;
> > +}
> > +
> > +static char *ap_proxy_worker_full_url(apr_pool_t *p, char *url, char
> *split)
> > +{
> > + if (split) {
> > + return apr_pstrcat(p, split, "|", url, NULL);
> > + }
> > + else {
> > + return url;
> > + }
> > +}
> > +
> > PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
> > proxy_balancer
> *balancer,
> > proxy_server_conf
> *conf,
> > @@ -1905,8 +1951,13 @@ 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 = *url, *uds_url = NULL;
> > + if (apr_table_get(r->notes, "rewrite-proxy")) {
> > + real_url = ap_proxy_worker_real_url(r->pool, *url,
> &uds_url);
> > + }
> > + *worker = ap_proxy_get_worker(r->pool, NULL, conf, real_url);
> > if (*worker) {
> > + *url = real_url;
> > ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> > "%s: found worker %s for %s",
> > (*worker)->s->scheme, (*worker)->s->name,
> *url);
> > @@ -1931,8 +1982,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;
> > @@ -1952,27 +2001,20 @@ 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_status_t rv;
> > 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);
> > 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:",
> real_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 = '|';
> > - }
> > }
> > }
> > }
> > @@ -3481,4 +3523,6 @@ void proxy_util_register_hooks(apr_pool_t *p)
> > {
> > APR_REGISTER_OPTIONAL_FN(ap_proxy_retry_worker);
> > APR_REGISTER_OPTIONAL_FN(ap_proxy_clear_connection);
> > + APR_REGISTER_OPTIONAL_FN(ap_proxy_worker_real_url);
> > + APR_REGISTER_OPTIONAL_FN(ap_proxy_worker_full_url);
> > }
> > Index: modules/proxy/mod_proxy.c
> > ===================================================================
> > --- modules/proxy/mod_proxy.c (revision 1570613)
> > +++ modules/proxy/mod_proxy.c (working copy)
> > @@ -32,6 +32,9 @@ APR_DECLARE_OPTIONAL_FN(char *, ssl_var_lookup,
> > conn_rec *, request_rec *, char *));
> > #endif
> >
> > +static char *(*proxy_worker_real_url)(apr_pool_t *p, char *url,
> > + char **split) = NULL;
> > +
> > #ifndef MAX
> > #define MAX(x,y) ((x) >= (y) ? (x) : (y))
> > #endif
> > @@ -1444,36 +1447,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)
> > {
> > @@ -1565,7 +1538,8 @@ static const char *
> > }
> >
> > new->fake = apr_pstrdup(cmd->pool, f);
> > - new->real = apr_pstrdup(cmd->pool, de_socketfy(cmd->pool, r));
> > + new->real = apr_pstrdup(cmd->pool,
> > + proxy_worker_real_url(cmd->temp_pool, r,
> NULL));
> > new->flags = flags;
> > if (use_regex) {
> > new->regex = ap_pregcomp(cmd->pool, f, AP_REG_EXTENDED);
> > @@ -1614,7 +1588,7 @@ 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(cmd->temp_pool,
> NULL, conf, new->real);
> > int reuse = 0;
> > if (!worker) {
> > const char *err = ap_proxy_define_worker(cmd->pool,
> &worker, NULL, conf, r, 0);
> > @@ -1626,14 +1600,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);
> > @@ -2093,7 +2068,9 @@ 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(cmd->temp_pool, balancer, conf,
> > + proxy_worker_real_url(cmd->temp_pool,
> > + name, NULL));
> > if (!worker) {
> > ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, cmd->server,
> APLOGNO(01147)
> > "Defining worker '%s' for balancer '%s'",
> > @@ -2179,7 +2156,9 @@ 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(cmd->temp_pool, NULL, conf,
> > +
> proxy_worker_real_url(cmd->temp_pool,
> > + name, NULL));
> > if (!worker) {
> > if (in_proxy_section) {
> > err = ap_proxy_define_worker(cmd->pool, &worker, NULL,
> > @@ -2319,7 +2298,9 @@ 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));
> > +
> proxy_worker_real_url(cmd->temp_pool,
> > +
> (char*)conf->p,
> > + NULL));
> > if (!worker) {
> > err = ap_proxy_define_worker(cmd->pool, &worker, NULL,
> > sconf, conf->p, 0);
> > @@ -2712,6 +2693,8 @@ static void register_hooks(apr_pool_t *p)
> >
> > /* register optional functions within proxy_util.c */
> > proxy_util_register_hooks(p);
> > +
> > + proxy_worker_real_url =
> APR_RETRIEVE_OPTIONAL_FN(ap_proxy_worker_real_url);
> > }
> >
> > AP_DECLARE_MODULE(proxy) =
> > [EOS]
> > <httpd-trunk-uds2.patch>
>
>
