Problems with commit feature negotiation and write-through proxies

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Problems with commit feature negotiation and write-through proxies

Philip Martin
I run a write-through proxy for svn.apache.org using mod_dav_svn's
SVNMasterURI directive.  Commits now fail when using trunk on the client
and slave because they negotiate svndiff2 which the s.a.o master does
not support.

I can reproduce locally using a trunk slave and 1.9 master:

  $ svn import -mm repo/format http://localhost:8888/slave/f
  Adding         repo/format
  svn: E185000: Svndiff has invalid header

In the past we supported different versions on the master and slave and
we introduced the SVNMasterVersion directive to configure it.
Unfortunately that information is not immediately available in
get_vsn_options() where the server advertises some commit features.  If
I simply remove the SVN_DAV_NS_DAV_SVN_SVNDIFF2 header from
get_vsn_options() then I can commit.

I'm not sure how we fix this.  Do we delay the svndiff2 negotiation
until later in the commit?  Do we abandon mixed master/slave versions?

There is a similar problem with SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM,
the slave advertises support but the master does not provide it.  In
this case the commit does not fail although neither the client nor the
server sends X-SVN-Result-Fulltext-MD5.  I think that means we commit
without checksum verification.

--
Philip
Reply | Threaded
Open this post in threaded view
|

Re: Problems with commit feature negotiation and write-through proxies

Evgeny Kotkov
Philip Martin <[hidden email]> writes:

> In the past we supported different versions on the master and slave and
> we introduced the SVNMasterVersion directive to configure it.
> Unfortunately that information is not immediately available in
> get_vsn_options() where the server advertises some commit features.  If
> I simply remove the SVN_DAV_NS_DAV_SVN_SVNDIFF2 header from
> get_vsn_options() then I can commit.
>
> I'm not sure how we fix this.  Do we delay the svndiff2 negotiation
> until later in the commit?  Do we abandon mixed master/slave versions?

I think that we might be able to selectively stop advertising the new-
in-1.10 capabilities based on the SVNMasterVersion value, similarly to
how we handle SVN_DAV_NS_DAV_SVN_EPHEMERAL_TXNPROPS.

Perhaps, we could do something along the lines of the attached patch?


Thanks,
Evgeny Kotkov

dav-proxy-commit-negotiation-fixup-v1.patch.txt (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Problems with commit feature negotiation and write-through proxies

Philip Martin
Evgeny Kotkov <[hidden email]> writes:

> Philip Martin <[hidden email]> writes:
>
>> In the past we supported different versions on the master and slave and
>> we introduced the SVNMasterVersion directive to configure it.
>> Unfortunately that information is not immediately available in
>> get_vsn_options() where the server advertises some commit features.  If
>> I simply remove the SVN_DAV_NS_DAV_SVN_SVNDIFF2 header from
>> get_vsn_options() then I can commit.
>>
>> I'm not sure how we fix this.  Do we delay the svndiff2 negotiation
>> until later in the commit?  Do we abandon mixed master/slave versions?
>
> I think that we might be able to selectively stop advertising the new-
> in-1.10 capabilities based on the SVNMasterVersion value, similarly to
> how we handle SVN_DAV_NS_DAV_SVN_EPHEMERAL_TXNPROPS.
>
> Perhaps, we could do something along the lines of the attached patch?

We already have system for handling create-txn and create-txn-with-props
so I was thinking of extending that code:

Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 1820704)
+++ subversion/mod_dav_svn/version.c (working copy)
@@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
   /* Mergeinfo is a special case: here we merely say that the server
    * knows how to handle mergeinfo -- whether the repository does too
@@ -297,6 +294,19 @@ get_option(const dav_resource *resource,
         { "create-txn-with-props",  { 1, 8, 0, "" } },
       };
 
+      /* These capabilities are used during commit and when acting as
+         a WebDAV slave (SVNMasterURI) their availablity depends on
+         the master version (SVNMasterVersion) rather than our own
+         (slave) version. */
+      struct capability_versions_t {
+        const char *capability_name;
+        svn_version_t min_version;
+      } capabilities[] = {
+        { SVN_DAV_NS_DAV_SVN_SVNDIFF1, { 1, 7, 0, ""} },
+        { SVN_DAV_NS_DAV_SVN_SVNDIFF2, { 1, 10, 0, ""} },
+        { SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM, { 1, 10, 0, ""} },
+      };
+
       /* Add the header which indicates that this server can handle
          replay REPORTs submitted against an HTTP v2 revision resource. */
       apr_table_addn(r->headers_out, "DAV",
@@ -347,6 +357,21 @@ get_option(const dav_resource *resource,
           apr_table_addn(r->headers_out, SVN_DAV_SUPPORTED_POSTS_HEADER,
                          apr_pstrdup(r->pool, posts_versions[i].post_name));
         }
+
+      /* Report the capabilites that rely on the master version. */
+      for (i = 0; i < sizeof(capabilities)/sizeof(capabilities[0]); ++i)
+        {
+          if (master_version
+              && (!svn_version__at_least(master_version,
+                                         capabilities[i].min_version.major,
+                                         capabilities[i].min_version.minor,
+                                         capabilities[i].min_version.patch)))
+            continue;
+
+          apr_table_addn(r->headers_out, "DAV",
+                         capabilities[i].capability_name);
+        
+        }
     }
 
   return NULL;


--
Philip
Reply | Threaded
Open this post in threaded view
|

Re: Problems with commit feature negotiation and write-through proxies

Evgeny Kotkov
Philip Martin <[hidden email]> writes:

> We already have system for handling create-txn and create-txn-with-props
> so I was thinking of extending that code:
>
> Index: subversion/mod_dav_svn/version.c
> ===================================================================
> --- subversion/mod_dav_svn/version.c    (revision 1820704)
> +++ subversion/mod_dav_svn/version.c    (working copy)
> @@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
>    /* Mergeinfo is a special case: here we merely say that the server
>     * knows how to handle mergeinfo -- whether the repository does too
> @@ -297,6 +294,19 @@ get_option(const dav_resource *resource,
>          { "create-txn-with-props",  { 1, 8, 0, "" } },
>        };
>
> +      /* These capabilities are used during commit and when acting as
> +         a WebDAV slave (SVNMasterURI) their availablity depends on
> +         the master version (SVNMasterVersion) rather than our own
> +         (slave) version. */
> +      struct capability_versions_t {
> +        const char *capability_name;
> +        svn_version_t min_version;
> +      } capabilities[] = {
> +        { SVN_DAV_NS_DAV_SVN_SVNDIFF1, { 1, 7, 0, ""} },
> +        { SVN_DAV_NS_DAV_SVN_SVNDIFF2, { 1, 10, 0, ""} },
> +        { SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM, { 1, 10, 0, ""} },
> +      };

I would be fine with this approach as well, but perhaps with a few tweaks:

 - I think that it would be better to handle all four capabilities that
   relate to proxied writes in the same place:

     SVN_DAV_NS_DAV_SVN_EPHEMERAL_TXNPROPS
     SVN_DAV_NS_DAV_SVN_SVNDIFF1
     SVN_DAV_NS_DAV_SVN_SVNDIFF2
     SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM

   Otherwise, the handling becomes asymmetric, as the first of them is still
   checked using the separate dav_svn__check_ephemeral_txnprops_support()
   function, but the three new capabilities are checked using the new array-
   based approach.

 - The SVN_DAV_NS_DAV_SVN_SVNDIFF1 capability should probably be
   bound to version 1.10 instead of 1.7.

   The reason for that is that mod_dav_svn didn't properly advertise
   svndiff1 support with this specific header until 1.10, and I think
   that we should have the same behavior for the setups with a write-
   through proxy.

   (Even though mod_dav_svn has been able to parse it for a long time,
    currently ra_serf avoids sending svndiff1 to servers that don't
    advertise it with this specific header, as there might be third-party
    implementations of the Subversion's HTTP protocol that can only
    parse svndiff0).


Thanks,
Evgeny Kotkov
Reply | Threaded
Open this post in threaded view
|

Re: Problems with commit feature negotiation and write-through proxies

Evgeny Kotkov
Evgeny Kotkov <[hidden email]> writes:

>> Index: subversion/mod_dav_svn/version.c
>> ===================================================================
>> --- subversion/mod_dav_svn/version.c    (revision 1820704)
>> +++ subversion/mod_dav_svn/version.c    (working copy)
>> @@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
>>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
>>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
>>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
>> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
>> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
>> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
>>    apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
>>    /* Mergeinfo is a special case: here we merely say that the server
>>     * knows how to handle mergeinfo -- whether the repository does too
>> @@ -297,6 +294,19 @@ get_option(const dav_resource *resource,
>>          { "create-txn-with-props",  { 1, 8, 0, "" } },
>>        };
>>
>> +      /* These capabilities are used during commit and when acting as
>> +         a WebDAV slave (SVNMasterURI) their availablity depends on
>> +         the master version (SVNMasterVersion) rather than our own
>> +         (slave) version. */
>> +      struct capability_versions_t {
>> +        const char *capability_name;
>> +        svn_version_t min_version;
>> +      } capabilities[] = {
>> +        { SVN_DAV_NS_DAV_SVN_SVNDIFF1, { 1, 7, 0, ""} },
>> +        { SVN_DAV_NS_DAV_SVN_SVNDIFF2, { 1, 10, 0, ""} },
>> +        { SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM, { 1, 10, 0, ""} },
>> +      };
>
> I would be fine with this approach as well, but perhaps with a few tweaks:

I also notice that this patch only advertises the new capabilities if the
server is configured to enable HTTPv2.  This is probably unintended, as
I think that there is no reason to disable these PUT-related capabilities
(such as being able to parse svndiff1/svndiff2) if the server is configured
to prefer HTTPv1 protocol with SVNAdvertiseV2Protocol off.


Thanks,
Evgeny Kotkov
Reply | Threaded
Open this post in threaded view
|

Re: Problems with commit feature negotiation and write-through proxies

Philip Martin
In reply to this post by Evgeny Kotkov
Evgeny Kotkov <[hidden email]> writes:

> I would be fine with this approach as well, but perhaps with a few tweaks:

Right, I'll incorporate those changes, test it and commit.

--
Philip