You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2018/01/10 02:54:51 UTC
Problems with commit feature negotiation and write-through proxies
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
Re: Problems with commit feature negotiation and write-through proxies
Posted by Philip Martin <ph...@codematters.co.uk>.
Evgeny Kotkov <ev...@visualsvn.com> 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
Re: Problems with commit feature negotiation and write-through proxies
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> 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
Re: Problems with commit feature negotiation and write-through proxies
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Philip Martin <ph...@codematters.co.uk> 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
Re: Problems with commit feature negotiation and write-through proxies
Posted by Philip Martin <ph...@codematters.co.uk>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> Philip Martin <ph...@codematters.co.uk> 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
Re: Problems with commit feature negotiation and write-through proxies
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Philip Martin <ph...@codematters.co.uk> 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