Hi,
Thanks for the feedback! I updated the patch, 'needs-private-lo'
option enables kerberos, ldap, load_balance and ssl extra tests now.
> On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote:
> > Yeah, I could live with something like that from the security standpoint.
> > Not sure if it helps Nazir's use-case though. Maybe we could invent
> > categories that can be used in place of individual test names?
> > For now,
Yes, that is not ideal for my use-case but still better.
On Tue, 5 Sept 2023 at 00:09, Noah Misch <n...@leadboat.com> wrote:
>
> I could imagine categories for filesystem bytes and RAM bytes. Also, while
> needs-private-lo has a bounded definition, "slow" doesn't. If today's one
> "slow" test increases check-world duration by 1.1x, we may not let a
> 100x-increase test use the same keyword.
I agree. I didn't create a new category as 'slow' but still open to suggestions.
I am not very familiar with perl syntax, I would like to hear your
opinions on how the implementation of the check_extra_text_enabled()
function could be done better.
Regards,
Nazir Bilal Yavuz
Microsoft
From 3a33161eef699e4fbcfeb2d62ec387621a331d78 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Tue, 5 Sep 2023 20:14:15 +0300
Subject: [PATCH v2] Create shorthand for including extra tests that treat the
loopback interface as private
This patch aims to make running full testsuite easier without having to
keep up with new tests and updates.
Create 'needs-private-lo' option for PG_TEST_EXTRA to enable extra
test suites that treat the loopback interface as private. That is
achieved by creating check_extra_tests_enabled() function in
the Test/Utils.pm file. This function takes the test's name as an
input and checks if PG_TEST_EXTRA contains this extra test or any
option that enables this extra test.
---
doc/src/sgml/regress.sgml | 10 +++++
.../libpq/t/004_load_balance_dns.pl | 2 +-
src/test/kerberos/t/001_auth.pl | 2 +-
src/test/ldap/t/001_auth.pl | 2 +-
src/test/ldap/t/002_bindpasswd.pl | 2 +-
src/test/modules/Makefile | 2 +-
.../t/001_mutated_bindpasswd.pl | 2 +-
src/test/perl/PostgreSQL/Test/Utils.pm | 45 +++++++++++++++++++
src/test/recovery/t/027_stream_regress.pl | 3 +-
src/test/ssl/t/001_ssltests.pl | 2 +-
src/test/ssl/t/002_scram.pl | 2 +-
src/test/ssl/t/003_sslinfo.pl | 2 +-
12 files changed, 65 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 675db86e4d7..a9ceb0342d3 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -313,6 +313,16 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance'
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><literal>needs_private_lo</literal></term>
+ <listitem>
+ <para>
+ Enables the extra tests that treat the loopback interface as a private.
+ Currently enables kerberos, ldap, load_balance and ssl extra tests.
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
Tests for features that are not supported by the current build
diff --git a/src/interfaces/libpq/t/004_load_balance_dns.pl b/src/interfaces/libpq/t/004_load_balance_dns.pl
index 875070e2120..62eeb21843e 100644
--- a/src/interfaces/libpq/t/004_load_balance_dns.pl
+++ b/src/interfaces/libpq/t/004_load_balance_dns.pl
@@ -6,7 +6,7 @@ use PostgreSQL::Test::Utils;
use PostgreSQL::Test::Cluster;
use Test::More;
-if ($ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+if (!PostgreSQL::Test::Utils::check_extra_text_enabled('load_balance'))
{
plan skip_all =>
'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 0deb9bffc8d..59574178afc 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -28,7 +28,7 @@ if ($ENV{with_gssapi} ne 'yes')
{
plan skip_all => 'GSSAPI/Kerberos not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bkerberos\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('kerberos'))
{
plan skip_all =>
'Potentially unsafe test GSSAPI/Kerberos not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 3e113fd6ebb..9db07e801e9 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ldap} ne 'yes')
{
plan skip_all => 'LDAP not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
{
plan skip_all =>
'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ldap/t/002_bindpasswd.pl b/src/test/ldap/t/002_bindpasswd.pl
index bcd4aa2b742..a1b1bd8c22f 100644
--- a/src/test/ldap/t/002_bindpasswd.pl
+++ b/src/test/ldap/t/002_bindpasswd.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ldap} ne 'yes')
{
plan skip_all => 'LDAP not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
{
plan skip_all =>
'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index e81873cb5ae..5c977908eec 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -42,7 +42,7 @@ endif
# Test runs an LDAP server, so only run if ldap is in PG_TEST_EXTRA
ifeq ($(with_ldap),yes)
-ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
+ifneq (,$(filter all ldap,$(PG_TEST_EXTRA)))
SUBDIRS += ldap_password_func
else
ALWAYS_SUBDIRS += ldap_password_func
diff --git a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
index c96c8d7a4de..0fc9d365287 100644
--- a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
+++ b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ldap} ne 'yes')
{
plan skip_all => 'LDAP not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
{
plan skip_all =>
'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f4..b9d2d51e8c6 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -268,6 +268,51 @@ sub all_tests_passing
=pod
+=item check_extra_text_enabled()
+
+Return 1 if the extra test given as an input is enabled. Otherwise, return 0.
+
+=cut
+
+sub check_extra_text_enabled
+{
+ my ($extra_test) = @_;
+ # Set the extra tests that treat the loopback interface as a private.
+ my %needs_private_lo = map { $_ => 1 } (
+ 'kerberos',
+ 'ldap',
+ 'load_balance',
+ 'ssl',
+ );
+ # Set the tests categories
+ my %test_categories = (
+ 'needs_private_lo' => {%needs_private_lo},
+ );
+
+ # Look if the individual extra test is enabled.
+ if ($ENV{PG_TEST_EXTRA} =~ m/\b$extra_test\b/)
+ {
+ return 1;
+ }
+ # Traverse the test categories, look if both the test catagory
+ # is enabled and extra test exists in this category.
+ else
+ {
+ foreach my $test_category (keys %test_categories)
+ {
+ if ($ENV{PG_TEST_EXTRA} =~ m/\b$test_category\b/
+ && exists $test_categories{$test_category}{$extra_test})
+ {
+ return 1;
+ }
+ }
+ }
+
+ return 0;
+}
+
+=pod
+
=item tempdir(prefix)
Securely create a temporary directory inside C<$tmp_check>, like C<mkdtemp>,
diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl
index f2f4e77626f..ba2014fb4cd 100644
--- a/src/test/recovery/t/027_stream_regress.pl
+++ b/src/test/recovery/t/027_stream_regress.pl
@@ -32,8 +32,7 @@ $node_primary->append_conf('postgresql.conf', 'synchronize_seqscans = off');
# WAL consistency checking is resource intensive so require opt-in with the
# PG_TEST_EXTRA environment variable.
-if ( $ENV{PG_TEST_EXTRA}
- && $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
+if (PostgreSQL::Test::Utils::check_extra_text_enabled('wal_consistency_checking'))
{
$node_primary->append_conf('postgresql.conf',
'wal_consistency_checking = all');
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 23248d71b06..1ddc636ee5f 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -17,7 +17,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 27abd02abf1..0fa5e0e709c 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 5306aad8023..78fa9f2e188 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
--
2.40.1
