[PATCH] Support FreeBSD jails for testing

[Sergey Kandaurov]

On Oct 19, 2015, at 8:57 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> Hello!
> 
> On Fri, Oct 16, 2015 at 06:24:11PM +0100, Steven Hartland wrote:
> 
>> On 16/10/2015 13:05, Maxim Dounin wrote:
>>> Hello!
>>> 
>>> On Fri, Oct 16, 2015 at 12:09:49AM +0000, Steven Hartland wrote:
>>> 
>>>> # HG changeset patch
>>>> # User Steven Hartland <steven.hartland at multiplay.co.uk>
>>>> # Date 1444954080 0
>>>> #      Fri Oct 16 00:08:00 2015 +0000
>>>> # Node ID c22d8299e7040e0de6f85b4e96d0dd953f7af644
>>>> # Parent  78b4e12e6efe642aff591234db0f0b040cae9b5e
>>>> Support FreeBSD jails for testing
>>>> 
>>>> Ensure the test directory is read and writable to the test user.
>>>> 
>>>> If you request 127.0.0.1 in a FreeBSD jail without specific access
>>>> to 127.0.0.1 then the socket binds to the interface address to
>>>> maintain compatibility. This results in the log entries being
>>>> from the bound interface address. To prevent failure compare
>>>> with the bound IP when requesting 127.0.0.1 in combined test.
>>> This jails behaviour is known to cause many problems, in
>>> particular, it makes impossible nginx binary upgrades unless all
>>> listen sockets are explicitly bound to jail's IP address.
>>> 
>>> Fortunately, this was resolved several years ago by introduction
>>> of multi-IP jails.  You may try to use them for tests instead.
>>> 
>>> Adding quirks everywhere to support this brain-damaged "no
>>> 127.0.0.1" case looks like a wrong way to go for me.  Especially
>>> given the fact that simple solution exists for years.
>>> 
>>> [...]
>> That doesn't fix the directory permission issue which causes pretty much
>> every test to fail, so is this still an option for inclusion?
> 
> Directory premissions may vary depending on umask used.  If your 
> umask breaks tests for you - you may try changing it while running 
> tests.  It also shouldn't be important when running tests under 
> non-privileged user.
> 
> I don't think this change should be added at all, and don't 
> see how it’s related to FreeBSD jails your patch says it's about.

Same here.

> 
>> With regards to binding 127.0.0.1, yes its possible to bind it using multi
>> IP, but doing so breaks security if you share it with the host, so its only
>> possible in some situations and usually only for a proper loop back address
>> which wouldn't be 127.0.0.1 just in that /24.
> 
> AFAIR, multi-IP jails used to provide per-jail loopback 
> addresses.  But may be I'm wrong here and mistaken with wildcard 
> addresses.
> 
>> I do agree quirks aren't ideal, but as its only the one test I thought it
>> would be nice to have given there's a simple and reliable way to correct
>> said test.
>> 
>> With this in mind would you be up for making an exception in this case?
> 
> As long as it's the only test affected we may consider it.
> Sergey, could you please take a look?

I have no strong objections, but see below.

: # HG changeset patch
: # User Steven Hartland <steven.hartland at multiplay.co.uk>
: # Date 1444954080 0
: #      Fri Oct 16 00:08:00 2015 +0000
: # Node ID c22d8299e7040e0de6f85b4e96d0dd953f7af644
: # Parent  78b4e12e6efe642aff591234db0f0b040cae9b5e
: Support FreeBSD jails for testing

Style: missing prefix “Tests: ” used for tests, and a trailing dot.

: 
: Ensure the test directory is read and writable to the test user.

Not applicable as discussed above.

: 
: If you request 127.0.0.1 in a FreeBSD jail without specific access
: to 127.0.0.1 then the socket binds to the interface address to
: maintain compatibility. This results in the log entries being
: from the bound interface address. To prevent failure compare
: with the bound IP when requesting 127.0.0.1 in combined test.

I’d rather refer to ea574aae77cb for brevity.

: 
: diff -r 78b4e12e6efe -r c22d8299e704 access_log.t
: --- a/access_log.t	Mon Oct 12 12:57:03 2015 +0300
: +++ b/access_log.t	Fri Oct 16 00:08:00 2015 +0000
: @@ -155,11 +155,17 @@ SKIP: {
: 
: # verify that by default, 'combined' format is used, 'off' disables logging
: 
: -$log = $t->read_file('combined.log');
: -like($log,
: -	qr!^\Q127.0.0.1 - - [\E .*
: -		\Q] "GET /combined HTTP/1.0" 200 2 "-" "-"\E$!x,
: -	'default log format');
: +SKIP: {
: +	my $sock = IO::Socket::INET->new(LocalAddr => '127.0.0.1');
: +	skip(‘127.0.0.1 local address required', 1) unless defined $sock;

This looks like an excessive step.
If perl could not successfully bind to 127.0.0.1 (or whatever it thinks to),
nginx won’t start as well with failure effectively skipping all the tests.

: +
: +	$log = $t->read_file('combined.log');
: +	my $addr = $sock->sockhost;

I suggest to use a method with explicit parentheses,
also for consistency with the rest nginx test suite.

: +	like($log,
: +		qr!^\Q$addr - - [\E .*
: +			\Q] "GET /combined HTTP/1.0" 200 2 "-" "-"\E$!x,
: +		'default log format');
: +}
: 
: # verify that log filtering works
: 

I intend to commit this change:

# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1445289338 -10800
#      Tue Oct 20 00:15:38 2015 +0300
# Node ID 0af5555e0b7b130d7162d304b074932331267882
# Parent  cdd3659d114487d55119e1bb45f9a5fb44dbf79c
Tests: unbreak access_log tests in jails.

See ea574aae77cb for details.

Reported by Steven Hartland, see
http://mailman.nginx.org/pipermail/nginx-devel/2015-October/007393.html.

diff -r cdd3659d1144 -r 0af5555e0b7b access_log.t
--- a/access_log.t	Mon Oct 19 20:43:22 2015 +0300
+++ b/access_log.t	Tue Oct 20 00:15:38 2015 +0300
@@ -155,9 +155,11 @@ SKIP: {
 
 # verify that by default, 'combined' format is used, 'off' disables logging
 
+my $addr = IO::Socket::INET->new(LocalAddr => '127.0.0.1')->sockhost();
+
 $log = $t->read_file('combined.log');
 like($log,
-	qr!^\Q127.0.0.1 - - [\E .*
+	qr!^\Q$addr - - [\E .*
 		\Q] "GET /combined HTTP/1.0" 200 2 "-" "-"\E$!x,
 	'default log format');
 

-- 
Sergey Kandaurov