threads / patch / 47516

patchhttp: fix v1 protocol tests with apache httpd < 2.4

Subject: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

## tl;dr

6 messages between Dec 31, 2017 and Jan 3, 2018. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Todd Zullinger· Dec 31, 2017, 02:32 UTC · lore

The apache config used by tests was updated to use the SetEnvIf directive to set the Git-Protocol header in 19113a26b6 ("http: tell server that the client understands v1", 2017-10-16).

Setting the Git-Protocol header is restricted to httpd >= 2.4, but mod_setenvif and the SetEnvIf directive work with lower versions, at least as far back as 2.0, according to the httpd documentation:

    https://httpd.apache.org/docs/2.0/mod/mod_setenvif.html
Drop the restriction.  Tested with httpd 2.2 and 2.4.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

These tests fail with 2.16.0-rc0 on CentOS-6, which uses httpd-2.2.

I removed the version restriction entirely rather than adjust the version. I believe SetEnvIf works on httpd >= 2.0. I'm not sure if we aim to support anything less than httpd 2.0, but I'm betting not. If that's incorrect, I can add some IfVersion conditions.

As noted in the commit message, I only tested with httpd 2.2 and 2.4 (on CentOS 6/7 and Fedora 26-28). If anyone has older httpd systems which need support, it would be great if they could test this before 2.16.0 is finalized, so we can avoid shipping with any failing tests.

 t/lib-httpd/apache.conf | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Show changes to t/lib-httpd/apache.conf +4 −6
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index df19436314..724d9ae462 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -25,6 +25,9 @@ ErrorLog error.log
 <IfModule !mod_headers.c>
 	LoadModule headers_module modules/mod_headers.so
 </IfModule>
+<IfModule !mod_setenvif.c>
+	LoadModule setenvif_module modules/mod_setenvif.so
+</IfModule>
 
 <IfVersion < 2.4>
 LockFile accept.lock
@@ -67,9 +70,6 @@ LockFile accept.lock
 <IfModule !mod_unixd.c>
 	LoadModule unixd_module modules/mod_unixd.so
 </IfModule>
-<IfModule !mod_setenvif.c>
-	LoadModule setenvif_module modules/mod_setenvif.so
-</IfModule>
 </IfVersion>
 
 PassEnv GIT_VALGRIND
@@ -79,9 +79,7 @@ PassEnv ASAN_OPTIONS
 PassEnv GIT_TRACE
 PassEnv GIT_CONFIG_NOSYSTEM
 
-<IfVersion >= 2.4>
-	SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
-</IfVersion>
+SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
 
 Alias /dumb/ www/
 Alias /auth/dumb/ www/auth/dumb/
-- 
2.16.0.rc0
Brandon Williams· Jan 3, 2018, 00:21 UTC · re: Todd Zullinger · lore

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

On 12/30, Todd Zullinger wrote:
Show 29 quoted lines
> The apache config used by tests was updated to use the SetEnvIf
> directive to set the Git-Protocol header in 19113a26b6 ("http: tell
> server that the client understands v1", 2017-10-16).
> 
> Setting the Git-Protocol header is restricted to httpd >= 2.4, but
> mod_setenvif and the SetEnvIf directive work with lower versions, at
> least as far back as 2.0, according to the httpd documentation:
> 
>     https://httpd.apache.org/docs/2.0/mod/mod_setenvif.html
> 
> Drop the restriction.  Tested with httpd 2.2 and 2.4.
> 
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> 
> These tests fail with 2.16.0-rc0 on CentOS-6, which uses
> httpd-2.2.
> 
> I removed the version restriction entirely rather than adjust
> the version.  I believe SetEnvIf works on httpd >= 2.0.  I'm
> not sure if we aim to support anything less than httpd 2.0,
> but I'm betting not.  If that's incorrect, I can add some
> IfVersion conditions.
> 
> As noted in the commit message, I only tested with httpd 2.2
> and 2.4 (on CentOS 6/7 and Fedora 26-28).  If anyone has older
> httpd systems which need support, it would be great if they
> could test this before 2.16.0 is finalized, so we can avoid
> shipping with any failing tests.

Seems good to me. I think I was just being overly cautious when i was implementing those patches. Thanks!

Show 42 quoted lines
> 
>  t/lib-httpd/apache.conf | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index df19436314..724d9ae462 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -25,6 +25,9 @@ ErrorLog error.log
>  <IfModule !mod_headers.c>
>  	LoadModule headers_module modules/mod_headers.so
>  </IfModule>
> +<IfModule !mod_setenvif.c>
> +	LoadModule setenvif_module modules/mod_setenvif.so
> +</IfModule>
>  
>  <IfVersion < 2.4>
>  LockFile accept.lock
> @@ -67,9 +70,6 @@ LockFile accept.lock
>  <IfModule !mod_unixd.c>
>  	LoadModule unixd_module modules/mod_unixd.so
>  </IfModule>
> -<IfModule !mod_setenvif.c>
> -	LoadModule setenvif_module modules/mod_setenvif.so
> -</IfModule>
>  </IfVersion>
>  
>  PassEnv GIT_VALGRIND
> @@ -79,9 +79,7 @@ PassEnv ASAN_OPTIONS
>  PassEnv GIT_TRACE
>  PassEnv GIT_CONFIG_NOSYSTEM
>  
> -<IfVersion >= 2.4>
> -	SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
> -</IfVersion>
> +SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
>  
>  Alias /dumb/ www/
>  Alias /auth/dumb/ www/auth/dumb/
> -- 
> 2.16.0.rc0
> 
-- 
Brandon Williams
Todd Zullinger· Jan 3, 2018, 00:39 UTC · re: Brandon Williams · lore

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

Brandon Williams wrote:
> Seems good to me.  I think I was just being overly cautious when i was
> implementing those patches.  Thanks!
Cool, thanks for taking a look.

In this case, the test isn't dependent on apache > 2.4, but before I checked that I wondered if we had any way to run a test only if the apache version was greater or lesser than some release. Luckily, I didn't have to work out such a method. :)

I don't know if there's a clean way to do that automatically, short of parsing the output of 'httpd -v' should we ever need to add such a prereq.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Sometimes you get the blues because your baby leaves you. Sometimes
you get'em 'cause she comes back.
    -- B.B. King
Jeff King· Jan 3, 2018, 06:17 UTC · re: Todd Zullinger · lore

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

On Tue, Jan 02, 2018 at 07:39:46PM -0500, Todd Zullinger wrote:
Show 15 quoted lines
> Brandon Williams wrote:
> > Seems good to me.  I think I was just being overly cautious when i was
> > implementing those patches.  Thanks!
> 
> Cool, thanks for taking a look.
> 
> In this case, the test isn't dependent on apache > 2.4, but
> before I checked that I wondered if we had any way to run a
> test only if the apache version was greater or lesser than
> some release.  Luckily, I didn't have to work out such a
> method. :)
> 
> I don't know if there's a clean way to do that
> automatically, short of parsing the output of 'httpd -v'
> should we ever need to add such a prereq.

In the general case, we could probably define an endpoint within an <If> block, and then try to access the endpoint from the test script.

E.g., something like:
    <IfVersion >= 2.4>
    Alias /have-2.4.txt www/yes.txt
    </IfVersion>
in the apache config, and then:
  test_lazy_prereq APACHE24 '
        echo yes >"$HTTPD_DOCUMENT_ROOT_PATH/yes.txt" &&
        curl -f "$HTTPD_URL/have-2.4.txt"
  '

in the test script (of course we may not want to depend on having command-line curl, but we could replace that with "git ls-remote" or similar).

One nice thing about that approach is that it can be extended to other "If" blocks, like if we have a particular module available, or if ssl is configured.

-Peff
Todd Zullinger· Jan 3, 2018, 16:21 UTC · re: Jeff King · lore

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

Jeff King wrote:
Show 28 quoted lines
> On Tue, Jan 02, 2018 at 07:39:46PM -0500, Todd Zullinger wrote:
>> I don't know if there's a clean way to do that
>> automatically, short of parsing the output of 'httpd -v'
>> should we ever need to add such a prereq.
> 
> In the general case, we could probably define an endpoint within an <If>
> block, and then try to access the endpoint from the test script.
> 
> E.g., something like:
> 
>     <IfVersion >= 2.4>
>     Alias /have-2.4.txt www/yes.txt
>     </IfVersion>
> 
> in the apache config, and then:
> 
>   test_lazy_prereq APACHE24 '
>         echo yes >"$HTTPD_DOCUMENT_ROOT_PATH/yes.txt" &&
>         curl -f "$HTTPD_URL/have-2.4.txt"
>   '
> 
> in the test script (of course we may not want to depend on having
> command-line curl, but we could replace that with "git ls-remote" or
> similar).
> 
> One nice thing about that approach is that it can be extended to other
> "If" blocks, like if we have a particular module available, or if ssl is
> configured.

That's quite elegant. I even modified an IfVersion block and didn't think about using it that way to create a prereq. Neat!

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
You're not drunk if you can lie on the floor without holding on.
    -- Dean Martin
Jeff King· Jan 3, 2018, 05:56 UTC · re: Todd Zullinger · lore

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

On Sat, Dec 30, 2017 at 09:32:34PM -0500, Todd Zullinger wrote:
Show 11 quoted lines
> The apache config used by tests was updated to use the SetEnvIf
> directive to set the Git-Protocol header in 19113a26b6 ("http: tell
> server that the client understands v1", 2017-10-16).
> 
> Setting the Git-Protocol header is restricted to httpd >= 2.4, but
> mod_setenvif and the SetEnvIf directive work with lower versions, at
> least as far back as 2.0, according to the httpd documentation:
> 
>     https://httpd.apache.org/docs/2.0/mod/mod_setenvif.html
> 
> Drop the restriction.  Tested with httpd 2.2 and 2.4.

Makes sense. I think the only way this could backfire is if somebody has a funny build that doesn't include mod_setenvif at all. But I don't think we can know that for sure without applying this and seeing if anybody screams.

Show 5 quoted lines
> I removed the version restriction entirely rather than adjust
> the version.  I believe SetEnvIf works on httpd >= 2.0.  I'm
> not sure if we aim to support anything less than httpd 2.0,
> but I'm betting not.  If that's incorrect, I can add some
> IfVersion conditions.

IIRC, anything less than 2.0 is broken already. And it's not worth changing that, given the age (and I think there were some pretty severe hardships in making 1.3.x, but it's been a while).

-Peff

← back to recent threads