On Sat, Feb 17, 2018 at 08:34:41AM +0900, Michael Paquier wrote: > For this an environment variable seems suited to me. Say if > PG_TAP_ALLOW_INSECURE is set, then the tests said "insecure" are allowed > to run. If the tests are tried to be run, then they are just skipped > with a nice log message to tell you about it. The cool part about this > design is that all the tests that are not part of check-world could be > integrated in it. Most developers don't run regression tests on a > shared resource. Let me think about a full-fledged patch first. > Documentation also matters a lot.
Attached is what I have finished with. I have gathered the feedback from everybody on this thread and I think that the result can satisfy all the requirements mentioned: - 0001 is a small patch which makes the SSL and LDAP test suite fail immediately if the build's ./configure is not set up with necessary build options. This uses TestLib::check_pg_config to do the checks. - 0002 introduces a new environment variable which can be used to decide if an extra test suite can be used or not. I have named it PROVE_EXTRA_ALLOWED, and can be used as such: make -C src/test check PROVE_EXTRA_ALLOWED=3D'ssl ldap' This includes also some documentation. Note that with default settings the tests are skipped to keep things secure. 0002 is close to the logic mentioned by Peter E just upthread. To be more consistent with PROVE_TESTS and PROVE_FLAGS I also chose a prove variable as that's related to TAP at the end. I am of course open to better names. -- Michael
From c702f3366a4b144bea76e738f744b10ce9c9c57d Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Sat, 17 Feb 2018 21:16:29 +0900
Subject: [PATCH 1/2] Prevent SSL and LDAP tests from running without support
in build
An extra set of checks in each one of the test files is added to make
the tests fail should they be invoked without the proper build options.
This makes use of TestLib::check_pg_config to check for the build
configuration.
---
src/test/ldap/t/001_auth.pl | 4 ++++
src/test/ssl/t/001_ssltests.pl | 4 ++++
src/test/ssl/t/002_scram.pl | 4 ++++
3 files changed, 12 insertions(+)
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index a83d96ae91..9d5065c494 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -4,6 +4,10 @@ use TestLib;
use PostgresNode;
use Test::More tests => 19;
+# LDAP tests are not supported without proper build options
+die "LDAP tests not supported without support in build" unless
+ check_pg_config("#define USE_LDAP 1");
+
my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
$ldap_bin_dir = undef; # usually in PATH
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index e53bd12ae9..bf68a727eb 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -6,6 +6,10 @@ use Test::More tests => 40;
use ServerSetup;
use File::Copy;
+# SSL tests are not supported without proper build options
+die "SSL tests not supported without support in build" unless
+ check_pg_config("#define USE_OPENSSL 1");
+
#### Some configuration
# This is the hostname used to connect to the server. This cannot be a
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 67c1409a6e..8e79b6a99f 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -8,6 +8,10 @@ use Test::More tests => 5;
use ServerSetup;
use File::Copy;
+# SSL tests are not supported without proper build
+die "SSL tests not supported without support in build" unless
+ check_pg_config("#define USE_OPENSSL 1");
+
# This is the hostname used to connect to the server.
my $SERVERHOSTADDR = '127.0.0.1';
--
2.16.1
From e8d8071cbb972324eb75de3bb1d700e93e4ee928 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Sat, 17 Feb 2018 22:39:39 +0900
Subject: [PATCH 2/2] Add PROVE_EXTRA_ALLOWED to control optional test suites
By default, SSL and LDAP test suites are not allowed to run as they are
not secure for multi-user environments, which is why they are not part
of check-world. This commit adds an extra make variable which can be
used to optionally enable them if wanted. The user can make use of the
variable like that for example:
make -C src/test check PROVE_EXTRA_ALLOWED='ssl ldap'
PROVE_EXTRA_ALLOWED needs to be a list of items separated by
whitespaces, and supports two values for now: 'ssl' and 'ldap' to be
able to run respectively tests in src/test/ssl and src/test/ldap.
In consequence, the SSL and LDAP test suites are added to check-world
but they are skipped except if the user has asked for them to be
enabled.
---
doc/src/sgml/regress.sgml | 15 +++++++++++++++
src/test/Makefile | 9 ++++-----
src/test/ldap/t/001_auth.pl | 13 ++++++++++++-
src/test/perl/TestLib.pm | 21 +++++++++++++++++++++
src/test/ssl/t/001_ssltests.pl | 13 ++++++++++++-
src/test/ssl/t/002_scram.pl | 13 ++++++++++++-
6 files changed, 76 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 53716a029f..e6559dae2a 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -675,6 +675,21 @@ make -C src/bin check PROVE_FLAGS='--timer'
See the manual page of <command>prove</command> for more information.
</para>
+ <para>
+ TAP tests under <filename>src/test/ssl</filename> and
+ <filename>src/test/ldap</filename> are not secure to run on a multi-system
+ environment. You can decide which test suites to additionally allow by
+ setting the <command>make</command> variable
+ <varname>PROVE_EXTRA_ALLOWED</varname> to define a list of tests separated
+ by a whitespace.
+<programlisting>
+make -C src/test check PROVE_EXTRA_ALLOWED='ssl ldap'
+</programlisting>
+ As of now, two test types are supported: <literal>ssl</literal> to allow
+ tests in <filename>src/test/ssl</filename> to be run, and
+ <literal>ldap</literal> for <filename>src/test/ldap</filename>.
+ </para>
+
<para>
The TAP tests require the Perl module <literal>IPC::Run</literal>.
This module is available from CPAN or an operating system package.
diff --git a/src/test/Makefile b/src/test/Makefile
index 73abf163f1..c4ae0965b2 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -12,13 +12,12 @@ subdir = src/test
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-SUBDIRS = perl regress isolation modules authentication recovery subscription
+SUBDIRS = perl regress isolation ldap modules authentication recovery \
+ ssl subscription
# We don't build or execute examples/, locale/, or thread/ by default,
-# but we do want "make clean" etc to recurse into them. Likewise for
-# ldap/ and ssl/, because these test suites are not secure to run on a
-# multi-user system.
-ALWAYS_SUBDIRS = examples ldap locale thread ssl
+# but we do want "make clean" etc to recurse into them.
+ALWAYS_SUBDIRS = examples locale thread
# We want to recurse to all subdirs for all standard targets, except that
# installcheck and install should not recurse into the subdirectory "modules".
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 9d5065c494..ca4c5d47ee 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -2,7 +2,18 @@ use strict;
use warnings;
use TestLib;
use PostgresNode;
-use Test::More tests => 19;
+use Test::More;
+
+# Check if test is allowed by user. Be sure to check that before the
+# build compatibility.
+if (check_extra_allowed("ldap"))
+{
+ plan tests => 19;
+}
+else
+{
+ plan skip_all => 'LDAP test suite not allowed to run';
+}
# LDAP tests are not supported without proper build options
die "LDAP tests not supported without support in build" unless
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index fdd427608b..e9fc09f5c5 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -26,6 +26,7 @@ our @EXPORT = qw(
slurp_dir
slurp_file
append_to_file
+ check_extra_allowed
check_pg_config
system_or_bail
system_log
@@ -240,6 +241,26 @@ sub check_pg_config
return $match;
}
+# Check if the test specified by the name given by caller is authorized to
+# run or not. We check for a match in the list of entries using whitespace
+# as separator in the environment variable PROVE_EXTRA_ALLOWED.
+sub check_extra_allowed
+{
+ my $test_name = shift;
+
+ if (defined($ENV{PROVE_EXTRA_ALLOWED}))
+ {
+ my @tests = split / /, $ENV{PROVE_EXTRA_ALLOWED};
+
+ foreach my $test (@tests)
+ {
+ return 1 if ($test eq $test_name)
+ }
+ }
+
+ return 0;
+}
+
#
# Test functions
#
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index bf68a727eb..071d6ccc1b 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -2,10 +2,21 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 40;
+use Test::More;
use ServerSetup;
use File::Copy;
+# Check if test is allowed by user. Be sure to check that before the
+# build compatibility.
+if (check_extra_allowed("ssl"))
+{
+ plan tests => 40;
+}
+else
+{
+ plan skip_all => 'SSL test suite not allowed to run';
+}
+
# SSL tests are not supported without proper build options
die "SSL tests not supported without support in build" unless
check_pg_config("#define USE_OPENSSL 1");
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 8e79b6a99f..1b5efb44a3 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -4,10 +4,21 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More;
use ServerSetup;
use File::Copy;
+# Check if test is allowed by user. Be sure to check that before the
+# build compatibility.
+if (check_extra_allowed("ssl"))
+{
+ plan tests => 5;
+}
+else
+{
+ plan skip_all => 'SSL test suite not allowed to run';
+}
+
# SSL tests are not supported without proper build
die "SSL tests not supported without support in build" unless
check_pg_config("#define USE_OPENSSL 1");
--
2.16.1
signature.asc
Description: PGP signature
