Disabling src/test/[ssl|ldap] when not building with SSL/LDAP support
Hi all,
In order to run tests consistently on the whole tree, I use a simple
alias which tests also things like src/test/ssl and src/test/ldap on the
way.
Lately, I am getting annoyed by $subject when working on OpenSSL stuff
as sometimes I need to test things with and without SSL support to make
sure that a patch is rightly shaped. However if configure is built
without SSL support then src/test/ssl just fails. The same applies to
src/test/ldap.
Could it be possible to disable them using something like the attached
if a build is done without SSL and/or LDAP?
Thanks,
--
Michael
Attachments:
disable-tap-ssl-ldap.patchtext/x-diff; charset=us-asciiDownload
diff --git a/configure.in b/configure.in
index 4d26034579..aee3ab0867 100644
--- a/configure.in
+++ b/configure.in
@@ -682,6 +682,7 @@ PGAC_ARG_BOOL(with, ldap, no,
[build with LDAP support],
[AC_DEFINE([USE_LDAP], 1, [Define to 1 to build with LDAP support. (--with-ldap)])])
AC_MSG_RESULT([$with_ldap])
+AC_SUBST(with_ldap)
#
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d980f81046..dcb8dc5d90 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -186,6 +186,7 @@ with_tcl = @with_tcl@
with_openssl = @with_openssl@
with_selinux = @with_selinux@
with_systemd = @with_systemd@
+with_ldap = @with_ldap@
with_libxml = @with_libxml@
with_libxslt = @with_libxslt@
with_system_tzdata = @with_system_tzdata@
diff --git a/src/test/ldap/Makefile b/src/test/ldap/Makefile
index fef5742b82..983b37e71a 100644
--- a/src/test/ldap/Makefile
+++ b/src/test/ldap/Makefile
@@ -14,10 +14,18 @@ top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
check:
+ifeq ($(with_ldap),yes)
$(prove_check)
+else
+ @echo "No tests as LDAP is not supported."
+endif
installcheck:
+ifeq ($(with_ldap),yes)
$(prove_installcheck)
+else
+ @echo "No tests as LDAP is not supported."
+endif
clean distclean maintainer-clean:
rm -rf tmp_check
diff --git a/src/test/ssl/Makefile b/src/test/ssl/Makefile
index 4e9095529a..87872d07ae 100644
--- a/src/test/ssl/Makefile
+++ b/src/test/ssl/Makefile
@@ -131,7 +131,15 @@ clean distclean maintainer-clean:
rm -rf tmp_check
check:
+ifeq ($(with_openssl),yes)
$(prove_check)
+else
+ @echo "No tests as OpenSSL is not supported."
+endif
installcheck:
+ifeq ($(with_openssl),yes)
$(prove_installcheck)
+else
+ @echo "No tests as OpenSSL is not supported."
+endif
Michael Paquier <michael.paquier@gmail.com> writes:
In order to run tests consistently on the whole tree, I use a simple
alias which tests also things like src/test/ssl and src/test/ldap on the
way.
Lately, I am getting annoyed by $subject when working on OpenSSL stuff
as sometimes I need to test things with and without SSL support to make
sure that a patch is rightly shaped. However if configure is built
without SSL support then src/test/ssl just fails. The same applies to
src/test/ldap.
Could it be possible to disable them using something like the attached
if a build is done without SSL and/or LDAP?
That seems like possibly not such a great idea. If somebody were using
a run of src/test/ssl to check their build, they would now get no
notification if they'd forgotten to build --with-openssl.
Perhaps we could introduce some new targets, "check-if-enabled" or so,
that act like you want.
regards, tom lane
On Thu, Feb 08, 2018 at 09:28:10AM -0500, Tom Lane wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
That seems like possibly not such a great idea. If somebody were using
a run of src/test/ssl to check their build, they would now get no
notification if they'd forgotten to build --with-openssl.Perhaps we could introduce some new targets, "check-if-enabled" or so,
that act like you want.
Per this argument, we need to do something even for check and
installcheck anyway, no? Except that what you are suggesting is to make
the tests fail instead of silently being bypassed. Copying an
expression you used recently, this boils down to not spend CPU cycles
for nothing. The TAP tests showing in red all over your screen don't
show any useful information either as one may be building with SSL
support, and still getting failures because he/she is working on an
SSL-related feature.
I prefer making the tests personally not fail, as when building without
SSL one needs to move down to run ./configure again, so he likely knows
what is is doing. Bypassing them also has the advantage to not do
failure check dances, particularly in bash when using temporarily set
+e/-e to avoid a problem, so this makes things easier for most cases.
--
Michael
On Sat, Feb 10, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Feb 08, 2018 at 09:28:10AM -0500, Tom Lane wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
That seems like possibly not such a great idea. If somebody were using
a run of src/test/ssl to check their build, they would now get no
notification if they'd forgotten to build --with-openssl.Perhaps we could introduce some new targets, "check-if-enabled" or so,
that act like you want.Per this argument, we need to do something even for check and
installcheck anyway, no? Except that what you are suggesting is to make
the tests fail instead of silently being bypassed. Copying an
expression you used recently, this boils down to not spend CPU cycles
for nothing. The TAP tests showing in red all over your screen don't
show any useful information either as one may be building with SSL
support, and still getting failures because he/she is working on an
SSL-related feature.I prefer making the tests personally not fail, as when building without
SSL one needs to move down to run ./configure again, so he likely knows
what is is doing. Bypassing them also has the advantage to not do
failure check dances, particularly in bash when using temporarily set
+e/-e to avoid a problem, so this makes things easier for most cases.
One complication with the LDAP tests is that
src/test/ldap/t/001_auth.pl has greater requirements than --with-ldap.
For example, on a Debian system you probably only need the
libldap2-dev package installed for --with-ldap to build, but
001_auth.pl also requires the slapd package to be installed (the
OpenLDAP server). On some other systems including macOS and maybe
AIX, a conforming LDAP client library (maybe OpenLDAP or maybe some
other implementation) is part of the base system but I don't think
slapd is.
I agree that it would be nice if the build farm (and my unofficial
patch tester for that matter) could automatically test the LDAP stuff
when running on a suitable system, but I think it would need to be
based not just on ifeq ($(with_ldap),yes) as you showed. I think the
test would need a way to report that it can't find slapd so it can't
run, but not consider that to be a hard failure. Or something like
that...
It might also be a good idea if you could tell 001_auth.pl where slapd
and openssl (used by 001_auth.pl to make certificates) are through
environment variables, in case anyone wants to run those tests against
an installation other than one in the hardcoded paths we told it about
for Linux, macOS (with Brew) and FreeBSD.
--
Thomas Munro
http://www.enterprisedb.com
On Sat, Feb 10, 2018 at 4:32 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
I agree that it would be nice if the build farm (and my unofficial
patch tester for that matter) could automatically test the LDAP stuff
when running on a suitable system, but I think it would need to be
based not just on ifeq ($(with_ldap),yes) as you showed. I think the
test would need a way to report that it can't find slapd so it can't
run, but not consider that to be a hard failure. Or something like
that...
Hmm. I guess I just changed the subject a bit there to running those
tests from check-world, if possible, by default. But as
src/test/Makefile says, those test suites "are not secure to run on a
multi-user system". You're talking about making the tests not fail if
you tried to run them directly (not from check-world, but by direct
invocation) when you didn't build with the right options. I take back
what I said: it's probably better if you run those tests explicitly
when you know you have the prerequisites installed and you're OK with
the security implications (for example I should probably just do that
on those Travis CI patch tester builds). Sorry for the noise.
--
Thomas Munro
http://www.enterprisedb.com
On Sat, Feb 10, 2018 at 04:44:38PM +1300, Thomas Munro wrote:
On Sat, Feb 10, 2018 at 4:32 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:I agree that it would be nice if the build farm (and my unofficial
patch tester for that matter) could automatically test the LDAP stuff
when running on a suitable system, but I think it would need to be
based not just on ifeq ($(with_ldap),yes) as you showed. I think the
test would need a way to report that it can't find slapd so it can't
run, but not consider that to be a hard failure. Or something like
that...Hmm. I guess I just changed the subject a bit there to running those
tests from check-world, if possible, by default. But as
src/test/Makefile says, those test suites "are not secure to run on a
multi-user system".
Yeah, I am not advocating that. Note that the SSL tests could become
part of the default if we have one day a way do so SSL with a Unix
domain socket. There were some discussions a couple of years back but,
if I recall correctly, the arguing has stuck about the way to shape the
HBA entries for the configuration. There are also few use cases so the
enthusiasm has not lasted long.
You're talking about making the tests not fail if
you tried to run them directly (not from check-world, but by direct
invocation) when you didn't build with the right options. I take back
what I said: it's probably better if you run those tests explicitly
when you know you have the prerequisites installed and you're OK with
the security implications (for example I should probably just do that
on those Travis CI patch tester builds). Sorry for the noise.
Yes, you need to go into the test's paths to run them as well. The
problem is also that the information provided by the TAP tests exploding
because the build does not support those is pretty useless. It is true
that you could filter things by using TestLib::check_pg_config, but do
we really want to spend cycles for that when a Makefile check can do the
job better and in a cheaper way?
--
Michael
On Sat, Feb 10, 2018 at 2:02 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
I agree that it would be nice if the build farm (and my unofficial
patch tester for that matter) could automatically test the LDAP stuff
when running on a suitable system, but I think it would need to be
based not just on ifeq ($(with_ldap),yes) as you showed. I think the
test would need a way to report that it can't find slapd so it can't
run, but not consider that to be a hard failure. Or something like
that...
I think we could do that in the buildfarm by providing an
extra_tap_tests config setting. It would be up to the owner to ensure
that there was a suitable test environment available.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I would like to see a global setting of some kind that specifies which
extra tests are allowed to be run. This could be a configure argument
or an environment/make variable. Rather than make it a list of tests,
specify which facilities you have available, e.g.,
PG_EXTRA_TEST_FACILTITIES='tcpip openssl slapd'
Then you could combine that with the actual build configuration to skip
tests that the build doesn't support.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Feb 12, 2018 at 10:22:13PM -0500, Peter Eisentraut wrote:
I would like to see a global setting of some kind that specifies which
extra tests are allowed to be run. This could be a configure argument
or an environment/make variable. Rather than make it a list of tests,
specify which facilities you have available, e.g.,PG_EXTRA_TEST_FACILTITIES='tcpip openssl slapd'
Then you could combine that with the actual build configuration to skip
tests that the build doesn't support.
Hm. Wouldn't it be enough to just spread the use of
TestLib::check_pg_config and use SKIP on the tests where things cannot
be run then? I see no need to invent an extra facility if all the
control is already in pg_config.h. For slapd, you can actually just
check if it can be executed in the PATH defined at the top of
001_auth.pl in $slapd, and skip the tests if the thing cannot be run.
We have enough flexibility in perl and TAP to allow that to work
reliably.
--
Michael
On 2/12/18 23:00, Michael Paquier wrote:
Hm. Wouldn't it be enough to just spread the use of
TestLib::check_pg_config and use SKIP on the tests where things cannot
be run then? I see no need to invent an extra facility if all the
control is already in pg_config.h. For slapd, you can actually just
check if it can be executed in the PATH defined at the top of
001_auth.pl in $slapd, and skip the tests if the thing cannot be run.
We have enough flexibility in perl and TAP to allow that to work
reliably.
Maybe that would work.
We still need a way to configure whether we want to run tests that open
TCP/IP listen sockets.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 16, 2018 at 03:32:46PM -0500, Peter Eisentraut wrote:
Maybe that would work.
We still need a way to configure whether we want to run tests that open
TCP/IP listen sockets.
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.
--
Michael
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
Attachments:
0001-Prevent-SSL-and-LDAP-tests-from-running-without-supp.patchtext/x-diff; charset=us-asciiDownload
From c702f3366a4b144bea76e738f744b10ce9c9c57d Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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
0002-Add-PROVE_EXTRA_ALLOWED-to-control-optional-test-sui.patchtext/x-diff; charset=us-asciiDownload
From e8d8071cbb972324eb75de3bb1d700e93e4ee928 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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
On 2/17/18 08:48, Michael Paquier wrote:
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.
I'm not sure why we need that. The tests will presumably fail anyway.
- 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.
I think sticking this into the Perl code is too complicated and
inflexible. We might want to use the same mechanism for non-TAP tests
as well.
What I had in mind would consist of something like this in
src/test/Makefile:
ifeq ($(with_ldap),yes)
ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
SUBDIRS += ldap
endif
endif
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Feb 24, 2018 at 10:51:22AM -0500, Peter Eisentraut wrote:
On 2/17/18 08:48, Michael Paquier wrote:
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.I'm not sure why we need that. The tests will presumably fail anyway.
Sure. But then I think that it would be nice to show up on screen the
reason why the test failed if possible. As of now if SSL is missing the
whole run shows in red without providing much useful information.
Instead of 0001 as shaped previously, why not using as well diag to show
the failure on the screen?
For example the following block at the top of each test:
if (!check_pg_config("#define USE_OPENSSL 1"))
{
diag "SSL tests not supported without support in build";
die;
}
Would result in this output:
t/001_ssltests.pl .. # SSL tests not supported without support in build
# Looks like your test exited with 255 before it could output anything.
t/001_ssltests.pl .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 40/40 subtests
I would find that more helful to make the difference between an
implementation failure and a test failing because of a missing
dependency.
I think sticking this into the Perl code is too complicated and
inflexible. We might want to use the same mechanism for non-TAP tests
as well.What I had in mind would consist of something like this in
src/test/Makefile:ifeq ($(with_ldap),yes)
ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
SUBDIRS += ldap
endif
endif
OK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can
take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that
documented is really necessary in my opinion. Any idea for a better
name?
--
Michael
On 2/24/18 18:29, Michael Paquier wrote:
Sure. But then I think that it would be nice to show up on screen the
reason why the test failed if possible. As of now if SSL is missing the
whole run shows in red without providing much useful information.
Instead of 0001 as shaped previously, why not using as well diag to show
the failure on the screen?For example the following block at the top of each test:
if (!check_pg_config("#define USE_OPENSSL 1"))
{
diag "SSL tests not supported without support in build";
die;
}
I think BAIL_OUT() is intended for this.
What I had in mind would consist of something like this in
src/test/Makefile:ifeq ($(with_ldap),yes)
ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
SUBDIRS += ldap
endif
endifOK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can
take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that
documented is really necessary in my opinion. Any idea for a better
name?
I don't have a great idea about the name. The value should be
space-separated to work better with make functions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Feb 28, 2018 at 10:02:37AM -0500, Peter Eisentraut wrote:
On 2/24/18 18:29, Michael Paquier wrote:
Sure. But then I think that it would be nice to show up on screen the
reason why the test failed if possible. As of now if SSL is missing the
whole run shows in red without providing much useful information.
Instead of 0001 as shaped previously, why not using as well diag to show
the failure on the screen?For example the following block at the top of each test:
if (!check_pg_config("#define USE_OPENSSL 1"))
{
diag "SSL tests not supported without support in build";
die;
}I think BAIL_OUT() is intended for this.
That's a better idea. I have reworked that in 0001. What do you think?
This has the same effect as diag(), which shows information directly to
the screen, and that's the behavior I was looking for.
What I had in mind would consist of something like this in
src/test/Makefile:ifeq ($(with_ldap),yes)
ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
SUBDIRS += ldap
endif
endifOK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can
take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that
documented is really necessary in my opinion. Any idea for a better
name?I don't have a great idea about the name. The value should be
space-separated to work better with make functions.
I have concluded about using the most simple name: PG_TEST_EXTRA. A
documentation attempt is added as well. This is unfortunately close to
EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as
well, however this is used only for the core regression tests now, so a
separated variable seems adapted to me.
Thoughts and reviews are welcome.
--
Michael
Attachments:
0001-Prevent-SSL-and-LDAP-tests-from-running-without-supp.patchtext/x-diff; charset=us-asciiDownload
From 98ef83f68975279267f290dfdc772d1e3725ecd8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 2 Mar 2018 17:00:23 +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..f1d2f9a1e6 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
+BAIL_OUT("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 4b097a69bf..cb8bc81f9a 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 => 62;
use ServerSetup;
use File::Copy;
+# SSL tests are not supported without proper build options
+BAIL_OUT("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 9460763a65..be1f49fe21 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 => 6;
use ServerSetup;
use File::Copy;
+# SSL tests are not supported without proper build
+BAIL_OUT("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.2
0002-Add-PG_TEST_EXTRA-to-control-optional-test-suites.patchtext/x-diff; charset=us-asciiDownload
From 29c605031929da4acb2754c923cd1cb99c8820ef Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 2 Mar 2018 17:19:34 +0900
Subject: [PATCH 2/2] Add PG_TEST_EXTRA 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 PG_TEST_EXTRA='ssl ldap'
PG_TEST_EXTRA 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. If the build does not support SSL or LDAP, those tests are
automatically ignored.
---
configure.in | 1 +
doc/src/sgml/regress.sgml | 17 +++++++++++++++++
src/Makefile.global.in | 1 +
src/test/Makefile | 16 +++++++++++++++-
4 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/configure.in b/configure.in
index 4d26034579..aee3ab0867 100644
--- a/configure.in
+++ b/configure.in
@@ -682,6 +682,7 @@ PGAC_ARG_BOOL(with, ldap, no,
[build with LDAP support],
[AC_DEFINE([USE_LDAP], 1, [Define to 1 to build with LDAP support. (--with-ldap)])])
AC_MSG_RESULT([$with_ldap])
+AC_SUBST(with_ldap)
#
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index ca2716a6d7..0a5946203c 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -211,6 +211,23 @@ make installcheck-world
option <option>--enable-tap-tests</option>. This is recommended for
development, but can be omitted if there is no suitable Perl installation.
</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>PG_TEST_EXTRA</varname> to define a list of tests separated
+ by a whitespace.
+<programlisting>
+make -C src/test check PG_TEST_EXTRA='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>. Those
+ tests will not be run if the build does not support respectively
+ <acronym>SSL</acronym> and <acronym>LDAP</acronym> even if listed.
+ </para>
</sect2>
<sect2>
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d980f81046..dcb8dc5d90 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -186,6 +186,7 @@ with_tcl = @with_tcl@
with_openssl = @with_openssl@
with_selinux = @with_selinux@
with_systemd = @with_systemd@
+with_ldap = @with_ldap@
with_libxml = @with_libxml@
with_libxslt = @with_libxslt@
with_system_tzdata = @with_system_tzdata@
diff --git a/src/test/Makefile b/src/test/Makefile
index 73abf163f1..2fa3463c87 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -14,10 +14,24 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = perl regress isolation modules authentication recovery subscription
+# Test suites which are not safe by default, but can be run if enforced
+# by the users via the whitespace-separated list in variable PG_TEST_EXTRA.
+ifeq ($(with_ldap),yes)
+ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
+SUBDIRS += ldap
+endif
+endif
+ifeq ($(with_openssl),yes)
+ifneq (,$(filter ssl,$(PG_TEST_EXTRA)))
+SUBDIRS += ssl
+endif
+endif
+
# 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.
+# multi-user system, still those can be enforced if wanted using
+# PG_TEST_EXTRA.
ALWAYS_SUBDIRS = examples ldap locale thread ssl
# We want to recurse to all subdirs for all standard targets, except that
--
2.16.2
On 3/2/18 03:23, Michael Paquier wrote:
That's a better idea. I have reworked that in 0001. What do you think?
This has the same effect as diag(), which shows information directly to
the screen, and that's the behavior I was looking for.
I ended up using plan skip_all, which seems to be intended for this
purpose, according to the documentation.
I have concluded about using the most simple name: PG_TEST_EXTRA. A
documentation attempt is added as well. This is unfortunately close to
EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as
well, however this is used only for the core regression tests now, so a
separated variable seems adapted to me.
Committed. Very nice.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Mar 03, 2018 at 08:58:18AM -0500, Peter Eisentraut wrote:
On 3/2/18 03:23, Michael Paquier wrote:
That's a better idea. I have reworked that in 0001. What do you think?
This has the same effect as diag(), which shows information directly to
the screen, and that's the behavior I was looking for.I ended up using plan skip_all, which seems to be intended for this
purpose, according to the documentation.
Yes, that's what I got as well first but...
I have concluded about using the most simple name: PG_TEST_EXTRA. A
documentation attempt is added as well. This is unfortunately close to
EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as
well, however this is used only for the core regression tests now, so a
separated variable seems adapted to me.Committed. Very nice.
Thanks for the commit, Peter, PG_TEST_EXTRA is already set is my
environment.
--
Michael