Re: Create shorthand for including all extra tests
On 15.01.24 09:54, Nazir Bilal Yavuz wrote: Hi, On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut wrote: On 05.09.23 19:26, Nazir Bilal Yavuz wrote: Thanks for the feedback! I updated the patch, 'needs-private-lo' option enables kerberos, ldap, load_balance and ssl extra tests now. As was discussed, I don't think "needs private lo" is the only condition for these tests. At least kerberos and ldap also need extra software installed, and load_balance might need editing the system's hosts file. So someone would still need to familiarize themselves with these tests individually before setting a global option like this. Also, if we were to create test groupings like this, I think the implementation should be different. The way you have it, there is a sort of central registry of all affected tests in src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. I would prefer a more decentralized approach where each test decides on its own whether to run, with pseudo-code conditionals like if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains "needs-private-lo")) skip_all Anyway, at the moment, I don't see a sensible way to group these things beyond what we have now (effectively, "ldap" is already a group, because it affects more than one test suite). Right now, we have six possible values, which is probably just about doable to keep track of manually. If we get a lot more, then we need to look into this again, but maybe then we'll also have more patterns to group things around. I see your point. It looks like the best option is to reevaluate this if there are more PG_TEST_EXTRA options. Ok, I'm closing this commitfest entry.
Re: Create shorthand for including all extra tests
Hi, On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut wrote: > > On 05.09.23 19:26, Nazir Bilal Yavuz wrote: > > Thanks for the feedback! I updated the patch, 'needs-private-lo' > > option enables kerberos, ldap, load_balance and ssl extra tests now. > > As was discussed, I don't think "needs private lo" is the only condition > for these tests. At least kerberos and ldap also need extra software > installed, and load_balance might need editing the system's hosts file. > So someone would still need to familiarize themselves with these tests > individually before setting a global option like this. > > Also, if we were to create test groupings like this, I think the > implementation should be different. The way you have it, there is a > sort of central registry of all affected tests in > src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. > I would prefer a more decentralized approach where each test decides > on its own whether to run, with pseudo-code conditionals like > > if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains > "needs-private-lo")) >skip_all > > Anyway, at the moment, I don't see a sensible way to group these things > beyond what we have now (effectively, "ldap" is already a group, because > it affects more than one test suite). Right now, we have six possible > values, which is probably just about doable to keep track of manually. > If we get a lot more, then we need to look into this again, but maybe > then we'll also have more patterns to group things around. I see your point. It looks like the best option is to reevaluate this if there are more PG_TEST_EXTRA options. -- Regards, Nazir Bilal Yavuz Microsoft
Re: Create shorthand for including all extra tests
On 05.09.23 19:26, Nazir Bilal Yavuz wrote: Thanks for the feedback! I updated the patch, 'needs-private-lo' option enables kerberos, ldap, load_balance and ssl extra tests now. As was discussed, I don't think "needs private lo" is the only condition for these tests. At least kerberos and ldap also need extra software installed, and load_balance might need editing the system's hosts file. So someone would still need to familiarize themselves with these tests individually before setting a global option like this. Also, if we were to create test groupings like this, I think the implementation should be different. The way you have it, there is a sort of central registry of all affected tests in src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. I would prefer a more decentralized approach where each test decides on its own whether to run, with pseudo-code conditionals like if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains "needs-private-lo")) skip_all Anyway, at the moment, I don't see a sensible way to group these things beyond what we have now (effectively, "ldap" is already a group, because it affects more than one test suite). Right now, we have six possible values, which is probably just about doable to keep track of manually. If we get a lot more, then we need to look into this again, but maybe then we'll also have more patterns to group things around.
Re: Create shorthand for including all extra tests
On 04.09.23 22:30, Tom Lane wrote: Noah Misch writes: On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: On 4 Sep 2023, at 17:01, Tom Lane wrote: I think this is a seriously bad idea. The entire point of not including certain tests in check-world by default is that the omitted tests are security hazards, so a developer or buildfarm owner should review each one before deciding whether to activate it on their machine. Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard: they treat the loopback interface as private, so anyone with access to loopback interface ports can hijack the test. I'd be fine with e.g. PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting the tester to review the tests to rediscover this common factor. 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, PG_TEST_EXTRA="needs-private-lo slow" would cover the territory of "all", and I think it'd be very seldom that we'd have to invent new categories here (though maybe I lack imagination today). At least the kerberos tests also appear to require a lot of randomness for their setup, and sometimes in VM environments they hang for minutes until they get that. I suppose that would go under "slow". Also, at least in my mind, when we added the kerberos and ldap tests, a partial reason for excluding them from the default run was "requires additional unusual software to be installed". The additional kerberos and ldap server software used in those tests is not covered by configure/meson, so it's a bit more DIY.
Re: Create shorthand for including all extra tests
> On 4 Sep 2023, at 23:09, Noah Misch 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. Agreed, the names should be descriptive enough to contain a boundary. Any new test which is orders of magnitude slower than an existing test suite most likely will have one/more boundary characteristics not shared with existing suites. The test in 20210423204306.5osfpkt2ggaed...@alap3.anarazel.de for autovacuum wraparound comes to mind as one that would warrant a new category. > If one introduced needs-private-lo, the present spelling of "all" would be > "needs-private-lo wal_consistency_checking". I think it makes sense to invent a new PG_TEST_EXTRA category which (for now) only contains wal_consistency_checking to make it consistent, such that "all" can be achieved by a set of categories. -- Daniel Gustafsson
Re: Create shorthand for including all extra tests
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 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
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'
+
+
+ needs_private_lo
+
+
+ Enables the extra tests that treat the loopback interface as a private.
+ Currently enables kerberos, ldap, load_balance and ssl extra tests.
+
+
+
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
Re: Create shorthand for including all extra tests
On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote: > Noah Misch writes: > > On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: > >> On 4 Sep 2023, at 17:01, Tom Lane wrote: > >>> I think this is a seriously bad idea. The entire point of not including > >>> certain tests in check-world by default is that the omitted tests are > >>> security hazards, so a developer or buildfarm owner should review each > >>> one before deciding whether to activate it on their machine. > > > Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same > > hazard: > > they treat the loopback interface as private, so anyone with access to > > loopback interface ports can hijack the test. I'd be fine with e.g. > > PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting > > the tester to review the tests to rediscover this common factor. > > 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, > > PG_TEST_EXTRA="needs-private-lo slow" > > would cover the territory of "all", and I think it'd be very seldom > that we'd have to invent new categories here (though maybe I lack > imagination today). 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. If one introduced needs-private-lo, the present spelling of "all" would be "needs-private-lo wal_consistency_checking". Looks okay to me. Doing nothing here wouldn't be ruinous, of course.
Re: Create shorthand for including all extra tests
Noah Misch writes: > On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: >> On 4 Sep 2023, at 17:01, Tom Lane wrote: >>> I think this is a seriously bad idea. The entire point of not including >>> certain tests in check-world by default is that the omitted tests are >>> security hazards, so a developer or buildfarm owner should review each >>> one before deciding whether to activate it on their machine. > Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard: > they treat the loopback interface as private, so anyone with access to > loopback interface ports can hijack the test. I'd be fine with e.g. > PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting > the tester to review the tests to rediscover this common factor. 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, PG_TEST_EXTRA="needs-private-lo slow" would cover the territory of "all", and I think it'd be very seldom that we'd have to invent new categories here (though maybe I lack imagination today). regards, tom lane
Re: Create shorthand for including all extra tests
On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: > > On 4 Sep 2023, at 17:01, Tom Lane wrote: > > Nazir Bilal Yavuz writes: > >> I created an 'all' option for PG_TEST_EXTRA to enable all test suites > >> defined under PG_TEST_EXTRA. > > > > I think this is a seriously bad idea. The entire point of not including > > certain tests in check-world by default is that the omitted tests are > > security hazards, so a developer or buildfarm owner should review each > > one before deciding whether to activate it on their machine. > > I dunno, I've certainly managed to not run the tests I hoped to multiple > times. > I think it could be useful for sandboxed testrunners which are destroyed after > each run. There is for sure a foot-gun angle to it, no question about that. Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard: they treat the loopback interface as private, so anyone with access to loopback interface ports can hijack the test. I'd be fine with e.g. PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting the tester to review the tests to rediscover this common factor.
Re: Create shorthand for including all extra tests
> On 4 Sep 2023, at 17:01, Tom Lane wrote: > > Nazir Bilal Yavuz writes: >> I created an 'all' option for PG_TEST_EXTRA to enable all test suites >> defined under PG_TEST_EXTRA. > > I think this is a seriously bad idea. The entire point of not including > certain tests in check-world by default is that the omitted tests are > security hazards, so a developer or buildfarm owner should review each > one before deciding whether to activate it on their machine. I dunno, I've certainly managed to not run the tests I hoped to multiple times. I think it could be useful for sandboxed testrunners which are destroyed after each run. There is for sure a foot-gun angle to it, no question about that. -- Daniel Gustafsson
Re: Create shorthand for including all extra tests
Nazir Bilal Yavuz writes: > I created an 'all' option for PG_TEST_EXTRA to enable all test suites > defined under PG_TEST_EXTRA. I think this is a seriously bad idea. The entire point of not including certain tests in check-world by default is that the omitted tests are security hazards, so a developer or buildfarm owner should review each one before deciding whether to activate it on their machine. regards, tom lane
Create shorthand for including all extra tests
Hi,
I realized that I forgot to add the new extra test to my test scripts.
So, I thought maybe we can use shorthand for including all extra
tests. With that, running a full testsuite is easier without having to
keep up with new tests and updates.
I created an 'all' option for PG_TEST_EXTRA to enable all test suites
defined under PG_TEST_EXTRA. I created the 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 'all' or this
test's name.
I thought another advantage could be that this can be used in CI. But
when 'wal_consistency_checking' is enabled, CI times get longer since
it does resource intensive operations.
Any kind of feedback would be appreciated.
Regards,
Nazir Bilal Yavuz
Microsoft
From be91a0aaf926c83bf266f92e0523f41ca333b048 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz
Date: Mon, 4 Sep 2023 16:58:42 +0300
Subject: [PATCH v1] Create shorthand for including all extra tests
This patch aims to make running full testsuite easier without having to
keep up with new tests and updates.
Create 'all' option for PG_TEST_EXTRA to enable all test suites defined
under PG_TEST_EXTRA. That is achieved by creating
check_extra_tests_enabled() function in Test/Utils.pm file. This
function takes the test's name as an input and checks if PG_TEST_EXTRA
contains 'all' or this test's name.
---
doc/src/sgml/regress.sgml| 9 +
src/interfaces/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 | 16
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, 35 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 675db86e4d7..40269c258ef 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -262,6 +262,15 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance'
The following values are currently supported:
+
+ all
+
+
+ Enables all extra tests.
+
+
+
+
kerberos
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 6331c976dcb..2fdcff24785 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,7 +43,7 @@ endif
# Test runs an LDAP server, so only run if ldap is in PG_TEST_EXTRA
ifeq ($(w
