[PATCH] Support FreeBSD jails for testing

[Steven Hartland]

On 19/10/2015 22:26, Sergey Kandaurov wrote:
> 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');
Looks good to me :)