[PATCH] ensure mod_proxy present when mod_dav_svn configured for mirroring

[Dave Brown-7]

[[[
In mod_dav_svn, when processing an SVNMasterURI directive,
check that mod_proxy is available in the httpd runtime.
Forwarding writes to a master from a slave requires
the mod_proxy handler, and when it isn't present, the
failure is ugly & opaque.  Apache's core, default handler
sends back a "405 Not Allowed," for non-GET which looks
like an authz failure.

BTW, there's precedent for using ap_find_linked_module()
to check that module dependencies are present.  Namely,
mod_rewrite looks for mod_proxy, and mod_authnz_ldap looks
for util_ldap.

* subversion/mod_dav_svn/mod_dav_svn.c
  (SVNMasterURI_cmd): use ap_find_linked_module() to
   ensure that mod_proxy is available

]]]

Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 40267)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -219,6 +219,12 @@
  SVNMasterURI_cmd(cmd_parms *cmd, void *config, const char *arg1)
  {
    dir_conf_t *conf = config;
+  /* Since SVNMasterURI requires mod_proxy's handler
+   * (r->handler = "proxy-server" in mirror.c) make
+   * sure it is present.
+   */
+  if (ap_find_linked_module("mod_proxy.c") == NULL)
+    return "module mod_proxy not loaded, required for SVNMasterURI";

    conf->master_uri = apr_pstrdup(cmd->pool, arg1);

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2412741

[Julian Foad]

On Thu, 2009-10-29, Dave Brown wrote:

> [[[
> In mod_dav_svn, when processing an SVNMasterURI directive,
> check that mod_proxy is available in the httpd runtime.
> Forwarding writes to a master from a slave requires
> the mod_proxy handler, and when it isn't present, the
> failure is ugly & opaque.  Apache's core, default handler
> sends back a "405 Not Allowed," for non-GET which looks
> like an authz failure.
>
> BTW, there's precedent for using ap_find_linked_module()
> to check that module dependencies are present.  Namely,
> mod_rewrite looks for mod_proxy, and mod_authnz_ldap looks
> for util_ldap.
>
> * subversion/mod_dav_svn/mod_dav_svn.c
>   (SVNMasterURI_cmd): use ap_find_linked_module() to
>    ensure that mod_proxy is available
> ]]]

Hi Dave.

As I understood your explanation to me the other day:

The problem is if you try to use the DAV write-through proxy, but don't
have mod_proxy installed, then Subversion would issue an obscure and
unhelpful error message at the time of trying to use it.

The fix is twofold: check for mod_proxy being installed as soon as we
know we need it, and issue a nice error message.

This sounds great, exactly the sort of improvement we need.

If nobody thinks of a problem with it, I will commit it. I will try to
test it first, but I know you have done so.

Thanks for the enhancement!
- Julian

> Index: subversion/mod_dav_svn/mod_dav_svn.c
> ===================================================================
> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 40267)
> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
> @@ -219,6 +219,12 @@
>   SVNMasterURI_cmd(cmd_parms *cmd, void *config, const char *arg1)
>   {
>     dir_conf_t *conf = config;
> +  /* Since SVNMasterURI requires mod_proxy's handler
> +   * (r->handler = "proxy-server" in mirror.c) make
> +   * sure it is present.
> +   */
> +  if (ap_find_linked_module("mod_proxy.c") == NULL)
> +    return "module mod_proxy not loaded, required for SVNMasterURI";
>
>     conf->master_uri = apr_pstrdup(cmd->pool, arg1);

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414702

[Dave Brown-7]

Julian Foad wrote:

> On Thu, 2009-10-29, Dave Brown wrote:
>> [[[
>> In mod_dav_svn, when processing an SVNMasterURI directive,
>> check that mod_proxy is available in the httpd runtime.
>> Forwarding writes to a master from a slave requires
>> the mod_proxy handler, and when it isn't present, the
>> failure is ugly & opaque.  Apache's core, default handler
>> sends back a "405 Not Allowed," for non-GET which looks
>> like an authz failure.
>>
>> BTW, there's precedent for using ap_find_linked_module()
>> to check that module dependencies are present.  Namely,
>> mod_rewrite looks for mod_proxy, and mod_authnz_ldap looks
>> for util_ldap.
>>
>> * subversion/mod_dav_svn/mod_dav_svn.c
>>   (SVNMasterURI_cmd): use ap_find_linked_module() to
>>    ensure that mod_proxy is available
>> ]]]
>
> Hi Dave.
>
> As I understood your explanation to me the other day:
>
> The problem is if you try to use the DAV write-through proxy, but don't
> have mod_proxy installed, then Subversion would issue an obscure and
> unhelpful error message at the time of trying to use it.

Yes.  The symptoms I saw were the same as what these guys were seeing on
this thread:

http://old.nabble.com/Problem-committing-to-a-write-thru-proxy-td20524286.html

And it's not at all obvious that "405" means "mod_proxy unavailable."
Further, you don't get mod_proxy if you configure httpd-2.2.x with
"most" shared modules (--enable-mods-shared=most).  I don't know how
common configuring with "most" modules are, but it's how I ran into this.

My one concern was getting a false negative on the check that this patch
enables, i.e., mod_proxy is present in the configuration, but
ap_find_linked_module() doesn't find it for some reason.  But again,
there's precendent in httpd's own modules for using ap_find_linked()
module in this way.  I also tried with mod_proxy statically linked, with
the expected result as well.

>
> The fix is twofold: check for mod_proxy being installed as soon as we
> know we need it, and issue a nice error message.
>
> This sounds great, exactly the sort of improvement we need.
>
> If nobody thinks of a problem with it, I will commit it. I will try to
> test it first, but I know you have done so.

Thanks for looking at it.  I think it's a righteous fix, but would love
any more confirmation we can get.

-dave

>
> Thanks for the enhancement!
> - Julian
>
>
>> Index: subversion/mod_dav_svn/mod_dav_svn.c
>> ===================================================================
>> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 40267)
>> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
>> @@ -219,6 +219,12 @@
>>   SVNMasterURI_cmd(cmd_parms *cmd, void *config, const char *arg1)
>>   {
>>     dir_conf_t *conf = config;
>> +  /* Since SVNMasterURI requires mod_proxy's handler
>> +   * (r->handler = "proxy-server" in mirror.c) make
>> +   * sure it is present.
>> +   */
>> +  if (ap_find_linked_module("mod_proxy.c") == NULL)
>> +    return "module mod_proxy not loaded, required for SVNMasterURI";
>>
>>     conf->master_uri = apr_pstrdup(cmd->pool, arg1);
>
>
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414720

[C. Michael Pilato]

Julian Foad wrote:

> On Thu, 2009-10-29, Dave Brown wrote:
>> [[[
>> In mod_dav_svn, when processing an SVNMasterURI directive,
>> check that mod_proxy is available in the httpd runtime.
>> Forwarding writes to a master from a slave requires
>> the mod_proxy handler, and when it isn't present, the
>> failure is ugly & opaque.  Apache's core, default handler
>> sends back a "405 Not Allowed," for non-GET which looks
>> like an authz failure.
>>
>> BTW, there's precedent for using ap_find_linked_module()
>> to check that module dependencies are present.  Namely,
>> mod_rewrite looks for mod_proxy, and mod_authnz_ldap looks
>> for util_ldap.
>>
>> * subversion/mod_dav_svn/mod_dav_svn.c
>>   (SVNMasterURI_cmd): use ap_find_linked_module() to
>>    ensure that mod_proxy is available
>> ]]]
>
> Hi Dave.
>
> As I understood your explanation to me the other day:
>
> The problem is if you try to use the DAV write-through proxy, but don't
> have mod_proxy installed, then Subversion would issue an obscure and
> unhelpful error message at the time of trying to use it.
>
> The fix is twofold: check for mod_proxy being installed as soon as we
> know we need it, and issue a nice error message.
>
> This sounds great, exactly the sort of improvement we need.
>
> If nobody thinks of a problem with it, I will commit it. I will try to
> test it first, but I know you have done so.

Sorry, Dave, this patch (which I rather witnessed the conception of) skipped
past my radar somehow.

+1 to commit, Julian (with the small request that a blank line precede the
newly added comment).

--
C. Michael Pilato <cmpilato@...>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414725

[Julian Foad]

C. Michael Pilato wrote:
> Sorry, Dave, this patch (which I rather witnessed the conception of) skipped
> past my radar somehow.
>
> +1 to commit, Julian (with the small request that a blank line precede the
> newly added comment).

Thanks, Mike.

Committed revision 40385.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414756

[Daniel Rall]

On Thu, Nov 5, 2009 at 7:58 AM, Julian Foad <julianfoad@...> wrote:

> C. Michael Pilato wrote:
>> Sorry, Dave, this patch (which I rather witnessed the conception of) skipped
>> past my radar somehow.
>>
>> +1 to commit, Julian (with the small request that a blank line precede the
>> newly added comment).
>
> Thanks, Mike.
>
> Committed revision 40385.

Looks great.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414979

[Philip Martin-4]

Julian Foad <julianfoad@...> writes:

> C. Michael Pilato wrote:
>> Sorry, Dave, this patch (which I rather witnessed the conception of) skipped
>> past my radar somehow.
>>
>> +1 to commit, Julian (with the small request that a blank line precede the
>> newly added comment).
>
> Thanks, Mike.
>
> Committed revision 40385.

It's better, but still not enough on my box.  Without mod_proxy I get
a 405:

  svn: Server sent unexpected return value (405 Method Not Allowed) in response to MKACTIVITY request for '/slave/!svn/act/3e451258-0c27-4616-b515-1b4d1bbc0424'

but with mod_proxy and without mod_proxy_html I get a 500:

  svn: Server sent unexpected return value (500 Internal Server Error) in response to MKACTIVITY request for '/slave/!svn/act/8cc2a957-2956-4ea2-8287-ade9c64bb873'

With the 500 I do get an error in error_log:

  [Wed Nov 11 09:18:49 2009] [warn] proxy: No protocol handler was valid for the URL /slave/!svn/act/8cc2a957-2956-4ea2-8287-ade9c64bb873. If you are using a DSO version of mod_proxy, make sure the proxy submodules are included in the configuration using LoadModule.

but it doesn't contain the magic word dav_proxy_html.

Should we add a similar check for dav_proxy_html?

--
Philip

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416431

[Philip Martin]

Philip Martin <philip.martin@...> writes:

> but it doesn't contain the magic word dav_proxy_html.
>
> Should we add a similar check for dav_proxy_html?

mod_proxy_html not dav_proxy_html

--
Philip

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416434

[Philip Martin-4]

Philip Martin <philip@...> writes:

> Philip Martin <philip.martin@...> writes:
>
>> but it doesn't contain the magic word dav_proxy_html.
>>
>> Should we add a similar check for dav_proxy_html?
>
> mod_proxy_html not dav_proxy_html

Gah! That's mod_proxy_http.  MOD and HTTP!

--
Philip

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416510

[Justin Erenkrantz-2]

On Wed, Nov 11, 2009 at 12:41 PM, Philip Martin
<philip.martin@...> wrote:

> Philip Martin <philip@...> writes:
>
>> Philip Martin <philip.martin@...> writes:
>>
>>> but it doesn't contain the magic word dav_proxy_html.
>>>
>>> Should we add a similar check for dav_proxy_html?
>>
>> mod_proxy_html not dav_proxy_html
>
> Gah! That's mod_proxy_http.  MOD and HTTP!

Yah, I was going to say - html made no sense there.

+1 for adding the check for mod_proxy_http.  -- justin

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416663